backport: partial bitcoin#25717, #25963 - anti-dos header sync - #7514
backport: partial bitcoin#25717, #25963 - anti-dos header sync#7514knst wants to merge 2 commits into
Conversation
|
✅ Final review complete — no blockers (commit fabb2ce) |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 05d490d3f8
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| /** Return a CBlockLocator that refers to a block in this chain (by default the tip). */ | ||
| CBlockLocator GetLocator(const CBlockIndex* pindex = nullptr) const; | ||
| /** Return a CBlockLocator that refers to the tip in of this chain. */ | ||
| CBlockLocator GetLocator() const; |
There was a problem hiding this comment.
Removing the CChain::GetLocator(const CBlockIndex*) overload here leaves src/node/interfaces.cpp:1087 still calling active.GetLocator(index), so any build of ChainImpl::findBlock(...FoundBlock().locator(...)) fails with no matching member function instead of using the new free GetLocator(index). The backport guidance specifically warns to resolve these API conflicts rather than only matching upstream shape.
AGENTS.md reference: AGENTS.md:L153-L155
Useful? React with 👍 / 👎.
| std::string msg_type = UsesCompressedHeaders(peer) ? NetMsgType::GETHEADERS2 : NetMsgType::GETHEADERS; | ||
| MaybeSendGetHeaders(pto, | ||
| msg_type, m_chainman.ActiveChain().GetLocator(state.m_chain_sync.m_work_header->pprev), | ||
| msg_type, GetLocator(state.m_chain_sync.m_work_header->pprev), |
There was a problem hiding this comment.
When m_work_header is the genesis block, m_work_header->pprev is null; the old member call treated a null argument as the active tip and sent a genesis locator, but the new free GetLocator(nullptr) serializes an empty locator. Peers in this code handle an empty getheaders locator by looking up only hashStop, and because MaybeSendGetHeaders passes a zero stop hash they return no headers, so an initial-sync eviction probe from a genesis-only node can never elicit the headers needed to prove the peer has caught up.
Useful? React with 👍 / 👎.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (22)
🚧 Files skipped from review as they are similar to previous changes (22)
WalkthroughThe PR extracts block locator construction into free functions and updates locator callers. Header-tip notifications now pass height and timestamp instead of block-index pointers. Qt synchronization updates now use the typed Estimated code review effort: 4 (Complex) | ~45 minutes Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant Validation
participant ClientUI
participant ClientModel
participant BitcoinGUI
participant InformationWidget
Validation->>ClientUI: send header height and timestamp
ClientUI->>ClientModel: deliver BlockTip and synchronization state
ClientModel->>BitcoinGUI: emit tip data and SyncType
BitcoinGUI->>InformationWidget: update block information
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/chain.h`:
- Around line 481-482: Update the remaining caller in FillBlock to use the
standalone ::GetLocator(index) function instead of active.GetLocator(index),
matching the argument-free CChain::GetLocator declaration and restoring
compilation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 2aebc17c-9a2c-4058-97ad-766fe93eb103
📒 Files selected for processing (22)
src/chain.cppsrc/chain.hsrc/index/base.cppsrc/interfaces/node.hsrc/net_processing.cppsrc/node/interface_ui.cppsrc/node/interface_ui.hsrc/node/interfaces.cppsrc/primitives/block.hsrc/qt/bitcoin.cppsrc/qt/bitcoingui.cppsrc/qt/bitcoingui.hsrc/qt/clientmodel.cppsrc/qt/clientmodel.hsrc/qt/informationwidget.cppsrc/qt/informationwidget.hsrc/qt/rpcconsole.cppsrc/qt/rpcconsole.hsrc/qt/sendcoinsdialog.cppsrc/qt/sendcoinsdialog.hsrc/test/skiplist_tests.cppsrc/validation.cpp
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The locator and header-tip refactors are generally adapted consistently, but the indexed locator API migration is incomplete. FillBlock still calls the removed CChain::GetLocator(const CBlockIndex*) overload, causing src/node/interfaces.cpp to fail compilation until it uses the new free function.
Validated blockers were found in the Codex precheck. Sonnet is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— dash-core-commit-history (completed),gpt-5.6-sol— backport-reviewer (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 1 blocking
1 additional finding(s) omitted (not in diff).
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `src/node/interfaces.cpp`:
- [BLOCKING] src/node/interfaces.cpp:1087: bitcoin#25717 locator refactor misses the bitcoin#25494 FillBlock caller
This backport removes the indexed `CChain::GetLocator(const CBlockIndex*)` overload and introduces the free `GetLocator(const CBlockIndex*)` replacement, but `FillBlock` still calls `active.GetLocator(index)`. No matching member function now exists, so this translation unit cannot compile. Upstream commit `ed470940cdd` updated this exact caller as part of the same API refactor; apply that omitted transformation here.
BACKPORT NOTE: This PR doesn't actually have enabled anything relevant to anti-DoS header sync. Pre-sync is deleted, not disabled - no HeadersSyncState, no commitment buffer, no PRESYNC/REDOWNLOAD state. Header acceptance hasn't changed path: CheckBlockHeader → ContextualCheckBlockHeader → ChainLock conflict check → checkpoints, and headers are downloaded once. This PR includes multiple useful refactorings and code changes to reduce conflicts for further backports and reduce divergency between Dash Core codebase and Bitcoin Core. Survived changes: - refactoring of GetLocator and its usages - changed setNumBlocks interface in qt code - changed interface of notification uiInterface.NotifyHeaderTip --------------- 3add234 ui: show header pre-synchronization progress (Pieter Wuille) 738421c Emit NotifyHeaderTip signals for pre-synchronization progress (Pieter Wuille) 376086f Make validation interface capable of signalling header presync (Pieter Wuille) 93eae27 Test large reorgs with headerssync logic (Suhas Daftuar) 3555473 Track headers presync progress and log it (Pieter Wuille) 03712dd Expose HeadersSyncState::m_current_height in getpeerinfo() (Suhas Daftuar) 150a548 Test headers sync using minchainwork threshold (Suhas Daftuar) 0b6aa82 Add unit test for HeadersSyncState (Suhas Daftuar) 83c6a0c Reduce spurious messages during headers sync (Suhas Daftuar) ed6cddd Require callers of AcceptBlockHeader() to perform anti-dos checks (Suhas Daftuar) 551a8d9 Utilize anti-DoS headers download strategy (Suhas Daftuar) ed47094 Add functions to construct locators without CChain (Pieter Wuille) 84852bb Add bitdeque, an std::deque<bool> analogue that does bit packing. (Pieter Wuille) 1d4cfa4 Add function to validate difficulty changes (Suhas Daftuar) Pull request description: New nodes starting up for the first time lack protection against DoS from low-difficulty headers. While checkpoints serve as our protection against headers that fork from the main chain below the known checkpointed values, this protection only applies to nodes that have been able to download the honest chain to the checkpointed heights. We can protect all nodes from DoS from low-difficulty headers by adopting a different strategy: before we commit to storing a header in permanent storage, first verify that the header is part of a chain that has sufficiently high work (either `nMinimumChainWork`, or something comparable to our tip). This means that we will download headers from a given peer twice: once to verify the work on the chain, and a second time when permanently storing the headers. The p2p protocol doesn't provide an easy way for us to ensure that we receive the same headers during the second download of peer's headers chain. To ensure that a peer doesn't (say) give us the main chain in phase 1 to trick us into permanently storing an alternate, low-work chain in phase 2, we store commitments to the headers during our first download, which we validate in the second download. Some parameters must be chosen for commitment size/frequency in phase 1, and validation of commitments in phase 2. In this PR, those parameters are chosen to both (a) minimize the per-peer memory usage that an attacker could utilize, and (b) bound the expected amount of permanent memory that an attacker could get us to use to be well-below the memory growth that we'd get from the honest chain (where we expect 1 new block header every 10 minutes). After this PR, we should be able to remove checkpoints from our code, which is a nice philosophical change for us to make as well, as there has been confusion over the years about the role checkpoints play in Bitcoin's consensus algorithm. Thanks to Pieter Wuille for collaborating on this design. ACKs for top commit: Sjors: re-tACK 3add234 mzumsande: re-ACK 3add234 sipa: re-ACK 3add234 glozow: ACK 3add234 Tree-SHA512: e7789d65f62f72141b8899eb4a2fb3d0621278394d2d7adaa004675250118f89a4e4cb42777fe56649d744ec445ad95141e10f6def65f0a58b7b35b2e654a875 Co-authored-by: fanquake <fanquake@gmail.com>
…tidy fixup 6b24dfe CBlockLocator: performance-move-const-arg Clang tidy fixups (Jon Atack) Pull request description: Fix Clang-tidy CI errors on master. See https://cirrus-ci.com/task/4806752200818688?logs=ci#L4696 for an example. ACKs for top commit: MarcoFalke: review ACK 6b24dfe vasild: ACK 6b24dfe Tree-SHA512: 7a67acf7b42da07b63fbb392236e9a7be8cf35c36e37ca980c4467fe8295c2eda8aef10f41a1e3036cd9ebece47fa957fc3256033f853bd6a97ce2ca42799a0a Co-authored-by: MacroFake <falke.marco@gmail.com>
05d490d to
fabb2ce
Compare
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Codex + Sonnet
Carried-forward prior finding: FIXED. At exact head fabb2ce, FillBlock now uses the free GetLocator(index) API, and genuinely new findings in the latest delta: none. Full-current-PR Codex and Sonnet reviewer evidence identified no remaining in-scope correctness or backport-adaptation issues.
Source: reviewers codex/general=gpt-5.6-sol(completed); codex/dash-core-commit-history=gpt-5.6-sol(completed); codex/backport-reviewer=gpt-5.6-sol(completed); claude/general=claude-sonnet-5(completed); claude/dash-core-commit-history=claude-sonnet-5(failed); claude/backport-reviewer=claude-sonnet-5(failed); claude/dash-core-commit-history=claude-sonnet-5(failed); claude/backport-reviewer=claude-sonnet-5(completed); claude/dash-core-commit-history=claude-sonnet-5(failed); claude/dash-core-commit-history=claude-sonnet-5(completed); verifier=codex/final-verifier=gpt-5.6-sol(completed) fallback_for_sonnet_verifier=true; coordinator=openclaw-agent/cliproxy/gpt-5.6-sol(orchestration-only).
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— dash-core-commit-history (completed),gpt-5.6-sol— backport-reviewer (completed) - Verifier:
gpt-5.6-sol— final-verifier (fallback) - Sonnet reviewers:
claude-sonnet-5— general (completed),claude-sonnet-5— dash-core-commit-history (failed),claude-sonnet-5— backport-reviewer (failed),claude-sonnet-5— dash-core-commit-history (failed),claude-sonnet-5— backport-reviewer (completed),claude-sonnet-5— dash-core-commit-history (failed),claude-sonnet-5— dash-core-commit-history (completed)
2 participants
Issue being fixed or feature implemented
This PR replaces #7318
What was done?
This PR doesn't actually have enabled anything relevant to anti-DoS header sync.
Pre-sync is deleted, not disabled - no HeadersSyncState, no commitment buffer, no PRESYNC/REDOWNLOAD state.
Header acceptance hasn't changed path: CheckBlockHeader → ContextualCheckBlockHeader → ChainLock conflict check → checkpoints, and headers are downloaded once.
This PR includes multiple useful refactorings and code changes to reduce conflicts for further backports and reduce divergency between Dash Core codebase and Bitcoin Core.
Survived changes:
How Has This Been Tested?
Run unit & functional tests.
Breaking Changes
N/A
Checklist: