fuzz: Test headers pre-sync through p2p

marcofleon commented at 3:35 pm on August 15, 2024: contributor

This PR reopens #28043. It’s a regression fuzz test for #26355 and a couple bugs that were addressed in #25717. This should help us move forward with the removal of mainnet checkpoints.

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.

DrahtBot commented at 3:35 pm on August 15, 2024: 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
ACK	dergoegge, instagibbs, brunoerg, naumenkogs
Concept ACK	mzumsande, TheCharlatan

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:

#29664 (p2p: When close to the tip, download blocks in parallel from additional peers to prevent stalling by mzumsande)
#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 Aug 15, 2024

dergoegge commented at 3:40 pm on August 15, 2024: member

Concept ACK

Thanks for picking this up!

instagibbs commented at 3:44 pm on August 15, 2024: member

I started considering adding a fuzzing harness for timewarp attacks(and mitigations). It would also rely on this type of PoW-checking avoidance.

concept ACK

marcofleon force-pushed on Aug 15, 2024

DrahtBot added the label CI failed on Aug 15, 2024

DrahtBot commented at 4:07 pm on August 15, 2024: contributor

🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/28819298588

Make sure to run all tests locally, according to the documentation.

The failure may happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

brunoerg commented at 4:37 pm on August 15, 2024: contributor

Concept ACK

DrahtBot removed the label CI failed on Aug 15, 2024

hebasto added the label Needs CMake port on Aug 16, 2024

in src/test/fuzz/p2p_headers_sync.cpp:51 in 9d84717c92 outdated

46+    m_connections.clear();
47+    auto& connman = static_cast<ConnmanTestMsg&>(*m_node.connman);
48+    connman.StopNodes();
49+
50+    NodeId id{0};
51+    std::vector<ConnectionType> conn_types = {};

brunoerg commented at 12:58 pm on August 20, 2024:

In 9d84717c92ed052dea6f78c715833d328835ba79 “fuzz: Add harness for p2p headers sync”: Is there any reason to use resize instead of simply doing:

 0diff --git a/src/test/fuzz/p2p_headers_sync.cpp b/src/test/fuzz/p2p_headers_sync.cpp
 1index 8de39431b8..f474263620 100644
 2--- a/src/test/fuzz/p2p_headers_sync.cpp
 3+++ b/src/test/fuzz/p2p_headers_sync.cpp
 4@@ -48,10 +48,11 @@ void HeadersSyncSetup::ResetAndInitialize()
 5     connman.StopNodes();
 6 
 7     NodeId id{0};
 8-    std::vector<ConnectionType> conn_types = {};
 9-    conn_types.resize(1, ConnectionType::OUTBOUND_FULL_RELAY);
10-    conn_types.resize(conn_types.size() + 1, ConnectionType::BLOCK_RELAY);
11-    conn_types.resize(conn_types.size() + 1, ConnectionType::INBOUND);
12+    std::vector<ConnectionType> conn_types = {
13+        ConnectionType::OUTBOUND_FULL_RELAY,
14+        ConnectionType::BLOCK_RELAY,
15+        ConnectionType::INBOUND
16+    };

marcofleon commented at 4:31 pm on August 21, 2024:

You’re right, that’s simpler. Fixed.

brunoerg commented at 6:09 pm on August 20, 2024: contributor

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.

Perhaps it would be good to mention this in docs?

in src/test/fuzz/p2p_headers_sync.cpp:184 in 9d84717c92 outdated

