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

Reviews

See the guideline for information on the review process.

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:

• #28453 (wallet: Receive silent payment transactions by achow101)
• #28450 (WIP: Add package evaluation fuzzer by instagibbs)
• #28368 (Fee Estimator updates from Validation Interface/CScheduler thread by ismaelsadeeq)

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.

cc @instagibbs

@sipa you might want to weigh in as well

Pushed a test - it should fail on master with the assertion.

Generally, the conditions to hit the case are:

• An almost-full mempool.
• mempool_evicted_tx (parent) A to-be-evicted tx that is submitted ahead of time. Has a very low feerate and is evicted immediately when mempool overflows. It has an output coin_to_disappear
• A package consisting of:
  • cpfp_parent (child_a) parent that spends coin_to_disappear and fails a check that requires its input to be loaded and makes it eligible for reconsideration (e.g. below mempool minimum feerate).
  • high-feerate parent (child_b), high feerate so that they are submitted individually and trigger eviction.
    • I've used 6 high-feerate parents in this test to make it easy to trigger overflow.
    • In the EA test, child_b triggers eviction due to proactive trimming of the sponsorless parent instead of mempool overflow.
  • package_child (child_c) spending the above parents.
    • The child must pay a fee high enough to CPFP the low-feerate parent.
  • Additional requirements:
    • The low-feerate parent needs to be checked before the higher-feerate parents so that its inputs are loaded before the eviction happens. Otherwise it will just hit missing-inputs. This also means that as a side effect of the fee-based linearization in validate package transactions with their in-package ancestor sets #26711 (which would put the low-feerate parent at the end), this test won't trigger. But I don't think the linearization is an adequate solution to this problem.
    • To make the test interesting, it should be low enough that the to-be-evicted tx is evicted regardless of when the eviction happens.

Timeline of what happens:

1. mempool is almost full
2. mempool_evicted_tx is submitted to mempool
3. AcceptPackage(package):
   1. AcceptSingleTransaction(cpfp_parent). This loads coin_to_disappear into m_view . After the fee is checked, the tx fails and we continue onto the next transactions.
   2. for each of the n high-feerate parents, AcceptSingleTransaction(tx). These succeed and, in Finalize(), the transactions are submitted.
      1. (Without PR's changes) this also calls LimitMempool() which evicts by descendant score until the mempool is within size limits again.
      2. This kicks out mempool_evicted_tx, which means the m_viewmempool will no longer have a coin for coin_to_disappear. However, the coin has already been cached in m_view and it doesn't know this.
   3. AcceptSingleTransaction(package_child) fails due to missing inputs.
   4. AcceptMultipleTransactions([cpfp_parent, package_child])
      1. PreChecks looks for the inputs of each transaction, including the now-nonexistent coin_to_disappear. This succeeds because m_view has a coin for it.
      2. The CPFP works, so both transactions are submitted in SubmitPackage() , which calls ConsensusScriptChecks() which calls CheckInputsFromMempoolAndCache() which asserts that the coins all exist.
         1. We can't find the coin in the mempool (which is where it used to be).
         2. So we assume we will find it in the UTXO set. We call AccessCoin() and assert !coinFromUTXOSet.IsSpent(). This fails because the coin does not exist (which is the same as isspent).

the above test description sounds right. The eldest ancestor gets trimmed due to size limitations, then things go wonky without this change

Updated the approach + added a test for replacement case.

There are now 2 fixes:
(1) Clear any non-chainstate coins at the end of AcceptSubPackage so they can't be used in a different AcceptSubPackage.
(2) Don't trim in the middle of package evaluation.

Technically (1) is sufficient, but I think (2) is also beneficial as it avoids changing the mempool minimum feerate (which affects how we group transactions) mid-package evaluation. It would be sad if a CPFP package contains a parent that's just barely above mempool minimum feerate, so we submit it, then evict it (due to later parent), then can't submit the child... when we could have just submitted all of them and evicted something else.

reACK 32c1dd1

inclusion of 7d7f7a1 duplicate-txid check in CheckPackage which prevents the assertion being hit for the 2nd of the duplicate txids

I'm reviewing this PR more carefully, after seeing @instagibbs' comment here: #31122 (comment)

As I understand it, I think there are two key behaviors introduced in this PR:

1. Clearing state between subpackage evaluations (specifically, clearing the coins in m_viewmempool)
2. Deferring mempool limiting until the end of package validation

However, the specific issue that @glozow described above (#28251 (comment)), where we need to ensure that package validation fails if an input is evicted mid-validation, seems to be completely mitigated by behavior (1). And this behavior is tested in the mempool_limit test introduced in this PR.

As far as I can tell from reviewing 3ea71fe (#28251), behavior (2) is designed to allow package validation to succeed in a case where we have, say, a child C with two parents A and B, and we submit a package [A, B, C] and potentially trigger this outcome:

• A is accepted singly
• B is accepted singly, but the mempool overflows and A is trimmed
• C is rejected for missing inputs

So by preventing mempool limiting from taking place mid-package, the choice of what to evict from the mempool is deferred until after C is able to make it in, potentially changing the lowest-descendant-score transaction in the mempool from A to something else (as A's descendant score might be pretty high after C is accepted).

However, I don't think this PR introduced any test for this second behavior, of ensuring success is possible even if an earlier parent might be trimmed mid-validation.

Does this summary sound right to others? Have I overlooked other behavior changes/bugfixes that were introduced here?

As I understand it, I think there are two key behaviors introduced in this PR

However, the specific issue that @glozow described above (#28251 (comment)), where we need to ensure that package validation fails if an input is evicted mid-validation, seems to be completely mitigated by behavior (1).

As far as I can tell from reviewing 3ea71fe (#28251), behavior (2) is designed to allow package validation to succeed in a case where...

However, I don't think this PR introduced any test for this second behavior,

This looks accurate to me. I've looked a little at the tests (it's been a while 😅) and can't really point to one for deferring evictions. Will review #31152.