Skip to content

Commit 2bf721e

Browse files
committed
Merge bitcoin#30661: fuzz: Test headers pre-sync through p2p
a97f43d fuzz: Add harness for p2p headers sync (marcofleon) a0eaa47 Add FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION in PoW check (marcofleon) a3f6f5a build: Automatically define FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION for fuzz builds (marcofleon) 0c02d4b net_processing: Make MAX_HEADERS_RESULTS a PeerManager option (marcofleon) Pull request description: This PR reopens bitcoin#28043. It's a regression fuzz test for bitcoin#26355 and [a couple bugs](bitcoin@ed6cddd) that were addressed in bitcoin#25717. This should help us move forward with the [removal of mainnet checkpoints](bitcoin#25725). It seems like the main concern in bitcoin#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 a97f43d dergoegge: reACK a97f43d instagibbs: tested ACK a97f43d brunoerg: ACK a97f43d Tree-SHA512: 60b0bc6aadd8ca4c39db9cbba2da2debaaf68afcb6a8dd75c1ce48ca9e3996948fda8020930b6771a424e0f7c41b0b1068db4aa7dbe517f8fc152f1f712058ad
2 parents c38e999 + a97f43d commit 2bf721e

File tree

9 files changed

+227
-11
lines changed

9 files changed

+227
-11
lines changed

CMakeLists.txt

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -249,6 +249,7 @@ if(BUILD_FOR_FUZZING)
249249

250250
target_compile_definitions(core_interface INTERFACE
251251
ABORT_ON_FAILED_ASSUME
252+
FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION
252253
)
253254
endif()
254255

src/net_processing.cpp