176+            [&]() NO_THREAD_SAFETY_ANALYSIS {
177+                // Send a compact block
178+                auto block = finalized_block();
179+                CBlockHeaderAndShortTxIDs cmpct_block{block, fuzzed_data_provider.ConsumeIntegral<uint64_t>()};
180+
181+                auto headers_msg = NetMsg::Make(NetMsgType::CMPCTBLOCK, TX_WITH_WITNESS(cmpct_block));

brunoerg commented at 6:24 pm on August 20, 2024:

In 9d84717c92ed052dea6f78c715833d328835ba79 “fuzz: Add harness for p2p headers sync”: I can be wrong but I think that when sending a compact block, the peer will always reject it due to low-work header or the PoW bypass would work here?

marcofleon commented at 4:52 pm on August 21, 2024:

You’re right, I think you’re talking about this part here: https://github.com/bitcoin/bitcoin/blob/60b816439eb4bd837778d424628cd3978e0856d9/src/net_processing.cpp#L4754-L4758

It was added as a bug fix in https://github.com/bitcoin/bitcoin/pull/25717/commits/ed6cddd98e32263fc116a4380af6d66da20da990 and is part of what this harness is meant to test. If you remove that code from net_processing and run this test, it should crash.

brunoerg commented at 5:39 pm on August 21, 2024:

Cool, thank you.

in src/test/fuzz/p2p_headers_sync.cpp:146 in 9d84717c92 outdated

141+    g_testing_setup->ResetAndInitialize();
142+
143+    FuzzedDataProvider fuzzed_data_provider{buffer.data(), buffer.size()};
144+
145+    CBlockHeader base{Params().GenesisBlock()};
146+    SetMockTime(Params().GenesisBlock().nTime);

brunoerg commented at 8:54 pm on August 20, 2024:

nit:

0    SetMockTime(base.nTime);

marcofleon commented at 4:32 pm on August 21, 2024:

Done.

marcofleon force-pushed on Aug 21, 2024

marcofleon commented at 4:58 pm on August 21, 2024: contributor

Perhaps it would be good to mention this in docs?

Agreed @brunoerg. We can add it post-merge, assuming people are on board with the method.

in src/pow.cpp:141 in 4e87dcc205 outdated

133@@ -134,7 +134,18 @@ bool PermittedDifficultyTransition(const Consensus::Params& params, int64_t heig
134     return true;
135 }
136 
137+// Bypasses the actual proof of work check during fuzz testing with a simplified validation checking whether
138+// the most signficant bit of the last byte of the hash is set.
139 bool CheckProofOfWork(uint256 hash, unsigned int nBits, const Consensus::Params& params)
140+{
141+#ifdef FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION

dergoegge commented at 12:14 pm on August 23, 2024:

I think the use of FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION is a good approach. CheckProofOfWorkImpl will always be a blocker in fuzz tests, so getting it out of the way for all harnesses that aren’t testing the check itself makes sense.

dergoegge commented at 12:14 pm on August 23, 2024: member

Code review ACK 4e87dcc205ba2bb7996dc077ad46b1b1e309dd8e

Coverage report.

DrahtBot requested review from instagibbs on Aug 23, 2024

DrahtBot requested review from brunoerg on Aug 23, 2024

instagibbs commented at 3:48 pm on August 27, 2024: member

I think some text in the fuzz case itself would help motivate what it’s trying to accomplish. Right now it’s pretty bare so I’m not sure what kinds of bugs/issues it is trying to unsurface.

marcofleon force-pushed on Aug 29, 2024

hebasto removed the label Needs CMake port on Aug 29, 2024

marcofleon force-pushed on Aug 30, 2024

marcofleon commented at 4:36 pm on August 30, 2024: contributor

@instagibbs I’ve elaborated a bit in the harness (and added a couple one liners). Let me know if that last comment helps at all. Happy to add more if you see an opportunity for clarification or have something specific in mind.

net_processing: Make MAX_HEADERS_RESULTS a PeerManager option 0c02d4b2bd

build: Automatically define FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION for fuzz builds a3f6f5acd8

Add FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION in PoW check

To avoid PoW being a blocker for fuzz tests,
`FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION` is used in fuzz builds to
bypass the actual PoW validation in `CheckProofOfWork`. It's
replaced with a check on the last byte of the hash, which allows the
fuzzer to quickly generate (in)valid blocks by checking a single bit,
rather than performing the full PoW computation.

If PoW is the target of a fuzz test, then it should call
`CheckProofOfWorkImpl`.

a0eaa4749f

marcofleon force-pushed on Sep 2, 2024

in src/test/fuzz/p2p_headers_sync.cpp:156 in 787232638f outdated

151+
152+    LIMITED_WHILE(fuzzed_data_provider.ConsumeBool(), 100)
153+    {
154+        auto finalized_block = [&]() {
155+            auto prev = WITH_LOCK(cs_main,
156+                                  return PickValue(fuzzed_data_provider, chainman.m_blockman.m_block_index))

instagibbs commented at 4:56 pm on September 5, 2024:

won’t this always build off genesis?

marcofleon commented at 5:39 pm on September 9, 2024:

You’re right, fixed. This also made me realize that I don’t really need the loop. But I’m thinking to do a followup where it would build on the chain of 16 headers, as opposed to just sending 16 headers one time and resetting. So I think I’ll keep the loop there for now, as it shouldn’t make a difference as far as I’m aware.

in src/test/fuzz/p2p_headers_sync.cpp:197 in 787232638f outdated

192+                g_testing_setup->SendMessage(fuzzed_data_provider, std::move(headers_msg));
193+            });
194+    }
195+
196+    // The headers/blocks sent in this test should never be stored, as the chains don't have the work required
197+    // to meet the anti-DoS work threshold. So, if at any point the block index grew in size, then there's a bug

instagibbs commented at 5:01 pm on September 5, 2024:

I modified this harness to use REGTEST chainparams with a minimum chainwork setting of 0x0 and the harness has yet to fail, fwiw.

Might need to make sure this harness is getting deep enough to actually check what’s being intended?

marcofleon commented at 5:48 pm on September 9, 2024:

With REGTEST enabled, it’s blocked here: https://github.com/bitcoin/bitcoin/blob/712a2b5453cdf2568fece94b969d6e0923b6ba16/src/validation.cpp#L4204-L4209

Using MAIN params it passes because those soft forks aren’t deployed and so the header is added to the block index and you’ll get the failure. I could add header.nVersion = 4 in the ConsumeHeader function of the harness to make it pass for both main and regtest. What do you think?

instagibbs commented at 5:50 pm on September 9, 2024:

Let the fuzzer choose the version, I think?

in src/test/fuzz/p2p_headers_sync.cpp:136 in 787232638f outdated

131+    static auto setup = MakeNoLogFileContext<HeadersSyncSetup>(ChainType::MAIN, {.extra_args = {"-checkpoints=0"}});
132+    g_testing_setup = setup.get();
133+}
134+} // namespace
135+
136+FUZZ_TARGET(p2p_headers_sync, .init = initialize)

