validation: docs and cleanups for MemPoolAccept coins views #32973

pull glozow wants to merge 6 commits into bitcoin:master from glozow:2025-07-delete-midpkg-test changing 3 files +33 −105
  1. glozow commented at 7:39 pm on July 14, 2025: member

    Deletes test_mid_package_eviction that is brittle and already covered in other places. It was introduced in #28251 addressing 2 issues: (1) calling LimitMempoolSize() in the middle of package validation and (2) not updating coins view cache when the mempool contents change, leading to “disappearing coins.”

    (1) If you let AcceptSingleTransaction call LimitMempoolSize in the middle of package validation, you should get a failure in test_mid_package_eviction_success (the package is rejected):

     0diff --git a/src/validation.cpp b/src/validation.cpp
     1index f2f6098e214..4bd6f059849 100644
     2--- a/src/validation.cpp
     3+++ b/src/validation.cpp
     4@@ -1485,7 +1485,7 @@ MempoolAcceptResult MemPoolAccept::AcceptSingleTransaction(const CTransactionRef
     5     FinalizeSubpackage(args);
     6 
     7     // Limit the mempool, if appropriate.
     8-    if (!args.m_package_submission && !args.m_bypass_limits) {
     9+    if (!args.m_bypass_limits) {
    10         LimitMempoolSize(m_pool, m_active_chainstate.CoinsTip());
    11         // If mempool contents change, then the m_view cache is dirty. Given this isn't a package
    12         // submission, we won't be using the cache anymore, but clear it anyway for clarity.
    

    Mempool modifications have a pretty narrow interface since #31122 and TrimToSize() cannot be called while there is an outstanding mempool changeset. So I think there is a low likelihood of accidentally reintroducing this problem and not immediately hitting e.g. a fuzzer crash on this line https://github.com/bitcoin/bitcoin/blob/b53fab1467fde73c40402e2022b25edfff1e4668/src/txmempool.cpp#L1143

    (2) If you remove the CleanupTemporaryCoins() call from ClearSubPackageState() you should get a failure from test_mid_package_replacement:

     0diff --git a/src/validation.cpp b/src/validation.cpp
     1index f2f6098e214..01b904b69ef 100644
     2--- a/src/validation.cpp
     3+++ b/src/validation.cpp
     4@@ -779,7 +779,7 @@ private:
     5         m_subpackage = SubPackageState{};
     6 
     7         // And clean coins while at it
     8-        CleanupTemporaryCoins();
     9+        // CleanupTemporaryCoins();
    10     }
    11 };
    

    I also added/cleaned up the documentation about coins views to hopefully make it extremely clear when people should CleanupTemporaryCoins.

  2. [doc] always CleanupTemporaryCoins after a mempool trim ba02c30b8a
  3. [doc] MemPoolAccept coins views 98ba2b1db2
  4. don't make a copy of m_non_base_coins d8140f5f05
  5. [doc] remove references to now-nonexistent Finalize() function c3cd7fcb2c
  6. glozow added the label Docs on Jul 14, 2025
  7. DrahtBot commented at 7:39 pm on July 14, 2025: contributor

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

    Code Coverage & Benchmarks

    For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32973.

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    ACK instagibbs, marcofleon, sdaftuar

    If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #28676 ([WIP] Cluster mempool implementation by sdaftuar)

    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.

  8. instagibbs commented at 9:29 pm on July 15, 2025: member

    in op and commit message: test_mid_package_evaluation doesn’t exist, it’s named test_mid_package_eviction

    The bull case for keeping test_mid_package_replacement is that it involves an RBF, not a mempool overflow, right? The comment in that test seems slightly erroneous, mentioning replaced_tx needs to be at the bottom of the mempool, when I think it just has to be low feerate enough to be RBF’d?

  9. in src/validation.cpp:730 in 98ba2b1db2 outdated
    724@@ -725,8 +725,27 @@ class MemPoolAccept
    725 
    726 private:
    727     CTxMemPool& m_pool;
    728+
    729+    /** Holds a cached view of available coins from the UTXO set, mempool, and artificial temporary coins (to enable package validation).
    730+     * The view doesn't track whether a coin previously existed but has now been spent. We detect conflicts in other ways:
    


    instagibbs commented at 1:14 pm on July 16, 2025:

    The view doesn’t track whether a coin previously existed but has now been spent

    been spent in mempool/package contexts, I presume


    glozow commented at 5:26 pm on July 16, 2025:
    Yes. Should I reword?

    instagibbs commented at 6:59 pm on July 16, 2025:
    s/has now been spent/has now been spent in the mempool/ ?

    glozow commented at 1:06 pm on July 17, 2025:
    Will change if I need to retouch
  10. instagibbs approved
  11. instagibbs commented at 1:31 pm on July 16, 2025: member

    ACK b5805eddefced4cd8bb6f8069ea0a4bf199bacff

    confirmed that the remaining tests cases fail when specific key clauses are modified, that it seems implausible that the test adds much assurances, and honestly is not a particularly maintainable test. The new comments are helpful for me to trace the code in a directed way and confirm what it’s saying.

  12. [cleanup] delete brittle test_mid_package_eviction
    This test was introduced in #28251 to ensure that the mempool is not
    trimmed in the middle of a package evaluation and the m_view cache
    is updated when evictions and replacements happen so coins are no longer
    visible in subsequent package transactions. These two things have
    coverage in other tests as well, and are pretty unlikely to happen.
    
    This test is also brittle: it requires evaluation of the parents in a
    particular order, and creates a transaction that itself is not
    enough to trigger eviction but will be pushed out immediately by the
    package spending from it. While the current magic number 2000 works, we
    do not have a way to query remaining space in the mempool if mempool
    data structures change, and it can differ across platforms.
    f3a613aa5b
  13. [doc] reword comments in test_mid_package_replacement
    The comment about eviction seems to be erroneously copy-pasted. Reword
    another comment for clarity.
    b6d4688f77
  14. glozow force-pushed on Jul 16, 2025
  15. glozow commented at 5:31 pm on July 16, 2025: member

    op and commit message: test_mid_package_evaluation doesn’t exist, it’s named test_mid_package_eviction

    Fixed

    The bull case for keeping test_mid_package_replacement is that it involves an RBF, not a mempool overflow, right? The comment in that test seems slightly erroneous, mentioning replaced_tx needs to be at the bottom of the mempool, when I think it just has to be low feerate enough to be RBF’d?

    Thanks, yeah, I think it had an incorrect copy-paste from the other test. I think this one is good to keep for when we’re working on generalized package validation.

  16. marcofleon commented at 12:53 pm on July 17, 2025: contributor

    ACK b6d4688f77df9e31fd64e2be300f55bb8e944bd0

    Verified that the two functional tests fail with the suggested buggy patches. Seems fine to remove this test if it doesn’t provide any new assurances.

    I think there is a low likelihood of accidentally reintroducing this problem and not immediately hitting e.g. a fuzzer crash on this line

    I also tried to see if we’d get a fuzz crash after applying the first patch and it doesn’t seem to be happening. I’m running tx_package_eval with a small mempool in order to trigger the assertion in TrimToSize(). I also commented out the added CleanupTemporaryCoins() and still nothing. Fwiw if I add LimitMempoolSize() right before ClearSubPackageState() in AcceptSubPackage there is an almost immediate crash.

    If I’m misunderstanding the package validation code or running the wrong target, let me know. I haven’t looked into this too thoroughly (e.g. checking coverage) but I plan on investigating some more later.

  17. glozow commented at 1:02 pm on July 17, 2025: member

    Fwiw if I add LimitMempoolSize() right before ClearSubPackageState() in AcceptSubPackage there is an almost immediate crash.

    That’s the kind of scenario I was thinking - somehow reintroducing a trim somewhere mid-subpackage.

  18. instagibbs commented at 1:55 pm on July 17, 2025: member

    @marcofleon

    This seed: ALoAAAAAAAABAAAAAAAAAAAAAAAAAZNpogAyAAAAAAAAgAAAAAAAAAAAAAEAAAAAAAAAAAB5AAAA AAAhAQAAAAEAAAAAAAAAAAAAAAABk2miADIAAAAAAACAAAAAAAAAAAAAAQAAAAAAAAAAAHkAAAAA ACEBAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAABwAAAGTaaIAMgAAAAAAAAEAAAAAAAAAAAABAAAA AAAAAAAAAAAAAAAAAAABAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAn////AZNpogAyAAAAAAAAAQAA AAAAAAAAAAEAAABAAAAAAAAAAAAAAAAAAAABAAAAAAAAAAAAAAAAAACNAAEAAAAAAAAAAAAAAAAA AAAAAQAAAAAAAAAAAAAAAAAAAAAAAABkYXRhZGlyYXRhAAAA

    Should cause failure if you apply this patch:

     0diff --git a/src/validation.cpp b/src/validation.cpp
     1index f2f6098e21..b1daee1f28 100644
     2--- a/src/validation.cpp
     3+++ b/src/validation.cpp
     4@@ -780,5 +780,5 @@ private:
     5 
     6         // And clean coins while at it
     7-        CleanupTemporaryCoins();
     8+        //CleanupTemporaryCoins();
     9     }
    10 };
    11@@ -1486,9 +1486,9 @@ MempoolAcceptResult MemPoolAccept::AcceptSingleTransaction(const CTransactionRef
    12 
    13     // Limit the mempool, if appropriate.
    14-    if (!args.m_package_submission && !args.m_bypass_limits) {
    15+    if (!args.m_bypass_limits) {
    16         LimitMempoolSize(m_pool, m_active_chainstate.CoinsTip());
    17         // If mempool contents change, then the m_view cache is dirty. Given this isn't a package
    18         // submission, we won't be using the cache anymore, but clear it anyway for clarity.
    19-        CleanupTemporaryCoins();
    20+        //CleanupTemporaryCoins();
    21 
    22         if (!m_pool.exists(ws.m_hash)) {
    

    No coin cleanup, and lets it call LimitMempoolSize in single tx subpackage case.

  19. marcofleon commented at 3:00 pm on July 18, 2025: contributor

    Thanks, was able to reproduce.

    0fuzz: ../../src/validation.cpp:428: bool CheckInputsFromMempoolAndCache(const CTransaction &, TxValidationState &, const CCoinsViewCache &, const CTxMemPool &, unsigned int, PrecomputedTransactionData &, CCoinsViewCache &, ValidationCache &): Assertion `!coinFromUTXOSet.IsSpent()' failed.
    

    Now the comment for m_view in 98ba2b1db2eb81e08b550ba0d5069a9289f30e13 makes more sense to me as well.

  20. sdaftuar commented at 8:27 pm on July 28, 2025: member
    utACK b6d4688f77df9e31fd64e2be300f55bb8e944bd0
  21. fanquake merged this on Jul 29, 2025
  22. fanquake closed this on Jul 29, 2025


github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2025-08-02 12:13 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me