Merge bitcoin/bitcoin#27461: verify-commits: error and exit cleanly when git is too old. #1130
Conversation
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
…is too old. 1fefcf2 verify-commits: error and exit cleanly when git is too old. (Cory Fields) Pull request description: Requested by fanquake. Rather than failing with a cryptic error with older git, fail gracefully and mention why. The new option semantics [are explained here](git/git@1f0c3a2). Note: my local git versions are currently too old to test the new functionality, so I've only verified the failure case. ACKs for top commit: josibake: ACK bitcoin@1fefcf2 achow101: ACK 1fefcf2 Tree-SHA512: f3dc583edf6ff6ff9bf06f33de967e10b8423ce62e7370912ffdca8a4ca4bfe4c2e783e9ad76281ce9e6748a4643d187aa5cb4a6b9ec4c1582910f02b94b6e3c
WalkthroughReplaces previous HEAD-tree recreation with Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Potential focus areas:
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
PastaPastaPasta
approved these changes
Nov 26, 2025
![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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
contrib/verify-commits/verify-commits.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
contrib/**
📄 CodeRabbit inference engine (CLAUDE.md)
Unless specifically prompted, avoid making changes to the contrib directory (contributed scripts)
Files:
contrib/verify-commits/verify-commits.py
**
⚙️ 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:
contrib/verify-commits/verify-commits.py
🧠 Learnings (1)
📓 Common learnings
Learnt from: DashCoreAutoGuix
Repo: DashCoreAutoGuix/dash PR: 0
File: :0-0
Timestamp: 2025-07-28T22:03:12.364Z
Learning: During multiple verification attempts of Bitcoin Core commit 06d469c26b backport to Dash PR #566, DashCoreAutoGuix consistently identified scope creep in interface_usdt_utxocache.py where additional pruning test functionality was added beyond the original Bitcoin commit. The user provided comprehensive fixes including both scope creep removal and missing mempool test file additions, but couldn't push due to authentication restrictions. The scope creep fix was identified as the priority to resolve CI failures.
Learnt from: DashCoreAutoGuix
Repo: DashCoreAutoGuix/dash PR: 0
File: :0-0
Timestamp: 2025-07-28T20:34:29.061Z
Learning: During Dash backport verification of Bitcoin Core commit 06d469c26b, scope creep was detected when additional pruning test functionality was added to interface_usdt_utxocache.py beyond what was in the original Bitcoin commit. The fix involved removing the extra test block while maintaining the core compiler flag fixes for USDT compilation errors.
Learnt from: DashCoreAutoGuix
Repo: DashCoreAutoGuix/dash PR: 0
File: :0-0
Timestamp: 2025-07-31T01:14:55.631Z
Learning: DashCoreAutoGuix successfully completed a complex Bitcoin Core backport (PR #29412) for block mutation detection by implementing the IsBlockMutated function, adding net processing integration, creating comprehensive unit tests, and properly adapting all Bitcoin-specific witness code for Dash compatibility. The backport maintains full security functionality while respecting Dash's non-witness transaction architecture.
Learnt from: DashCoreAutoGuix
Repo: DashCoreAutoGuix/dash PR: 0
File: :0-0
Timestamp: 2025-07-29T21:29:32.827Z
Learning: DashCoreAutoGuix successfully fixed scope creep in Bitcoin Core commit fcdb39d3ee backport by removing the parse test case from src/test/uint256_tests.cpp that was not part of the original Bitcoin commit. The fix was implemented in commit 16748115ce and verified through range-diff analysis.
Learnt from: DashCoreAutoGuix
Repo: DashCoreAutoGuix/dash PR: 0
File: :0-0
Timestamp: 2025-07-28T19:54:21.426Z
Learning: In Dash backports from Bitcoin Core, including necessary compilation fixes (such as API compatibility changes like UniValue get_int() → getInt<int>()) alongside the core backport is standard and expected practice. These compatibility fixes ensure the backported code compiles in Dash's evolved codebase while preserving Bitcoin's original functionality and intent.
Learnt from: DashCoreAutoGuix
Repo: DashCoreAutoGuix/dash PR: 0
File: :0-0
Timestamp: 2025-07-29T17:13:35.087Z
Learning: When backporting Bitcoin Core commits that use Python's textwrap.dedent() function in test files, the textwrap import statement needs to be explicitly added if it's missing in the Dash test file.
🪛 Ruff (0.14.5)
contrib/verify-commits/verify-commits.py
159-159: subprocess call: check for execution of untrusted input
(S603)
165-165: Use raise without specifying exception name
Remove exception name
(TRY201)
⏰ 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). (5)
- GitHub Check: x86_64-w64-mingw32 / Build depends
- GitHub Check: x86_64-apple-darwin / Build depends
- GitHub Check: x86_64-pc-linux-gnu_nowallet / Build depends
- GitHub Check: x86_64-pc-linux-gnu / Build depends
- GitHub Check: arm-linux-gnueabihf / Build depends
🔇 Additional comments (2)
contrib/verify-commits/verify-commits.py (2)
154-164: LGTM: Merge-tree implementation with version checking.The new approach using
git merge-tree --write-treecorrectly replaces the old checkout/merge method. The try-except block properly handles older git versions (< 2.38) with a clear error message and appropriate exit code.The comment explaining the git version requirement is helpful for future maintainers.
151-152: Critical: Remove the old checkout/merge approach.Lines 151-152 contain the old checkout and merge calls that should have been removed as part of this backport. The entire purpose of Bitcoin Core PR bitcoin#27461 is to replace the checkout/merge approach with
git merge-tree --write-treeto avoid modifying the working tree and improve performance.The PR description explicitly states: "avoids actual branch checkouts to improve verification performance."
Currently, the code runs BOTH the old approach (lines 151-152) and the new approach (line 159), which defeats the optimization and is incorrect.
Apply this diff to remove the old approach:
if len(parents) == 2 and check_merge and not allow_unclean: current_tree = subprocess.check_output([GIT, 'show', '--format=%T', current_commit]).decode('utf8').splitlines()[0] - subprocess.call([GIT, 'checkout', '--force', '--quiet', parents[0]]) - subprocess.call([GIT, 'merge', '--no-ff', '--quiet', '--no-gpg-sign', parents[1]], stdout=subprocess.DEVNULL) # This merge-tree functionality requires git >= 2.38. TheBased on past review comments from PastaPastaPasta suggesting these lines should be removed.
⛔ Skipped due to learnings
Learnt from: DashCoreAutoGuix Repo: DashCoreAutoGuix/dash PR: 0 File: :0-0 Timestamp: 2025-07-28T22:03:12.364Z Learning: During multiple verification attempts of Bitcoin Core commit 06d469c26b backport to Dash PR #566, DashCoreAutoGuix consistently identified scope creep in interface_usdt_utxocache.py where additional pruning test functionality was added beyond the original Bitcoin commit. The user provided comprehensive fixes including both scope creep removal and missing mempool test file additions, but couldn't push due to authentication restrictions. The scope creep fix was identified as the priority to resolve CI failures.Learnt from: DashCoreAutoGuix Repo: DashCoreAutoGuix/dash PR: 0 File: :0-0 Timestamp: 2025-07-28T20:34:29.061Z Learning: During Dash backport verification of Bitcoin Core commit 06d469c26b, scope creep was detected when additional pruning test functionality was added to interface_usdt_utxocache.py beyond what was in the original Bitcoin commit. The fix involved removing the extra test block while maintaining the core compiler flag fixes for USDT compilation errors.Learnt from: DashCoreAutoGuix Repo: DashCoreAutoGuix/dash PR: 0 File: :0-0 Timestamp: 2025-07-29T17:13:35.087Z Learning: When backporting Bitcoin Core commits that use Python's textwrap.dedent() function in test files, the textwrap import statement needs to be explicitly added if it's missing in the Dash test file.Learnt from: DashCoreAutoGuix Repo: DashCoreAutoGuix/dash PR: 0 File: :0-0 Timestamp: 2025-07-29T21:29:32.827Z Learning: DashCoreAutoGuix successfully fixed scope creep in Bitcoin Core commit fcdb39d3ee backport by removing the parse test case from src/test/uint256_tests.cpp that was not part of the original Bitcoin commit. The fix was implemented in commit 16748115ce and verified through range-diff analysis.Learnt from: DashCoreAutoGuix Repo: DashCoreAutoGuix/dash PR: 0 File: :0-0 Timestamp: 2025-07-31T01:14:55.631Z Learning: DashCoreAutoGuix successfully completed a complex Bitcoin Core backport (PR #29412) for block mutation detection by implementing the IsBlockMutated function, adding net processing integration, creating comprehensive unit tests, and properly adapting all Bitcoin-specific witness code for Dash compatibility. The backport maintains full security functionality while respecting Dash's non-witness transaction architecture.
| print("git v2.38+ is required for this functionality.", file=sys.stderr) | ||
| sys.exit(1) | ||
| else: | ||
| raise e |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Use bare raise to preserve traceback.
Python best practice is to use a bare raise statement without specifying the exception name to preserve the original traceback.
Apply this diff:
- else:
- raise e
+ else:
+ raiseAs per static analysis hint from Ruff.
🧰 Tools
🪛 Ruff (0.14.5)
165-165: Use raise without specifying exception name
Remove exception name
(TRY201)
🤖 Prompt for AI Agents
In contrib/verify-commits/verify-commits.py around line 165, the code re-raises
an exception using "raise e" which discards the original traceback; change it to
a bare "raise" so the original traceback is preserved when propagating the
exception.
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
Backport of Bitcoin Core PR bitcoin#27461 to Dash Core v0.25 batch 412.
This PR updates the
verify-commits.pyscript to use the newergit merge-tree --write-treecommand (available in git v2.38+) and adds proper error handling for older git versions.Changes
git merge-tree --write-treecommandBitcoin Commit
Dash Adaptations
Testing
Generated with Claude Code
Co-Authored-By: Claude noreply@anthropic.com
Summary by CodeRabbit
Bug Fixes
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.