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

pull dergoegge wants to merge 3 commits into bitcoin:master from dergoegge:2023-07-fuzz-pow changing 6 files +215 −9

dergoegge commented at 12:05 PM on July 7, 2023: member

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 commented at 12:05 PM on July 7, 2023: contributor


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.
DrahtBot added the label Tests on Jul 7, 2023
dergoegge marked this as a draft on Jul 7, 2023
dergoegge commented at 12:06 PM on July 7, 2023: member

Draft until #27499 is in.
in src/net_processing.h:67 in 4407469bd6 outdated
```
  51 | +        bool ignore_incoming_txs{false};
  52 | +        bool reconcile_txs{false};
  53 | +        uint32_t max_orphan_txs{DEFAULT_MAX_ORPHAN_TRANSACTIONS};
  54 | +        size_t max_extra_txs{DEFAULT_BLOCK_RECONSTRUCTION_EXTRA_TXN};
  55 | +        bool capture_messages{false};
  56 | +        uint32_t max_headers_result{MAX_HEADERS_RESULTS};
```
maflcko commented at 9:38 AM on July 8, 2023:

Looks like this is the only option that is required by the fuzz target, so maybe remove the others for now to hopefully reduce the number of conflicting pulls and remove the dependency on the other pull?

dergoegge commented at 10:37 AM on July 10, 2023:

3 of the PRs are mine (I don't care about rebasing) and the package relay stuff is gonna conflict either way, so I'd prefer keeping it as is.
DrahtBot added the label CI failed on Jul 18, 2023
in src/test/fuzz/p2p_headers_sync.cpp:127 in 4407469bd6 outdated
```
 122 | +void initialize()
 123 | +{
 124 | +    // Reduce proof-of-work check to one bit.
 125 | +    g_check_pow_mock = [](uint256 hash, unsigned int, const Consensus::Params&) {
 126 | +        return (hash.data()[31] & 0x80) == 0;
 127 | +    };
```
darosior commented at 7:21 PM on July 24, 2023:

Why not disabling it completely instead of mocking it?

dergoegge commented at 12:23 PM on July 25, 2023:

My thinking was that this should result in more coverage because we'll also cover paths for invalid PoW.
DrahtBot added the label Needs rebase on Jul 25, 2023
dergoegge force-pushed on Jul 25, 2023
dergoegge marked this as ready for review on Jul 25, 2023
dergoegge commented at 12:20 PM on July 25, 2023: member

Rebased and un-drafted, ready for review!
dergoegge force-pushed on Jul 25, 2023
in src/test/fuzz/p2p_headers_sync.cpp:159 in 8a7feb6573 outdated
```
 154 | +            CBlock block = ConsumeBlock(fuzzed_data_provider, prev.GetHash(), prev.nBits);
 155 | +            FinalizeHeader(block);
 156 | +            return block;
 157 | +        };
 158 | +
 159 | +        std::vector<std::function<void()>> actions = {
```
maflcko commented at 12:35 PM on July 25, 2023:

How is this different from CallOneOf?

dergoegge commented at 11:12 AM on July 30, 2023:

it's not, iirc I had some trouble with thread safety annotations but I'll give CallOneOf another try

maflcko commented at 7:29 AM on July 31, 2023:

I don't think you can use annotations one way or the other. As soon as you assign to std::function<void()>, the annotations are dropped, so you might as well not have them in the first place.

If you want them, and use CallOneOf, you can use the same trick to wrap each annotated lambda into a std::function<void()>{lambda_bla} temporary to achieve the same result of dropping any annotations.

dergoegge commented at 8:27 AM on July 31, 2023:

switched to CallOneOf and used NO_THREAD_SAFETY_ANALYSIS.
DrahtBot removed the label Needs rebase on Jul 25, 2023
DrahtBot removed the label CI failed on Jul 25, 2023
dergoegge force-pushed on Jul 31, 2023
DrahtBot added the label CI failed on Aug 26, 2023
dergoegge force-pushed on Sep 11, 2023
dergoegge force-pushed on Sep 11, 2023
dergoegge force-pushed on Sep 12, 2023
DrahtBot removed the label CI failed on Sep 12, 2023
DrahtBot added the label Needs rebase on Oct 5, 2023
dergoegge force-pushed on Oct 6, 2023
DrahtBot removed the label Needs rebase on Oct 8, 2023
laanwj added this to the milestone 27.0 on Oct 26, 2023
DrahtBot added the label CI failed on Nov 17, 2023

Type	Reviewers
Concept ACK	darosior

maflcko commented at 10:34 AM on November 17, 2023: member

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 force-pushed on Nov 20, 2023
dergoegge commented at 4:09 PM on November 20, 2023: member

Thanks! rebased.
darosior commented at 4:11 PM on November 20, 2023: member

Concept ACK. Planning on reviewing soon.
DrahtBot removed the label CI failed on Nov 20, 2023
DrahtBot added the label CI failed on Nov 30, 2023
mzumsande commented at 9:20 PM on December 1, 2023: contributor

Needs rebase (silent merge conflict).
dergoegge force-pushed on Dec 6, 2023
dergoegge commented at 3:01 PM on December 6, 2023: member

Needs rebase (silent merge conflict).

Thanks, rebased
DrahtBot removed the label CI failed on Dec 6, 2023
in src/pow.cpp:129 in a95524240b outdated
```
 121 | @@ -122,8 +122,14 @@ bool PermittedDifficultyTransition(const Consensus::Params& params, int64_t heig
 122 |      return true;
 123 |  }
 124 |  
 125 | +std::function<bool(uint256, unsigned int, const Consensus::Params&)> g_check_pow_mock = nullptr;
 126 | +
 127 |  bool CheckProofOfWork(uint256 hash, unsigned int nBits, const Consensus::Params& params)
 128 |  {
 129 | +    if (g_check_pow_mock) {
```
jamesob commented at 7:29 PM on January 9, 2024:

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.

dergoegge commented at 11:06 AM on January 24, 2024:

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.

dergoegge commented at 11:24 AM on January 24, 2024:

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

dergoegge commented at 2:19 PM on January 24, 2024:

Opened #29305 to discuss, I think it actually makes more sense than the mocking
DrahtBot added the label CI failed on Jan 13, 2024
Allow mocking CheckProofOfWork 467400cf31
[net processing] Make MAX_HEADERS_RESULT a PeerManager option 355a43cb7e
[fuzz] Test headers sync through the p2p interface 4516b454ba
dergoegge force-pushed on Jan 23, 2024
DrahtBot removed the label CI failed on Jan 23, 2024
dergoegge commented at 11:41 AM on January 26, 2024: member

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.
dergoegge closed this on Jan 26, 2024
mzumsande commented at 4:21 PM on January 26, 2024: contributor

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 commented at 11:46 AM on January 29, 2024: member

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) (https://github.com/bitcoin/bitcoin/pull/28043#discussion_r1446512814) should be fine.
glozow referenced this in commit 2bf721e76a on Sep 16, 2024
bitcoin locked this on Jan 28, 2025

Contributors

Labels

Milestone
27.0

Linked (view graph)

#25717 p2p: Implement anti-DoS headers sync #25725 consensus: Remove mainnet checkpoints #26355 p2p: Handle IsContinuationOfLowWorkHeadersSync return value correctly when new headers sync is started #29158 PoC: fuzz chainstate and block managers #29305 fuzz: Use FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION for pow checks #30661 fuzz: Test headers pre-sync through p2p

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

Code Coverage

Reviews

Conflicts