From 0c02d4b2bdbc7a3fc3031a63b3b16bafa669d51c Mon Sep 17 00:00:00 2001 From: marcofleon Date: Tue, 13 Aug 2024 11:32:07 +0100 Subject: [PATCH 1/4] net_processing: Make MAX_HEADERS_RESULTS a PeerManager option --- src/net_processing.cpp | 15 ++++++--------- src/net_processing.h | 5 +++++ 2 files changed, 11 insertions(+), 9 deletions(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index fe179107414..aa19bec69cd 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; @@ -2786,7 +2783,7 @@ bool PeerManagerImpl::CheckHeadersAreContinuous(const std::vector& bool PeerManagerImpl::IsContinuationOfLowWorkHeadersSync(Peer& peer, CNode& pfrom, std::vector& 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) { @@ -2880,7 +2877,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 @@ -3192,7 +3189,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", @@ -3200,7 +3197,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); @@ -4518,7 +4515,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 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)) { @@ -5002,7 +4999,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 a413db98e84..5f45fb1442a 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 make(CConnman& connman, AddrMan& addrman, From a3f6f5acd89f2f5bb136ec247f259d212e8944d0 Mon Sep 17 00:00:00 2001 From: marcofleon Date: Tue, 13 Aug 2024 15:47:19 +0100 Subject: [PATCH 2/4] build: Automatically define FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION for fuzz builds --- CMakeLists.txt | 1 + 1 file changed, 1 insertion(+) diff --git a/CMakeLists.txt b/CMakeLists.txt index 5ef80ffc6f9..8185194508a 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -255,6 +255,7 @@ if(BUILD_FOR_FUZZING) target_compile_definitions(core_interface INTERFACE ABORT_ON_FAILED_ASSUME + FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION ) endif() From a0eaa4749fe0f755e113eee70dee1989bdc07ad5 Mon Sep 17 00:00:00 2001 From: marcofleon Date: Tue, 13 Aug 2024 11:42:59 +0100 Subject: [PATCH 3/4] 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`. --- src/pow.cpp | 11 +++++++++++ src/pow.h | 1 + src/test/fuzz/integer.cpp | 2 +- src/test/fuzz/pow.cpp | 2 +- 4 files changed, 14 insertions(+), 2 deletions(-) diff --git a/src/pow.cpp b/src/pow.cpp index 50de8946be6..6c8e7e5d98a 100644 --- a/src/pow.cpp +++ b/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; diff --git a/src/pow.h b/src/pow.h index ec03f318a42..2b28ade273c 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/integer.cpp b/src/test/fuzz/integer.cpp index 02c6796d11e..87838b7b39f 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/pow.cpp b/src/test/fuzz/pow.cpp index 05cdb740e48..dba999ce4fa 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 hash = ConsumeDeserializable(fuzzed_data_provider); if (hash) { - (void)CheckProofOfWork(*hash, fuzzed_data_provider.ConsumeIntegral(), consensus_params); + (void)CheckProofOfWorkImpl(*hash, fuzzed_data_provider.ConsumeIntegral(), consensus_params); } } } From a97f43d63a6e835bae20b0bc5d536df98f55d8a0 Mon Sep 17 00:00:00 2001 From: marcofleon Date: Tue, 13 Aug 2024 12:08:42 +0100 Subject: [PATCH 4/4] fuzz: Add harness for p2p headers sync --- src/test/fuzz/CMakeLists.txt | 1 + src/test/fuzz/p2p_headers_presync.cpp | 200 ++++++++++++++++++++++++++ 2 files changed, 201 insertions(+) create mode 100644 src/test/fuzz/p2p_headers_presync.cpp diff --git a/src/test/fuzz/CMakeLists.txt b/src/test/fuzz/CMakeLists.txt index 0bff9bf784a..be1d108c48d 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/p2p_headers_presync.cpp b/src/test/fuzz/p2p_headers_presync.cpp new file mode 100644 index 00000000000..b86d03442dd --- /dev/null +++ b/src/test/fuzz/p2p_headers_presync.cpp @@ -0,0 +1,200 @@ +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +namespace { +constexpr uint32_t FUZZ_MAX_HEADERS_RESULTS{16}; + +class HeadersSyncSetup : public TestingSetup +{ + std::vector 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(*m_node.connman); + connman.StopNodes(); + + NodeId id{0}; + std::vector 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(*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(0x17058EBE, 0x1D00FFFF); + header.nTime = ConsumeTime(fuzzed_data_provider); + header.hashPrevBlock = prev_hash; + header.nVersion = fuzzed_data_provider.ConsumeIntegral(); + 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(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 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()}; + + 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(); +}