Miners can retrieve a block template, a consensus-valid block excluding the proof of work
(usually computed on separate, dedicated hardware) using the getblocktemplate RPC. The āminerā
(BlockAssembler)
generates this template using transactions from the mempool, attempting to maximize the fees in the
block while staying within the block weight and sigop limits.
Miners can also use the prioritisetransaction RPC to artificially raise or lower the fees of
specific transactions in their own mempools. The prioritisation is achieved through a
fee delta.
The āmodified feeā of a transaction is the sum of its base fee (total output value subtracted from
total input value) and the fee delta.
PR #7594 added ancestor package tracking to the
mempool. The mempool caches every transactionās ancestor feerate (total modified fees divided by
total virtual size of the transaction and all of the transactions it depends upon to be mined).
PR #7600 changed the mining algorithm to use
ancestor packages rather than individual transactions, which improves assessments of the incentive
compatibility of transactions and enables Child Pays for Parent (CPFP) fee-bumping.
The algorithm adds transactions from the mempool in ancestor feerate order; every time
it adds a transaction to the block template, it also adds each of its ancestors and
updates the remaining transactions in the mempool accordingly.
Rather than edit the mempool transactions itself, the miner creates a copy of the updated
entries in mapModifiedTx.
Review on #24364 unearthed some
unexpected behavior in the way these entries are edited.
PR #24538 fixes this unexpected behavior
and adds unit tests for mining prioritised transactions.
In your own words, how does the mining algorithm work (Hint: the main logic can be found in
addPackageTxs())?
3a. In what scenario does a transaction get added to mapModifiedTx?
3b. In what scenario does an entry in mapModifiedTx get further modified?
What is the bug fixed by this PR? Can you construct a specific case in which the bug leads to a
lower-fee transaction being included in the mempool? (Hint: the PR adds a test).
<larryruane> i was just going to ask that ... isn't it a good review practice to run any new or modified test without the production code change, and make sure the test fails?
<larryruane> i always forget about sigops ... is it common that a block is less than max weight because it's at the max sigops? or is that more of a sanity check?
<larryruane> stickies-v: without double-(multiple-) counting, right? so if a tx has 2 parents, and each of those shares a parent, we count that "grandparent" only once?
<larryruane> so when a new block is mined, it's possible for a tx's ancestor fee (and ancestor size) to decrease since some of its ancestors may be included in the new block
<svav> I am guessing now but ... is mapModifiedTx some sort of snapshot for a "potential" mempool, which has added a given transaction into the mempool to then evaluate total fee rates, and see how this compared to previous mapTx?
<stickies-v> we want to have a copy of the mempool where we can remove ancestors that have already been selected as part of a package, without actually affecting the mempool, I think?
<lightlike> why do we call UpdatePackagesForAdded() right at the beginning of addPackageTxs() ? what could have been already added at this points so that we might need to change mapModifiedTx?
<glozow> To summarize, it contains transactions that have not been selected yet, but some subset of their ancestors have. So we can't just use the ancestor feerate cached in their mempool entries.
<lightlike> when adjusting the modified entry, the actual feerate was used, not the modified one. so things would be wrong if miners had prioritised the transaction.
<larryruane> so _normally_ the two are the same, but if the tx had its feerate modified (using the prioritisetransaction RPC, then it will be wrong without this fix
<larryruane> ok now i have a question, who is the world found this bug?? Oh the PR description (first comment) explains it, the result of an earlier review! that's great
<larryruane> prioritysettransaction seems like one of those features that if core didn't implement it, someone else would (so may as well standardize it)
<larryruane> really basic question: the mempool gets persisted to disk, right? so if the node goes down, then when we come back up again, we'll have the mempool from before, with all the modifications?
<glozow> larryruane: modified fees are used in mempool acceptance logic, too. If you prioritise with a negative amount, it'll also not make it into your mempool
<glozow> theStack: it would be nice if people could fee-bump the normal way :) if people need to pay miners out-of-band, there's something wrong with our fee bumping
<lightlike> to save time - aborting early instead of trying out the entire mempool when the block is almost full so most transaction won't fit anymore.
<theStack> i wonder where the magic number 4000 comes from btw... is this derived from a consensus limit on how large the coinbase is allowed to be? (if there is such a limit)
<larryruane> glozow: the code you linked to most recently, `addPackageTxs` ... git blame seems to show it was added 6 years ago, is that accurate? I thought packages were a recent addition (that you mostly implemented)
<lightlike> larryruane: I think the package logic has been used, child-pays-for-parent works after all. It's just that the parent currently needs a high enough feerate to make it into the mempool (even if it's not enough to get mined)
<glozow> Yeah larryruane: to answer your question about the packages, the nice thing is we've had CPFP for 6 years, but the problem is it only works for transactions already in the mempool.