Skip to content

Conversation

@xdustinface
Copy link
Collaborator

@xdustinface xdustinface commented Dec 2, 2025

This is basically only used as regression test for the genesis block hashes. Which we already also test in separate tests <network>_genesis_full_block (except for regtest which gets added here in this PR) anyway so its redundant.

Im trying to fixup the retest/devnet parameters in #227 but since devnet/regtest genesis are the same the use of pub struct ChainHash([u8; 32]); doest work anymore with:

impl TryFrom<ChainHash> for Network {
    type Error = UnknownChainHash;

    fn try_from(chain_hash: ChainHash) -> Result<Self, Self::Error> {
        match chain_hash {
            // Note: any new network entries must be matched against here.
            ChainHash::DASH => Ok(Network::Dash),
            ChainHash::TESTNET => Ok(Network::Testnet),
            ChainHash::DEVNET => Ok(Network::Devnet),
            ChainHash::REGTEST => Ok(Network::Regtest),
            _ => Err(UnknownChainHash(chain_hash)),
        }
    }
}

because we can't capture the same value twice in there. So its either refactoring it or dropping it. I thought dropping makes more sense.

Note: The regtest test I added in here tests for invalid values but this will be fixed in #227.

Summary by CodeRabbit

  • Refactor

    • Removed deprecated ChainHash type and its associated network constants and utilities.
    • Removed UnknownChainHash error type from network conversion handling.
  • Tests

    • Updated genesis block validation test suite for improved Regtest network coverage.

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Dec 2, 2025

Walkthrough

This pull request removes the ChainHash type entirely from the codebase, including all associated public constants, methods, and trait implementations. Additionally, the UnknownChainHash error type and the TryFrom<ChainHash> for Network conversion are eliminated. A legacy chain-hash test is replaced with a focused Regtest genesis block validation test.

Changes

Cohort / File(s) Summary
ChainHash type removal
dash/src/blockdata/constants.rs
Removed ChainHash([u8; 32]) struct and all associated impls, including impl_array_newtype! and impl_bytes_newtype! macros. Deleted public constants (DASH, TESTNET, DEVNET, REGTEST) and methods (using_genesis_block, from_genesis_block_hash). Replaced legacy chain-hash test with new regtest_genesis_full_block test focused on validating Regtest genesis block fields.
UnknownChainHash error removal
dash/src/network/constants.rs
Removed UnknownChainHash error struct and its Display/error trait implementations. Deleted TryFrom<ChainHash> for Network conversion and associated error type. Removed imports of ChainHash and impl_std_error! macro.

Estimated Code Review Effort

🎯 2 (Simple) | ⏱️ ~10 minutes

  • Straightforward removal pattern applied consistently across two files; no complex logic changes
  • Reviewer should verify all ChainHash and UnknownChainHash references are properly eliminated throughout the codebase
  • Confirm the new regtest_genesis_full_block test adequately replaces coverage from removed tests

Poem

A rabbit hops through code so clean,
Removing chains no longer seen,
Old ChainHash fades away,
Simpler paths will have their day,
Hopping forward, lean and bright! 🐰

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the main change: removing the ChainHash type and related tests from the dash crate, which is the primary focus of this PR.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch chore/drop-chain-hash

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between c59518d and 9665ee5.

📒 Files selected for processing (2)
  • dash/src/blockdata/constants.rs (1 hunks)
  • dash/src/network/constants.rs (0 hunks)
💤 Files with no reviewable changes (1)
  • dash/src/network/constants.rs
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs

📄 CodeRabbit inference engine (CLAUDE.md)

