fix CTxMemPool::TrimToSize to put only confirmed coins in pvNoSpendsRemaining #19880
Conversation
|
@TheBlueMatt can you confirm on the original intent here? |
…emaining 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 (bitcoin#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).
216482d to
374c5d3
Compare
| for (const CTransaction& tx : txn) { | ||
| for (const CTxIn& txin : tx.vin) { | ||
| if (exists(txin.prevout.hash)) continue; | ||
| for (txiter iter : stage) { |
There was a problem hiding this comment.
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)?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
|
🐙 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". |
There was a problem hiding this comment.
Concept ACK, there are two changes here that are kind of intermingled (I'd recommend splitting them into different commits):
- Refactor
TrimToSizeto not create a vector ofCTransactioncopies for the entries - Call
RemoveStaged()at the very end to remove ambiguity of!existsmeaning 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.
| for (const CTransaction& tx : txn) { | ||
| for (const CTxIn& txin : tx.vin) { | ||
| if (exists(txin.prevout.hash)) continue; | ||
| for (txiter iter : stage) { |
There was a problem hiding this comment.
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.
| BOOST_CHECK(!pool.exists(tx1.GetHash())); | ||
| BOOST_CHECK(!pool.exists(tx2.GetHash())); | ||
| BOOST_CHECK(!pool.exists(tx3.GetHash())); | ||
| // This vector should only contain 'root' (not unconfirmed) outpoints |
There was a problem hiding this comment.
I don't think we use "root" anywhere else to refer to confirmed outpoints
| // This vector should only contain 'root' (not unconfirmed) outpoints | |
| // This vector should only contain confirmed outpoints |
There hasn't been much activity lately and the patch still needs rebase. What is the status here?
|
|
@markblundeberg are you still working on this? |
Closing for now. Feel free to reopen if you want to pick this back up. |
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 (#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 )