Merge bitcoin/bitcoin#30918: fuzz: Add check in p2p_headers_presync that chain work never exceeds minimum work

284bd17309 add check that chainwork doesn't exceed minimum work (marcofleon)
9aa5d1c3fc add clarification in comment (marcofleon)

Pull request description:

  A followup to https://github.com/bitcoin/bitcoin/pull/30661

  The added assertion just makes sure that the fuzz test is working as intended. If we're sure that the total work of the test chain is never more than minimum chain work, then we can be sure that the later assertion failure would actually mean that a bug in the headers presync logic was found.

  This PR also addresses:
  https://github.com/bitcoin/bitcoin/pull/30661#discussion_r1746614616
  https://github.com/bitcoin/bitcoin/pull/30661#discussion_r1764943665
  https://github.com/bitcoin/bitcoin/pull/30661#discussion_r1764961991

ACKs for top commit:
  instagibbs:
    reACK 284bd17309
  maflcko:
    review ACK 284bd17309
  achow101:
    ACK 284bd17309

Tree-SHA512: 76a9dffea4b6e13499c636d6ad26af06135319d25117c0eb40cf8dfcfdca6a4549c9b4d2ba835192ca355e0f8d476227aeabf8bdb68770def72a9fb521533fe5

src/net_processing.h

@@ -74,7 +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.
+        //! Number of headers sent in one getheaders message result (this is
+        //! a test-only option).
         uint32_t max_headers_result{MAX_HEADERS_RESULTS};
     };
 

src/test/fuzz/p2p_headers_presync.cpp

@@ -10,6 +10,7 @@
 #include <test/util/net.h>
 #include <test/util/script.h>
 #include <test/util/setup_common.h>
+#include <uint256.h>
 #include <validation.h>
 
 namespace {
@@ -20,9 +21,7 @@ class HeadersSyncSetup : public TestingSetup
     std::vector<CNode*> m_connections;
 
 public:
-    HeadersSyncSetup(const ChainType chain_type = ChainType::MAIN,
-                     TestOpts opts = {})
-        : TestingSetup(chain_type, opts)
+    HeadersSyncSetup(const ChainType chain_type, TestOpts opts) : TestingSetup(chain_type, opts)
     {
         PeerManager::Options peerman_opts;
         node::ApplyArgsManOptions(*m_node.args, peerman_opts);
@@ -116,9 +115,9 @@ CBlock ConsumeBlock(FuzzedDataProvider& fuzzed_data_provider, const uint256& pre
     return block;
 }
 
-void FinalizeHeader(CBlockHeader& header)
+void FinalizeHeader(CBlockHeader& header, const ChainstateManager& chainman)
 {
-    while (!CheckProofOfWork(header.GetHash(), header.nBits, Params().GetConsensus())) {
+    while (!CheckProofOfWork(header.GetHash(), header.nBits, chainman.GetParams().GetConsensus())) {
         ++(header.nNonce);
     }
 }
@@ -144,17 +143,20 @@ FUZZ_TARGET(p2p_headers_presync, .init = initialize)
 
     FuzzedDataProvider fuzzed_data_provider{buffer.data(), buffer.size()};
 
-    CBlockHeader base{Params().GenesisBlock()};
+    CBlockHeader base{chainman.GetParams().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())};
+    arith_uint256 total_work{WITH_LOCK(cs_main, return chainman.m_best_header->nChainWork)};
+
+    std::vector<CBlockHeader> all_headers;
 
     LIMITED_WHILE(fuzzed_data_provider.ConsumeBool(), 100)
     {
         auto finalized_block = [&]() {
             CBlock block = ConsumeBlock(fuzzed_data_provider, base.GetHash(), base.nBits);
-            FinalizeHeader(block);
+            FinalizeHeader(block, chainman);
             return block;
         };
 
@@ -167,10 +169,12 @@ FUZZ_TARGET(p2p_headers_presync, .init = initialize)
                 headers.resize(FUZZ_MAX_HEADERS_RESULTS);
                 for (CBlock& header : headers) {
                     header = ConsumeHeader(fuzzed_data_provider, base.GetHash(), base.nBits);
-                    FinalizeHeader(header);
+                    FinalizeHeader(header, chainman);
                     base = header;
                 }
 
+                all_headers.insert(all_headers.end(), headers.begin(), headers.end());
+
                 auto headers_msg = NetMsg::Make(NetMsgType::HEADERS, TX_WITH_WITNESS(headers));
                 g_testing_setup->SendMessage(fuzzed_data_provider, std::move(headers_msg));
             },
@@ -179,6 +183,8 @@ FUZZ_TARGET(p2p_headers_presync, .init = initialize)
                 auto block = finalized_block();
                 CBlockHeaderAndShortTxIDs cmpct_block{block, fuzzed_data_provider.ConsumeIntegral<uint64_t>()};
 
+                all_headers.push_back(block);
+
                 auto headers_msg = NetMsg::Make(NetMsgType::CMPCTBLOCK, TX_WITH_WITNESS(cmpct_block));
                 g_testing_setup->SendMessage(fuzzed_data_provider, std::move(headers_msg));
             },
@@ -186,9 +192,19 @@ FUZZ_TARGET(p2p_headers_presync, .init = initialize)
                 // Send a block
                 auto block = finalized_block();
 
+                all_headers.push_back(block);
+
                 auto headers_msg = NetMsg::Make(NetMsgType::BLOCK, TX_WITH_WITNESS(block));
                 g_testing_setup->SendMessage(fuzzed_data_provider, std::move(headers_msg));
             });
+
+        // This is a conservative overestimate, as base is only moved forward when sending headers. In theory,
+        // the longest chain generated by this test is 1600 (FUZZ_MAX_HEADERS_RESULTS * 100) headers. In that case,
+        // this variable will accurately reflect the chain's total work.
+        total_work += CalculateClaimedHeadersWork(all_headers);
+
+        // This test should never create a chain with more work than MinimumChainWork.
+        assert(total_work < chainman.MinimumChainWork());
     }
 
     // The headers/blocks sent in this test should never be stored, as the chains don't have the work required