Skip to content
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.

Sign up for GitHub

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

Jump to bottom

fuzz: don't allow adding duplicate transactions to the mempool #29990

Merged
merged 1 commit into from
Apr 30, 2024
Merged

fuzz: don't allow adding duplicate transactions to the mempool #29990

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/test/fuzz/partially_downloaded_block.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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()))) {
Copy link
Contributor

@paplorinc paplorinc Apr 29, 2024 •
edited

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();?

Copy link
Contributor

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.

Copy link
Member Author

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.

LOCK2(cs_main, pool.cs);
pool.addUnchecked(ConsumeTxMemPoolEntry(fuzzed_data_provider, *tx));
available.insert(i);
Expand Down