Repository navigation
params%feat!: apply the reference chain-params options - #105
PastaPastaPasta wants to merge 4 commits into
Conversation
ChainParamsBuilder takes the regtest and devnet options, typed or raw; build returns a Result. Options write the one ChainParams, LLMQ sizes included, and all_checkpoints adds a named devnet's checkpoint.
-sporkaddr and v23.1's -minsporkkeys resolve to SporkSigners, with key IDs read by base58ck. A dash-script test checks the reader against Recipient::from_base58c.
The dump prints the spork signers and derived LLMQ values and takes the spork options. gen.py writes the cases chainparams_cases.json5 lists, and dash-params and dash-primitives replay them.
The builder takes the reference options and build returns a Result.
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 28 minutes. View limit details
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. |
|
⛔ Final review complete — 1 blocking finding(s) (commit cdd6e24) · triage: critical |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! 🚀 New features to boost your workflow:
|
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Phase 1 + Phase 2
Verified the supplied findings against cdd6e24 and both pinned Dash Core parsers: Windows option-key normalization is missing and can silently discard consensus overrides. The checkpoint suggestion is unnecessary because the current table, its adjacent comment, and regression tests establish the ordering invariant. This was static-only validation; current CI has all build, test, lint, and cross-build checks passing, with only the non-blocking codecov/project/dash-num coverage check failing.
🔴 1 blocking
Review provenance
Source: reviewer 1: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); reviewer 4: gpt-6-astra (agent: phase2-reviewer, role: general); reviewer 5: gpt-6-astra (agent: phase2-reviewer, role: rust-quality); reviewer 6: gpt-6-astra (agent: phase2-reviewer, role: security-auditor); reviewer 7: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 8: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
criticalbygpt-6.1-sol(effort low) — The intricate option application in pkgs/params/src/builder.rs changes effective consensus activation heights, version-bits deployments, quorum parameters and devnet genesis checkpoints, meeting both the complexity and critical-surface criteria. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— rust-quality (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(antigravity quota lookup failed),glm-5.3-flash(not used above high effort; tier asks max) - Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— general (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— security-auditor (completed, effort xhigh); agentphase2-reviewer - Model comparison: every Phase-2 reviewer also ran on
gpt-6-astra; the verifier weighed both sets without knowing which model wrote which
🤖 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 `pkgs/params/src/builder.rs`:
- [BLOCKING] pkgs/params/src/builder.rs:257-265: Match Core's Windows option-key normalization
Both pinned Core releases lowercase option keys and translate an initial `/` to `-` on Windows before resolving the option ([v23.1.8](https://github.com/dashpay/dash/blob/728f5055836c6d29806412fc7223ac8fe05af991/src/util/system.cpp#L330-L340), [v24.0.0-rc.5](https://github.com/dashpay/dash/blob/8163290aa0307ed4bc2fc0ba15b78c20d61dfa88/src/util/system.cpp#L250-L260)). This parser instead requires a dash and an exact lowercase name on every platform. Consequently, valid Windows arguments such as `/dip3params=105:106` and `-DIP3PARAMS=105:106` return the builder unchanged as `Err`. With the documented whole-command-line pattern, which keeps the returned builder and continues, `build()` succeeds with regtest's default DIP3 heights 432/500 rather than the node's 105/106. Apply Core's Windows key normalization, or expose an explicit source-platform parsing mode, without lowercasing values or losing the original argument used in diagnostics. Add regression cases for slash-prefixed and mixed-case keys, including negations.
| let Some(name) = key.strip_prefix("--").or_else(|| key.strip_prefix('-')) else { | ||
| return Err(self); | ||
| }; | ||
| let (negated, name) = match name.strip_prefix("no") { | ||
| Some(name) => (true, name), | ||
| None => (false, name), | ||
| }; | ||
| let Some(option) = ChainOption::from_name(&format!("-{name}")) else { | ||
| return Err(self); |
There was a problem hiding this comment.
🔴 Blocking: Match Core's Windows option-key normalization
Both pinned Core releases lowercase option keys and translate an initial / to - on Windows before resolving the option (v23.1.8, v24.0.0-rc.5). This parser instead requires a dash and an exact lowercase name on every platform. Consequently, valid Windows arguments such as /dip3params=105:106 and -DIP3PARAMS=105:106 return the builder unchanged as Err. With the documented whole-command-line pattern, which keeps the returned builder and continues, build() succeeds with regtest's default DIP3 heights 432/500 rather than the node's 105/106. Apply Core's Windows key normalization, or expose an explicit source-platform parsing mode, without lowercasing values or losing the original argument used in diagnostics. Add regression cases for slash-prefixed and mixed-case keys, including negations.
source: gpt-6-astra (phase2-reviewer: general)
|
Closing, together with #104 (see #104 (comment)). Applying the reference chain-params options sets up regtest and devnet chains through tunables, which is outside the scope of a stateless SDK. That work belongs in rust-dashcore. 🤖 Posted autonomously by Claude on behalf of pasta. |
Motivation
Stacked on #104.
ChainSetupgives a network's chain parameters at a release, but only with the defaults. Regtest and devnet nodes are usually started with options that change them: activation heights, version-bits deployments, LLMQ types and sizes, budget and DIP3 heights, a named devnet, spork signers. A client that follows such a chain has to apply the same options and refuse the same values.This pull request applies the reference implementation's chain-params options to a
ChainSetup, asv23.1.8andv24.0.0-rc.5do. Each option is applied in the reference's order, and each refused value returns the reference's error text. The dump tool from #104 now also takes options, so every case in a new case list is checked against both releases.Additional Information
ChainParamsBuildertakes the options ofChainOption, the two spork options of the next row and these 19 (-testactivationheight,-vbparams,-dip3params,-budgetparams,-powtargetspacing,-devnet,-fastprune, the-llmq*type and size options,-highsubsidyblocks,-highsubsidyfactorand-minimumdifficultyblocks), typed withargandnegateor raw withparse_arg, which keeps the argument's spelling for the error text.parse_arggives the builder back unchanged asErrfor an argument that names no chain option, so a node's whole command line can be fed through it.buildreturnsResult<ChainSetup, ParamsError>. Options write the oneChainParamsaChainSetupholds, so everything read through it,ConsensusParams::llmqincluded, has the options applied. LLMQ size options live in the newCopyfieldConsensusParams::llmq_sizes. A named devnet's genesis block becomes its height-1 checkpoint, whichChainParams::all_checkpointsadds to the staticcheckpoints.-sporkaddrand-minsporkkeysresolve toSporkSigners, with key IDs read bybase58ck. v23.1 takes-minsporkkeysonly. Adash-scripttest checks the key ID reader againstRecipient::from_base58con every network at both releases.gen.pyruns the 212 cases thatchainparams_cases.json5lists at each release, and the corpora gain acasessection.dash-paramsreplays every case, anddash-primitiveschecks the genesis and devnet genesis blocks of the cases that change them.Rerunning
gen.pywith both releases' dumps and the case list reproduces the corpora byte for byte, andcontrib/params/README.mdgives the recipe. A repeated-devnetis refused by the reference at startup, outside its chain parameters, sodevnet_nametakes one name and the builder does not check for repeats.Breaking Changes
ChainParamsBuilder::buildreturnsResult<ChainSetup, ParamsError>instead ofChainSetup, since options can be refused.ConsensusParamshas the new public fieldllmq_sizes, so aConsensusParamsliteral must set it. The network tables useLlmqSizes::NONE.checkpointsholds height 0 only. UseChainParams::all_checkpointsfor a devnet's height-1 checkpoint.dash-paramsnow depends onbase58ckandthiserror.How Has This Been Tested?
tests/builder.rscover the typed and raw entry points, negation and the refusals the dump cannot reach, where the reference aborts.just test lint fuzz::test, CodeQL included) is green on each commit.Checklist
comments and concerns without said assistance. I assume authorship of the code submitted.
Agent disclosure
Code, commit messages and this description were generated with Claude Code using Claude Opus 5.5, running autonomously as an agent on behalf of the author. The agent also built both releases and generated the corpora with the tools in
contrib/params. The change was reviewed by a Claude Code agent using Opus 5.5 and a Codex agent using GPT-6.1.🤖 Generated with Claude Code