miner: bug fix? update for ancestor inclusion using modified fees, not base #24538

glozow · 2022-03-11T16:55:08Z

Came up while reviewing #24364, where some of us incorrectly assumed that we use the same fee deduction in CTxMemPoolModifiedEntry::nModFeesWithAncestors when first constructing an entry and in update_for_parent_inclusion.

Actually, the behavior is this: when a mempool entry's ancestor is included in the block template, we create a CTxMemPoolModifiedEntry for it, subtracting the ancestor's modified fees from nModFeesWithAncestors. If another ancestor is included, we update it again, but use the ancestor's base fees instead.

I can't explain why we use GetFee in one place and GetModifiedFee in the other, but I'm quite certain we should be using the same one for both.

And should it be base or modified fees? Modified, otherwise the child inherits the prioritisation of the parent, but only until the parent gets mined. If we want prioritisation to cascade down to current in-mempool descendants, we should probably document that in the prioritsetransaction helpstring and implement it in CTxMemPool::mapDeltas, not as a quirk in the mining code?

Wrote a test in which a mempool entry has 2 ancestors, both prioritised, and both included in a block template individually. This test should fail without the s/GetFee/GetModifiedFee commit.

glozow · 2022-03-11T16:55:52Z

cc @jamesob

sdaftuar · 2022-03-11T16:57:44Z

src/node/miner.h

