fuzz: Test headers pre-sync through p2p interface #28043

dergoegge · 2023-07-07T12:05:27Z

This PR adds a regression fuzz test for #26355 and some of the bugs found during review of #25717.

Should give us more confidence in doing #25725.

DrahtBot · 2023-07-07T12:05:30Z

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

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept ACK	darosior

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#26812 (test: add end-to-end tests for CConnman and PeerManager by vasild)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

dergoegge · 2023-07-07T12:06:22Z

Draft until #27499 is in.

src/net_processing.h

src/test/fuzz/p2p_headers_sync.cpp

dergoegge · 2023-07-25T12:20:44Z

Rebased and un-drafted, ready for review!

src/test/fuzz/p2p_headers_sync.cpp

maflcko · 2023-11-17T10:34:26Z

test/fuzz/p2p_headers_sync.cpp:187:96:   required from here
./primitives/transaction.h:327:42: error: ‘class CVectorWriter’ has no member named ‘GetParams’
  327 |         SerializeTransaction(*this, s, s.GetParams());
      |                                        ~~^~~~~~~~~
make[2]: *** [Makefile:17531: test/fuzz/fuzz-p2p_headers_sync.o] Error 1
make[2]: Leaving directory '/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src'
make[1]: *** [Makefile:20246: install-recursive] Error 1
make[1]: Leaving directory '/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src'
make: *** [Makefile:811: install-recursive] Error 1

Exit status: 2������������������������������������������������������������������������������������������������������������������������

dergoegge · 2023-11-20T16:09:56Z

Thanks! rebased.

darosior · 2023-11-20T16:11:42Z

Concept ACK. Planning on reviewing soon.

mzumsande · 2023-12-01T21:20:20Z

Needs rebase (silent merge conflict).

dergoegge · 2023-12-06T15:01:35Z

Needs rebase (silent merge conflict).

Thanks, rebased

jamesob · 2024-01-09T19:29:25Z