instagibbs commented at 5:02 pm on September 5, 2024:

I think it’s clearer to name this p2p_headers_presync as well as the file.

marcofleon commented at 5:49 pm on September 9, 2024:

Good suggestion, thank you.

in src/net_processing.h:77 in 0c02d4b2bd outdated

73@@ -71,6 +74,8 @@ class PeerManager : public CValidationInterface, public NetEventsInterface
74         //! Whether or not the internal RNG behaves deterministically (this is
75         //! a test-only option).
76         bool deterministic_rng{false};
77+        //! Number of headers sent in one getheaders message result.

maflcko commented at 7:10 am on September 6, 2024:

nit in 0c02d4b2bdbc7a3fc3031a63b3b16bafa669d51c: Looks like this is a test-only option, like the previous one, so would it not make sense to mention this as well?

DrahtBot added the label CI failed on Sep 9, 2024

marcofleon force-pushed on Sep 9, 2024

dergoegge approved

dergoegge commented at 10:30 am on September 10, 2024: member

Code review ACK 1243d2ffa30ebc8a148623e3cdabd61f70714932

CI timeout is unrelated

DrahtBot requested review from instagibbs on Sep 10, 2024

fuzz: Add harness for p2p headers sync a97f43d63a

marcofleon force-pushed on Sep 10, 2024

marcofleon commented at 10:58 am on September 10, 2024: contributor

