New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fuzz: don't allow adding duplicate transactions to the mempool #29990
Conversation
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code CoverageFor detailed information about the code coverage, see the test coverage report. ReviewsSee 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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm ACK cc15c5b
I guess this isn't due to a fuzz crash, because the only thing that's off is cachedInnerUsage
(and friends), which are not checked in this fuzz target?
Correct, I didn't see any fuzz crash on master, but this issue was leading to crashes in my cluster mempool branch, where the inconsistency manifested itself sooner. |
@@ -72,7 +72,7 @@ FUZZ_TARGET(partially_downloaded_block, .init = initialize_pdb) | |||
available.insert(i); | |||
} | |||
|
|||
if (add_to_mempool) { | |||
if (add_to_mempool && !pool.exists(GenTxid::Txid(tx->GetHash()))) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Considering the purpose of fuzzing is to test the combination of valid states, shouldn't we include tests that involve sending duplicate transactions to the mempool?
Or if that's irrelevant, since the proposed change affects how the add_to_mempool
flag behaves, would it be more transparent to redefine it to directly reflect whether a transaction can be added to the mempool, i.e. bool add_to_mempool = !pool.exists(GenTxid::Txid(tx->GetHash())) && fuzzed_data_provider.ConsumeBool();
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Considering the purpose of fuzzing is to test the combination of valid states, shouldn't we include tests that involve sending duplicate transactions to the mempool?
In this case, it is calling addUnchecked
directly that's why it would be proper to check whether the transaction is in mempool before. Note that, in practice, this function is used in MemPoolAccept::Finalize
which removes conflicting transactions from the mempool before adding.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this case, it is calling addUnchecked directly that's why it would be proper to check whether the transaction is in mempool before. Note that, in practice, this function is used in MemPoolAccept::Finalize which removes conflicting transactions from the mempool before adding.
Yes exactly. The correct interface for code that is doing no sanity checking would be AcceptToMemoryPool()
. Since we're bypassing that in the fuzz tests, we should do something else to avoid putting the mempool into an inconsistent state.
Concept ACK |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK cc15c5b
utACK cc15c5b makes sense to me |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
utACK cc15c5b
Filter duplicate transaction ids from being added to the mempool in the
partially_downloaded_block
fuzz target.I think a prerequisite for calling
CTxMemPool::addUnchecked
should be that the underlying txid doesn't already exist in the mempool (otherwiseaddUnchecked
would need a way to return failure, which we don't currently have).