**/*.rs: Never hardcode network parameters, addresses, or keys in Rust code
Use proper error types with thiserror crate and propagate errors appropriately in Rust code
Use tokio runtime for async operations in Rust code
Use conditional compilation with feature flags for optional features
Write unit tests for new functionality in Rust
Format Rust code with cargo fmt before committing
Ensure clippy passes with all warnings as errors on all Rust code

**/*.rs: Each crate keeps sources in src/; unit tests live alongside code with #[cfg(test)]
Maintain MSRV of 1.89 for Rust code
Format Rust code with rustfmt (see rustfmt.toml); run cargo fmt --all before commits
Lint Rust code with clippy; avoid unwrap()/expect() in library code; use error types (e.g., thiserror)
Use snake_case for function and variable names in Rust
Use UpperCamelCase for types and traits in Rust
Use SCREAMING_SNAKE_CASE for constants in Rust
Follow mixed editions (2021/2024) and crate-specific idioms; prefer async via tokio where applicable
Unit tests should be placed near code with descriptive names (e.g., test_parse_address_mainnet)
Use #[ignore] for network-dependent or long-running tests; run with -- --ignored flag
Never commit secrets or real keys; avoid logging sensitive data in Rust code
Keep test vectors deterministic in Rust test code

Files:

  • dash/src/blockdata/constants.rs
🧠 Learnings (14)
📓 Common learnings
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:17.310Z
Learning: Applies to dash-spv/tests/**/*.rs : Add real network tests with live Dash Core node integration that gracefully handle node unavailability
📚 Learning: 2025-12-01T08:00:17.310Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:17.310Z
Learning: Applies to dash-spv/src/**/*.rs : Use trait-based abstractions for swappable implementations (e.g., `NetworkManager`, `StorageManager`)

Applied to files:

  • dash/src/blockdata/constants.rs
📚 Learning: 2025-12-01T08:00:17.310Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:17.310Z
Learning: Applies to dash-spv/tests/**/*.rs : Add real network tests with live Dash Core node integration that gracefully handle node unavailability

Applied to files:

  • dash/src/blockdata/constants.rs
📚 Learning: 2025-02-25T06:13:52.858Z
Learnt from: QuantumExplorer
Repo: dashpay/rust-dashcore PR: 51
File: dash/src/sml/masternode_list/scores_for_quorum.rs:50-73
Timestamp: 2025-02-25T06:13:52.858Z
Learning: ScoreHash is a cryptographic hash in the rust-dashcore library, and therefore does not need collision handling when used as a key in collections like BTreeMap due to the extremely low probability of collisions.

Applied to files:

  • dash/src/blockdata/constants.rs
📚 Learning: 2025-12-01T08:00:17.310Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:17.310Z
Learning: Applies to dash-spv/src/wallet/**/*.rs : Implement wallet functionality with UTXO tracking, balance calculation with confirmation states, and transaction processing

Applied to files:

  • dash/src/blockdata/constants.rs
📚 Learning: 2025-12-01T07:59:46.008Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-01T07:59:46.008Z
Learning: Applies to **/tests/**/*.rs : Test both mainnet and testnet configurations

Applied to files:

  • dash/src/blockdata/constants.rs
📚 Learning: 2025-12-01T08:01:18.160Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-01T08:01:18.160Z
Learning: Cover critical parsing, networking, SPV, and wallet flows with tests; add regression tests for fixes; consider property tests with `proptest`

Applied to files:

  • dash/src/blockdata/constants.rs
📚 Learning: 2025-12-01T07:59:46.008Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-01T07:59:46.008Z
Learning: Applies to **/tests/**/*.rs : Write integration tests for network operations in Rust

Applied to files:

  • dash/src/blockdata/constants.rs
📚 Learning: 2025-12-01T08:00:37.906Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:37.906Z
Learning: Applies to key-wallet/**/{wallet,account,managed_account,address_pool}/**/*.rs : Always validate network consistency: use the account's network type when creating addresses and assert that operations use the expected network to prevent mainnet/testnet mixing

Applied to files:

  • dash/src/blockdata/constants.rs
📚 Learning: 2025-12-01T08:00:17.310Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:17.310Z
Learning: Applies to dash-spv/src/**/*.rs : Add comprehensive unit tests in-module for individual components