Addressed comments by @instagibbs. Thanks for the review!

dergoegge approved

dergoegge commented at 11:01 am on September 10, 2024: member

reACK a97f43d63a6e835bae20b0bc5d536df98f55d8a0

instagibbs approved

instagibbs commented at 3:09 pm on September 10, 2024: member

tested ACK a97f43d63a6e835bae20b0bc5d536df98f55d8a0

Fails when modifications are made on mainnet and regtest.

Another follow up improvement I’d like is direct assertion that the “chainwork” of the header chain never exceed minimum chainwork, or if that is exceeded, we can drop the assertion that the chain never extends past genesis block.

DrahtBot removed the label CI failed on Sep 10, 2024

marcofleon commented at 10:02 am on September 11, 2024: contributor

Another follow up improvement I’d like is direct assertion that the “chainwork” of the header chain never exceed minimum chainwork, or if that is exceeded, we can drop the assertion that the chain never extends past genesis block.

Agreed, I think something like this would need to be added as part of the building a chain on top of the initial set of headers.

dergoegge commented at 1:14 pm on September 11, 2024: member

@brunoerg @maflcko review ping :)

brunoerg approved

brunoerg commented at 1:18 pm on September 11, 2024: contributor

ACK a97f43d63a6e835bae20b0bc5d536df98f55d8a0

naumenkogs commented at 7:41 am on September 13, 2024: member

ACK a97f43d63a6e835bae20b0bc5d536df98f55d8a0

fanquake commented at 9:58 am on September 13, 2024: member

Ping also @mzumsande, given you expressed some reservations to this approach last time.

glozow commented at 2:47 pm on September 13, 2024: member

(This is a repost, sorry about that)

It’s a regression fuzz test for #26355

Should this fuzzer fail if I make this line return the result from IsContinuationOfLowWorkHeadersSync? https://github.com/bitcoin/bitcoin/blob/cf0120ff024aa73a56f2975c832fda6aa8146dfa/src/net_processing.cpp#L2900

I briefly fuzzed after making the change and didn’t hit anything. Likely I just didn’t fuzz enough, but posting just in case somebody has a fuzz output that I could use to verify.

mzumsande commented at 3:39 pm on September 13, 2024: contributor

Ping also @mzumsande, given you #28043#pullrequestreview-1846077000 to this approach last time.

No reservations with the current approach. Concept ACK, I haven’t reviewed the actual fuzz target in detail (and don’t know if I would get to that in time, considering this has already multiple acks).

Would it make sense to additionally have some belt-and-suspenders check somewhere at the start of bitcoind that FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION is not set, so it’s not only in the build system?

dergoegge commented at 4:16 pm on September 13, 2024: member

Would it make sense to additionally have some belt-and-suspenders check somewhere at the start of bitcoind that FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION is not set, so it’s not only in the build system?

I guess we could (e.g. in bitcoind.cpp) do something like:

0#ifdef FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION
1static_assert(false, "bitcoind can't be build with FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION");
2#endif

But I would be surprised if our existing tests do not already fail if bitcoind is accidentally compiled with FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION. Something as fundamental as the pow check should be covered by our unit and functional tests.

