…valuation

32c1dd1 [test] mempool coins disappearing mid-package evaluation (glozow)
a67f460 [refactor] split setup in mempool_limit test (glozow)
d086961 [test framework] add ability to spend only confirmed utxos (glozow)
3ea71fe [validation] don't LimitMempoolSize in any subpackage submissions (glozow)
d227b72 [validation] return correct result when already-in-mempool tx gets evicted (glozow)
9698b81 [refactor] back-fill results in AcceptPackage (glozow)
8ad7ad3 [validation] make PackageMempoolAcceptResult members mutable (glozow)
03b87c1 [validation] add AcceptSubPackage to delegate Accept* calls and clean up m_view (glozow)
3f01a3d [CCoinsViewMemPool] track non-base coins and allow Reset (glozow)
7d7f7a1 [policy] check for duplicate txids in package (glozow)

Pull request description:

  While we are evaluating a package, we split it into "subpackages" for evaluation (currently subpackages all have size 1 except the last one). If a subpackage has size 1, we may add a tx to mempool and call `LimitMempoolSize()`, which evicts transactions if the mempool gets full. We handle the case where the just-submitted transaction is evicted immediately, but we don't handle the case in which a transaction from a previous subpackage (either just submitted or already in mempool) is evicted. Mainly, since the coins created by the evicted transaction are cached in `m_view`, we don't realize the UTXO has disappeared until `CheckInputsFromMempoolAndCache` asserts that they exist. Also, the returned `PackageMempoolAcceptResult` reports that the transaction is in mempool even though it isn't anymore.

  Fix this by not calling `LimitMempoolSize()` until the very end, and editing the results map with "mempool full" if things fall out.

  Pointed out by instagibbs in bitcoin@faeed68 on top of the v3 PR.

ACKs for top commit:
  instagibbs:
    reACK bitcoin@32c1dd1

Tree-SHA512: 61e7f69db4712e5e5bfa27d037ab66bdd97f1bf60a8d9ffb96adb1f0609af012c810d681102ee5c7baec7b5fe8cb7c304a60c63ccc445d00d86a2b7f0e7ddb90

262ab8e Add package evaluation fuzzer (Greg Sanders)

Pull request description:

  This fuzzer target caught the issue in bitcoin/bitcoin#28251 within 5 minutes on master branch, and an additional issue which I've applied a preliminary patch to cover.

  Fuzzer target does the following:

  1) Picks mempool confgs, including max package size, count, mempool size, etc
  2) Generates 1 to 26 transactions with arbitrary coins/fees, the first N-1 spending only confirmed outpoints
  3) Nth transaction, if >1, sweeps all unconfirmed outpoints in mempool
  4) If N==1, it may submit it through single-tx submission path, to allow for more interesting topologies
  5) Otherwise submits through package submission interface
  6) Repeat 1-5  a few hundred times per mempool instance

  In other words, it ends up building chains of txns in the mempool using parents-and-children packages, which is currently the topology supported on master.

  The test itself is a direct rip of tx_pool.cpp, with a number of assertions removed because they were failing for unknown reasons, likely due to the notification changes of single tx submission to package, which is used to track addition/removal of transactions in the test. I'll continue working on re-adding these assertions for further invariant testing.

ACKs for top commit:
  murchandamus:
    ACK 262ab8e
  glozow:
    reACK 262ab8e
  dergoegge:
    tACK 262ab8e

Tree-SHA512: 190784777d0f2361b051b3271db8f79b7927e3cab88596d2c30e556da721510bd17f6cc96f6bb03403bbf0589ad3f799fa54e63c1b2bd92a2084485b5e3e96a5

262ab8e Add package evaluation fuzzer (Greg Sanders)

Pull request description:

  This fuzzer target caught the issue in bitcoin#28251 within 5 minutes on master branch, and an additional issue which I've applied a preliminary patch to cover.

  Fuzzer target does the following:

  1) Picks mempool confgs, including max package size, count, mempool size, etc
  2) Generates 1 to 26 transactions with arbitrary coins/fees, the first N-1 spending only confirmed outpoints
  3) Nth transaction, if >1, sweeps all unconfirmed outpoints in mempool
  4) If N==1, it may submit it through single-tx submission path, to allow for more interesting topologies
  5) Otherwise submits through package submission interface
  6) Repeat 1-5  a few hundred times per mempool instance

  In other words, it ends up building chains of txns in the mempool using parents-and-children packages, which is currently the topology supported on master.

  The test itself is a direct rip of tx_pool.cpp, with a number of assertions removed because they were failing for unknown reasons, likely due to the notification changes of single tx submission to package, which is used to track addition/removal of transactions in the test. I'll continue working on re-adding these assertions for further invariant testing.

