fix: repair makeseeds.py, getblockchaininfo[softforks] help text, drop extra generates from test, resolve macOS GID issue
#6929
Conversation
bitcoin#22550 adds assertions to validate CSV/CLTV activation and replaces the `generate()` calls with a helper `generate_to_height()` but bitcoin#22818 activates all forks from block 1 by default, so the `generate_to_height()` calls have been dropped. This leaves us with the `generate()` calls being swapped for assertions.
macOS uses GID 20 for staff but Linux uses it for dialout, so the build will fail because GID 20 is already used.
|
@coderabbitai review |
✅ No Merge Conflicts DetectedThis PR currently has no conflicts with other open PRs. |
✅ Actions performedReview triggered.
|
WalkthroughThis pull request modifies several components across infrastructure, data processing, RPC interfaces, and tests. The Dockerfile startup sequence is updated to conditionally apply group modifications only if the target group does not exist. The makeseeds.py script changes its address extraction logic to target a specific Sequence Diagram(s)sequenceDiagram
participant Client as RPC Client
participant RPC as getblockchaininfo()
participant Schema as Softforks Schema
Client->>RPC: Request blockchain info
RPC->>Schema: Build softforks response
rect rgb(240, 248, 255)
Note over Schema: Old: Static per-deployment keys
Schema->>Schema: Fixed keys for each deployment
end
rect rgb(173, 216, 230)
Note over Schema: New: Dynamic object structure
Schema->>Schema: Generic "xxxx" placeholder key
Schema->>Schema: Per-softfork: type + bip9/details
Schema->>Schema: Add height and active fields
end
RPC->>Client: Return updated softforks structure
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (4)
🧰 Additional context used📓 Path-based instructions (3)test/functional/**/*.py📄 CodeRabbit inference engine (CLAUDE.md)
Files:
contrib/**📄 CodeRabbit inference engine (CLAUDE.md)
Files:
src/**/*.{cpp,h,cc,cxx,hpp}📄 CodeRabbit inference engine (CLAUDE.md)
Files:
⏰ 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)
🔇 Additional comments (4)
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 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Greptile Overview
Greptile Summary
This PR addresses four independent fixes that resolve issues from previous backports and cross-platform compatibility problems. The changes repair the makeseeds.py script to work with the new ExtAddr structure where addresses are keyed by purpose codes like core_p2p, fix help text documentation for getblockchaininfo RPC's softforks field to accurately reflect the dynamic object structure, streamline the wallet_signrawtransactionwithwallet.py functional test by removing unnecessary block generation now that softforks activate at height 1, and resolve a Docker build failure for macOS users where GID 20 conflicts between macOS's staff group and Linux's dialout group. These are all cleanup and correction changes that don't alter core functionality.
Important Files Changed
| Filename | Score | Overview |
|---|---|---|
| contrib/seeds/makeseeds.py | 5/5 | Fixed masternode address access to use new core_p2p purpose code structure |
| src/rpc/blockchain.cpp | 5/5 | Corrected help text for softforks field to match actual dynamic object output |
| test/functional/wallet_signrawtransactionwithwallet.py | 5/5 | Removed obsolete block generation, added softfork activation assertions, reduced locktime to 100 |
| contrib/containers/ci/ci-slim.Dockerfile | 5/5 | Added conditional check to skip group modification when GID exists for macOS compatibility |
Confidence score: 5/5
- This PR is safe to merge with minimal risk as all changes are well-documented fixes to known issues with clear explanations
- Score reflects straightforward corrections with no complex logic changes, clear backport context, and targeted fixes for documented problems
- No files require special attention; all changes are simple, well-explained, and address specific documented issues with appropriate justification
Context used:
- Context from
dashboard- CLAUDE.md (source)
4 files reviewed, no comments
4 participants
Additional Information
The help text for
getblockchaininfo['softforks']was introduced in bitcoin#16060 (backported in dash#5255) but theRPCResulthelp text was introduced in bitcoin#17809, backported before bitcoin#16060. This resulted in the help text forsoftforksdeviating from upstream without the output to match (which is aligned with upstream).This has since been resolved as the output structure hasn't changed, only the help text.
bitcoin#22818 (backported in dash#6214) activated many softforks at height 1, which rendered the block generation calls in
wallet_signrawtransactionwithwallet.py(source) no longer necessary, additionally bitcoin#22550 (backported in dash#6189) added assertions to validate CSV/CLTV activation that were not included in the backport.This has since been resolved, replacing the
generatecalls with assertions.1000to100(matching with upstream, source) as we are no longer generating enough blocks to reach that height anymore.makeseeds.pyis currently mangled because dash#6666 updated the script to look for thecore_p2ppurpose even though purpose code support was spun-off into dash#6674. This meant, in between dash#6666 and dash#6674's merger,makeseeds.pywas broken.This necessitated a fix that was introduced as e714c06 (dash#6858) but meant that after dash#6674,
makeseeds.pyis broken again. This has been resolved as part of this PR.macOS users who want to utilise the
developcontainer (source) need to set theUSER_IDandGROUP_IDargsto avoid permissions issues. This creates a slight problem as the GID forstaffis 20 on macOS but Linux uses GID 20 fordialout, so creating a group with GID 20 will fail.As we only care about the GID, it is safe to skip the group creation step and assign the user to the group anyways as we do it by GID and not name (the user will be presented as a member of the
dialoutgroup). This should resolve issues for macOS users trying to build thedevelopcontainer.Breaking Changes
None expected.
Checklist