Merge bitcoin/bitcoin#30661: fuzz: Test headers pre-sync through p2p

a97f43d63a6e835bae20b0bc5d536df98f55d8a0 fuzz: Add harness for p2p headers sync (marcofleon)
a0eaa4749fe0f755e113eee70dee1989bdc07ad5 Add FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION in PoW check (marcofleon)
a3f6f5acd89f2f5bb136ec247f259d212e8944d0 build: Automatically define FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION for fuzz builds (marcofleon)
0c02d4b2bdbc7a3fc3031a63b3b16bafa669d51c net_processing: Make MAX_HEADERS_RESULTS a PeerManager option (marcofleon)

Pull request description:

  This PR reopens https://github.com/bitcoin/bitcoin/pull/28043. It's a regression fuzz test for https://github.com/bitcoin/bitcoin/pull/26355 and [a couple bugs](https://github.com/bitcoin/bitcoin/pull/25717/commits/ed6cddd98e32263fc116a4380af6d66da20da990) that were addressed in https://github.com/bitcoin/bitcoin/pull/25717. This should help us move forward with the [removal of mainnet checkpoints](https://github.com/bitcoin/bitcoin/pull/25725).

  It seems like the main concern in https://github.com/bitcoin/bitcoin/pull/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 a97f43d63a6e835bae20b0bc5d536df98f55d8a0
  dergoegge:
    reACK a97f43d63a6e835bae20b0bc5d536df98f55d8a0
  instagibbs:
    tested ACK a97f43d63a6e835bae20b0bc5d536df98f55d8a0
  brunoerg:
    ACK a97f43d63a6e835bae20b0bc5d536df98f55d8a0

Tree-SHA512: 60b0bc6aadd8ca4c39db9cbba2da2debaaf68afcb6a8dd75c1ce48ca9e3996948fda8020930b6771a424e0f7c41b0b1068db4aa7dbe517f8fc152f1f712058ad

8 files changed, 226 insertions, 11 deletions

diff --git a/src/net_processing.cpp b/src/net_processing.cpp
index 2fdd7987d1..b7d0f5360d 100644
--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -113,9 +113,6 @@ static const int MAX_BLOCKS_IN_TRANSIT_PER_PEER = 16;
 static constexpr auto BLOCK_STALLING_TIMEOUT_DEFAULT{2s};
 /** Maximum timeout for stalling block download. */
 static constexpr auto BLOCK_STALLING_TIMEOUT_MAX{64s};
-/** Number of headers sent in one getheaders result. We rely on the assumption that if a peer sends
- *  less than this number, we reached its tip. Changing this value is a protocol upgrade. */
-static const unsigned int MAX_HEADERS_RESULTS = 2000;
 /** Maximum depth of blocks we're willing to serve as compact blocks to peers
  *  when requested. For older blocks, a regular BLOCK response will be sent. */
 static const int MAX_CMPCTBLOCK_DEPTH = 5;