ACKs for top commit:
  murchandamus:
    ACK 262ab8e
  glozow:
    reACK 262ab8e
  dergoegge:
    tACK 262ab8e

Tree-SHA512: 190784777d0f2361b051b3271db8f79b7927e3cab88596d2c30e556da721510bd17f6cc96f6bb03403bbf0589ad3f799fa54e63c1b2bd92a2084485b5e3e96a5

Summary:
```
While we are evaluating a package, we split it into "subpackages" for evaluation (currently subpackages all have size 1 except the last one). If a subpackage has size 1, we may add a tx to mempool and call LimitMempoolSize(), which evicts transactions if the mempool gets full. We handle the case where the just-submitted transaction is evicted immediately, but we don't handle the case in which a transaction from a previous subpackage (either just submitted or already in mempool) is evicted. Mainly, since the coins created by the evicted transaction are cached in m_view, we don't realize the UTXO has disappeared until CheckInputsFromMempoolAndCache asserts that they exist. Also, the returned PackageMempoolAcceptResult reports that the transaction is in mempool even though it isn't anymore.

Fix this by not calling LimitMempoolSize() until the very end, and editing the results map with "mempool full" if things fall out.
```

Of course the replacement case doesn't apply to our codebase, but the mempool trimming does.

Backport of [[bitcoin/bitcoin#28251 | core#28251]] and [[bitcoin/bitcoin#27127 | core#27127]].
Includes a commit from [[bitcoin/bitcoin#26657 | core#26657]]:
bitcoin/bitcoin@d0a909a

Depends on D16443 and D16444.

Test Plan:
  ninja all check-all

Reviewers: #bitcoin_abc, PiRK

Reviewed By: #bitcoin_abc, PiRK

Subscribers: PiRK

Differential Revision: https://reviews.bitcoinabc.org/D16447

Summary:
```
While we are evaluating a package, we split it into "subpackages" for evaluation (currently subpackages all have size 1 except the last one). If a subpackage has size 1, we may add a tx to mempool and call LimitMempoolSize(), which evicts transactions if the mempool gets full. We handle the case where the just-submitted transaction is evicted immediately, but we don't handle the case in which a transaction from a previous subpackage (either just submitted or already in mempool) is evicted. Mainly, since the coins created by the evicted transaction are cached in m_view, we don't realize the UTXO has disappeared until CheckInputsFromMempoolAndCache asserts that they exist. Also, the returned PackageMempoolAcceptResult reports that the transaction is in mempool even though it isn't anymore.

Fix this by not calling LimitMempoolSize() until the very end, and editing the results map with "mempool full" if things fall out.
```

Of course the replacement case doesn't apply to our codebase, but the mempool trimming does.

Backport of [[bitcoin/bitcoin#28251 | core#28251]] and [[bitcoin/bitcoin#27127 | core#27127]].
Includes a commit from [[bitcoin/bitcoin#26657 | core#26657]]:
bitcoin/bitcoin@d0a909a

Depends on D16443 and D16444.

Test Plan:
  ninja all check-all

Reviewers: #bitcoin_abc, PiRK

Reviewed By: #bitcoin_abc, PiRK

Subscribers: PiRK

Differential Revision: https://reviews.bitcoinabc.org/D16447

This test was introduced in bitcoin#28251 to ensure that (1) the mempool is not
trimmed in the middle of a package evaluation and (2) the m_view cache
is updated when evictions and replacements happen so coins are no longer
visible in subsequent package transactions.

(1) is prevented in multiple ways: TrimToSize() cannot be called while a
changeset is outstanding.
(2) also has coverage through test_mid_package_replacement.

This test is also brittle: it 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.

This test was introduced in bitcoin#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.

This test was introduced in bitcoin#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.

This test was introduced in bitcoin#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.

…iews

b6d4688 [doc] reword comments in test_mid_package_replacement (glozow)
f3a613a [cleanup] delete brittle test_mid_package_eviction (glozow)
c3cd7fc [doc] remove references to now-nonexistent Finalize() function (glozow)
d8140f5 don't make a copy of m_non_base_coins (glozow)
98ba2b1 [doc] MemPoolAccept coins views (glozow)
ba02c30 [doc] always CleanupTemporaryCoins after a mempool trim (glozow)

Pull request description:

  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):
  ```
  diff --git a/src/validation.cpp b/src/validation.cpp
  index f2f6098..4bd6f059849 100644
  --- a/src/validation.cpp
  +++ b/src/validation.cpp
  @@ -1485,7 +1485,7 @@ MempoolAcceptResult MemPoolAccept::AcceptSingleTransaction(const CTransactionRef
       FinalizeSubpackage(args);

       // Limit the mempool, if appropriate.
  -    if (!args.m_package_submission && !args.m_bypass_limits) {
  +    if (!args.m_bypass_limits) {
           LimitMempoolSize(m_pool, m_active_chainstate.CoinsTip());
           // If mempool contents change, then the m_view cache is dirty. Given this isn't a package
           // 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`:
  ```
  diff --git a/src/validation.cpp b/src/validation.cpp
  index f2f6098..01b904b69ef 100644
  --- a/src/validation.cpp
  +++ b/src/validation.cpp
  @@ -779,7 +779,7 @@ private:
           m_subpackage = SubPackageState{};

           // And clean coins while at it
  -        CleanupTemporaryCoins();
  +        // CleanupTemporaryCoins();
       }
   };
  ```
  I also added/cleaned up the documentation about coins views to hopefully make it extremely clear when people should `CleanupTemporaryCoins`.

ACKs for top commit:
  instagibbs:
    reACK b6d4688
  sdaftuar:
    utACK b6d4688
  marcofleon:
    ACK b6d4688

Tree-SHA512: 79c68e263013b1153520f5453e6b579b8fe7e1d6a9952b1ac2c3c3c017034e6d21d7000a140bba4cc9d2ce50ea3a84cc6f91fd5febc52d7b3fa4f797955d987d

…valuation

32c1dd1 [test] mempool coins disappearing mid-package evaluation (glozow)
a67f460 [refactor] split setup in mempool_limit test (glozow)
d086961 [test framework] add ability to spend only confirmed utxos (glozow)
3ea71fe [validation] don't LimitMempoolSize in any subpackage submissions (glozow)
d227b72 [validation] return correct result when already-in-mempool tx gets evicted (glozow)
9698b81 [refactor] back-fill results in AcceptPackage (glozow)
8ad7ad3 [validation] make PackageMempoolAcceptResult members mutable (glozow)
03b87c1 [validation] add AcceptSubPackage to delegate Accept* calls and clean up m_view (glozow)
3f01a3d [CCoinsViewMemPool] track non-base coins and allow Reset (glozow)
7d7f7a1 [policy] check for duplicate txids in package (glozow)

Pull request description:

  While we are evaluating a package, we split it into "subpackages" for evaluation (currently subpackages all have size 1 except the last one). If a subpackage has size 1, we may add a tx to mempool and call `LimitMempoolSize()`, which evicts transactions if the mempool gets full. We handle the case where the just-submitted transaction is evicted immediately, but we don't handle the case in which a transaction from a previous subpackage (either just submitted or already in mempool) is evicted. Mainly, since the coins created by the evicted transaction are cached in `m_view`, we don't realize the UTXO has disappeared until `CheckInputsFromMempoolAndCache` asserts that they exist. Also, the returned `PackageMempoolAcceptResult` reports that the transaction is in mempool even though it isn't anymore.

  Fix this by not calling `LimitMempoolSize()` until the very end, and editing the results map with "mempool full" if things fall out.

  Pointed out by instagibbs in bitcoin@faeed68 on top of the v3 PR.

ACKs for top commit:
  instagibbs:
    reACK bitcoin@32c1dd1

Tree-SHA512: 61e7f69db4712e5e5bfa27d037ab66bdd97f1bf60a8d9ffb96adb1f0609af012c810d681102ee5c7baec7b5fe8cb7c304a60c63ccc445d00d86a2b7f0e7ddb90