Applied to files:

  • dash/src/blockdata/constants.rs
📚 Learning: 2025-12-01T08:00:17.310Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:17.310Z
Learning: Applies to dash-spv/tests/**/*.rs : Add integration tests in the `tests/` directory for comprehensive test suites

Applied to files:

  • dash/src/blockdata/constants.rs
📚 Learning: 2025-12-01T08:00:37.906Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:37.906Z
Learning: Applies to key-wallet/**/tests/**/*.rs : Use deterministic test vectors with fixed seeds and known test vectors in unit tests for key derivation, mnemonic generation, and address generation to ensure reproducibility

Applied to files:

  • dash/src/blockdata/constants.rs
📚 Learning: 2025-12-01T08:00:37.906Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:37.906Z
Learning: Applies to key-wallet/**/tests/**/*.rs : Organize tests by functionality: BIP32 key derivation, mnemonic generation/validation, address generation/validation, path derivation, and PSBT serialization in separate test files

Applied to files:

  • dash/src/blockdata/constants.rs
📚 Learning: 2025-12-01T08:00:37.906Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:37.906Z
Learning: Keep test vectors updated with Dash Core, monitor for new DIPs affecting wallet structure, maintain backward compatibility for serialized wallets, and update derivation paths for Platform features

Applied to files:

  • dash/src/blockdata/constants.rs
🧬 Code graph analysis (1)
dash/src/blockdata/constants.rs (1)
dash/src/blockdata/block.rs (2)
  • block_hash (59-63)
  • block_hash (211-213)
⏰ 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). (20)
  • GitHub Check: Core Components Tests
  • GitHub Check: Key Wallet Components Tests
  • GitHub Check: SPV Components Tests
  • GitHub Check: fuzz (hashes_sha512)
  • GitHub Check: fuzz (hashes_json)
  • GitHub Check: fuzz (hashes_sha1)
  • GitHub Check: fuzz (hashes_ripemd160)
  • GitHub Check: fuzz (dash_deserialize_block)
  • GitHub Check: fuzz (hashes_sha256)
  • GitHub Check: fuzz (dash_deser_net_msg)
  • GitHub Check: fuzz (dash_deserialize_amount)
  • GitHub Check: fuzz (hashes_sha512_256)
  • GitHub Check: fuzz (hashes_cbor)
  • GitHub Check: fuzz (dash_deserialize_witness)
  • GitHub Check: fuzz (dash_script_bytes_to_asm_fmt)
  • GitHub Check: fuzz (dash_deserialize_address)
  • GitHub Check: fuzz (dash_deserialize_script)
  • GitHub Check: fuzz (dash_outpoint_string)
  • GitHub Check: Pre-commit (macos-latest)
  • GitHub Check: Pre-commit (ubuntu-latest)
🔇 Additional comments (1)
dash/src/blockdata/constants.rs (1)

280-294: Confirm regtest genesis merkle root and hash against canonical values

The structure of regtest_genesis_full_block is consistent with the other <network>_genesis_full_block tests and provides the missing coverage for Network::Regtest. One thing to double‑check, though:

  • dash_genesis_tx() is shared across all networks, so with a single transaction in txdata, the merkle root is normally expected to be identical across Dash, Testnet, Devnet, and Regtest.
  • In this test, the expected merkle_root.to_string() ("babeaa0b...") differs from the "e0028e..." value asserted in the Dash/Testnet/Devnet tests above, even though the regtest branch in genesis_block still derives the merkle root from txdata[0].txid().

Please verify that both the merkle root and block_hash() literals here match the canonical Dash Core regtest genesis (e.g., via dash-cli -regtest getblockhash 0 and getblock 0) and that the divergence from the other networks’ merkle root is intentional, especially in light of the planned follow‑up in PR #227. As per coding guidelines, this test will serve as a long‑term regression guard once those parameters are finalized.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@xdustinface xdustinface merged commit 8520482 into v0.41-dev Dec 2, 2025
24 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants