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).
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For detailed information about the code coverage, see the test coverage report.
See 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.
lgtm ACK cc15c5bfd1eb3903de246c124702a7c66c687008
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?
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.
71@@ -72,7 +72,7 @@ FUZZ_TARGET(partially_downloaded_block, .init = initialize_pdb)
72 available.insert(i);
73 }
74
75- if (add_to_mempool) {
76+ if (add_to_mempool && !pool.exists(GenTxid::Txid(tx->GetHash()))) {
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();?
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.
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.
