Page MenuHomePhabricator

Use virtualsize for mining/mempool priority
AcceptedPublic

Authored by markblundeberg on Sat, Jan 11, 15:12.

Details

Reviewers
deadalnix
Group Reviewers
Restricted Project
Summary

This uses the virtualsize for influencing the mining priority / mempool eviction
priority under conditions of mempool congestion.

All other uses of size in mining/mempool continue to use actual size:

  • mempool entry minfeerate limitation (the 1 sat/byte regular limit)
  • mining minimum feerate calculation
  • mining block size calculations
  • ancestor size limit for chains
  • descendant size limit for chains
  • sorting order for net_processing mempool requests
  • calculation of 'bytes' in getmempoolinfo

This changes no behaviour under normal conditions, only under congestion.

Depends on D4906

Test Plan

ninja check-all

Diff Detail

Repository
rABC Bitcoin ABC
Branch
virtualsize_priority
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 9085
Build 16132: Bitcoin ABC Buildbot
Build 16131: arc lint + arc unit

Event Timeline

markblundeberg created this revision.Sat, Jan 11, 15:12
Herald added a reviewer: Restricted Project. · View Herald TranscriptSat, Jan 11, 15:12
markblundeberg added inline comments.Sat, Jan 11, 15:18
src/miner.cpp
459 ↗(On Diff #15339)

I'm not thrilled about this change... it makes a perfectly correct outcome, yet if there are a ton of transactions in mempool below the minfeerate then it will slow down template creation a bit.

Alternatively we can redefine blockMinFeeRate to be based on virtualsize feerate (i.e., just change packageSize -> packageVirtualSize here), however that would change behaviour since we would now suddenly stop mining some 1 sat/byte transactions by default, just because they had a few too many sigops. If we were to go that route then we should set the default -blockmintxfee to 0 (see D4902).

rm unused packageVirtualSize

markblundeberg edited the summary of this revision. (Show Details)

Update for changes in ancestor, add note that this functions
equally well from two possible parents (one or the other).

deadalnix requested changes to this revision.Sun, Jan 12, 15:08
deadalnix added inline comments.
src/test/mempool_tests.cpp
313 ↗(On Diff #15351)

fix style

504 ↗(On Diff #15351)

dito

src/validation.cpp
713 ↗(On Diff #15351)

Shouldn't this be gated by an activation flag?

This revision now requires changes to proceed.Sun, Jan 12, 15:08
markblundeberg requested review of this revision.Sun, Jan 12, 23:48
markblundeberg added inline comments.
src/validation.cpp
713 ↗(On Diff #15351)

We could, but I'm not sure it would matter much. As I understand it, this fee check exists basically just to avoid running script checks in case the mempool is too full and the transaction is expected to not make it in anyway.

To be clear I added this check mainly because the eviction policy (descendant scoring and TrimToSize) already changes in this diff, so pool.GetMinFee is in fact already returning feerate with sat/vbyte units.

Normal operation:

  • pool.GetMinFee returns 0 feerate, so this check doesn't matter.

Congested operation:

  • Mempool is full so low sat/vbyte transactions are getting evicted.
  • At the end of this function, if we try to insert a tx with too-low sat/vbyte, then pool.LimitSize will discard the tx and we will reject with "mempool full".
  • When txes are evicted from LimitSize, then it will raise pool.GetMinFee to a nonzero value.
  • Further lowfee transactions that try to get in will now bail early with a "mempool min fee not met" error.

If we use nSize here it just means that under congestion, dense sigops transactions are likely to bypass this check, but they will end up being anyway rejected with "mempool full" instead of "mempool min fee not met".

markblundeberg edited the summary of this revision. (Show Details)

rebase onto D4906 for CI

markblundeberg planned changes to this revision.Mon, Jan 13, 00:14

I have a feeling this needs some tests...

Can modify mempool_limit.py to do some weird sigops things and maybe something based on mining_prioritisetransaction, but that's a bit of a stretch.

There is another side-effect here, which is that bumping GetMinFee can technically bring sigops (and virtualsize) into play for wallet estimation. To accurately capture this would really suck, since we would need to communicate two fee rates (one for min relay fee in sat/byte, and one for the mempool rolling fee floor in sat/vbyte).

I think that gives an argument for dropping bytespersigop from 50 to exactly 34, which is the size of a P2PKH output (that has 1 sigop) and is also enough to let any P2SH multisig input slide by (a 1-of-15 might have 15 sigops in an input of ~633 bytes). Thus having bytespersigop=34 guarantees that any 'normal' transaction cannot have virtualsize > size, and so the wallet doesn't need to think about sigops at all.

Still even with 50, it's hard to make a 'normal' transaction that goes over the limit. For example you could have 1 P2SH 2-of-3 multisig branching out to 10 P2PKH outputs. Or, 1 P2PKH branching out to 10 P2PKH outputs. These would actually be rather rare, and only matter in the case of congestion.

A note for curiosity: I tried to make the same kind of test using fundrawtransaction / signrawtransactionwithwallet RPC, however it took about 100 ms per tx. So with 500 txes, that made a 50 second long test, yuck! This test runs in 2 seconds.

deadalnix requested changes to this revision.Sun, Jan 19, 16:42

Requesting change, but most of it are actually questions. The only thing I'm really suspicious of is the block template thing.

src/miner.cpp
468 ↗(On Diff #15652)

This effectively change the complexity of the template creation from O(blocksize) to O(mempool size). It is probably not worth it unless I'm missing something? This seems to only be used include a few low fee high sigops txns extra in the block template. Am I missing something?

470 ↗(On Diff #15652)

AFAIK, there sis not test for this.

src/test/mempool_tests.cpp
329 ↗(On Diff #15652)

We o not format comment this way.

518 ↗(On Diff #15652)

dito

test/functional/abc-mempool-limit-sigops.py
107 ↗(On Diff #15652)

getbestblockhash also exists.

173 ↗(On Diff #15652)

else ? I'm not sure if this is a mistake or a weird python feature.

IMO you'd be better off just having a boolean mempool_full = False, set it to true when the mempool is full, and assert that it is true after the loop.

This revision now requires changes to proceed.Sun, Jan 19, 16:42
markblundeberg added inline comments.Mon, Jan 20, 00:03
test/functional/abc-mempool-limit-sigops.py
173 ↗(On Diff #15652)
markblundeberg added inline comments.Mon, Jan 20, 04:48
src/miner.cpp
468 ↗(On Diff #15652)

Yes, I believe that's right.

If we left it as-is, suppose blockmintxfee = 10, and there are these packages, in order of vbytes feerate:

A: 12 sat/byte, 12 sat/vbyte
B: 9 sat/byte, 9 sat/vbyte
C: 100 sat/byte, 8 sat/vbyte
D: 8 sat/byte, 7.8 sat/vbyte

Packages up to (and including) A would be included in block, but transaction C would not be included, and that's because of B. (if B were not present, then C would get included but not D).

For default settings with blockmintxfee = minrelaytxfee = 1 sat/byte, there would be no difference in normal operation, UNLESS it's after a reorg and there is a single 0.99 sat/byte low-sigops transaction in mempool, then all high sigops <0.99 sat/vbyte transactions would never get cleared out of mempool until that transaction goes away, which would mean the mempool could explode in size.

As mentioned earlier, an acceptable alternative would be to redefine this check to use packageVirtualSize instead of packageSize. But we should then also do D4902 (we don't want to stop mining below 1 sat/vbyte as do we accept such transactions).

markblundeberg added inline comments.Mon, Jan 20, 05:46
src/miner.cpp
468 ↗(On Diff #15652)

Hmm, thinking about it a bit more, that latter scenario does suck. Imagine there is a reorg including a single 0.99 sat/byte transaction, then all 0.98 sat/vbyte transactions might not get mined for a long time.

0.98 sat/vbyte isn't abusive, and does occur in normal usage. For example a 1 sat/byte transaction that fans out 1 P2PKH input to 11 P2PKH outputs has 11 sigops in ~532 bytes, i.e., 0.97 sat/vbyte.

markblundeberg marked 6 inline comments as done.Tue, Jan 21, 06:08
markblundeberg added inline comments.
test/functional/abc-mempool-limit-sigops.py
107 ↗(On Diff #15652)

yes, though I need the height here anyway

markblundeberg updated this revision to Diff 15697.EditedTue, Jan 21, 06:09
markblundeberg marked an inline comment as done.

expand tests greatly -- more logging statements, test a package with a very low sat/vbyte parent, and add blockmintxfee tests.

small tweak - instead of using assert A.issubset(B), use
assert_equal(A.difference(B), set()) -- the latter one prints
out which A transactions are missing from B, if it fails.

deadalnix requested changes to this revision.Sat, Jan 25, 14:25

Split out the miner change and we can land the rest. Let's not delay due to open discussion.

src/miner.cpp
469

You need to split this in another patch so that the rest of the code doesn't get blocked by it.

IMO this is the wrong tradeof. The only txns for which it makes a difference are the one very rich in sigops, and in the case of a reorg where someone mined them, so that same one will likely mine them again later on anyways.

Regardless we should be continuing this discussion in another patch if you want to pursue this, it does not make sense to bundle this together with the rest and it is just delaying everything.

src/validation.cpp
687

likestamp

This revision now requires changes to proceed.Sat, Jan 25, 14:25

Split out the miner change and we can land the rest. Let's not delay due to open discussion.

The behaviour is incorrect and the test fails without the change to miner.cpp. You can submit Diffs with optimizations once the correct behaviour is established.

markblundeberg requested review of this revision.Sun, Jan 26, 04:24
markblundeberg added inline comments.
src/miner.cpp
469

Try building this without the change and run tests and observe why it matters.

deadalnix accepted this revision.Mon, Jan 27, 01:04
This revision is now accepted and ready to land.Mon, Jan 27, 01:04