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. |
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. |
| } | ||
|
|
||
| if (add_to_mempool) { | ||
| if (add_to_mempool && !pool.exists(GenTxid::Txid(tx->GetHash()))) { |
There was a problem hiding this comment.
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.
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.
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 |
|
utACK cc15c5b makes sense to me |
…to the mempool cc15c5b fuzz: don't allow adding duplicate transactions to the mempool (Suhas Daftuar) Pull request description: 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 (otherwise `addUnchecked` would need a way to return failure, which we don't currently have). ACKs for top commit: glozow: utACK cc15c5b makes sense to me maflcko: lgtm ACK cc15c5b brunoerg: ACK cc15c5b dergoegge: utACK cc15c5b Tree-SHA512: 85f84ce405aba584e6d00391515f0a86c5648ce8b2da69036e50a6c1f6833d050d09b1972cc5ffbe7c4edb3e5f7f965ef34bd839deeddac27a889cc8d2e53b8f
Filter duplicate transaction ids from being added to the mempool in the
partially_downloaded_blockfuzz target.I think a prerequisite for calling
CTxMemPool::addUncheckedshould be that the underlying txid doesn't already exist in the mempool (otherwiseaddUncheckedwould need a way to return failure, which we don't currently have).