@@ -116,7 +116,7 @@ struct update_for_parent_inclusion

    void operator() (CTxMemPoolModifiedEntry &e)
    {
-        e.nModFeesWithAncestors -= iter->GetFee();
+        e.nModFeesWithAncestors -= iter->GetModifiedFee();


Oops. Good catch!

maflcko · 2022-03-11T16:59:56Z

Concept ACK

DrahtBot · 2022-03-12T21:33:45Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#24595 (deploymentstatus: move g_versionbitscache global to ChainstateManager by ajtowns)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

No behavior changes. Recommend using --color-moved=dimmed_zebra.

glozow · 2022-03-14T16:04:57Z

Rebased for #24080.

theStack · 2022-04-20T18:11:08Z

Concept ACK

ccdle12 · 2022-04-25T09:56:08Z

tested ACK e4303c3

and I guess concept ACK as well, since it seems undesirable for a miner to think they are prioritizing a single tx but actually has after effects on other txs (descendants) in terms of selection

maflcko · 2022-05-06T09:04:55Z

LGTM. I also checked that without the fix, CNB will incorrectly include the 0-fee, non-prioritized child:

diff --git a/src/test/miner_tests.cpp b/src/test/miner_tests.cpp
index 7e26e732f5..fa67ddb419 100644
--- a/src/test/miner_tests.cpp
+++ b/src/test/miner_tests.cpp
@@ -530,15 +530,15 @@ void MinerTestingSetup::TestPrioritisedMining(const CChainParams& chainparams, c
     m_node.mempool->addUnchecked(entry.Fee(0).SpendsCoinbase(false).FromTx(tx));
 
     auto pblocktemplate = AssemblerForTest(chainparams).CreateNewBlock(scriptPubKey);
-    BOOST_REQUIRE_EQUAL(pblocktemplate->block.vtx.size(), 6U);
+    BOOST_REQUIRE_EQUAL(pblocktemplate->block.vtx.size(), 7U);
     BOOST_CHECK(pblocktemplate->block.vtx[1]->GetHash() == hashFreeParent);
     BOOST_CHECK(pblocktemplate->block.vtx[2]->GetHash() == hashFreePrioritisedTx);
     BOOST_CHECK(pblocktemplate->block.vtx[3]->GetHash() == hashParentTx);
     BOOST_CHECK(pblocktemplate->block.vtx[4]->GetHash() == hashPrioritsedChild);
     BOOST_CHECK(pblocktemplate->block.vtx[5]->GetHash() == hashFreeChild);
+        // The FreeParent and FreeChild's prioritisations *does* impact the child.
+        BOOST_CHECK(pblocktemplate->block.vtx[6]->GetHash() == hashFreeGrandchild);
     for (size_t i=0; i<pblocktemplate->block.vtx.size(); ++i) {
-        // The FreeParent and FreeChild's prioritisations should not impact the child.
-        BOOST_CHECK(pblocktemplate->block.vtx[i]->GetHash() != hashFreeGrandchild);
         // De-prioritised transaction should not be included.
         BOOST_CHECK(pblocktemplate->block.vtx[i]->GetHash() != hashMediumFeeTx);
     }

maflcko · 2022-05-06T09:05:50Z

ACK e4303c3 🚗

Show signature

Signature:

…ing modified fees, not base e4303c3 [unit test] prioritisation in mining (glozow) 7a8d606 [miner] bug fix: update for parent inclusion using modified fee (glozow) 0f9a444 MOVEONLY: group miner tests into MinerTestingSetup functions (glozow) Pull request description: Came up while reviewing bitcoin#24364, where some of us incorrectly assumed that we use the same fee deduction in `CTxMemPoolModifiedEntry::nModFeesWithAncestors` when first constructing an entry and in `update_for_parent_inclusion`. Actually, the behavior is this: when a mempool entry's ancestor is included in the block template, we create a `CTxMemPoolModifiedEntry` for it, subtracting the ancestor's modified fees from `nModFeesWithAncestors`. If another ancestor is included, we update it again, but use the ancestor's _base_ fees instead. I can't explain why we use `GetFee` in one place and `GetModifiedFee` in the other, but I'm quite certain we should be using the same one for both. And should it be base or modified fees? Modified, otherwise the child inherits the prioritisation of the parent, but only until the parent gets mined. If we want prioritisation to cascade down to current in-mempool descendants, we should probably document that in the `prioritsetransaction` helpstring and implement it in `CTxMemPool::mapDeltas`, not as a quirk in the mining code? Wrote a test in which a mempool entry has 2 ancestors, both prioritised, and both included in a block template individually. This test should fail without the s/GetFee/GetModifiedFee commit. ACKs for top commit: ccdle12: tested ACK e4303c3 MarcoFalke: ACK e4303c3 🚗 Tree-SHA512: 4cd94106fbc9353e9f9b6d5af268ecda5aec7539245298c940ca220606dd0737264505bfaae1f83d94765cc2d9e1a6e913a765048fe6c19292482241761a6762

…t base Summary: ``` Came up while reviewing #24364, where some of us incorrectly assumed that we use the same fee deduction in CTxMemPoolModifiedEntry::nModFeesWithAncestors when first constructing an entry and in update_for_parent_inclusion. Actually, the behavior is this: when a mempool entry's ancestor is included in the block template, we create a CTxMemPoolModifiedEntry for it, subtracting the ancestor's modified fees from nModFeesWithAncestors. If another ancestor is included, we update it again, but use the ancestor's base fees instead. I can't explain why we use GetFee in one place and GetModifiedFee in the other, but I'm quite certain we should be using the same one for both. And should it be base or modified fees? Modified, otherwise the child inherits the prioritisation of the parent, but only until the parent gets mined. If we want prioritisation to cascade down to current in-mempool descendants, we should probably document that in the prioritsetransaction helpstring and implement it in CTxMemPool::mapDeltas, not as a quirk in the mining code? Wrote a test in which a mempool entry has 2 ancestors, both prioritised, and both included in a block template individually. This test should fail without the s/GetFee/GetModifiedFee commit. ``` Backport of [[bitcoin/bitcoin#24538 | core#24538]]. Depends on D12951. Test Plan: ninja check-all Reviewers: #bitcoin_abc, sdulfari Reviewed By: #bitcoin_abc, sdulfari Subscribers: sdulfari Differential Revision: https://reviews.bitcoinabc.org/D12953

glozow added the Mining label Mar 11, 2022

sdaftuar reviewed Mar 11, 2022

View reviewed changes

maflcko mentioned this pull request Mar 11, 2022

refactor: remove duplicate code from BlockAssembler #24364

Merged

DrahtBot mentioned this pull request Mar 13, 2022

policy: Remove unused locktime flags #24080

Merged

glozow added 3 commits March 14, 2022 16:02

MOVEONLY: group miner tests into MinerTestingSetup functions

0f9a444

No behavior changes. Recommend using --color-moved=dimmed_zebra.

[miner] bug fix: update for parent inclusion using modified fee

7a8d606

[unit test] prioritisation in mining

e4303c3

glozow force-pushed the 2022-03-miner-prioritised branch from fc292c4 to e4303c3 Compare March 14, 2022 16:04

DrahtBot mentioned this pull request Mar 19, 2022

deploymentstatus: move g_versionbitscache global to ChainstateManager #24595

Merged

DrahtBot mentioned this pull request Apr 29, 2022

refactor: Remove unused CTxMemPool::clear() helper #19909

Merged

maflcko merged commit b2e7811 into bitcoin:master May 6, 2022

glozow deleted the 2022-03-miner-prioritised branch May 8, 2022 19:33

bitcoin locked and limited conversation to collaborators May 8, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

miner: bug fix? update for ancestor inclusion using modified fees, not base #24538

miner: bug fix? update for ancestor inclusion using modified fees, not base #24538

Uh oh!

glozow commented Mar 11, 2022

Uh oh!

glozow commented Mar 11, 2022

Uh oh!

sdaftuar Mar 11, 2022

Uh oh!

maflcko commented Mar 11, 2022

Uh oh!

DrahtBot commented Mar 12, 2022 •

edited

Loading

Uh oh!

glozow commented Mar 14, 2022

Uh oh!

theStack commented Apr 20, 2022

Uh oh!

ccdle12 commented Apr 25, 2022

Uh oh!

maflcko commented May 6, 2022

Uh oh!

maflcko commented May 6, 2022

Uh oh!

Uh oh!

miner: bug fix? update for ancestor inclusion using modified fees, not base #24538

miner: bug fix? update for ancestor inclusion using modified fees, not base #24538

Uh oh!

Conversation

glozow commented Mar 11, 2022

Uh oh!

glozow commented Mar 11, 2022

Uh oh!

sdaftuar Mar 11, 2022

Choose a reason for hiding this comment

Uh oh!

maflcko commented Mar 11, 2022

Uh oh!

DrahtBot commented Mar 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

glozow commented Mar 14, 2022

Uh oh!

theStack commented Apr 20, 2022

Uh oh!

ccdle12 commented Apr 25, 2022

Uh oh!

maflcko commented May 6, 2022

Uh oh!

maflcko commented May 6, 2022

Uh oh!

Uh oh!

DrahtBot commented Mar 12, 2022 •

edited

Loading