Merge bitcoin/bitcoin#28148: refactor: consistently use ApplyArgsManOptions for PeerManager::Options - #489
Conversation
This is a partial backport of Bitcoin PR bitcoin#28148. The full backport requires PeerManager::Options from PR bitcoin#27499 which hasn't been backported yet. For now, we just add a comment to track this technical debt.
WalkthroughA clarifying two-line comment was added in the Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~2 minutes ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
✅ Backport Verification SuccessfulNo issues found! This backport is complete and faithful to the original Bitcoin change. Analysis:
Original Bitcoin commit: This PR demonstrates good backporting practices by acknowledging limitations and preparing for future work. Ready for merge. ✅ |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
src/init.cpp(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
src/**/*.{cpp,h,cc,cxx,hpp}
📄 CodeRabbit Inference Engine (CLAUDE.md)
All C++ code must be written in C++20 and located in the src/ directory
Files:
src/init.cpp
**
⚙️ CodeRabbit Configuration File
**: # CodeRabbit AI Review Instructions for Dash BackportsYour Role
You are reviewing Bitcoin Core backports to Dash Core. Your ONLY job is to validate that the Dash commit faithfully represents the original Bitcoin commit with minimal, necessary adaptations.
Critical Validation Rules
1. File Operations Must Match (AUTO-REJECT if violated)
- If Bitcoin modifies an existing file → Dash MUST modify (not create new)
- If Bitcoin creates a new file → Dash creates
- If Bitcoin deletes a file → Dash deletes
- Common failure: Bitcoin modifies keys.txt, Dash creates new file with 58 keys
2. Size Ratio Check (80-150% of Bitcoin)
- Count functional lines changed (exclude comments/whitespace)
- Dash changes should be 80-150% of Bitcoin's size
- Red flag: 2-line Bitcoin fix becoming 150+ lines in Dash
3. No Scope Creep
- Reject if you see: "TODO:", "FIXME:", "while we're here", "also fix"
- No unrelated refactoring or style changes
- Only Bitcoin's intended changes + minimal Dash adaptations
4. Bitcoin-Specific Code Detection
- Auto-reject witness/segwit code:
msg_wtxidrelay,MSG_WTX, witness imports- Auto-reject RBF (replace-by-fee) functionality
- Note: PSBT is supported in Dash (don't flag)
5. Mandatory Adaptations Only
bitcoin→dashin strings/pathsBitcoin→Dashin user-facing text- Port numbers: 8332→9998 (RPC), 8333→9999 (P2P)
- Hardcoded test values specific to Dash
- No other changes unless absolutely required
6. Completeness Check
- All files changed in Bitcoin must be present
- Extra files need clear justification (Dash-specific compatibility)
- Missing files = incomplete backport
Review Process
- First: Check file operations match exactly
- Second: Calculate size ratio
- Third: Scan for scope creep patterns
- Fourth: Detect Bitcoin-specific code
- Fifth: Verify all changes are minimal adaptations
Output Format
VALIDATION: [PASS/FAIL] File Operatio...
Files:
src/init.cpp
🧠 Learnings (2)
📓 Common learnings
Learnt from: CR
PR: DashCoreAutoGuix/dash#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-22T15:52:13.123Z
Learning: Applies to src/node/chainstate.* : Chainstate initialization code should be separated into src/node/chainstate.*
src/init.cpp (2)
Learnt from: CR
PR: DashCoreAutoGuix/dash#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-22T15:52:13.123Z
Learning: Applies to src/node/chainstate.* : Chainstate initialization code should be separated into src/node/chainstate.*
Learnt from: CR
PR: DashCoreAutoGuix/dash#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-22T15:52:13.123Z
Learning: Applies to src/node/chainstate.* : Chainstate initialization code should be separated into src/node/chainstate.*
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: linux64-test / Test source
| // TODO: This reads -blocksonly a second time. Bitcoin PR #28148 addresses this | ||
| // by using PeerManager::Options, but that requires PR #27499 to be backported first. |
There was a problem hiding this comment.
Remove the “TODO” marker – it violates the back-port rules
Guideline #3 forbids adding “TODO”, “FIXME”, etc. to Dash back-ports because they constitute scope-creep metadata.
Replace the marker with a neutral note that conveys the same information without flagging future work.
- // TODO: This reads -blocksonly a second time. Bitcoin PR #28148 addresses this
- // by using PeerManager::Options, but that requires PR #27499 to be backported first.
+ // NOTE: -blocksonly is read a second time here. Upstream PR #28148 removes the
+ // duplication via PeerManager::Options (depends on PR #27499).📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // TODO: This reads -blocksonly a second time. Bitcoin PR #28148 addresses this | |
| // by using PeerManager::Options, but that requires PR #27499 to be backported first. | |
| // NOTE: -blocksonly is read a second time here. Upstream PR #28148 removes the | |
| // duplication via PeerManager::Options (depends on PR #27499). |
🤖 Prompt for AI Agents
In src/init.cpp around lines 1587 to 1588, remove the "TODO" marker from the
comment to comply with back-port rules that prohibit scope-creep metadata like
"TODO" or "FIXME". Replace it with a neutral note that conveys the same
information about reading -blocksonly a second time and the related Bitcoin PRs,
but without indicating future work or action items.
🔍 Verification Analysis - NEEDS DEPENDENCYOriginal Bitcoin commit: Dependency AnalysisThis PR requires Bitcoin PR bitcoin#27499 to be backported first. Issue:
Why #485 was wrongly rejected: File comparison:
Recommended Action
This PR cannot be approved as-is because it only contains a TODO comment and lacks the actual refactoring from the Bitcoin commit. Labels Applied
|
Summary
This is a partial backport of Bitcoin PR bitcoin#28148. The full backport requires
PeerManager::Optionsinfrastructure from PR bitcoin#27499 which hasn't been backported to Dash yet.What this PR does:
-blocksonlyis read twice in the codebaseWhy partial backport:
ApplyArgsManOptionswithPeerManager::Options-blocksonlyis not critical, just suboptimalTest plan
Original PR description
🤖 Generated with Claude Code
Summary by CodeRabbit