fix CTxMemPool::TrimToSize to put only confirmed coins in pvNoSpendsRemaining

markblundeberg commented at 1:47 am on September 5, 2020: none

Only return confirmed (not from mempool) outpoints in pvNoSpendsRemaining, as is the intended behaviour.

With the previous code, removed chains of tx would have all internally-spent outpoints included in this vector, which is not the intended result. It seems to have been incorrectly coded from the very start (https://github.com/bitcoin/bitcoin/pull/6872) but the bug is benign as this result is only used to uncache coins.

As a bonus, no copying of CTransaction is needed now.

An existing test case is modified to test this behaviour (the modified test fails under prior behaviour).

(this is an upstreaming of https://gitlab.com/bitcoin-cash-node/bitcoin-cash-node/-/merge_requests/761 )

markblundeberg commented at 1:47 am on September 5, 2020: none

@TheBlueMatt can you confirm on the original intent here?

fanquake added the label Mempool on Sep 5, 2020

fix CTxMemPool::TrimToSize to put only confirmed coins in pvNoSpendsRemaining

Only return confirmed (not from mempool) outpoints in `pvNoSpendsRemaining`,
as is the intended behaviour.

With the previous code, removed chains of tx would have all internally-spent outpoints
included in this vector, which is not the intended result. It seems to have been
incorrectly coded from the very start (https://github.com/bitcoin/bitcoin/pull/6872)
but the bug is benign as this result is only used to uncache coins.

As a bonus, no copying of CTransaction is needed now.

An existing test case is modified to test this behaviour (the modified test fails
under prior behaviour).

374c5d3117

markblundeberg force-pushed on Sep 5, 2020

in src/txmempool.cpp:1061 in 374c5d3117

1066-        RemoveStaged(stage, false, MemPoolRemovalReason::SIZELIMIT);
1067-        if (pvNoSpendsRemaining) {
1068-            for (const CTransaction& tx : txn) {
1069-                for (const CTxIn& txin : tx.vin) {
1070-                    if (exists(txin.prevout.hash)) continue;
1071+            for (txiter iter : stage) {

epson121 commented at 5:15 pm on September 7, 2020:

If I understand this correctly, the change allows for removal of txn vector (for optimization), but behaviour should be the same (as I don’t see RemoveStaged having access to txn vector in the old code, thus not modifying it anywhere)?

markblundeberg commented at 7:52 am on September 11, 2020:

Not sure I understand your question… the behaviour is indeed altered since RemoveStaged removes stuff from mempool and thus alters the result of exists().

Oh, I guess also worth mentioning: the reason txn had to be created in the prior version is that the contents of stage become invalid after RemoveStaged. Now that RemoveStaged is the final step of the loop, this is no longer a concern so txn doesn’t need to exist now.

glozow commented at 3:40 pm on November 4, 2021:

Correct me if I’m wrong: an example you’re imagining is A and B are being removed from the mempool. B spends an output of A, but since we’ve already called RemoveStaged at this point, exists(A) will return false, and we incorrectly place that outpoint into pvNoSpendsRemaining. There is no behavior change because Uncache will just skip the outpoint if it’s not in the coins cache.

DrahtBot commented at 3:09 am on October 22, 2021: member

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#23325 (mempool: delete exists(uint256) function by glozow)

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.

DrahtBot commented at 11:31 am on October 22, 2021: member

🐙 This pull request conflicts with the target branch and needs rebase.

Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a “draft”.

DrahtBot added the label Needs rebase on Oct 22, 2021

in src/test/mempool_tests.cpp:474 in 374c5d3117

470+    std::vector<COutPoint> vNoSpendsRemaining;
471+    pool.TrimToSize(GetVirtualTransactionSize(CTransaction(tx1)), &vNoSpendsRemaining); // mempool is limited to tx1's size in memory usage, so nothing fits
472     BOOST_CHECK(!pool.exists(tx1.GetHash()));
473     BOOST_CHECK(!pool.exists(tx2.GetHash()));
474     BOOST_CHECK(!pool.exists(tx3.GetHash()));
475+    // This vector should only contain 'root' (not unconfirmed) outpoints

glozow commented at 3:41 pm on November 4, 2021:

I don’t think we use “root” anywhere else to refer to confirmed outpoints

0    // This vector should only contain confirmed outpoints

glozow commented at 3:46 pm on November 4, 2021: member

Concept ACK, there are two changes here that are kind of intermingled (I’d recommend splitting them into different commits):

Refactor TrimToSize to not create a vector of CTransaction copies for the entries
Call RemoveStaged() at the very end to remove ambiguity of !exists meaning confirmed UTXO or no-longer-in-mempool UTXO.

The background for why we generate the vector of outpoints is: If you remove a transaction from the mempool and it spends a UTXO that’s sitting in your coins cache, you should also uncache that coin, since you no longer expect to look up that coin in the near future.

DrahtBot commented at 12:17 pm on February 22, 2022: member

Is it still relevant? ➡️ Please solve the conflicts to make it ready for review and to ensure the CI passes.
Is it no longer relevant? ➡️ Please close.
Did the author lose interest or time to work on this? ➡️ Please close it and mark it ‘Up for grabs’ with the label, so that it can be picked up in the future.

glozow commented at 1:41 pm on February 25, 2022: member

@markblundeberg are you still working on this?

fanquake added the label Up for grabs on May 12, 2022

fanquake commented at 1:27 pm on May 12, 2022: member

markblundeberg are you still working on this?

Closing for now. Feel free to reopen if you want to pick this back up.

fanquake closed this on May 12, 2022

DrahtBot locked this on May 12, 2023

fix CTxMemPool::TrimToSize to put only confirmed coins in pvNoSpendsRemaining #19880

Conflicts