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

a97f43d63a fuzz: Add harness for p2p headers sync (marcofleon)
a0eaa4749f Add FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION in PoW check (marcofleon)
a3f6f5acd8 build: Automatically define FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION for fuzz builds (marcofleon)
0c02d4b2bd 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](ed6cddd98e) 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 a97f43d63a
  dergoegge:
    reACK a97f43d63a
  instagibbs:
    tested ACK a97f43d63a
  brunoerg:
    ACK a97f43d63a

Tree-SHA512: 60b0bc6aadd8ca4c39db9cbba2da2debaaf68afcb6a8dd75c1ce48ca9e3996948fda8020930b6771a424e0f7c41b0b1068db4aa7dbe517f8fc152f1f712058ad

CMakeLists.txt

@@ -249,6 +249,7 @@ if(BUILD_FOR_FUZZING)
 
   target_compile_definitions(core_interface INTERFACE
     ABORT_ON_FAILED_ASSUME
+    FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION
   )
 endif()
 

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;
         }

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,

src/pow.cpp

@@ -134,7 +134,18 @@ 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;

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

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

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));

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();
+}

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);
             }
         }
     }