Page MenuHomePhabricator

Add virtualsize tracking to mempool
AbandonedPublic

Authored by markblundeberg on Jan 11 2020, 09:45.

Details

Reviewers
deadalnix
Group Reviewers
Restricted Project
Summary

This adds virtualsize to the mempool, which will be used for modifying
the mining/eviction priority of transactions (which matters under conditions
of mempool congestion.)
The virtual size is conceptually the actual transaction size with added
fudge factors to represent consumption of other block-limited resources
besides block space. Currently, just SigOps.

In Core, they don't separately track virtual size but rather use virtual size
in place of the size itself. (Core's defines it as:
size_mempool = virtualsize_mempool = max(virtualsize_bip141, sigopscount*20)
)

We cannot just copy Core here for two important reasons:

  • Our sigops limit varies with block size -- see comment in miner.cpp which means if we used virtualsize everywhere then we could generate invalid block templates where it it thought it had more sigops than are actually available. (this problem will be addressed with sigchecks upgrade, where the limit is flat)
  • On BCH we avoid sudden changes in basic policy rules, because we want to keep consistent mempool policy. If we changed the min feerate from sat/byte to sat/vbyte this would introduce an easy double spend vector just like if we were to suddenly raise the unconf chain length limit.

(this Diff merely adds the virtualsize tracking, which is as-yet unused)

Depends on D4896 and D4904

Test Plan

ninja all check

Diff Detail

Repository
rABC Bitcoin ABC
Branch
virtualsize-track
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 8866
Build 15707: Default Diff Build & Tests
Build 15706: arc lint + arc unit

Event Timeline

markblundeberg created this revision.Jan 11 2020, 09:45
Herald added a reviewer: Restricted Project. · View Herald TranscriptJan 11 2020, 09:45
markblundeberg planned changes to this revision.Jan 11 2020, 11:42
markblundeberg edited the summary of this revision. (Show Details)

rebase & update tests

deadalnix added inline comments.Jan 11 2020, 23:22
src/miner.h
58 ↗(On Diff #15334)

This could be computed on the fly, but more importantly, I don't understand what's the play here? It seems to me like this is to be used to compute fee per byte and use that to select the best possible candidates for inclusion, but you'll still have to count sigops/size to make sure it doesn't go over.

Caching infos is usually a bad idea, because then you get cache coherency problems. If it is cheap to compute, then it's probably best to do so.

src/txmempool.h
97 ↗(On Diff #15334)

I think this is the wrong doxygen comment type.

markblundeberg added inline comments.Jan 12 2020, 00:03
src/miner.h
58 ↗(On Diff #15334)

We could indeed compute VirtualSize(sum(sizes), sum(sigops)) quickly on the fly, but that's not the same thing as sum(Virtualsize(size, sigop)) since it's not a strictly linear function (the std::max is a nonlinearity). So it's not just caching vs noncaching, but also a difference in behaviour.

As I mentioned to you privately I do actually think the former is more accurate in some ways, but it does result in some weirdness: the package virtualsize may in some cases not actually increase when more transactions are added, if the ancestors are dense in sigops (e.g., p2sh -> p2pkh transactions) and the new child tx has 0 sigops (e.g., a p2pkh -> p2sh transaction). In fact if VirtualSize is defined more weirdly than the current definition, it could even decrease upon adding a tx. Although weird, I can't actually see any issues resulting from this, provided we only use it for priority.

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

update ; split off sigops mempool test additions to D4904

deadalnix requested changes to this revision.Jan 12 2020, 14:56

It seems like this is now obsolete.

This revision now requires changes to proceed.Jan 12 2020, 14:56
markblundeberg abandoned this revision.Jan 18 2020, 04:50