Repository navigation
Conversation
AuxPoW headers carry the parent coinbase transaction and merkle branches, so a 'headers' message is no longer bounded by MAX_HEADERS_RESULTS alone, and every peer rejects a message larger than MAX_PROTOCOL_MESSAGE_LENGTH. - getheaders responses stop before the header that would make the message exceed MAX_PROTOCOL_MESSAGE_LENGTH, always including at least one header, and are serialized after cs_main is released. - Header announcements that would exceed the limit fall back to an inv for the tip. - A received 'headers' message with fewer than the maximum number of headers but at least MAX_PROTOCOL_MESSAGE_LENGTH - MAX_BLOCK_SERIALIZED_SIZE bytes may have more headers behind it, in both the normal and the low-work headers sync. A message cut short by size reaches that size, because the header that did not fit belongs to a block of at most MAX_BLOCK_SERIALIZED_SIZE bytes. Peers running earlier versions accept every message this produces, where an oversized message previously ended the connection. They treat a message cut short by size as the end of the headers: - Once their headers chain has reached their minimum chain work, they continue at the next block announcement. - Before that, during their low-work headers sync, they do not continue past such a message. This is no worse than before, when the connection ended at the same point. The size threshold assumes that the AuxPoW kept with each header is the one of its block. A header first received with a different, larger AuxPoW keeps it, so a message cut short before such a header can be smaller than the threshold. The receiver then treats it as complete and continues at the next block announcement. Consensus rules are unchanged. Refs #258 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Codex Review: Didn't find any major issues. Nice work! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
Review input for #266 (head #272 is stacked on this branch. It adds 8 unit tests and
All of them pass against #266. Since #272 is stacked, CI won't run on it until it's merged into this branch or retargeted. Finding: a stored oversized header payload can make a cut-short reply look final (no test fails today)
Minor
Design note: re-locking and looking up |
Add unit tests for #266, which bounds the size of headers messages: - peerman_tests: the header count or message size decides whether a received headers message makes us ask for more, at MAX_HEADERS_RESULTS and at THRESHOLD_HEADERS_MESSAGE_SIZE. A reply of 2,000 small headers is unchanged byte for byte. Replies through large AuxPoW headers stay within MAX_PROTOCOL_MESSAGE_LENGTH and reach the tip. A header whose stored AuxPoW is close to 4 MB goes alone. The normal and low-work requesters keep asking after messages cut short by size. A v1.0.0 sender's last message over the threshold costs one empty reply. An announcement too large for one message falls back to inv. - headers_sync_chainwork_tests: HeadersSyncState continues through messages cut short by size in PRESYNC and REDOWNLOAD. Also add the release notes for #266. Refs #258 Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
Summary
AuxPoW headers carry the parent coinbase transaction and merkle branches, so a
headersmessage is no longer bounded byMAX_HEADERS_RESULTSalone. Every peer rejects a message larger thanMAX_PROTOCOL_MESSAGE_LENGTH(4,000,000 bytes) and ends the connection. A run of blocks with large AuxPoW payloads could therefore make agetheadersresponse, or a header announcement, larger than any peer accepts, and headers sync could not get past it.What changed (
src/net_processing.cpp):getheadersresponses stop before the header that would make the message exceedMAX_HEADERS_MESSAGE_SIZE(=MAX_PROTOCOL_MESSAGE_LENGTH). A response always includes at least one header.cs_mainand serialized after it is released. TheCBlockcopies share the block index's immutable AuxPoW payloads.pindexBestHeaderSentis reset to the last header actually sent, after the message is queued. An announcement made while the message was being serialized is repeated, which keeps the existing "reset, not max" behaviour.SendMessagesthat would exceed the limit fall back to aninvfor the tip. The peer then asks for the headers through the boundedgetheaderspath.MayHaveMoreHeaders()treats aheadersmessage as possibly cut short when it has the maximum number of headers, or when it is at leastTHRESHOLD_HEADERS_MESSAGE_SIZE=MAX_PROTOCOL_MESSAGE_LENGTH - MAX_BLOCK_SERIALIZED_SIZE(2,000,000) bytes.MAX_BLOCK_SERIALIZED_SIZEbytes. A single header never does.full_headers_messageforHeadersSyncState(presync and redownload), and the IBD check inUpdatePeerStateForReceivedHeaders.getheaders, answered with a smaller or empty message.Consensus rules are unchanged. There is no protocol version bump.
Refs #258
Testing
Built with
cmake -B build -G Ninja -DCMAKE_BUILD_TYPE=Release -DBUILD_GUI=OFF -DBUILD_BENCH=OFF -DBUILD_FUZZ_BINARY=OFF(targetstest_qbit qbitd qbit-cli). Results at this head,f18fb5069a:New functional test
test/functional/p2p_auxpow_headers_size.py(registered intest_runner.py):submitauxblock.getheadersresponse to a raw peer is at most 4,000,000 and at least 2,000,000 bytes, is cut short, and continues contiguously.invalidateblock/reconsiderblock) falls back toinv.-minimumchainworkat the tip, so it goes through presync and redownload.net_processing.cppchange it fails (AssertionError: 4000000 < 5602133).Unit tests:
build/bin/test_qbit --run_test='pqc_tests:pqc_sigcache_tests:txvalidationcache_tests:script_tests:net_tests:net_peer_connection_tests:net_peer_eviction_tests:netbase_tests:denialofservice_tests:peerman_tests:headers_sync_chainwork_tests:blockencodings_tests': 133 cases, 586,298 assertions, no errors.Functional tests:
build/test/functional/test_runner.py --jobs=4over the headers-sync, compact-block and AuxPoW scripts: 20 of 20 runs passed.p2p_auxpow_headers_size,feature_auxpow,feature_auxpow_commitment_activation,feature_minchainwork,feature_minchainwork_requested_blockp2p_block_sync(v1, v2),p2p_compactblocks,p2p_compactblocks_blocksonly,p2p_compactblocks_hb(v1, v2),p2p_fingerprint,p2p_getdatap2p_headers_sync_with_minchainwork,p2p_ibd_stalling(v1, v2),p2p_initial_headers_sync,p2p_invalid_messages,p2p_sendheaders,p2p_unrequested_blocksEarlier runs on the same tree: full
ctest --test-dir build -j4, 176 of 176 passed (script_assets_testsskipped, as on the base).lint-includes,lint-files,lint-tests,lint-locale-dependence,lint-include-guardsandgit diff --checkare clean.Mixed versions: checked with a base-branch
qbitdstanding in for an earlier release (the headers logic innet_processing.cppis identical betweenv1.0.0and the base). A realv1.0.0binary was not run.Target Branch
mainor a maintainer-requested release branch such as0.1.x.This targets
1.x.xfor v1.0.1 (#181 prepares that release), and then 1.1.0-rc2.Risk / Review Notes
Notes:
MAX_REDOWNLOAD_AUXPOW_SERIALIZED_SIZEinsrc/headerssync.cpp), and any limit on AuxPoW payload size. Both are separate decisions.Docs / Process Impact
Choose exactly one:
headersmessages within the protocol message limit.🤖 Generated with Claude Code
Need help on this PR? Tag
@codesmith-botwith what you need.