src/pow.cpp

 bool CheckProofOfWork(uint256 hash, unsigned int nBits, const Consensus::Params& params)
 {
+    if (g_check_pow_mock) {


Should probably have some belt-and-suspenders check that we're 100% not running on mainnet, maybe based on the contents of params and whatever applicable global.

An alternative to the mocking would be to use FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION in all places that call CheckProofOfWork (i.e. validation, mining) since there is literally zero benefit in having this be a blocker for fuzzing.

https://github.com/dergoegge/bitcoin/tree/2024-01-fuzz-unsafe-pow

Opened #29305 to discuss, I think it actually makes more sense than the mocking

dergoegge · 2024-01-26T11:41:54Z

It's been more than 6 months since I opened this without substantial review, so closing. Let me know if anyone is actually interested in reviewing and I'll reopen.

mzumsande

just fyi, I started reviewing at some point, my problem was that I don't like test-specific / mocking code in production code (especially the consensus-critical parts), so I wasn't really comfortable (concept)-acking it.
On the other hand, I didn't see a better way to do it and fuzzing is great obviously...

As a result, I was undecided and didn't write anything. Maybe it'd be worth to have a general discussion about the extent of mocking / test-specific production code we want as a project, since there are obviously both advantages and disadvantages to it. If there was a general consensus that these kind of mocks are fine I'd be happy to review/ack the actual fuzz target.

maflcko · 2024-01-29T11:46:16Z

I'd say they are fine, if there is no better way. With global functions and variables, I think the only way to mock them is also via a global approach. Here, adding the assert(params!=main) (#28043 (comment)) should be fine.

a97f43d fuzz: Add harness for p2p headers sync (marcofleon) a0eaa47 Add FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION in PoW check (marcofleon) a3f6f5a build: Automatically define FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION for fuzz builds (marcofleon) 0c02d4b net_processing: Make MAX_HEADERS_RESULTS a PeerManager option (marcofleon) Pull request description: This PR reopens #28043. It's a regression fuzz test for #26355 and [a couple bugs](ed6cddd) that were addressed in #25717. This should help us move forward with the [removal of mainnet checkpoints](#25725). It seems like the main concern in #28043 was the global mock function for proof of work. This PR aims to be an improvement by replacing the previous approach with a fuzz build configured using `FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION`. This ensures that the simplified test code will never be in a release binary. If we agree this is the way to go, there are some other places (for future targets) where this method could be used. In this target, PoW isn't being tested, so the goal is to bypass the check and let the fuzzer do its thing. In the other harnesses where PoW is actually being fuzzed, `CheckProofOfWork` is now `CheckProofOfWorkImpl`. So, the only change to that function is in the name. More about `FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION` can be found at https://llvm.org/docs/LibFuzzer.html#fuzzer-friendly-build-mode and https://github.com/AFLplusplus/AFLplusplus/blob/stable/docs/fuzzing_in_depth.md#d-modifying-the-target. ACKs for top commit: naumenkogs: ACK a97f43d dergoegge: reACK a97f43d instagibbs: tested ACK a97f43d brunoerg: ACK a97f43d Tree-SHA512: 60b0bc6aadd8ca4c39db9cbba2da2debaaf68afcb6a8dd75c1ce48ca9e3996948fda8020930b6771a424e0f7c41b0b1068db4aa7dbe517f8fc152f1f712058ad

DrahtBot added the Tests label Jul 7, 2023

dergoegge marked this pull request as draft July 7, 2023 12:06

maflcko reviewed Jul 8, 2023

View reviewed changes

src/net_processing.h Show resolved Hide resolved

DrahtBot added the CI failed label Jul 18, 2023

DrahtBot mentioned this pull request Jul 19, 2023

net processing, refactor: Decouple PeerManager from gArgs #27499

Merged

darosior reviewed Jul 24, 2023

View reviewed changes

src/test/fuzz/p2p_headers_sync.cpp Show resolved Hide resolved

DrahtBot added the Needs rebase label Jul 25, 2023

dergoegge force-pushed the 2023-07-fuzz-pow branch from 4407469 to 0aca787 Compare July 25, 2023 12:19

dergoegge marked this pull request as ready for review July 25, 2023 12:20

dergoegge force-pushed the 2023-07-fuzz-pow branch from 0aca787 to 8a7feb6 Compare July 25, 2023 12:25

DrahtBot removed Needs rebase CI failed labels Jul 25, 2023

maflcko reviewed Jul 28, 2023

View reviewed changes

src/test/fuzz/p2p_headers_sync.cpp Outdated Show resolved Hide resolved

DrahtBot mentioned this pull request Jul 30, 2023

Relay own transactions only via short-lived Tor or I2P connections #27509

Closed

5 tasks

dergoegge force-pushed the 2023-07-fuzz-pow branch from 8a7feb6 to f718a2c Compare July 31, 2023 08:28

DrahtBot added the CI failed label Aug 26, 2023

dergoegge force-pushed the 2023-07-fuzz-pow branch 3 times, most recently from 6b4edf3 to 6bac3ba Compare September 12, 2023 10:26

DrahtBot removed the CI failed label Sep 12, 2023

DrahtBot removed the Needs rebase label Oct 8, 2023

laanwj added this to the 27.0 milestone Oct 26, 2023

DrahtBot added the CI failed label Nov 17, 2023

dergoegge force-pushed the 2023-07-fuzz-pow branch from 924e6ea to 2b746ab Compare November 20, 2023 16:09

DrahtBot removed the CI failed label Nov 20, 2023

DrahtBot added the CI failed label Nov 30, 2023

dergoegge force-pushed the 2023-07-fuzz-pow branch from 2b746ab to a955242 Compare December 6, 2023 15:00

DrahtBot removed the CI failed label Dec 6, 2023

DrahtBot mentioned this pull request Dec 11, 2023

Nuke adjusted time from validation (attempt 2) #28956

Merged

darosior mentioned this pull request Dec 30, 2023

PoC: fuzz chainstate and block managers #29158

Closed

jamesob reviewed Jan 9, 2024

View reviewed changes

DrahtBot added the CI failed label Jan 13, 2024

dergoegge added 3 commits January 23, 2024 14:56

Allow mocking CheckProofOfWork

467400c

[net processing] Make MAX_HEADERS_RESULT a PeerManager option

355a43c

[fuzz] Test headers sync through the p2p interface

4516b45

dergoegge force-pushed the 2023-07-fuzz-pow branch from a955242 to 4516b45 Compare January 23, 2024 14:58

DrahtBot removed the CI failed label Jan 23, 2024

dergoegge mentioned this pull request Jan 24, 2024

fuzz: Use FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION for pow checks #29305

Closed

dergoegge closed this Jan 26, 2024

mzumsande reviewed Jan 26, 2024

View reviewed changes

marcofleon mentioned this pull request Aug 15, 2024

fuzz: Test headers pre-sync through p2p #30661

Merged

bitcoin locked and limited conversation to collaborators Jan 28, 2025

fuzz: Test headers pre-sync through p2p interface #28043

fuzz: Test headers pre-sync through p2p interface #28043

Uh oh!

Conversation

dergoegge commented Jul 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Jul 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage

Reviews

Conflicts

Uh oh!

dergoegge commented Jul 7, 2023

Uh oh!

Uh oh!

Uh oh!

dergoegge commented Jul 25, 2023

Uh oh!

Uh oh!

maflcko commented Nov 17, 2023

Uh oh!

dergoegge commented Nov 20, 2023

Uh oh!

darosior commented Nov 20, 2023

Uh oh!

mzumsande commented Dec 1, 2023

Uh oh!

dergoegge commented Dec 6, 2023

Uh oh!

jamesob Jan 9, 2024

Choose a reason for hiding this comment

Uh oh!

dergoegge Jan 24, 2024

Choose a reason for hiding this comment

Uh oh!

dergoegge Jan 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dergoegge Jan 24, 2024

Choose a reason for hiding this comment

Uh oh!

dergoegge commented Jan 26, 2024

Uh oh!

mzumsande left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko commented Jan 29, 2024

Uh oh!

Uh oh!

dergoegge commented Jul 7, 2023 •

edited

Loading

DrahtBot commented Jul 7, 2023 •

edited

Loading

dergoegge Jan 24, 2024 •

edited

Loading