I think it would be better not to add anything like the above because one might fuzz bitcoind directly, e.g. we could consider dropping the custom honggfuzz netdriver patch and use FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION instead (https://github.com/bitcoin/bitcoin/blob/master/doc/fuzzing.md#fuzzing-the-bitcoin-core-p2p-layer-using-honggfuzz-netdriver). For such a fuzzing approach, having the blockers removed would be required as well.

brunoerg commented at 5:06 pm on September 13, 2024: contributor

I briefly fuzzed after making the change and didn’t hit anything. Likely I just didn’t fuzz enough, but posting just in case somebody has a fuzz output that I could use to verify.

I left different mutations of it running overnight (12h+) and I also didn’t get a crash for this one.

I think it would be better not to add anything like the above because one might fuzz bitcoind directly, e.g. we could consider dropping the custom honggfuzz netdriver patch and use FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION instead (https://github.com/bitcoin/bitcoin/blob/master/doc/fuzzing.md#fuzzing-the-bitcoin-core-p2p-layer-using-honggfuzz-netdriver). For such a fuzzing approach, having the blockers removed would be required as well.

Concept ACK for this. Also, the custom honggfuzz netdriver patch is outdated.

marcofleon commented at 5:07 pm on September 13, 2024: contributor

I briefly fuzzed after making the change and didn’t hit anything. Likely I just didn’t fuzz enough, but posting just in case somebody has a fuzz output that I could use to verify. @glozow thanks for reviewing. I’m looking into this more. Good catch actually. Turns out I only partially reverted that commit when testing (only moved the return true out of the else in TryLowWorkHeadersSync). In that case, the fuzzer crashes on this line: https://github.com/bitcoin/bitcoin/blob/e43ce250c6fb65cc0c8903296c8ab228539d2204/src/net_processing.cpp#L3192 I’m not quite sure why yet.

Anyway, I’ll fuzz over the weekend and (hopefully) get back to you with an input that should produce a crash with the full reversion of 7ad15d11005eac36421398530da127333d87ea80.

marcofleon commented at 5:09 pm on September 13, 2024: contributor

I left different mutations of it running overnight (12h+) and I also didn’t get a crash for this one.

Good to know, thanks. I’ll mark this PR as a draft for now until I figure what’s going on.

marcofleon marked this as a draft on Sep 13, 2024

marcofleon marked this as ready for review on Sep 16, 2024

TheCharlatan commented at 9:23 am on September 16, 2024: contributor

I think the use of FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION is a good approach.

Concept ACK

marcofleon commented at 9:29 am on September 16, 2024: contributor

@brunoerg @glozow When you get the chance, try this input: presync_crash.txt

The assert in the harness should fail with that input after reverting 7ad15d1

brunoerg commented at 11:56 am on September 16, 2024: contributor

The assert in the harness should fail with that input after reverting https://github.com/bitcoin/bitcoin/commit/7ad15d11005eac36421398530da127333d87ea80

Yes, confirmed!

 0Assertion failed: (WITH_LOCK(cs_main, return chainman.m_blockman.m_block_index.size()) == original_index_size), function p2p_headers_presync_fuzz_target, file p2p_headers_presync.cpp, line 197.
 1==80161== ERROR: libFuzzer: deadly signal
 2    [#0](/bitcoin-bitcoin/0/) 0x107edce84 in __sanitizer_print_stack_trace+0x28 (libclang_rt.asan_osx_dynamic.dylib:arm64+0x5ce84)
 3    [#1](/bitcoin-bitcoin/1/) 0x104a53b80 in fuzzer::PrintStackTrace() FuzzerUtil.cpp:210
 4    [#2](/bitcoin-bitcoin/2/) 0x104a39bfc in fuzzer::Fuzzer::CrashCallback() FuzzerLoop.cpp:231
 5    [#3](/bitcoin-bitcoin/3/) 0x18bec9a20 in _sigtramp+0x34 (libsystem_platform.dylib:arm64+0x3a20)
 6    [#4](/bitcoin-bitcoin/4/) 0x454100018be99cbc  (<unknown module>)
 7    [#5](/bitcoin-bitcoin/5/) 0x4b6a80018bda5a3c  (<unknown module>)
 8    [#6](/bitcoin-bitcoin/6/) 0xdd7700018bda4d2c  (<unknown module>)
 9    [#7](/bitcoin-bitcoin/7/) 0xa510800102909c48  (<unknown module>)
10    [#8](/bitcoin-bitcoin/8/) 0x102d403a8 in LLVMFuzzerTestOneInput fuzz.cpp:209
11    [#9](/bitcoin-bitcoin/9/) 0x104a3b024 in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) FuzzerLoop.cpp:614
12    [#10](/bitcoin-bitcoin/10/) 0x104a27d68 in fuzzer::RunOneTest(fuzzer::Fuzzer*, char const*, unsigned long) FuzzerDriver.cpp:327
13    [#11](/bitcoin-bitcoin/11/) 0x104a2d000 in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) FuzzerDriver.cpp:862
14    [#12](/bitcoin-bitcoin/12/) 0x104a54468 in main FuzzerMain.cpp:20
15    [#13](/bitcoin-bitcoin/13/) 0x18bb190dc  (<unknown module>)
16    [#14](/bitcoin-bitcoin/14/) 0xc6207ffffffffffc  (<unknown module>)

glozow merged this on Sep 16, 2024

glozow closed this on Sep 16, 2024

marcofleon commented at 5:27 pm on September 17, 2024: contributor

Another follow up improvement I’d like is direct assertion that the “chainwork” of the header chain never exceed minimum chainwork, or if that is exceeded, we can drop the assertion that the chain never extends past genesis block.

Followup here: #30918

Also, I was mistaken in this comment. The test as is already does build a chain that is more than 16 headers long. So, there was no need to address this in the followup.

in src/test/fuzz/p2p_headers_presync.cpp:24 in a97f43d63a

19+{
20+    std::vector<CNode*> m_connections;
21+
22+public:
23+    HeadersSyncSetup(const ChainType chain_type = ChainType::MAIN,
24+                     TestOpts opts = {})

maflcko commented at 12:12 pm on September 18, 2024:

nit: Any reason to specify default values when they are not used?

in src/test/fuzz/p2p_headers_presync.cpp:147 in a97f43d63a

142+
143+    g_testing_setup->ResetAndInitialize();
144+
145+    FuzzedDataProvider fuzzed_data_provider{buffer.data(), buffer.size()};
146+
147+    CBlockHeader base{Params().GenesisBlock()};

maflcko commented at 12:25 pm on September 18, 2024:

style nit: When there is a local alias of a global, my preference is to use the local: chainman.GetParams(), over Params(), but up to you. (Same for FinalizeHeader)

maflcko commented at 12:38 pm on September 18, 2024: member

Nice.

Left some style nits. Feel free to ignore.

Is there a set of fuzz inputs, or a coverage report?

marcofleon commented at 4:59 pm on September 19, 2024: contributor

@maflcko thanks for the review! Your comments make sense to me, I’ll address them in the followup.

I just added some inputs for this target to qa-assets. Here is a coverage report.

maflcko commented at 12:33 pm on September 20, 2024: member

Thanks! However, I think there was some kind of issue with the fuzz inputs, so that the macOS 14 CI ran into timeouts.

I’ve removed them temporarily again in https://github.com/bitcoin-core/qa-assets/commit/84cea7068728bc2c765b9da680eedcb85f9f3c1b . This should allow for some time to investigate, while unbreaking the CI.

achow101 referenced this in commit 0894748316 on Sep 20, 2024

in src/test/fuzz/p2p_headers_presync.cpp:62 in a97f43d63a

57+    for (auto conn_type : conn_types) {
58+        CAddress addr{};
59+        m_connections.push_back(new CNode(id++, nullptr, addr, 0, 0, addr, "", conn_type, false));
60+        CNode& p2p_node = *m_connections.back();
61+
62+        connman.Handshake(

vasild commented at 7:07 am on September 26, 2024:

Without a socket (second argument to CNode constructor is nullptr) this will not test much of the handshake:

https://github.com/bitcoin/bitcoin/blob/65f6e7078b17e6e73d74dfeb00159878099fee1e/src/net.cpp#L1600-L1605

Any special reason to go socket-less? I am working on a CConnman refactor that is changing this anyway, should I assign a fuzzed socket to the node?

maflcko commented at 7:27 am on September 26, 2024:

Send and receive doesn’t go through sockets in this test. Handshake is just a method to initialize some fields with dummy or mocked data.

Edit: Whether or not a socket is used shouldn’t matter for this test, which is a net_processing/validation test, not a low-level net test.

fuzz: Test headers pre-sync through p2p #30661

Code Coverage

Reviews

Conflicts