Page MenuHomePhabricator

Use virtualsize for mining/mempool priority
ClosedPublic

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

Details

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 Passed
Unit
No Test Coverage
Build Status
Buildable 8884
Build 15743: Default Diff Build & Tests
Build 15742: arc lint + arc unit

Event Timeline

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).

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.Jan 12 2020, 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.Jan 12 2020, 15:08
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

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.Jan 19 2020, 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.Jan 19 2020, 16:42
test/functional/abc-mempool-limit-sigops.py
173 ↗(On Diff #15652)
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).

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 added inline comments.
test/functional/abc-mempool-limit-sigops.py
107 ↗(On Diff #15652)

yes, though I need the height here anyway

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.Jan 25 2020, 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 ↗(On Diff #15699)

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 ↗(On Diff #15699)

likestamp

This revision now requires changes to proceed.Jan 25 2020, 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 added inline comments.
src/miner.cpp
469 ↗(On Diff #15699)

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

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