p2p: Reconsider package transactions that are immediately trimmed #36328

1. 
   instagibbs commented at 4:26 PM on September 24, 2026: member

   In-master, if a package is entered into mempool at the lowerst chunk feerate, pushing the mempool oversize, this causes that transaction (package) to be trimmed immediately. This is fine and expected, but the returned validation result TX_MEMPOOL_POLICY precludes the now-evicted parent from being considered again (i.e. fetched / processed as package) until after another block is found.

   This is not a major issue, but would result in unneeded censorship of someone low-balling package feerates.

   The already-in-mempool case needs a bit more logic due to the FeeFailure's requirement of package feerate knowledge, so I opted to defer it for now.

   Related to finding made by Loupe, and subsequently Red Team.

2. validation: make package transactions trimmed after submission reconsiderable

   A transaction evicted by LimitMempoolSize() right after being accepted
   by itself is TX_RECONSIDERABLE: a package with a higher feerate may
   still get it in. Transactions submitted together as a package and
   evicted by the final trim in AcceptPackage() were TX_MEMPOOL_POLICY
   instead, as that code predates TX_RECONSIDERABLE.
   
   For 1p1c relay this put an evicted parent and child in the reject
   filter until the next block, so a new child could not bump the parent
   back in.

   47c9070b9d
3. test: 1p1c package evicted after submission can be bumped by a new child before a new block 7235f02718
4. 
   DrahtBot commented at 4:26 PM on September 24, 2026: contributor

   <!--e57a25ab6845829454e8d69fc972939a-->

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

   <!--006a51241073e994b41acfe9ec718e94-->

   External sites

   • Code coverage & benchmarks: https://corecheck.dev/bitcoin/bitcoin/pulls/36328
   • Ralph review <img src=https://review.fish.foo/assets/bitcoin/bitcoin/36328.png alt="Ralph review status"> https://git.fish.foo/bitcoin/bitcoin/pulls/36328#author-ralph

   <!--021abf342d371248e50ceaed478a90ca-->

   Reviews

   See the guideline and AI policy for information on the review process.

   Type	Reviewers
   ACK	mzumsande, ismaelsadeeq

   If your review is incorrectly listed, please copy-paste <code>&lt;!--meta-tag:bot-skip--&gt;</code> into the comment that the bot should ignore.

   <!--174a7506f384e20aa4161008e828411d-->

   Conflicts

   No conflicts as of last run.

   <!--5faf32d7da4f0f540f40219e4f7537a3-->

5. instagibbs force-pushed on Sep 24, 2026
6. DrahtBot added the label CI failed on Sep 24, 2026
7. 
   DrahtBot commented at 4:40 PM on September 24, 2026: contributor

   <!--85328a0da195eb286784d51f73fa0af9-->

   🚧 At least one of the CI tasks failed. _{Task ASan + LSan + UBSan + integer: https://github.com/bitcoin/bitcoin/actions/runs/36027432164/job/107727417441} _{LLM reason (✨ experimental): CI failed to compile test_bitcoin because Clang -Wthread-safety (treated as -Werror) reported an exclusive cs_main lock is missing when calling ProcessNewPackage in txpackage_tests.cpp.}

   <details><summary>Hints</summary>

   Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

   • Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.

   • A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.

   • An intermittent issue.

   Leave a comment here, if you need help tracking down a confusing failure.

   </details>

8. DrahtBot removed the label CI failed on Sep 24, 2026
9. 
   instagibbs commented at 4:12 PM on September 28, 2026: member

   This PR closes out R10: https://gist.github.com/instagibbs/880a76535fd802fced9f9a5253cdbb40

10. instagibbs renamed this:
    Reconsider package transactions that are immediately trimmed
    p2p: Reconsider package transactions that are immediately trimmed
    on Sep 28, 2026
11. DrahtBot added the label P2P on Sep 28, 2026
12. fanquake added this to the milestone 33.0 on Sep 29, 2026
13. 
    mzumsande commented at 5:37 PM on November 4, 2026: contributor

    Concept ACK Seems to me that the first part is much more important because it could be encountered via p2p 1p1c. The second part seems less urgent, and I think it could wait until after removing the fee information from FeeFailure, as suggested in the OP. As far as I understand, the second part would mostly be encountered via rpc anyway, where it makes no observable difference (same error string). By delaying it we wouldn't need to introduce deduplicated_feerates.

14. test: package transactions evicted by the final trim are reconsiderable f19e4bfc9b
15. instagibbs force-pushed on Nov 4, 2026
16. 
    instagibbs commented at 6:45 PM on November 4, 2026: member

    @mzumsande split it up, thanks

17. 
    mzumsande commented at 12:11 PM on November 6, 2026: contributor

    Code Review ACK f19e4bfc9be59d32502215e9624a5a18600e26f6

18. sedited requested review from ismaelsadeeq on Nov 6, 2026
19. in src/test/txpackage_tests.cpp:1277 in f19e4bfc9b

    1272 | +    for (const auto& tx : package) {
    1273 | +        const auto& tx_result{result.m_tx_results.at(tx->GetWitnessHash())};
    1274 | +        BOOST_CHECK_EQUAL(tx_result.m_state.GetResult(), TxValidationResult::TX_RECONSIDERABLE);
    1275 | +        BOOST_CHECK_EQUAL(tx_result.m_state.GetRejectReason(), "mempool full");
    1276 | +        BOOST_CHECK(tx_result.m_effective_feerate == package_feerate);
    1277 | +        BOOST_CHECK(tx_result.m_wtxids_fee_calculations == std::vector<Wtxid>({tx_parent->GetWitnessHash(), tx_child->GetWitnessHash()}));
    

    ---

    ---

    

    ismaelsadeeq commented at 12:42 PM on November 9, 2026:

    nit: can be boost check equal

20. 
    ismaelsadeeq commented at 1:04 PM on November 9, 2026: member

    ACK f19e4bfc9be59d32502215e9624a5a18600e26f6

    I also went through the other TX_MEMPOOL_POLICY reasons. The "max feerate exceeded" one is a local only guard that can't be triggered over p2p, others are legitimate except one left deliberately by the OP. For that, I think a user who hits it can workaround the rejection by e.g. running with a larger -maxmempool, or starting from an empty mempool and not accepting incoming txs (-blocksonly). larger -maxmempool is optimistic but better because a blocksonly node relays only its own transactions and accepts none from peers, so it could be a source leak. This should be fixed too.

Contributors

instagibbsDrahtBotmzumsandeismaelsadeeq

Milestone
33.0