Skip to content

fuzz: Mock GetRand in process_message(s) #28537

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
Closed
dergoegge wants to merge 2 commits into from
Closed

fuzz: Mock GetRand in process_message(s) #28537

dergoegge wants to merge 2 commits into from

Conversation

@dergoegge
Copy link
Member

@dergoegge dergoegge commented Sep 26, 2023

This PR introduces g_mock_get_rand to allow tests to mock GetRand and makes g_mock_get_rand consume from the fuzzed data in the process_message(s) targets.

Mocking GetRand reduces the non-determinism in these targets and results in more stable fuzzing.

@DrahtBot
Copy link
Contributor

DrahtBot commented Sep 26, 2023 •
edited
Loading

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Reviews

See the guideline for information on the review process.

Type Reviewers
Concept ACK brunoerg

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

@DrahtBot DrahtBot added the Tests label Sep 26, 2023
dergoegge
dergoegge commented Sep 26, 2023
View reviewed changes
src/test/fuzz/process_message.cpp Outdated
Copy link
Member Author

@dergoegge dergoegge Sep 26, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not having this exception leads to an endless loop here:

bitcoin/src/net_processing.cpp

Lines 5225 to 5227 in c9f2882

do {
nonce = GetRand<uint64_t>();
} while (nonce == 0);

src/random.cpp Outdated
Copy link
Member Author

@dergoegge dergoegge Sep 26, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Setting g_mock_deterministic_tests is not a great alternative as it makes GetRand always return the same value.

Copy link
Member

@maflcko maflcko Sep 27, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: It is bad enough that there is a single test-only mock in here. Now, having two, it would make sense to remove one of them again. (Maybe in a follow-up)

@brunoerg
Copy link
Contributor

brunoerg commented Sep 26, 2023

Concept ACK

@maflcko
Copy link
Member

maflcko commented Sep 27, 2023

Can you add examples where this matters?

I presume one example would be ping-pong handling?

Longer term it would be nice to nuke the global GetRand and use a mockable instance everywhere.

@dergoegge
Copy link
Member Author

dergoegge commented Sep 27, 2023

Can you add examples where this matters?

bitcoin/src/net_processing.cpp

Line 5369 in c9f2882

peer.m_next_send_feefilter = current_time + GetRandomDuration<std::chrono::microseconds>(MAX_FEEFILTER_CHANGE_DELAY);

bitcoin/src/net_processing.cpp

Line 1106 in c9f2882

m_next_inv_to_inbounds = GetExponentialRand(now, average_interval);

bitcoin/src/net_processing.cpp

Line 5270 in c9f2882

peer.m_next_addr_send = GetExponentialRand(current_time, AVG_ADDRESS_BROADCAST_INTERVAL);

bitcoin/src/net_processing.cpp

Line 5264 in c9f2882

peer.m_next_local_addr_send = GetExponentialRand(current_time, AVG_LOCAL_ADDRESS_BROADCAST_INTERVAL);

@maflcko
Copy link
Member

maflcko commented Sep 27, 2023

The examples you linked to should still be non-deterministic, because the feefilter rounder is random, as well as the address relay, no?

Also, no transactions are ever relayed, so while this makes the coverage data more deterministic, the benefits should be minimal? Considering that this may affect all fuzz inputs, I'd prefer to go the full and complete route instead.

maflcko
maflcko reviewed Sep 27, 2023
View reviewed changes
src/test/fuzz/process_message.cpp
FuzzedDataProvider fuzzed_data_provider(buffer.data(), buffer.size());

g_mock_get_rand = [&](uint64_t max) -> uint64_t {
if (fuzzed_data_provider.remaining_bytes() == 0) return 1;
Copy link
Member

@maflcko maflcko Sep 27, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this return max - 1, otherwise it may return an invalid value when GetRand(1) is called?

@dergoegge dergoegge marked this pull request as draft September 27, 2023 14:42
@dergoegge dergoegge closed this Sep 28, 2023
@maflcko
Copy link
Member

maflcko commented Sep 29, 2023

Would be nice to start using one FastRandomContext in PeerManager (locked by the message thread mutex), or more, if needed. The FRC would be deterministic in tests.

No need to fix everything, but it shouldn't be too hard to fix one instance (e.g. ping-pong handling) with the added benefit of not breaking the fuzz input format. Happy to review that.

In the future, the deterministic seed can be read from the fuzz input (in a breaking fuzz change), and more places can be converted.

@dergoegge
Copy link
Member Author

dergoegge commented Oct 2, 2023

Would be nice to start using one FastRandomContext in PeerManager

This doesn't solve our problem in the fuzz tests since the peerman is global, so this will only help in fuzz tests that create a new peerman each iteration.

@dergoegge dergoegge mentioned this pull request Oct 2, 2023
Merged
fanquake added a commit that referenced this pull request Oct 5, 2023
@fanquake
Merge #28558: Make PeerManager own a FastRandomContext
52c6904 
4cafe9f [test] Make PeerManager's rng deterministic in tests (dergoegge)
fecec3e [net processing] FeeFilterRounder doesn't own a FastRandomContext (dergoegge)
47520ed [net processing] Make fee filter rounder non-global (dergoegge)
77506f4 [net processing] Addr shuffle uses PeerManager's rng (dergoegge)
a648dd7 [net processing] PushAddress uses PeerManager's rng (dergoegge)
87c7067 [net processing] PeerManager holds a FastRandomContext (dergoegge)

Pull request description:

  This lets us avoid some non-determinism in tests (also see #28537).

ACKs for top commit:
  MarcoFalke:
    re-ACK 4cafe9f  🕗
  glozow:
    concept && light code review ACK 4cafe9f

Tree-SHA512: 3c18700773d0bc547ccb6442c41567e6f26b0b50fab5b79620da417ec91b9c0ae1395d15258da3aa4a91447b8ce560145dd135e39fbbd0610749e528e665b111
Frank-GER pushed a commit to syscoin/syscoin that referenced this pull request Oct 13, 2023
@fanquake
Merge bitcoin#28558: Make PeerManager own a FastRandomContext
e241eb4 
4cafe9f [test] Make PeerManager's rng deterministic in tests (dergoegge)
fecec3e [net processing] FeeFilterRounder doesn't own a FastRandomContext (dergoegge)
47520ed [net processing] Make fee filter rounder non-global (dergoegge)
77506f4 [net processing] Addr shuffle uses PeerManager's rng (dergoegge)
a648dd7 [net processing] PushAddress uses PeerManager's rng (dergoegge)
87c7067 [net processing] PeerManager holds a FastRandomContext (dergoegge)

Pull request description:

  This lets us avoid some non-determinism in tests (also see bitcoin#28537).

ACKs for top commit:
  MarcoFalke:
    re-ACK 4cafe9f  🕗
  glozow:
    concept && light code review ACK 4cafe9f

Tree-SHA512: 3c18700773d0bc547ccb6442c41567e6f26b0b50fab5b79620da417ec91b9c0ae1395d15258da3aa4a91447b8ce560145dd135e39fbbd0610749e528e665b111
@bitcoin bitcoin locked and limited conversation to collaborators Oct 1, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer

@maflcko maflcko maflcko left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Tests

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@dergoegge @DrahtBot @brunoerg @maflcko