Repository navigation
test: unit coverage and release notes for #266 - #272
Merged
kiwidream merged 1 commit intoOct 30, 2026
Merged
Conversation
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>
Member
Author
|
@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. Keep it up! 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". |
This was referenced Oct 30, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR adds unit tests and a v1.0.1 release-notes fragment for #266 ("net: bound the size of headers messages we send"), which addresses #258. It makes no implementation changes. Every test runs #266's code at
f18fb5069athroughPeerManagerorHeadersSyncState.Refs #266
Refs #258
#266 keeps its rule private:
MayHaveMoreHeaders(),THRESHOLD_HEADERS_MESSAGE_SIZEandHeadersMessageEntrySize()are local tonet_processing.cpp. To build test input,src/test/util/net.{h,cpp}mirrors them under the same names, andSplitIntoHeadersMessages()mirrors how the sender fills a reply. The rule itself is tested through the real entry point instead: a test callsProcessMessagewith aheadersmessage and checks whether the node then sendsgetheaders. If #266 exported the rule fromnet_processing.h, the mirrors could be removed.src/test/peerman_tests.cppheaders_message_count_or_size_decides_whether_to_ask_for_moreTHRESHOLD_HEADERS_MESSAGE_SIZE - 1bytes does not, and one of exactlyTHRESHOLD_HEADERS_MESSAGE_SIZEbytes does.getheaders_reply_with_small_headers_is_unchangedgetheaders_replies_with_large_auxpow_headers_stay_within_receive_limitgetheaders_reply_with_oversized_index_auxpow_stays_within_receive_limitheaders_sync_requests_more_after_byte_limited_messagelow_work_headers_sync_continues_through_byte_limited_messagesheaders_announcement_too_large_for_one_message_reverts_to_invheadersmessage. Three blocks with 1.9 MB AuxPoW headers are announced with aninvfor the tip and noheadersmessage.src/test/headers_sync_chainwork_tests.cppheaders_sync_continues_through_byte_limited_messagesHeadersSyncStateon its own. Judged by header count alone, PRESYNC gives up after the first message cut short by size. With #266's rule, the sync goes from PRESYNC to REDOWNLOAD to FINAL across three messages and releases all 8 headers with their payloads.doc/release-notes-266.mddescribes the change and records the known limitations deferred to later releases:I did not add a functional test, because #266's
p2p_auxpow_headers_size.pyalready covers the regtest scenario.Findings on #266
None of the tests above fails against #266.
AcceptBlockHeaderdoes not replace it when the block arrives. If that copy is larger than any block could carry, a reply that stops before that header can be smaller thanTHRESHOLD_HEADERS_MESSAGE_SIZE, and the receiver takes it as the peer's tip.UpdatePeerStateForReceivedHeadersdisconnects that outbound peer.net_processing.cpp: replace the stored payload with the block's when the block is accepted, or refuse header copies whose entry is larger than the block size bound.THRESHOLD_HEADERS_MESSAGE_SIZEcomment overstates. It says that a single header never reaches the threshold. That holds for a block's own AuxPoW but not for a stored oversized one:getheaders_reply_with_oversized_index_auxpow_stays_within_receive_limitsends a single header in a 3,999,996-byte message.MayHaveMoreHeaders()treats that message as possibly cut short, so sync continues there, which is the right outcome.MAX_HEADERS_RESULTSinnet_processing.h("We rely on the assumption that if a peer sends less than this number, we reached its tip").full_headers_messageinheaderssync.h.MAX_REDOWNLOAD_AUXPOW_SERIALIZED_SIZErationale inheaderssync.cpp. It is derived from count-full messages, but is now only a per-peer memory bound.doc/release-notes-266.md.Testing
cmake -B build -DCMAKE_BUILD_TYPE=RelWithDebInfo -DBUILD_GUI=OFF -DBUILD_TESTS=ON -DBUILD_BENCH=ON -DENABLE_WALLET=ON -DWITH_ZMQ=OFF -DWITH_CCACHE=ON, thencmake --build build -j12.Results at
f98842ebde(#266'sf18fb5069aplus this commit):>=to>in net: bound the size of headers messages we send #266'sMayHaveMoreHeaders()makesheaders_message_count_or_size_decides_whether_to_ask_for_morefail. I reverted the change afterwards. Before porting, the same tests ran against an equivalent implementation. Removing the sender's size cap, the receiver's size rule or theinvfallback each made the matching tests fail.build/bin/test_qbitran 1,302 cases with no errors.build/test/functional/test_runner.py --jobs=4passed 15 of 15 runs: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_headers_sync_with_minchainwork,p2p_initial_headers_sync,p2p_sendheaders,p2p_unrequested_blocksgit diff --check, which is clean. I did not run the lint suite.core-checksdoes not run until the PR is retargeted.Target Branch
mainor a maintainer-requested release branch such as0.1.x.This PR targets
djh58/headers-message-size, the branch of #266, as the coordinator requested. #266 targets1.x.xfor v1.0.1.1.x.xis frozen for v1.0.1 (#181). A maintainer must lift the freeze before #266 merges, and the v1.0.1 release notes in #181 need this entry.Risk / Review Notes
Notes:
Docs / Process Impact
doc/release-notes-266.md.🤖 Generated with Claude Code
Need help on this PR? Tag
@codesmith-botwith what you need.