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

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.

cc @jamesob

Concept ACK

<!--e57a25ab6845829454e8d69fc972939a-->

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

<!--174a7506f384e20aa4161008e828411d-->

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.

Rebased for #24080.

Concept ACK

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

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);
     }

ACK e4303c337c8423f21c2c72ee1bcca3aaf46fa1cb 🚗

<details><summary>Show signature</summary>

Signature:

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

ACK e4303c337c8423f21c2c72ee1bcca3aaf46fa1cb 🚗
-----BEGIN PGP SIGNATURE-----

iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
pUhmJAwAuY9dx5XOXwABGKOcz5KDvk2LekoeYMJgUe9H0Lql2sEi/BcZcU95c3HN
f3yRTEHWneiqhUz4jNcks+wXTM2e93wSTGZc0AxF20B2VUlnks1MD5EC1MQK2Bzc
Ip+yh43QZck21xoyyyOnBjiVO0gk8a+AAjSxHRmGB5rni2dEhSFX/nZef9fEGZq2
Cjp65fd8V6JOHiMIdHrscp228jm5K8Dsa/c6ucqGZuab8kl9vFj/GpV4FrisIlFo
7t6fJv9fSb6MWILwr3C7Hu8hDZr2Dhd13O1T2afhoBxD9W2sfLQcmtTuQvjZl5qt
St5PtWeztYcaMKWUlBcFHjsbdatTFYosA2qFiCHRUPGwf/UBeEGZlz0XlqvzRD9X
nMj+/oSGKSStTSszhIyIk7DHb4ZhRmKkq7n32GBSZq4qGXmmfy8S00LwQO1I8OOK
PhISXSJ3aj0O4w7KrgmrwrnP8do257J4NCk1v9NHFtEmxBN8p96t1rPqYg+bBwdm
xhL7ZPDe
=zM+y
-----END PGP SIGNATURE-----

</details>