@@ -2780,7 +2777,7 @@ bool PeerManagerImpl::CheckHeadersAreContinuous(const std::vector<CBlockHeader>&
 bool PeerManagerImpl::IsContinuationOfLowWorkHeadersSync(Peer& peer, CNode& pfrom, std::vector<CBlockHeader>& headers)
 {
     if (peer.m_headers_sync) {
-        auto result = peer.m_headers_sync->ProcessNextHeaders(headers, headers.size() == MAX_HEADERS_RESULTS);
+        auto result = peer.m_headers_sync->ProcessNextHeaders(headers, headers.size() == m_opts.max_headers_result);
         // If it is a valid continuation, we should treat the existing getheaders request as responded to.
         if (result.success) peer.m_last_getheaders_timestamp = {};
         if (result.request_more) {
@@ -2874,7 +2871,7 @@ bool PeerManagerImpl::TryLowWorkHeadersSync(Peer& peer, CNode& pfrom, const CBlo
         // Only try to sync with this peer if their headers message was full;
         // otherwise they don't have more headers after this so no point in
         // trying to sync their too-little-work chain.
-        if (headers.size() == MAX_HEADERS_RESULTS) {
+        if (headers.size() == m_opts.max_headers_result) {
             // Note: we could advance to the last header in this set that is
             // known to us, rather than starting at the first header (which we
             // may already have); however this is unlikely to matter much since
@@ -3186,7 +3183,7 @@ void PeerManagerImpl::ProcessHeadersMessage(CNode& pfrom, Peer& peer,
     assert(pindexLast);
 
     // Consider fetching more headers if we are not using our headers-sync mechanism.
-    if (nCount == MAX_HEADERS_RESULTS && !have_headers_sync) {
+    if (nCount == m_opts.max_headers_result && !have_headers_sync) {
         // Headers message had its maximum size; the peer may have more headers.
         if (MaybeSendGetHeaders(pfrom, GetLocator(pindexLast), peer)) {
             LogDebug(BCLog::NET, "more getheaders (%d) to end to peer=%d (startheight:%d)\n",
@@ -3194,7 +3191,7 @@ void PeerManagerImpl::ProcessHeadersMessage(CNode& pfrom, Peer& peer,
         }
     }
 
-    UpdatePeerStateForReceivedHeaders(pfrom, peer, *pindexLast, received_new_header, nCount == MAX_HEADERS_RESULTS);
+    UpdatePeerStateForReceivedHeaders(pfrom, peer, *pindexLast, received_new_header, nCount == m_opts.max_headers_result);
 
     // Consider immediately downloading blocks.
     HeadersDirectFetchBlocks(pfrom, peer, *pindexLast);
@@ -4512,7 +4509,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
 
         // we must use CBlocks, as CBlockHeaders won't include the 0x00 nTx count at the end
         std::vector<CBlock> vHeaders;
-        int nLimit = MAX_HEADERS_RESULTS;
+        int nLimit = m_opts.max_headers_result;
         LogDebug(BCLog::NET, "getheaders %d to %s from peer=%d\n", (pindex ? pindex->nHeight : -1), hashStop.IsNull() ? "end" : hashStop.ToString(), pfrom.GetId());
         for (; pindex; pindex = m_chainman.ActiveChain().Next(pindex))
         {
@@ -4996,7 +4993,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
 
         // Bypass the normal CBlock deserialization, as we don't want to risk deserializing 2000 full blocks.
         unsigned int nCount = ReadCompactSize(vRecv);
-        if (nCount > MAX_HEADERS_RESULTS) {
+        if (nCount > m_opts.max_headers_result) {
             Misbehaving(*peer, strprintf("headers message size = %u", nCount));
             return;
         }
diff --git a/src/net_processing.h b/src/net_processing.h
index a413db98e8..5f45fb1442 100644
--- a/src/net_processing.h
+++ b/src/net_processing.h
@@ -31,6 +31,9 @@ static const bool DEFAULT_PEERBLOOMFILTERS = false;
 static const bool DEFAULT_PEERBLOCKFILTERS = false;
 /** Maximum number of outstanding CMPCTBLOCK requests for the same block. */
 static const unsigned int MAX_CMPCTBLOCKS_INFLIGHT_PER_BLOCK = 3;
+/** Number of headers sent in one getheaders result. We rely on the assumption that if a peer sends
+ *  less than this number, we reached its tip. Changing this value is a protocol upgrade. */
+static const unsigned int MAX_HEADERS_RESULTS = 2000;
 
 struct CNodeStateStats {
     int nSyncHeight = -1;
@@ -71,6 +74,8 @@ public:
         //! Whether or not the internal RNG behaves deterministically (this is
         //! a test-only option).
         bool deterministic_rng{false};
+        //! Number of headers sent in one getheaders message result.
+        uint32_t max_headers_result{MAX_HEADERS_RESULTS};
     };
 
     static std::unique_ptr<PeerManager> make(CConnman& connman, AddrMan& addrman,
diff --git a/src/pow.cpp b/src/pow.cpp
index 50de8946be..6c8e7e5d98 100644
--- a/src/pow.cpp
+++ b/src/pow.cpp
@@ -134,8 +134,19 @@ bool PermittedDifficultyTransition(const Consensus::Params& params, int64_t heig
     return true;
 }
 
+// Bypasses the actual proof of work check during fuzz testing with a simplified validation checking whether
+// the most signficant bit of the last byte of the hash is set.
 bool CheckProofOfWork(uint256 hash, unsigned int nBits, const Consensus::Params& params)
 {
+#ifdef FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION
+    return (hash.data()[31] & 0x80) == 0;
+#else
+    return CheckProofOfWorkImpl(hash, nBits, params);
+#endif
+}
+
+bool CheckProofOfWorkImpl(uint256 hash, unsigned int nBits, const Consensus::Params& params)
+{
     bool fNegative;
     bool fOverflow;
     arith_uint256 bnTarget;
diff --git a/src/pow.h b/src/pow.h
index ec03f318a4..2b28ade273 100644
--- a/src/pow.h
+++ b/src/pow.h
@@ -19,6 +19,7 @@ unsigned int CalculateNextWorkRequired(const CBlockIndex* pindexLast, int64_t nF
 
 /** Check whether a block hash satisfies the proof-of-work requirement specified by nBits */
 bool CheckProofOfWork(uint256 hash, unsigned int nBits, const Consensus::Params&);
+bool CheckProofOfWorkImpl(uint256 hash, unsigned int nBits, const Consensus::Params&);
 
 /**
  * Return false if the proof-of-work requirement specified by new_nbits at a
diff --git a/src/test/fuzz/CMakeLists.txt b/src/test/fuzz/CMakeLists.txt
index 165add2e5a..1c7b0d5c25 100644
--- a/src/test/fuzz/CMakeLists.txt
+++ b/src/test/fuzz/CMakeLists.txt
@@ -72,6 +72,7 @@ add_executable(fuzz
   netbase_dns_lookup.cpp
   node_eviction.cpp
   p2p_handshake.cpp
+  p2p_headers_presync.cpp
   p2p_transport_serialization.cpp
   package_eval.cpp
   parse_hd_keypath.cpp
diff --git a/src/test/fuzz/integer.cpp b/src/test/fuzz/integer.cpp
index e27b2065d5..b9e3154106 100644
--- a/src/test/fuzz/integer.cpp
+++ b/src/test/fuzz/integer.cpp
@@ -69,7 +69,7 @@ FUZZ_TARGET(integer, .init = initialize_integer)
     const bool b = fuzzed_data_provider.ConsumeBool();
 
     const Consensus::Params& consensus_params = Params().GetConsensus();
-    (void)CheckProofOfWork(u256, u32, consensus_params);
+    (void)CheckProofOfWorkImpl(u256, u32, consensus_params);
     if (u64 <= MAX_MONEY) {
         const uint64_t compressed_money_amount = CompressAmount(u64);
         assert(u64 == DecompressAmount(compressed_money_amount));
diff --git a/src/test/fuzz/p2p_headers_presync.cpp b/src/test/fuzz/p2p_headers_presync.cpp
new file mode 100644
index 0000000000..b86d03442d
--- /dev/null
+++ b/src/test/fuzz/p2p_headers_presync.cpp
@@ -0,0 +1,200 @@
+#include <blockencodings.h>
+#include <net.h>
+#include <net_processing.h>
+#include <netmessagemaker.h>
+#include <node/peerman_args.h>
+#include <pow.h>
+#include <test/fuzz/FuzzedDataProvider.h>
+#include <test/fuzz/fuzz.h>
+#include <test/fuzz/util.h>
+#include <test/util/net.h>
+#include <test/util/script.h>
+#include <test/util/setup_common.h>
+#include <validation.h>
+
+namespace {
+constexpr uint32_t FUZZ_MAX_HEADERS_RESULTS{16};
+
+class HeadersSyncSetup : public TestingSetup
+{
+    std::vector<CNode*> m_connections;
+
+public:
+    HeadersSyncSetup(const ChainType chain_type = ChainType::MAIN,
+                     TestOpts opts = {})
+        : TestingSetup(chain_type, opts)
+    {
+        PeerManager::Options peerman_opts;
+        node::ApplyArgsManOptions(*m_node.args, peerman_opts);
+        peerman_opts.max_headers_result = FUZZ_MAX_HEADERS_RESULTS;
+        m_node.peerman = PeerManager::make(*m_node.connman, *m_node.addrman,
+                                           m_node.banman.get(), *m_node.chainman,
+                                           *m_node.mempool, *m_node.warnings, peerman_opts);
+
+        CConnman::Options options;
+        options.m_msgproc = m_node.peerman.get();
+        m_node.connman->Init(options);
+    }
+
+    void ResetAndInitialize() EXCLUSIVE_LOCKS_REQUIRED(NetEventsInterface::g_msgproc_mutex);
+    void SendMessage(FuzzedDataProvider& fuzzed_data_provider, CSerializedNetMsg&& msg)
+        EXCLUSIVE_LOCKS_REQUIRED(NetEventsInterface::g_msgproc_mutex);
+};
+
+void HeadersSyncSetup::ResetAndInitialize()
+{
+    m_connections.clear();
+    auto& connman = static_cast<ConnmanTestMsg&>(*m_node.connman);
+    connman.StopNodes();
+
+    NodeId id{0};
+    std::vector<ConnectionType> conn_types = {
+        ConnectionType::OUTBOUND_FULL_RELAY,
+        ConnectionType::BLOCK_RELAY,
+        ConnectionType::INBOUND
+    };
+
+    for (auto conn_type : conn_types) {
+        CAddress addr{};
+        m_connections.push_back(new CNode(id++, nullptr, addr, 0, 0, addr, "", conn_type, false));
+        CNode& p2p_node = *m_connections.back();
+
+        connman.Handshake(
+            /*node=*/p2p_node,
+            /*successfully_connected=*/true,
+            /*remote_services=*/ServiceFlags(NODE_NETWORK | NODE_WITNESS),
+            /*local_services=*/ServiceFlags(NODE_NETWORK | NODE_WITNESS),
+            /*version=*/PROTOCOL_VERSION,
+            /*relay_txs=*/true);
+
+        connman.AddTestNode(p2p_node);
+    }
+}
+
+void HeadersSyncSetup::SendMessage(FuzzedDataProvider& fuzzed_data_provider, CSerializedNetMsg&& msg)
+{
+    auto& connman = static_cast<ConnmanTestMsg&>(*m_node.connman);
+    CNode& connection = *PickValue(fuzzed_data_provider, m_connections);
+
+    connman.FlushSendBuffer(connection);
+    (void)connman.ReceiveMsgFrom(connection, std::move(msg));
+    connection.fPauseSend = false;
+    try {
+        connman.ProcessMessagesOnce(connection);
+    } catch (const std::ios_base::failure&) {
+    }
+    m_node.peerman->SendMessages(&connection);
+}
+
+CBlockHeader ConsumeHeader(FuzzedDataProvider& fuzzed_data_provider, const uint256& prev_hash, uint32_t prev_nbits)
+{
+    CBlockHeader header;
+    header.nNonce = 0;
+    // Either use the previous difficulty or let the fuzzer choose
+    header.nBits = fuzzed_data_provider.ConsumeBool() ?
+                       prev_nbits :
+                       fuzzed_data_provider.ConsumeIntegralInRange<uint32_t>(0x17058EBE, 0x1D00FFFF);
+    header.nTime = ConsumeTime(fuzzed_data_provider);
+    header.hashPrevBlock = prev_hash;
+    header.nVersion = fuzzed_data_provider.ConsumeIntegral<int32_t>();
+    return header;
+}
+
+CBlock ConsumeBlock(FuzzedDataProvider& fuzzed_data_provider, const uint256& prev_hash, uint32_t prev_nbits)
+{
+    auto header = ConsumeHeader(fuzzed_data_provider, prev_hash, prev_nbits);
+    // In order to reach the headers acceptance logic, the block is
+    // constructed in a way that will pass the mutation checks.
+    CBlock block{header};
+    CMutableTransaction tx;
+    tx.vin.resize(1);
+    tx.vout.resize(1);
+    tx.vout[0].nValue = 0;
+    tx.vin[0].scriptSig.resize(2);
+    block.vtx.push_back(MakeTransactionRef(tx));
+    block.hashMerkleRoot = block.vtx[0]->GetHash();
+    return block;
+}
+
+void FinalizeHeader(CBlockHeader& header)
+{
+    while (!CheckProofOfWork(header.GetHash(), header.nBits, Params().GetConsensus())) {
+        ++(header.nNonce);
+    }
+}
+
+// Global setup works for this test as state modification (specifically in the
+// block index) would indicate a bug.
+HeadersSyncSetup* g_testing_setup;
+
+void initialize()
+{
+    static auto setup = MakeNoLogFileContext<HeadersSyncSetup>(ChainType::MAIN, {.extra_args = {"-checkpoints=0"}});
+    g_testing_setup = setup.get();
+}
+} // namespace
+
+FUZZ_TARGET(p2p_headers_presync, .init = initialize)
+{
+    ChainstateManager& chainman = *g_testing_setup->m_node.chainman;
+
+    LOCK(NetEventsInterface::g_msgproc_mutex);
+
+    g_testing_setup->ResetAndInitialize();
+
+    FuzzedDataProvider fuzzed_data_provider{buffer.data(), buffer.size()};
+
+    CBlockHeader base{Params().GenesisBlock()};
+    SetMockTime(base.nTime);
+
+    // The chain is just a single block, so this is equal to 1
+    size_t original_index_size{WITH_LOCK(cs_main, return chainman.m_blockman.m_block_index.size())};
+
+    LIMITED_WHILE(fuzzed_data_provider.ConsumeBool(), 100)
+    {
+        auto finalized_block = [&]() {
+            CBlock block = ConsumeBlock(fuzzed_data_provider, base.GetHash(), base.nBits);
+            FinalizeHeader(block);
+            return block;
+        };
+
+        // Send low-work headers, compact blocks, and blocks
+        CallOneOf(
+            fuzzed_data_provider,
+            [&]() NO_THREAD_SAFETY_ANALYSIS {
+                // Send FUZZ_MAX_HEADERS_RESULTS headers
+                std::vector<CBlock> headers;
+                headers.resize(FUZZ_MAX_HEADERS_RESULTS);
+                for (CBlock& header : headers) {
+                    header = ConsumeHeader(fuzzed_data_provider, base.GetHash(), base.nBits);
+                    FinalizeHeader(header);
+                    base = header;
+                }
+
+                auto headers_msg = NetMsg::Make(NetMsgType::HEADERS, TX_WITH_WITNESS(headers));
+                g_testing_setup->SendMessage(fuzzed_data_provider, std::move(headers_msg));
+            },
+            [&]() NO_THREAD_SAFETY_ANALYSIS {
+                // Send a compact block
+                auto block = finalized_block();
+                CBlockHeaderAndShortTxIDs cmpct_block{block, fuzzed_data_provider.ConsumeIntegral<uint64_t>()};
+
+                auto headers_msg = NetMsg::Make(NetMsgType::CMPCTBLOCK, TX_WITH_WITNESS(cmpct_block));
+                g_testing_setup->SendMessage(fuzzed_data_provider, std::move(headers_msg));
+            },
+            [&]() NO_THREAD_SAFETY_ANALYSIS {
+                // Send a block
+                auto block = finalized_block();
+
+                auto headers_msg = NetMsg::Make(NetMsgType::BLOCK, TX_WITH_WITNESS(block));
+                g_testing_setup->SendMessage(fuzzed_data_provider, std::move(headers_msg));
+            });
+    }
+
+    // The headers/blocks sent in this test should never be stored, as the chains don't have the work required
+    // to meet the anti-DoS work threshold. So, if at any point the block index grew in size, then there's a bug
+    // in the headers pre-sync logic.
+    assert(WITH_LOCK(cs_main, return chainman.m_blockman.m_block_index.size()) == original_index_size);
+
+    g_testing_setup->m_node.validation_signals->SyncWithValidationInterfaceQueue();
+}
diff --git a/src/test/fuzz/pow.cpp b/src/test/fuzz/pow.cpp
index 05cdb740e4..dba999ce4f 100644
--- a/src/test/fuzz/pow.cpp
+++ b/src/test/fuzz/pow.cpp
@@ -80,7 +80,7 @@ FUZZ_TARGET(pow, .init = initialize_pow)
         {
             const std::optional<uint256> hash = ConsumeDeserializable<uint256>(fuzzed_data_provider);
             if (hash) {
-                (void)CheckProofOfWork(*hash, fuzzed_data_provider.ConsumeIntegral<unsigned int>(), consensus_params);
+                (void)CheckProofOfWorkImpl(*hash, fuzzed_data_provider.ConsumeIntegral<unsigned int>(), consensus_params);
             }
         }
     }