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

glozow commented at 4:55 pm on March 11, 2022: member

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 added the label Mining on Mar 11, 2022

glozow commented at 4:55 pm on March 11, 2022: member

cc @jamesob

in src/node/miner.h:119 in 1177b7965f outdated

115@@ -116,7 +116,7 @@ struct update_for_parent_inclusion
116 
117     void operator() (CTxMemPoolModifiedEntry &e)
118     {
119-        e.nModFeesWithAncestors -= iter->GetFee();
120+        e.nModFeesWithAncestors -= iter->GetModifiedFee();

sdaftuar commented at 4:57 pm on March 11, 2022:

Oops. Good catch!

MarcoFalke commented at 4:59 pm on March 11, 2022: member

Concept ACK

DrahtBot commented at 9:33 pm on March 12, 2022: member

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.

MOVEONLY: group miner tests into MinerTestingSetup functions

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

0f9a44461c

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

[unit test] prioritisation in mining e4303c337c

glozow force-pushed on Mar 14, 2022

glozow commented at 4:04 pm on March 14, 2022: member

Rebased for #24080.

theStack commented at 6:11 pm on April 20, 2022: member

Concept ACK

ccdle12 commented at 9:56 am on April 25, 2022: contributor

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

MarcoFalke commented at 9:04 am on May 6, 2022: member

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

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

MarcoFalke commented at 9:05 am on May 6, 2022: member

ACK e4303c337c8423f21c2c72ee1bcca3aaf46fa1cb 🚗

Signature:

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3ACK e4303c337c8423f21c2c72ee1bcca3aaf46fa1cb 🚗
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
 7pUhmJAwAuY9dx5XOXwABGKOcz5KDvk2LekoeYMJgUe9H0Lql2sEi/BcZcU95c3HN
 8f3yRTEHWneiqhUz4jNcks+wXTM2e93wSTGZc0AxF20B2VUlnks1MD5EC1MQK2Bzc
 9Ip+yh43QZck21xoyyyOnBjiVO0gk8a+AAjSxHRmGB5rni2dEhSFX/nZef9fEGZq2
10Cjp65fd8V6JOHiMIdHrscp228jm5K8Dsa/c6ucqGZuab8kl9vFj/GpV4FrisIlFo
117t6fJv9fSb6MWILwr3C7Hu8hDZr2Dhd13O1T2afhoBxD9W2sfLQcmtTuQvjZl5qt
12St5PtWeztYcaMKWUlBcFHjsbdatTFYosA2qFiCHRUPGwf/UBeEGZlz0XlqvzRD9X
13nMj+/oSGKSStTSszhIyIk7DHb4ZhRmKkq7n32GBSZq4qGXmmfy8S00LwQO1I8OOK
14PhISXSJ3aj0O4w7KrgmrwrnP8do257J4NCk1v9NHFtEmxBN8p96t1rPqYg+bBwdm
15xhL7ZPDe
16=zM+y
17-----END PGP SIGNATURE-----

MarcoFalke merged this on May 6, 2022

MarcoFalke closed this on May 6, 2022

glozow deleted the branch on May 8, 2022

sidhujag referenced this in commit c8d2687323 on May 9, 2022

Fabcien referenced this in commit be44aca7ae on Jan 6, 2023

DrahtBot locked this on May 8, 2023

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

Conflicts