Repository navigation
params%feat!: add ChainSetup, devnet tables and per-release chain parameters - #104
PastaPastaPasta wants to merge 5 commits into
Conversation
ChainSetup is a network's parameters at a Release, one ChainParams read through Deref plus extra per-chain values. V23_1 keeps v23.1.8's values and Network gains Devnet with u8 discriminants a test pins.
DevnetGenesisParams finds a devnet's genesis nonce with X11 (dash-pow, default features off, portable forwarded). Primitives builds the block, and a test ties both encoders over every push size of the devnet name.
contrib/params builds a reference implementation tag from git archive, links a dump of its chain parameters and masternode payments against the node's libraries, and gen.py writes the corpora from those dumps.
Every chain with no options at v23.1.8 and v24.0.0-rc.5, from the public tags, and the payments around each reallocation period. Primitives checks the genesis and devnet genesis block bytes.
dash-params now covers the devnet and both known releases.
|
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 — no blockers (commit b93d4bb) · 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 head b93d4bb and the pinned Core sources. No blocking defects were confirmed; the retained findings concern deployment ordering, serialization-boundary coverage, API design and documentation. This was static verification only: current CI reports passing lint, stable-platform and cross-platform checks, with several MSRV build-and-test jobs still pending.
🟡 5 suggestion(s) | 💬 1 nitpick(s)
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: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 5: 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 large, intricate diff directly adds release-specific consensus parameter tables and devnet genesis construction, including proof-of-work target checking and consensus-relevant coinbase encoding in pkgs/params/src/genesis.rs::DevnetGenesisParams::find. - 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
🤖 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/consensus.rs`:
- [SUGGESTION] pkgs/params/src/consensus.rs:70-71: Match Core's documented buried-deployment order
`ALL` promises the reference implementation's order, but both pinned releases declare `DEPLOYMENT_DERSIG` before `DEPLOYMENT_CLTV`; see [v24's declaration](https://github.com/dashpay/dash/blob/8163290aa0307ed4bc2fc0ba15b78c20d61dfa88/src/consensus/params.h#L22-L27). This array instead yields `bip65` before `bip66`. Swap these entries and add an explicit sequence assertion: the reference-dump test inserts deployments into a `BTreeMap`, so it cannot detect this ordering mismatch.
In `pkgs/primitives/tests/genesis_valid.rs`:
- [SUGGESTION] pkgs/primitives/tests/genesis_valid.rs:64: Exercise the larger script-push and CompactSize boundaries
The largest name in this cross-crate test is 256 bytes. Consequently, neither the PUSHDATA4 branch nor the hand-written CompactSize `0xfe` branch is exercised end to end. The prefix unit test checks PUSHDATA4 in isolation, while the Core block fixtures use the short default name. Add names of 65,531 and 65,532 bytes to cross the serialized script-length boundary, and 65,535 and 65,536 bytes to cross the PUSHDATA2/PUSHDATA4 boundary. These cases verify that the manually hashed coinbase and the typed transaction encoder continue to produce the same block hash.
In `pkgs/params/src/genesis.rs`:
- [SUGGESTION] pkgs/params/src/genesis.rs:71-84: Keep mined devnet descriptors internally consistent
`find()` computes `nonce` and `hash` from the other fields, but all fields remain publicly mutable. Changing `name`, for example, leaves the stored hash stale while `Block::from` rebuilds the coinbase and produces a different header hash. Public construction also bypasses the name validation, and `script_sig()` silently omits the push prefix for an oversized name. This is an API-invariant concern rather than a demonstrated failure of the default setup. Following AGENTS.md's instruction to make invalid states unrepresentable, keep the mined fields private with getters and validated construction, or explicitly separate an editable input descriptor from the mined result.
In `pkgs/params/src/setup/tables.rs`:
- [SUGGESTION] pkgs/params/src/setup/tables.rs:39-58: Link the Core sources for the two non-dumped heights
These two heights are deliberately absent from the reference dumps, and the setup test only pins their SDK literals. Both values are correct, but the PR description provides no source links for independently checking this exception to the dump-based verification. Add the pinned Core references to the PR description: [the mainnet difficulty-tolerance condition](https://github.com/dashpay/dash/blob/8163290aa0307ed4bc2fc0ba15b78c20d61dfa88/src/validation.cpp#L4487-L4495) and [the quorum activation constant and condition](https://github.com/dashpay/dash/blob/8163290aa0307ed4bc2fc0ba15b78c20d61dfa88/src/validation.cpp#L6689-L6724). No additional source-code comments or artificial dump constants are needed.
In `pkgs/params/src/setup.rs`:
- [SUGGESTION] pkgs/params/src/setup.rs:147-153: Expose ChainParams through an explicit accessor
The new wrapper exposes its contained `ChainParams` through implicit field forwarding and coercion, making those implicit conversions part of the public API. An explicit `chain(&self) -> &ChainParams` accessor would distinguish the release-aware wrapper from its underlying parameter table and let callers express that boundary directly. The current `Deref` design is intentional and does not violate a stated repository rule, so this is a non-blocking API-design suggestion rather than a correctness defect.
- [NITPICK] pkgs/params/src/setup.rs:98-99: Describe the currently supported devnet name
The only constructor sets this private field to `"devnet"`, and there is no named-devnet builder option in this head. Document the current getter behavior instead of implying that this API can already return a configured `devnet-<name>`. The broader name format remains appropriate on `DevnetGenesisParams::find`, which does accept caller-supplied names.
| Self::Cltv, | ||
| Self::DerSig, |
There was a problem hiding this comment.
🟡 Suggestion: Match Core's documented buried-deployment order
ALL promises the reference implementation's order, but both pinned releases declare DEPLOYMENT_DERSIG before DEPLOYMENT_CLTV; see v24's declaration. This array instead yields bip65 before bip66. Swap these entries and add an explicit sequence assertion: the reference-dump test inserts deployments into a BTreeMap, so it cannot detect this ordering mismatch.
| Self::Cltv, | |
| Self::DerSig, | |
| Self::DerSig, | |
| Self::Cltv, |
source: gpt-6.1-sol (phase2-reviewer: general)
| #[case::longest_direct_push("x".repeat(75), &[0x51, 0x4b])] | ||
| #[case::shortest_pushdata1("x".repeat(76), &[0x51, 0x4c, 0x4c])] | ||
| #[case::longest_pushdata1("x".repeat(255), &[0x51, 0x4c, 0xff])] | ||
| #[case::shortest_pushdata2("x".repeat(256), &[0x51, 0x4d, 0x00, 0x01])] |
There was a problem hiding this comment.
🟡 Suggestion: Exercise the larger script-push and CompactSize boundaries
The largest name in this cross-crate test is 256 bytes. Consequently, neither the PUSHDATA4 branch nor the hand-written CompactSize 0xfe branch is exercised end to end. The prefix unit test checks PUSHDATA4 in isolation, while the Core block fixtures use the short default name. Add names of 65,531 and 65,532 bytes to cross the serialized script-length boundary, and 65,535 and 65,536 bytes to cross the PUSHDATA2/PUSHDATA4 boundary. These cases verify that the manually hashed coinbase and the typed transaction encoder continue to produce the same block hash.
| #[case::shortest_pushdata2("x".repeat(256), &[0x51, 0x4d, 0x00, 0x01])] | |
| #[case::shortest_pushdata2("x".repeat(256), &[0x51, 0x4d, 0x00, 0x01])] | |
| #[case::largest_u16_script("x".repeat(65_531), &[0x51, 0x4d, 0xfb, 0xff])] | |
| #[case::smallest_u32_script("x".repeat(65_532), &[0x51, 0x4d, 0xfc, 0xff])] | |
| #[case::longest_pushdata2("x".repeat(65_535), &[0x51, 0x4d, 0xff, 0xff])] | |
| #[case::shortest_pushdata4("x".repeat(65_536), &[0x51, 0x4e, 0x00, 0x00, 0x01, 0x00])] |
source: gpt-6.1-sol (phase2-reviewer: rust-quality)
| pub struct DevnetGenesisParams { | ||
| /// The devnet's name, `devnet` or `devnet-<name>`, which the coinbase input | ||
| /// script pushes. | ||
| pub name: String, | ||
| /// Hash of the genesis block it builds on. | ||
| pub prev_hash: Hash256, | ||
| /// Block timestamp (unix epoch seconds). | ||
| pub time: u32, | ||
| /// Compact difficulty target. | ||
| pub bits: u32, | ||
| /// Nonce used for proof-of-work. | ||
| pub nonce: u32, | ||
| /// Hash of the block. | ||
| pub hash: Hash256, |
There was a problem hiding this comment.
🟡 Suggestion: Keep mined devnet descriptors internally consistent
find() computes nonce and hash from the other fields, but all fields remain publicly mutable. Changing name, for example, leaves the stored hash stale while Block::from rebuilds the coinbase and produces a different header hash. Public construction also bypasses the name validation, and script_sig() silently omits the push prefix for an oversized name. This is an API-invariant concern rather than a demonstrated failure of the default setup. Following AGENTS.md's instruction to make invalid states unrepresentable, keep the mined fields private with getters and validated construction, or explicitly separate an editable input descriptor from the mined result.
source: muse-spark-1.3-contributor (phase1-reviewer: general, rust-quality)
| const LLMQ_25_67_ACTIVATION_HEIGHT: BlockHeight = BlockHeight::from_u32(847_000); | ||
|
|
||
| /// Hard-coded heights of every chain but mainnet. | ||
| const TEST_HEIGHTS: HardCodedHeights = HardCodedHeights { | ||
| pre_dgw_tolerance_until: None, | ||
| llmq_25_67_activation_height: LLMQ_25_67_ACTIVATION_HEIGHT, | ||
| }; | ||
|
|
||
| /// The values of `network`. | ||
| pub(super) const fn extras(network: Network) -> Extras { | ||
| match network { | ||
| Network::Main => Extras { | ||
| require_routable_external_ip: true, | ||
| allow_multiple_addresses_from_group: false, | ||
| llmq_connection_retry_timeout: 60, | ||
| fulfilled_request_expire_time: 60 * 60, | ||
| hard_coded: HardCodedHeights { | ||
| pre_dgw_tolerance_until: Some(BlockHeight::from_u32(68_589)), | ||
| llmq_25_67_activation_height: LLMQ_25_67_ACTIVATION_HEIGHT, | ||
| }, |
There was a problem hiding this comment.
🟡 Suggestion: Link the Core sources for the two non-dumped heights
These two heights are deliberately absent from the reference dumps, and the setup test only pins their SDK literals. Both values are correct, but the PR description provides no source links for independently checking this exception to the dump-based verification. Add the pinned Core references to the PR description: the mainnet difficulty-tolerance condition and the quorum activation constant and condition. No additional source-code comments or artificial dump constants are needed.
source: muse-spark-1.3-contributor (phase1-reviewer: general, rust-quality)
| impl Deref for ChainSetup { | ||
| type Target = ChainParams; | ||
|
|
||
| fn deref(&self) -> &ChainParams { | ||
| &self.chain | ||
| } | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: Expose ChainParams through an explicit accessor
The new wrapper exposes its contained ChainParams through implicit field forwarding and coercion, making those implicit conversions part of the public API. An explicit chain(&self) -> &ChainParams accessor would distinguish the release-aware wrapper from its underlying parameter table and let callers express that boundary directly. The current Deref design is intentional and does not violate a stated repository rule, so this is a non-blocking API-design suggestion rather than a correctness defect.
source: muse-spark-1.3-contributor (phase1-reviewer: general, rust-quality)
| /// The devnet's name on a devnet, `devnet` or `devnet-<name>`, and `None` | ||
| /// elsewhere. |
There was a problem hiding this comment.
💬 Nitpick: Describe the currently supported devnet name
The only constructor sets this private field to "devnet", and there is no named-devnet builder option in this head. Document the current getter behavior instead of implying that this API can already return a configured devnet-<name>. The broader name format remains appropriate on DevnetGenesisParams::find, which does accept caller-supplied names.
| /// The devnet's name on a devnet, `devnet` or `devnet-<name>`, and `None` | |
| /// elsewhere. | |
| /// The devnet's name on a devnet, currently always `devnet`, and `None` | |
| /// elsewhere. |
source: muse-spark-1.3-contributor (phase1-reviewer: general, rust-quality)
kwvg
left a comment
kwvg
left a comment
There was a problem hiding this comment.
The premise of this PR is contentious at best and a complete reshaping of the SDK's purpose at worst. Most of this PR adds stateful functions that are better positioned for rust-dashcore. The planning agent responsible for this branch needs to rework the larger plan and specifically not try to introduce persistent state or tunables to this SDK and decide how to rework this PR and its associated stack.
| // classic reward era, for a block value of 1000000000 duffs, around each block | ||
| // reward reallocation period, for dash-params' period table test. | ||
| // | ||
| // "realloc_start" is the first height of the first period and |
There was a problem hiding this comment.
These are bare network facts, a Python script that extracts the fields from established RPCs to piece together this corpora is much more preferable to adding a dump script that needs to be injected in a live client with the chain state already there. All other C++ based test programs either probe programmatic behaviour or run in the Boost test framework, this does not.
| // | ||
| // Dump source sha256 70ab0c6408c6b65224fbfd57fbedbd8772393816880ab05373a8a6696e674052. | ||
| // Regenerate as contrib/params/README.md describes. | ||
| { |
There was a problem hiding this comment.
What does this corpora validate that chainparams.json5 doesn't? Most material facts are defined at genesis or after a hard fork, so checkpointing various releases doesn't give us greater insight than the dump from the most latest release.
| /// names its devnet in a single coinbase transaction. | ||
| #[derive(Clone, Debug, Eq, Hash, PartialEq)] | ||
| #[cfg_attr(feature = "codec", derive(Unencodable))] | ||
| pub struct DevnetGenesisParams { |
There was a problem hiding this comment.
Devnets are created on the spot and are entirely built on tunables, they are far outside of scope of a stateless SDK. A regtest can still be assembled as a closed network with fixed parameters and used for debugging this SDK but devnets are for feature testing and development. Devnets are already implemented in rust-dashcore (source) and are best kept there as statefulness is their domain.
| } | ||
|
|
||
| /// Appends the CompactSize encoding of `n`. | ||
| fn compact_size(buf: &mut Vec<u8>, n: usize) { |
There was a problem hiding this comment.
We already have a compact size encoder in dash-types
base-sdk/pkgs/types/src/compact.rs
Lines 95 to 110 in 449f16b
| pow_dgw_height: BlockHeight::from_u32(34_140), | ||
| minimum_chain_work: Arith256::ZERO, | ||
| default_assume_valid: Hash256::ZERO, | ||
| minimum_difficulty_blocks: 0, |
There was a problem hiding this comment.
Tunables for the difficulty retargeting algo are not stateless, when the algorithms are introduced in dash-primitives, there won't be an option to specify network-specific exemptions since those are policy decisions over consensus, not baseline consensus rules.
| /// On a devnet, its devnet genesis block hashes to the devnet genesis hash, | ||
| /// which the setup's height-1 checkpoint holds. | ||
| #[rstest] | ||
| fn setup_genesis_blocks_match_their_hashes( |
There was a problem hiding this comment.
Tautological test, if known hash != hash of genesis block constructed, the genesis is wrong
|
Agreed on all points. Trimming won't fix this, so we are withdrawing the direction of this stack:
🤖 Posted autonomously by Claude on behalf of pasta. |
) * [5e4a64e] params%test: add a script that writes the BRR payment corpus (pasta) * [66a8306] params%feat: add `BRR_PERIODS_PERMILLE`, the BRR masternode shares (pasta) * [c0266f7] params%doc: update changelog (pasta) * [cc21325] params%doc: trim the BRR test and corpus script docs (pasta) Description: ````` ## Motivation `ConsensusParams` already holds the heights that decide the masternode share of the block value before `v20` (`brr_height`, `superblock_cycle` and `v20_height`), but not the shares themselves. This pull request adds the 19 block reward reallocation shares. They are checked against the masternode payments that nodes of the reference implementation report for mainnet and testnet blocks. As asked on #104, the corpus comes from a Python script that reads established RPCs. ## Additional Information | Description | Notes | Reference implementation | Corpus | | ----------- | ----- | ------------------------ | ------ | | Add a script that writes the BRR payment corpus | `pkgs/params/contrib/brr_payments.py` uses the standard library only. It reads one node per chain over JSON-RPC: `getblockchaininfo`, `getdeploymentinfo` for the `realloc` and `v20` heights, `getgovernanceinfo` for the superblock cycle, `getblockhash`, `getblockstats` for the block value (subsidy plus fees), `masternode payments` for the payment, `getblock` to require that the coinbase pays each payee, and `getnetworkinfo` for the version. It writes a row for the first and last block of each reallocation period before `v20`. `contrib` is excluded from the published crate. | none | `brr_payments.json5` | | Add `BRR_PERIODS_PERMILLE` | The masternode share of the block value in permille, for each period of three superblock cycles from the first superblock cycle after `brr_height` until `v20_height`. The last share then holds. The test takes each row's period from its chain's `ConsensusParams` and requires `payment == block_value * share / 1000`. It also requires the rows to cover all 19 shares. | [masternode payment](https://github.com/dashpay/dash/blob/8163290aa0307ed4bc2fc0ba15b78c20d61dfa88/src/masternode/payments.cpp#L62-L129), [reward era](https://github.com/dashpay/dash/blob/8163290aa0307ed4bc2fc0ba15b78c20d61dfa88/src/validation.cpp#L5916-L5922) | `brr_payments.json5` | | Update the changelog | "Added". | none | none | `brr_payments.json5` was written by the script from two nodes running the official `v24.0.0-rc.5` release (`dashcore-24.0.0-rc.5-x86_64-linux-gnu.tar.gz`, SHA-256 in the corpus header), each with no connections. It holds: - mainnet heights 1379128 to 1877608: the first and last block of periods 1 to 10, and the first block of period 11; - testnet heights 387504 to 388871: the first and last block of all 19 periods. ## Breaking Changes None. CI (`just test lint fuzz::test`, CodeQL included) is green on each commit. ## Checklist - [x] I have performed a self-review of my own code - [x] I have commented my code, particularly in hard-to-understand areas - [x] I have added or updated relevant unit/integration/functional tests - [x] I have made corresponding changes to the documentation - [ ] I did not use agentic assistance (i.e. LLMs, AI) except as [disclosed](#agent-disclosure) and can answer questions, comments and concerns without said assistance. I assume authorship of the code submitted. - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ ### 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 ran the release's nodes and the script that wrote `brr_payments.json5`. The change was reviewed by a Claude Code agent using Opus 5.5. 🤖 Generated with [Claude Code](https://claude.com/claude-code) `````
Motivation
Stacked on #100. The network tables in
dash-paramshold one release's values, for mainnet, testnet and regtest only. A node or client also needs the devnet, and the values of the release the network runs: mainnet runs v23.1.8 while v24.0 is a release candidate. v23.1.8 differs in the version-bits deployments, the Platform address HRP, the standard transaction policy, the sync data and checkpoints, the fixed seeds and the pruning values.This pull request adds
ChainSetup, the chain parameters of a network at aRelease(V23_1isv23.1.8,V24_0isv24.0.0-rc.5), and the devnet with its devnet genesis block. An in-tree dump tool produces corpora that check every value against the reference implementation at both tags. Command-line options (activation heights,-vbparams, LLMQ overrides, named devnets) follow in the next pull request.Additional Information
Release,ChainSetupand the devnet tableChainParamsBuilder::new(network, release).build()returns aChainSetup: oneChainParamsholding the release's values, read throughDeref, plus the per-chain values the tables lack (assumeutxo data, fixed seeds, masternode network settings, the devnet's name and genesis block, and two heights the reference implementation hard-codes outside its chain parameters).NetworkgainsDevnet, withu8discriminants 0 to 3,ALL,network_idandfrom_network_id.ConsensusParamsgains the devnet genesis hash and the devnet minimum-difficulty and high-subsidy fields. The network tables stay at v24.0.0-rc.5.DevnetGenesisParams::findsearches the devnet genesis nonce with X11 as the reference implementation does, sodash-paramsnow depends ondash-pow.dash-primitivesbuilds the block withFrom<&DevnetGenesisParams> for Block, and a test ties the coinbase encoders of both crates over every push size of the devnet name.contrib/params/build.shbuilds a release fromgit archiveof its tag and linkschainparams_dump.cppagainst the libraries its node links. The dump prints the parameters of a chain as the release selects it, and the masternode payment at a height.gen.pywrites the corpora, and the README has the recipe.ChainSetupagainst the reference dumpschainparams_v23_1.json5andchainparams_v24_0.json5hold every chain at each tag, andbrr_payments.json5holds the masternode payments around each block reward reallocation period. Each test flattens aChainSetupinto the corpus keys and requires the same keys and values.dash-primitiveschecks the genesis and devnet genesis block bytes.Each corpus header names its tag, the tag's commit and the sha256 of
chainparams_dump.cpp.gen.pyrefuses a dump built from another source, and the corpora are written only bygen.py. The two hard-coded heights (68589 on mainnet, and 847000) have no dump, so a test pins them.Breaking Changes
NetworkgainsDevnet, declared beforeRegtest, so exhaustive matches must handle it, and the discriminant ofRegtestis now 3.ConsensusParamshas four new public fields,hash_devnet_genesis_block,minimum_difficulty_blocks,high_subsidy_blocksandhigh_subsidy_factor, so struct literals and exhaustive patterns must name them.dash-paramsnow depends ondash-pow, which needs x86-64-v3 and AES, NEON and AES on AArch64, or 128-bit SIMD on WebAssembly. Other targets need the newportablefeature, whichdash-primitives,dash-scriptanddash-p2p-coreforward.How Has This Been Tested?
gen.pyagainst both builds reproduces the three corpora byte for byte.cargo check --features portablewith-C target-cpu=x86-64passes fordash-params,dash-primitives,dash-scriptanddash-p2p-core.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