Lines changed: 6 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -113,9 +113,6 @@ static const int MAX_BLOCKS_IN_TRANSIT_PER_PEER = 16;
113113
static constexpr auto BLOCK_STALLING_TIMEOUT_DEFAULT{2s};
114114
/** Maximum timeout for stalling block download. */
115115
static constexpr auto BLOCK_STALLING_TIMEOUT_MAX{64s};
116-
/** Number of headers sent in one getheaders result. We rely on the assumption that if a peer sends
117-
* less than this number, we reached its tip. Changing this value is a protocol upgrade. */
118-
static const unsigned int MAX_HEADERS_RESULTS = 2000;
119116
/** Maximum depth of blocks we're willing to serve as compact blocks to peers
120117
* when requested. For older blocks, a regular BLOCK response will be sent. */
121118
static const int MAX_CMPCTBLOCK_DEPTH = 5;
@@ -2780,7 +2777,7 @@ bool PeerManagerImpl::CheckHeadersAreContinuous(const std::vector<CBlockHeader>&
27802777
bool PeerManagerImpl::IsContinuationOfLowWorkHeadersSync(Peer& peer, CNode& pfrom, std::vector<CBlockHeader>& headers)
27812778
{
27822779
if (peer.m_headers_sync) {
2783-
auto result = peer.m_headers_sync->ProcessNextHeaders(headers, headers.size() == MAX_HEADERS_RESULTS);
2780+
auto result = peer.m_headers_sync->ProcessNextHeaders(headers, headers.size() == m_opts.max_headers_result);
27842781
// If it is a valid continuation, we should treat the existing getheaders request as responded to.
27852782
if (result.success) peer.m_last_getheaders_timestamp = {};
27862783
if (result.request_more) {
@@ -2874,7 +2871,7 @@ bool PeerManagerImpl::TryLowWorkHeadersSync(Peer& peer, CNode& pfrom, const CBlo
28742871
// Only try to sync with this peer if their headers message was full;
28752872
// otherwise they don't have more headers after this so no point in
28762873
// trying to sync their too-little-work chain.
2877-
if (headers.size() == MAX_HEADERS_RESULTS) {
2874+
if (headers.size() == m_opts.max_headers_result) {
28782875
// Note: we could advance to the last header in this set that is
28792876
// known to us, rather than starting at the first header (which we
28802877
// may already have); however this is unlikely to matter much since
@@ -3186,15 +3183,15 @@ void PeerManagerImpl::ProcessHeadersMessage(CNode& pfrom, Peer& peer,
31863183
assert(pindexLast);
31873184

31883185
// Consider fetching more headers if we are not using our headers-sync mechanism.
3189-
if (nCount == MAX_HEADERS_RESULTS && !have_headers_sync) {
3186+
if (nCount == m_opts.max_headers_result && !have_headers_sync) {
31903187
// Headers message had its maximum size; the peer may have more headers.
31913188
if (MaybeSendGetHeaders(pfrom, GetLocator(pindexLast), peer)) {
31923189
LogDebug(BCLog::NET, "more getheaders (%d) to end to peer=%d (startheight:%d)\n",
31933190
pindexLast->nHeight, pfrom.GetId(), peer.m_starting_height);
31943191
}
31953192
}
31963193

3197-
UpdatePeerStateForReceivedHeaders(pfrom, peer, *pindexLast, received_new_header, nCount == MAX_HEADERS_RESULTS);
3194+
UpdatePeerStateForReceivedHeaders(pfrom, peer, *pindexLast, received_new_header, nCount == m_opts.max_headers_result);
31983195

31993196
// Consider immediately downloading blocks.
32003197
HeadersDirectFetchBlocks(pfrom, peer, *pindexLast);
@@ -4512,7 +4509,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
45124509

45134510
// we must use CBlocks, as CBlockHeaders won't include the 0x00 nTx count at the end
45144511
std::vector<CBlock> vHeaders;
4515-
int nLimit = MAX_HEADERS_RESULTS;
4512+
int nLimit = m_opts.max_headers_result;
45164513
LogDebug(BCLog::NET, "getheaders %d to %s from peer=%d\n", (pindex ? pindex->nHeight : -1), hashStop.IsNull() ? "end" : hashStop.ToString(), pfrom.GetId());
45174514
for (; pindex; pindex = m_chainman.ActiveChain().Next(pindex))
45184515
{
@@ -4996,7 +4993,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
49964993

49974994
// Bypass the normal CBlock deserialization, as we don't want to risk deserializing 2000 full blocks.
49984995
unsigned int nCount = ReadCompactSize(vRecv);
4999-
if (nCount > MAX_HEADERS_RESULTS) {
4996+
if (nCount > m_opts.max_headers_result) {
50004997
Misbehaving(*peer, strprintf("headers message size = %u", nCount));
50014998
return;
50024999
}

src/net_processing.h

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,9 @@ static const bool DEFAULT_PEERBLOOMFILTERS = false;
3131
static const bool DEFAULT_PEERBLOCKFILTERS = false;
3232
/** Maximum number of outstanding CMPCTBLOCK requests for the same block. */
3333
static const unsigned int MAX_CMPCTBLOCKS_INFLIGHT_PER_BLOCK = 3;
34+
/** Number of headers sent in one getheaders result. We rely on the assumption that if a peer sends
35+
* less than this number, we reached its tip. Changing this value is a protocol upgrade. */
36+
static const unsigned int MAX_HEADERS_RESULTS = 2000;
3437

3538
struct CNodeStateStats {
3639
int nSyncHeight = -1;
@@ -71,6 +74,8 @@ class PeerManager : public CValidationInterface, public NetEventsInterface
7174
//! Whether or not the internal RNG behaves deterministically (this is
7275
//! a test-only option).
7376
bool deterministic_rng{false};
77+
//! Number of headers sent in one getheaders message result.
78+
uint32_t max_headers_result{MAX_HEADERS_RESULTS};
7479
};
7580

7681
static std::unique_ptr<PeerManager> make(CConnman& connman, AddrMan& addrman,

src/pow.cpp

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -134,7 +134,18 @@ bool PermittedDifficultyTransition(const Consensus::Params& params, int64_t heig
134134
return true;
135135
}
136136

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.
137139
bool CheckProofOfWork(uint256 hash, unsigned int nBits, const Consensus::Params& params)
140+
{
141+
#ifdef FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION
142+
return (hash.data()[31] & 0x80) == 0;
143+
#else
144+
return CheckProofOfWorkImpl(hash, nBits, params);
145+
#endif
146+
}
147+
148+
bool CheckProofOfWorkImpl(uint256 hash, unsigned int nBits, const Consensus::Params& params)
138149
{
139150
bool fNegative;
140151
bool fOverflow;

src/pow.h

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@ unsigned int CalculateNextWorkRequired(const CBlockIndex* pindexLast, int64_t nF
1919

2020
/** Check whether a block hash satisfies the proof-of-work requirement specified by nBits */
2121
bool CheckProofOfWork(uint256 hash, unsigned int nBits, const Consensus::Params&);
22+
bool CheckProofOfWorkImpl(uint256 hash, unsigned int nBits, const Consensus::Params&);
2223

2324
/**
2425
* Return false if the proof-of-work requirement specified by new_nbits at a

src/test/fuzz/CMakeLists.txt

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -72,6 +72,7 @@ add_executable(fuzz
7272
netbase_dns_lookup.cpp
7373
node_eviction.cpp
7474
p2p_handshake.cpp
75+
p2p_headers_presync.cpp
7576
p2p_transport_serialization.cpp
7677
package_eval.cpp
7778
parse_hd_keypath.cpp

src/test/fuzz/integer.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -69,7 +69,7 @@ FUZZ_TARGET(integer, .init = initialize_integer)
6969
const bool b = fuzzed_data_provider.ConsumeBool();
7070

7171
const Consensus::Params& consensus_params = Params().GetConsensus();
72-
(void)CheckProofOfWork(u256, u32, consensus_params);
72+
(void)CheckProofOfWorkImpl(u256, u32, consensus_params);
7373
if (u64 <= MAX_MONEY) {
7474
const uint64_t compressed_money_amount = CompressAmount(u64);
7575
assert(u64 == DecompressAmount(compressed_money_amount));

src/test/fuzz/p2p_headers_presync.cpp

Lines changed: 200 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,200 @@
1+
#include <blockencodings.h>
2+
#include <net.h>
3+
#include <net_processing.h>
4+
#include <netmessagemaker.h>
5+
#include <node/peerman_args.h>
6+
#include <pow.h>
7+
#include <test/fuzz/FuzzedDataProvider.h>
8+
#include <test/fuzz/fuzz.h>
9+
#include <test/fuzz/util.h>
10+
#include <test/util/net.h>
11+
#include <test/util/script.h>
12+
#include <test/util/setup_common.h>
13+
#include <validation.h>
14+
15+
namespace {
16+
constexpr uint32_t FUZZ_MAX_HEADERS_RESULTS{16};
17+
18+
class HeadersSyncSetup : public TestingSetup
19+
{
20+
std::vector<CNode*> m_connections;
21+
22+
public:
23+
HeadersSyncSetup(const ChainType chain_type = ChainType::MAIN,
24+
TestOpts opts = {})
25+
: TestingSetup(chain_type, opts)
26+
{
27+
PeerManager::Options peerman_opts;
28+
node::ApplyArgsManOptions(*m_node.args, peerman_opts);
29+
peerman_opts.max_headers_result = FUZZ_MAX_HEADERS_RESULTS;
30+
m_node.peerman = PeerManager::make(*m_node.connman, *m_node.addrman,
31+
m_node.banman.get(), *m_node.chainman,
32+
*m_node.mempool, *m_node.warnings, peerman_opts);
33+
34+
CConnman::Options options;
35+
options.m_msgproc = m_node.peerman.get();
36+
m_node.connman->Init(options);
37+
}
38+
39+
void ResetAndInitialize() EXCLUSIVE_LOCKS_REQUIRED(NetEventsInterface::g_msgproc_mutex);
40+
void SendMessage(FuzzedDataProvider& fuzzed_data_provider, CSerializedNetMsg&& msg)
41+
EXCLUSIVE_LOCKS_REQUIRED(NetEventsInterface::g_msgproc_mutex);
42+
};
43+
44+
void HeadersSyncSetup::ResetAndInitialize()
45+
{
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 = {
52+
ConnectionType::OUTBOUND_FULL_RELAY,
53+
ConnectionType::BLOCK_RELAY,
54+
ConnectionType::INBOUND
55+
};
56+
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(
63+
/*node=*/p2p_node,
64+
/*successfully_connected=*/true,
65+
/*remote_services=*/ServiceFlags(NODE_NETWORK | NODE_WITNESS),
66+
/*local_services=*/ServiceFlags(NODE_NETWORK | NODE_WITNESS),
67+
/*version=*/PROTOCOL_VERSION,
68+
/*relay_txs=*/true);
69+
70+
connman.AddTestNode(p2p_node);
71+
}
72+
}
73+
74+
void HeadersSyncSetup::SendMessage(FuzzedDataProvider& fuzzed_data_provider, CSerializedNetMsg&& msg)
75+
{
76+
auto& connman = static_cast<ConnmanTestMsg&>(*m_node.connman);
77+
CNode& connection = *PickValue(fuzzed_data_provider, m_connections);
78+
79+
connman.FlushSendBuffer(connection);
80+
(void)connman.ReceiveMsgFrom(connection, std::move(msg));
81+
connection.fPauseSend = false;
82+
try {
83+
connman.ProcessMessagesOnce(connection);
84+
} catch (const std::ios_base::failure&) {
85+
}
86+
m_node.peerman->SendMessages(&connection);
87+
}
88+
89+
CBlockHeader ConsumeHeader(FuzzedDataProvider& fuzzed_data_provider, const uint256& prev_hash, uint32_t prev_nbits)
90+
{
91+
CBlockHeader header;
92+
header.nNonce = 0;
93+
// Either use the previous difficulty or let the fuzzer choose
94+
header.nBits = fuzzed_data_provider.ConsumeBool() ?
95+
prev_nbits :
96+
fuzzed_data_provider.ConsumeIntegralInRange<uint32_t>(0x17058EBE, 0x1D00FFFF);
97+
header.nTime = ConsumeTime(fuzzed_data_provider);
98+
header.hashPrevBlock = prev_hash;
99+
header.nVersion = fuzzed_data_provider.ConsumeIntegral<int32_t>();
100+
return header;
101+
}
102+
103+
CBlock ConsumeBlock(FuzzedDataProvider& fuzzed_data_provider, const uint256& prev_hash, uint32_t prev_nbits)
104+
{
105+
auto header = ConsumeHeader(fuzzed_data_provider, prev_hash, prev_nbits);
106+
// In order to reach the headers acceptance logic, the block is
107+
// constructed in a way that will pass the mutation checks.
108+
CBlock block{header};
109+
CMutableTransaction tx;
110+
tx.vin.resize(1);
111+
tx.vout.resize(1);
112+
tx.vout[0].nValue = 0;
113+
tx.vin[0].scriptSig.resize(2);
114+
block.vtx.push_back(MakeTransactionRef(tx));
115+
block.hashMerkleRoot = block.vtx[0]->GetHash();
116+
return block;
117+
}
118+
119+
void FinalizeHeader(CBlockHeader& header)
120+
{
121+
while (!CheckProofOfWork(header.GetHash(), header.nBits, Params().GetConsensus())) {
122+
++(header.nNonce);
123+
}
124+
}
125+
126+
// Global setup works for this test as state modification (specifically in the
127+
// block index) would indicate a bug.
128+
HeadersSyncSetup* g_testing_setup;
129+
130+
void initialize()
131+
{
132+
static auto setup = MakeNoLogFileContext<HeadersSyncSetup>(ChainType::MAIN, {.extra_args = {"-checkpoints=0"}});
133+
g_testing_setup = setup.get();
134+
}
135+
} // namespace
136+
137+
FUZZ_TARGET(p2p_headers_presync, .init = initialize)
138+
{
139+
ChainstateManager& chainman = *g_testing_setup->m_node.chainman;
140+
141+
LOCK(NetEventsInterface::g_msgproc_mutex);
142+
143+
g_testing_setup->ResetAndInitialize();
144+
145+
FuzzedDataProvider fuzzed_data_provider{buffer.data(), buffer.size()};
146+
147+
CBlockHeader base{Params().GenesisBlock()};
148+
SetMockTime(base.nTime);
149+
150+
// The chain is just a single block, so this is equal to 1
151+
size_t original_index_size{WITH_LOCK(cs_main, return chainman.m_blockman.m_block_index.size())};
152+
153+
LIMITED_WHILE(fuzzed_data_provider.ConsumeBool(), 100)
154+
{
155+
auto finalized_block = [&]() {
156+
CBlock block = ConsumeBlock(fuzzed_data_provider, base.GetHash(), base.nBits);
157+
FinalizeHeader(block);
158+
return block;
159+
};
160+
161+
// Send low-work headers, compact blocks, and blocks
162+
CallOneOf(
163+
fuzzed_data_provider,
164+
[&]() NO_THREAD_SAFETY_ANALYSIS {
165+
// Send FUZZ_MAX_HEADERS_RESULTS headers
166+
std::vector<CBlock> headers;
167+
headers.resize(FUZZ_MAX_HEADERS_RESULTS);
168+
for (CBlock& header : headers) {
169+
header = ConsumeHeader(fuzzed_data_provider, base.GetHash(), base.nBits);
170+
FinalizeHeader(header);
171+
base = header;
172+
}
173+
174+
auto headers_msg = NetMsg::Make(NetMsgType::HEADERS, TX_WITH_WITNESS(headers));
175+
g_testing_setup->SendMessage(fuzzed_data_provider, std::move(headers_msg));
176+
},
177+
[&]() NO_THREAD_SAFETY_ANALYSIS {
178+
// Send a compact block
179+
auto block = finalized_block();
180+
CBlockHeaderAndShortTxIDs cmpct_block{block, fuzzed_data_provider.ConsumeIntegral<uint64_t>()};
181+
182+
auto headers_msg = NetMsg::Make(NetMsgType::CMPCTBLOCK, TX_WITH_WITNESS(cmpct_block));
183+
g_testing_setup->SendMessage(fuzzed_data_provider, std::move(headers_msg));
184+
},
185+
[&]() NO_THREAD_SAFETY_ANALYSIS {
186+
// Send a block
187+
auto block = finalized_block();
188+
189+
auto headers_msg = NetMsg::Make(NetMsgType::BLOCK, TX_WITH_WITNESS(block));
190+
g_testing_setup->SendMessage(fuzzed_data_provider, std::move(headers_msg));
191+
});
192+
}
193+
194+
// The headers/blocks sent in this test should never be stored, as the chains don't have the work required
195+
// to meet the anti-DoS work threshold. So, if at any point the block index grew in size, then there's a bug
196+
// in the headers pre-sync logic.
197+
assert(WITH_LOCK(cs_main, return chainman.m_blockman.m_block_index.size()) == original_index_size);
198+
199+
g_testing_setup->m_node.validation_signals->SyncWithValidationInterfaceQueue();
200+
}

src/test/fuzz/pow.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -80,7 +80,7 @@ FUZZ_TARGET(pow, .init = initialize_pow)
8080
{
8181
const std::optional<uint256> hash = ConsumeDeserializable<uint256>(fuzzed_data_provider);
8282
if (hash) {
83-
(void)CheckProofOfWork(*hash, fuzzed_data_provider.ConsumeIntegral<unsigned int>(), consensus_params);
83+
(void)CheckProofOfWorkImpl(*hash, fuzzed_data_provider.ConsumeIntegral<unsigned int>(), consensus_params);
8484
}
8585
}
8686
}

0 commit comments

Comments
 (0)