refactor(dashcore): extract Network into standalone dash-network crate#679
Merged
QuantumExplorer merged 3 commits intov0.42-devfrom Apr 23, 2026
Merged
refactor(dashcore): extract Network into standalone dash-network crate#679QuantumExplorer merged 3 commits intov0.42-devfrom
QuantumExplorer merged 3 commits intov0.42-devfrom
Conversation
Carves the `Network` enum (Mainnet/Testnet/Devnet/Regtest) out of `dashcore` and into a new dependency-light `dash-network` crate, so that leaf crates (wallets, seed lists, light clients, tools) can depend on the Dash network identity without pulling in the full protocol library. The new crate ships: - `Network` enum (unchanged variants; still `#[non_exhaustive]`) - `magic()` / `from_magic()` - `Display` / `FromStr` with canonical lowercase + "main"/"test"/"dev" aliases; new `ParseNetworkError` instead of the old `String` error - `v20_activation_height()` (pure u32 per-network constant) - `default_p2p_port()` — new helper, matches Dash Core's defaults - optional `serde` + `bincode` feature flags `dashcore` re-exports `Network` and `ParseNetworkError` at its previous path (`dashcore::Network` / `dashcore::network::constants::Network`) so downstream consumers are unaffected. The one method that depends on `BlockHash` — `known_genesis_block_hash` — is moved to a free function `dashcore::known_genesis_block_hash(network)` since `Network` is now a foreign type and inherent impls are not possible. Call sites in `dashcore` and `dash-spv` are updated to the new form. Because `Network` is `#[non_exhaustive]` and now foreign to `dashcore`, a number of `match` statements inside `dashcore` needed wildcard arms added; behavior is unchanged for the four known variants. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 40 minutes and 39 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (60)
📝 WalkthroughWalkthroughThe PR extracts the Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## v0.42-dev #679 +/- ##
=============================================
- Coverage 70.40% 69.76% -0.65%
=============================================
Files 247 319 +72
Lines 48029 66687 +18658
=============================================
+ Hits 33816 46525 +12709
- Misses 14213 20162 +5949
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
dash/src/sml/llmq_type/network.rs (1)
16-85:⚠️ Potential issue | 🟡 MinorInconsistent error handling for unknown
Networkvariants in trait methods.Four methods (
is_llmq_type,isd_llmq_type,chain_locks_type,platform_type) panic viaunreachable!()on an unknown variant, whileenabled_llmq_types()silently returns an emptyVec. SinceNetworkis a#[non_exhaustive]enum from an external crate, a newly added variant would create asymmetric behavior:get_all_dkg_windowswould silently no-op, but unconditional callers of the panicking methods (e.g.,isd_llmq_type()inmessage_request_verification.rs,quorum_helpers.rs, anddash-spv) would crash.Apply a consistent fallback strategy across all five methods. Given that
Networkis#[non_exhaustive]and this is library code, the coding guideline to "avoidunwrap()andexpect()in library code; use proper error types" suggests returningOption<LLMQType>orResult<LLMQType, NetworkError>instead of panicking.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dash/src/sml/llmq_type/network.rs` around lines 16 - 85, Change the trait and implementations for is_llmq_type, isd_llmq_type, chain_locks_type, platform_type, and enabled_llmq_types to return a non-panicking type (preferably Option<LLMQType> or Result<LLMQType, NetworkError> / Option<Vec<LLMQType>> or Result<Vec<LLMQType>, NetworkError>) instead of calling unreachable!() or returning an empty Vec; update each match to return None / Err(...) in the _ arm; add a simple NetworkError enum if using Result; and update all callers (e.g., isd_llmq_type usages in message_request_verification.rs, quorum_helpers.rs, and dash-spv) to handle the Option/Result appropriately.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@dash-network/src/lib.rs`:
- Around line 5-7: The crate-level docs refer to the removed method
Network::known_genesis_block_hash and must be updated to reflect that dashcore
now exposes a free function that returns a BlockHash; replace the sentence
mentioning Network::known_genesis_block_hash with wording that points readers to
the dashcore free function (e.g., "use the free function in dashcore to obtain
the genesis block hash") and update any intra-doc links to point to that free
function instead of the non-existent extension trait or method.
- Around line 213-217: The test default_p2p_ports_match_conventions only asserts
Mainnet/Testnet but default_p2p_port() covers four variants; update the test
(function default_p2p_ports_match_conventions) to also assert
Network::Devnet.default_p2p_port() and Network::Regtest.default_p2p_port() with
their expected values so all four Network variants are covered; locate the test
in lib.rs and add assertions for Network::Devnet and Network::Regtest matching
the convention used for Mainnet/Testnet.
In `@dash/src/address.rs`:
- Around line 1008-1013: The bech32 HRP mapping in the match over self.network()
is wrong for Testnet/Devnet — it currently returns "tb" (Bitcoin) instead of
Dash's "td"; update the match arm for Network::Testnet (and any Devnet variant
if present) so bech32_hrp = "td" (preserving Mainnet "ds" and Regtest "dsrt"),
ensure Address::from_str / Display round-trip for segwit addresses still uses
this HRP, and add/unignore a unit test like testnet_segwit_bech32_round_trip
(referencing Address::p2wpkh, Address::from_str, and require_network) to prevent
future regressions.
In `@dash/src/crypto/key.rs`:
- Around line 395-398: The match assigning the WIF prefix (ret[0] = match
self.network { ... }) currently uses a wildcard that maps any future Network
variants to 239 (Testnet), which will silently misclassify unknown networks;
change the match on self.network in the WIF serialization routine to enumerate
all known non-Mainnet variants explicitly (e.g., Network::Testnet,
Network::Regtest if present) and add a catch-all arm that returns an error (or
panics) so unknown Network variants fail-closed; ensure symmetry with the
from_wif parser by referencing the same Network variants and error path so
serialization does not map unknown enums to 239.
In `@dash/src/ffi/network.rs`:
- Line 22: The From<Network> for FFINetwork impl currently ends with `_ =>
unreachable!()` which is unsafe because Network is marked #[non_exhaustive];
change the conversion to a fallible conversion (implement TryFrom<Network> for
FFINetwork returning Result<FFINetwork, ConversionError>) and update call sites
that use network.into() (notably wallet_manager_create / places calling
network.into()) to handle the Result or perform an explicit expect with a
documented error only if you can guarantee exhaustiveness; replace the
unreachable!() arm with a Err variant (or map unknown variants to a safe default
only if intentionally desired) so panics cannot unwind across the FFI boundary.
In `@dash/src/network/constants.rs`:
- Around line 85-108: The runtime hex decoding and expect calls in
known_genesis_block_hash should be replaced with infallible, compile-time byte
arrays (or hex_literal::hex! constants) and stored as static/const values so no
allocation or fallible decode happens on each call; update the Network::Mainnet,
Network::Testnet, and Network::Regtest arms to return
Some(BlockHash::from_byte_array(<const [u8;32]>) ) directly (or reference a
static once cell), removing all expect/try_into usage and ensuring the
construction is infallible in library code.
---
Outside diff comments:
In `@dash/src/sml/llmq_type/network.rs`:
- Around line 16-85: Change the trait and implementations for is_llmq_type,
isd_llmq_type, chain_locks_type, platform_type, and enabled_llmq_types to return
a non-panicking type (preferably Option<LLMQType> or Result<LLMQType,
NetworkError> / Option<Vec<LLMQType>> or Result<Vec<LLMQType>, NetworkError>)
instead of calling unreachable!() or returning an empty Vec; update each match
to return None / Err(...) in the _ arm; add a simple NetworkError enum if using
Result; and update all callers (e.g., isd_llmq_type usages in
message_request_verification.rs, quorum_helpers.rs, and dash-spv) to handle the
Option/Result appropriately.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b3ef8e67-8b03-4f81-844f-21e50c64f129
📒 Files selected for processing (16)
.github/ci-groups.ymlCargo.tomldash-network/Cargo.tomldash-network/src/lib.rsdash-spv/src/client/lifecycle.rsdash/Cargo.tomldash/src/address.rsdash/src/blockdata/constants.rsdash/src/consensus/params.rsdash/src/crypto/key.rsdash/src/ffi/network.rsdash/src/lib.rsdash/src/network/constants.rsdash/src/sml/llmq_type/network.rsdash/src/sml/masternode_list/from_diff.rsdash/src/sml/masternode_list_engine/mod.rs
Comment on lines
1008
to
1013
| let bech32_hrp = match self.network() { | ||
| Network::Mainnet => "ds", | ||
| Network::Testnet | Network::Devnet => "tb", | ||
| Network::Regtest => "dsrt", | ||
| // Testnet / Devnet (and any future non-mainnet variants). | ||
| _ => "tb", | ||
| }; |
Contributor
There was a problem hiding this comment.
Critical: bech32 HRP regression — Dash testnet should be "td", not "tb".
"tb" is the Bitcoin testnet HRP (BIP-173). Dash's testnet bech32 HRP in this crate is "td", as the FromStr implementation at Line 1348 still asserts ("td" | "TD" => Some(Network::Testnet)) and as the previous/commented-out implementation at Lines 1420-1423 used. The wildcard collapse in this PR introduces three concrete defects:
- Formatting a
Testnet/Devnetsegwit address now produces a string liketb1…, which is an invalid Dash testnet address. Address::from_str→Display→from_strno longer round-trips for Testnet/Devnet segwit addresses — parsingtb…falls through to Base58 and errors.- The emitted strings are indistinguishable from Bitcoin testnet bech32 addresses, which is a footgun for wallets and explorers sharing formatting libraries.
This isn't caught by tests: test_bip173_350_vectors is #[ignore], and the testnet/devnet block in test_qr_string (Lines 1868-1871) is commented out. The new serde testnet round-trip test (serde_deserialize_network_checked_testnet_round_trip) only covers a base58 P2PKH (y…), not the segwit path.
🐛 Proposed fix
let bech32_hrp = match self.network() {
Network::Mainnet => "ds",
Network::Regtest => "dsrt",
- // Testnet / Devnet (and any future non-mainnet variants).
- _ => "tb",
+ // Testnet / Devnet (and any future non-mainnet variants).
+ _ => "td",
};Please also un-ignore / add a bech32 testnet round-trip test so this doesn't regress again, e.g.:
#[test]
fn testnet_segwit_bech32_round_trip() {
use crate::crypto::key::PublicKey;
let key = "03df154ebfcf29d29cc10d5c2565018bce2d9edbab267c31d2caf44a63056cf99f"
.parse::<PublicKey>().unwrap();
let addr = Address::p2wpkh(&key, Network::Testnet).unwrap();
let s = addr.to_string();
assert!(s.starts_with("td1"), "unexpected HRP: {}", s);
let parsed = Address::from_str(&s).unwrap().require_network(Network::Testnet).unwrap();
assert_eq!(parsed.script_pubkey(), addr.script_pubkey());
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| let bech32_hrp = match self.network() { | |
| Network::Mainnet => "ds", | |
| Network::Testnet | Network::Devnet => "tb", | |
| Network::Regtest => "dsrt", | |
| // Testnet / Devnet (and any future non-mainnet variants). | |
| _ => "tb", | |
| }; | |
| let bech32_hrp = match self.network() { | |
| Network::Mainnet => "ds", | |
| Network::Regtest => "dsrt", | |
| // Testnet / Devnet (and any future non-mainnet variants). | |
| _ => "td", | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@dash/src/address.rs` around lines 1008 - 1013, The bech32 HRP mapping in the
match over self.network() is wrong for Testnet/Devnet — it currently returns
"tb" (Bitcoin) instead of Dash's "td"; update the match arm for Network::Testnet
(and any Devnet variant if present) so bech32_hrp = "td" (preserving Mainnet
"ds" and Regtest "dsrt"), ensure Address::from_str / Display round-trip for
segwit addresses still uses this HRP, and add/unignore a unit test like
testnet_segwit_bech32_round_trip (referencing Address::p2wpkh,
Address::from_str, and require_network) to prevent future regressions.
The C-ABI mirror of `Network` belongs with its owning type. Moves `dash/src/ffi/network.rs` verbatim (plus additional unit tests) into `dash-network/src/ffi.rs` behind a new `ffi` feature flag. `dashcore`'s existing `ffi` feature now forwards to `dash-network/ffi`, and `dash/src/ffi/mod.rs` re-exports so the stable public path `dashcore::ffi::FFINetwork` / `dashcore::ffi::dashcore_network_get_name` is preserved for all downstream consumers (dash-spv-ffi, etc.). Added tests in the new location cover: - ABI discriminants (Mainnet=0, Testnet=1, Devnet=2, Regtest=3) - `FFINetwork <-> Network` round-trip - `dashcore_network_get_name` returns canonical lowercase names The extern symbol `dashcore_network_get_name` is still exported via the dashcore rlib (verified with `nm`). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
dash/Cargo.toml (1)
21-36:⚠️ Potential issue | 🟡 MinorRemoving the
ffifeature fromdashcoreis a breaking change for downstream crates.Previously
dashcoreexposed anffifeature that re-exportedFFINetworkanddashcore_network_get_name. After this PR, enablingdashcore/ffi(as e.g. older versions ofkey-wallet-ffidid) is a hard cargo error, not a graceful deprecation. The PR description says the intent is to "forward todash-network", but the manifest simply drops the feature — nothing forwards.If you want to keep the old opt-in working transparently for one release, add a forwarding feature:
default = ["secp-recovery", "bincode" ] +# Re-exports the FFI surface from `dash-network`. Prefer depending on +# `dash-network` directly with `features = ["ffi"]` in new code. +ffi = ["dash-network/ffi"](…and add a matching conditional
pub use dash_network::ffi;indash/src/lib.rsbehind#[cfg(feature = "ffi")].) If the breakage is intentional, please call it out in the CHANGELOG / release notes and ensure thedashcoreversion is bumped accordingly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dash/Cargo.toml` around lines 21 - 36, The PR removed the `ffi` feature which breaks downstream crates expecting `dashcore/ffi`; restore compatibility by adding an `ffi = ["dash-network/ffi"]` entry to the [features] table in Cargo.toml (alongside existing features like `secp-recovery` and `bincode`) and add a conditional re-export in dash/src/lib.rs: place `#[cfg(feature = "ffi")] pub use dash_network::ffi;` so enabling `dashcore/ffi` forwards to `dash-network` transparently; if the break was intentional instead, update the CHANGELOG/release notes and bump the crate version appropriately.dash-spv-ffi/src/config.rs (1)
117-122:⚠️ Potential issue | 🟡 MinorDevnet default port (29999) inconsistent with the new
default_p2p_port()helper, which returns 19799.The match block hardcodes network-to-port mappings instead of delegating to the centralized helper. This duplicates values and creates drift risk: Devnet currently uses
29999here but the helper atdash-network/src/lib.rs:116defines it as19799. Consolidate by callingcfg.network.default_p2p_port()instead, ensuring a single source of truth for all network port defaults.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dash-spv-ffi/src/config.rs` around lines 117 - 122, The hardcoded network-to-port match that sets default_port duplicates logic and uses an incorrect Devnet value (29999); replace the match with a single call to the centralized helper by assigning default_port = cfg.network.default_p2p_port() so the code uses the authoritative default_p2p_port() implementation (see the helper in dash-network lib) and removes the duplicated mapping in this module.
🧹 Nitpick comments (2)
dash/src/network/constants.rs (1)
53-59: Missing trait-level rustdoc on publicNetworkExt.Only
known_genesis_block_hashhas docs; the trait itself lacks a summary line explaining what it is and why consumers should import it (especially important since it's the recommended replacement for the removed inherent method and is referenced fromdash-network's crate-level docs).📝 Proposed doc
+/// Extension trait that attaches `dashcore`‑level data to the dependency‑light +/// [`Network`] type defined in the `dash-network` crate. pub trait NetworkExt { /// Returns the known genesis block hash for `network`, if one is hardcoded.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dash/src/network/constants.rs` around lines 53 - 59, Add a trait-level rustdoc comment for NetworkExt that succinctly explains what the trait provides (extension methods for Network), why consumers should import it (it's the recommended replacement for the removed inherent method), and a short pointer that known_genesis_block_hash returns hardcoded genesis hashes (Devnet returns None); update the doc so crate-level docs can reference NetworkExt as the recommended way to call network extension utilities and mention known_genesis_block_hash as an example.dash-network/build.rs (1)
17-36: Build script writes outsideOUT_DIRvia a fragileancestors().nth(3)walk.Cargo's convention is for build scripts to write only into
OUT_DIR; writing intotarget/<PROFILE>/include/<crate>/viaPath::ancestors().nth(3)couples to Cargo's internal OUT_DIR layout (target/<PROFILE>/build/<crate>-<hash>/out). Any future Cargo change to that layout silently breaks the header generation, and concurrent builds orCARGO_TARGET_DIRoverrides can race or surprise consumers. Presumably this mirrors the previous dashcore behavior to keep downstream C consumers working, but consider emitting toOUT_DIRand exposing the path viacargo:include=<path>/DEP_<name>_INCLUDEso dependents pick it up without build scripts mutating shared target subdirs.Also note the
unwrap()/expect()calls onenv::varwill simply abort the build with unhelpful messages if any ofCARGO_PKG_NAME/CARGO_MANIFEST_DIR/OUT_DIRare missing; acceptable for a build script, but a one-lineexpect("… (set by Cargo)")makes the failure mode obvious.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dash-network/build.rs` around lines 17 - 36, The build script currently walks ancestors of out_dir (ancestors().nth(3)) to write headers into target/<PROFILE>/include which is fragile; change it to emit headers into OUT_DIR instead by building include_dir from out_dir (e.g. Path::new(&out_dir).join("include").join(&crate_name)), create that directory and write output_path there, then print the published path to stdout as a cargo metadata line (cargo:include=...) so dependents can discover it (DEP_<crate>_INCLUDE convention); also replace any plain env::var unwraps/expect messages for CARGO_PKG_NAME, CARGO_MANIFEST_DIR, and OUT_DIR with clearer expect messages like "CARGO_PKG_NAME must be set by Cargo" to make failures explicit, and keep cbindgen::Builder usage the same but point its write_to_file to the new output_path.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@dash-network/src/lib.rs`:
- Around line 31-46: The Network enum was intended to be non-exhaustive but the
#[non_exhaustive] attribute is missing; add #[non_exhaustive] above the pub enum
Network declaration so external crates treat it as extensible and avoid making
future variant additions a breaking change, and remove any now-unnecessary
wildcard match accommodations in dashcore if they were added to handle a foreign
non-exhaustive enum.
- Around line 73-80: The doctest for Network::magic() imports the wrong crate
(it uses dashcore) causing doc tests to fail; update the example to import from
this crate (use dash_network::Network;) so the example uses the local Network
type and compiles under cargo test --doc -p dash-network, i.e., replace the
dashcore import with dash_network in the Network::magic() doc example.
In `@dash/Cargo.toml`:
- Line 46: Update the dash/Cargo.toml dependency for dash-network to point to a
dash-network crate using edition = "2024" to match dashcore and ensure
consistent editions across the workspace; open dash-network/Cargo.toml and
change its [package] edition to "2024" (and remove/align any conflicting edition
settings), and add an explicit rust-version = "1.89" either in the workspace
Cargo.toml or in dashcore's Cargo.toml so the workspace MSRV is enforced
consistently with dash-network's current rust-version.
---
Outside diff comments:
In `@dash-spv-ffi/src/config.rs`:
- Around line 117-122: The hardcoded network-to-port match that sets
default_port duplicates logic and uses an incorrect Devnet value (29999);
replace the match with a single call to the centralized helper by assigning
default_port = cfg.network.default_p2p_port() so the code uses the authoritative
default_p2p_port() implementation (see the helper in dash-network lib) and
removes the duplicated mapping in this module.
In `@dash/Cargo.toml`:
- Around line 21-36: The PR removed the `ffi` feature which breaks downstream
crates expecting `dashcore/ffi`; restore compatibility by adding an `ffi =
["dash-network/ffi"]` entry to the [features] table in Cargo.toml (alongside
existing features like `secp-recovery` and `bincode`) and add a conditional
re-export in dash/src/lib.rs: place `#[cfg(feature = "ffi")] pub use
dash_network::ffi;` so enabling `dashcore/ffi` forwards to `dash-network`
transparently; if the break was intentional instead, update the
CHANGELOG/release notes and bump the crate version appropriately.
---
Nitpick comments:
In `@dash-network/build.rs`:
- Around line 17-36: The build script currently walks ancestors of out_dir
(ancestors().nth(3)) to write headers into target/<PROFILE>/include which is
fragile; change it to emit headers into OUT_DIR instead by building include_dir
from out_dir (e.g. Path::new(&out_dir).join("include").join(&crate_name)),
create that directory and write output_path there, then print the published path
to stdout as a cargo metadata line (cargo:include=...) so dependents can
discover it (DEP_<crate>_INCLUDE convention); also replace any plain env::var
unwraps/expect messages for CARGO_PKG_NAME, CARGO_MANIFEST_DIR, and OUT_DIR with
clearer expect messages like "CARGO_PKG_NAME must be set by Cargo" to make
failures explicit, and keep cbindgen::Builder usage the same but point its
write_to_file to the new output_path.
In `@dash/src/network/constants.rs`:
- Around line 53-59: Add a trait-level rustdoc comment for NetworkExt that
succinctly explains what the trait provides (extension methods for Network), why
consumers should import it (it's the recommended replacement for the removed
inherent method), and a short pointer that known_genesis_block_hash returns
hardcoded genesis hashes (Devnet returns None); update the doc so crate-level
docs can reference NetworkExt as the recommended way to call network extension
utilities and mention known_genesis_block_hash as an example.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ae10f5b9-85fd-481e-9140-cba29d67417a
📒 Files selected for processing (60)
dash-network/Cargo.tomldash-network/build.rsdash-network/cbindgen.tomldash-network/src/ffi.rsdash-network/src/lib.rsdash-spv-ffi/Cargo.tomldash-spv-ffi/cbindgen.tomldash-spv-ffi/src/bin/ffi_cli.rsdash-spv-ffi/src/config.rsdash-spv-ffi/tests/dashd_sync/context.rsdash-spv-ffi/tests/test_client.rsdash-spv-ffi/tests/test_config.rsdash-spv-ffi/tests/test_types.rsdash-spv-ffi/tests/test_wallet_manager.rsdash-spv-ffi/tests/unit/test_async_operations.rsdash-spv-ffi/tests/unit/test_client_lifecycle.rsdash-spv-ffi/tests/unit/test_configuration.rsdash-spv-ffi/tests/unit/test_error_handling.rsdash-spv-ffi/tests/unit/test_memory_management.rsdash-spv-ffi/tests/unit/test_type_conversions.rsdash-spv/src/client/lifecycle.rsdash/Cargo.tomldash/build.rsdash/src/ffi/mod.rsdash/src/lib.rsdash/src/network/constants.rsdash/src/sml/masternode_list/from_diff.rsdash/src/sml/masternode_list_engine/mod.rsffi-c-tests/header-tests/all.cffi-c-tests/header-tests/dash-network.cffi-c-tests/header-tests/dashcore.ckey-wallet-ffi/Cargo.tomlkey-wallet-ffi/cbindgen.tomlkey-wallet-ffi/src/account.rskey-wallet-ffi/src/account_collection.rskey-wallet-ffi/src/account_derivation_tests.rskey-wallet-ffi/src/address.rskey-wallet-ffi/src/address_pool.rskey-wallet-ffi/src/address_tests.rskey-wallet-ffi/src/derivation.rskey-wallet-ffi/src/keys.rskey-wallet-ffi/src/keys_tests.rskey-wallet-ffi/src/managed_account.rskey-wallet-ffi/src/managed_wallet.rskey-wallet-ffi/src/managed_wallet_tests.rskey-wallet-ffi/src/transaction.rskey-wallet-ffi/src/wallet.rskey-wallet-ffi/src/wallet_manager.rskey-wallet-ffi/src/wallet_manager_serialization_tests.rskey-wallet-ffi/src/wallet_manager_tests.rskey-wallet-ffi/src/wallet_tests.rskey-wallet-ffi/tests/debug_wallet_add.rskey-wallet-ffi/tests/integration_test.rskey-wallet-ffi/tests/test_account_collection.rskey-wallet-ffi/tests/test_addr_simple.rskey-wallet-ffi/tests/test_import_wallet.rskey-wallet-ffi/tests/test_managed_account_collection.rskey-wallet-ffi/tests/test_passphrase_wallets.rskey-wallet/src/account/account_type.rskey-wallet/src/derivation.rs
💤 Files with no reviewable changes (6)
- key-wallet/src/derivation.rs
- ffi-c-tests/header-tests/dashcore.c
- dash/src/ffi/mod.rs
- key-wallet-ffi/src/keys_tests.rs
- dash/build.rs
- key-wallet/src/account/account_type.rs
✅ Files skipped from review due to trivial changes (38)
- key-wallet-ffi/tests/debug_wallet_add.rs
- dash-spv-ffi/tests/unit/test_error_handling.rs
- key-wallet-ffi/src/account_derivation_tests.rs
- key-wallet-ffi/tests/test_account_collection.rs
- dash-spv-ffi/tests/unit/test_async_operations.rs
- dash-spv-ffi/tests/test_config.rs
- dash-spv-ffi/tests/unit/test_type_conversions.rs
- ffi-c-tests/header-tests/all.c
- dash-spv-ffi/tests/test_client.rs
- key-wallet-ffi/cbindgen.toml
- key-wallet-ffi/src/address_pool.rs
- dash-spv-ffi/tests/unit/test_client_lifecycle.rs
- key-wallet-ffi/tests/integration_test.rs
- key-wallet-ffi/tests/test_addr_simple.rs
- key-wallet-ffi/tests/test_managed_account_collection.rs
- dash/src/sml/masternode_list_engine/mod.rs
- dash-spv-ffi/src/bin/ffi_cli.rs
- key-wallet-ffi/src/managed_wallet.rs
- dash-spv-ffi/tests/unit/test_configuration.rs
- key-wallet-ffi/src/wallet_manager_tests.rs
- key-wallet-ffi/src/managed_wallet_tests.rs
- key-wallet-ffi/src/account_collection.rs
- key-wallet-ffi/src/wallet_tests.rs
- key-wallet-ffi/src/wallet_manager_serialization_tests.rs
- key-wallet-ffi/tests/test_import_wallet.rs
- key-wallet-ffi/src/derivation.rs
- key-wallet-ffi/src/address.rs
- dash-network/src/ffi.rs
- key-wallet-ffi/src/keys.rs
- dash-spv/src/client/lifecycle.rs
- key-wallet-ffi/src/wallet.rs
- dash/src/sml/masternode_list/from_diff.rs
- key-wallet-ffi/src/managed_account.rs
- dash-spv-ffi/tests/test_wallet_manager.rs
- dash-spv-ffi/cbindgen.toml
- key-wallet-ffi/src/transaction.rs
- key-wallet-ffi/tests/test_passphrase_wallets.rs
- dash-network/cbindgen.toml
🚧 Files skipped from review as they are similar to previous changes (2)
- dash-network/Cargo.toml
- dash/src/lib.rs
ZocoLini
force-pushed
the
feat/dash-network-crate
branch
from
db00e40 to
195c913
Compare
ZocoLini
force-pushed
the
feat/dash-network-crate
branch
from
195c913 to
7bd6b6f
Compare
Member
Author
|
Reviewed |
QuantumExplorer
added a commit
that referenced
this pull request
Apr 24, 2026
Pulls in upstream changes through 0093278: - PR #678 (hardcoded masternode seed files + weekly auto-refresh) — already present on pw2 via direct branch merge (84adf54); the upstream versions are taken via `theirs` to stay canonical (edition 2024, `--max-peers 16` flag + `timeout-minutes: 60` in the workflow, "with probes" refresh step name, richer lib.rs doc comments, probe-enriched seed-file format). - PR #679 (refactor(dashcore): extract Network into standalone dash-network crate) — dashcore re-exports Network at its original path, so consumers are unaffected at the API level. Internal reroutes applied where needed. Conflict resolution: - Add/add (took theirs = v0.42-dev canonical): - .github/workflows/update-masternode-seeds.yml - dash-network-seeds/{Cargo.toml,src/lib.rs,seeds/mainnet.txt,seeds/testnet.txt} - masternode-seeds-fetcher/{Cargo.toml,src/main.rs} - Cargo.toml (root): unioned workspace members — kept pw2's dash-network-seeds + masternode-seeds-fetcher and added #679's new dash-network crate. - dash-spv/Cargo.toml: dropped pw2's now-defunct dash-network-seeds `features = ["dashcore"]` (the upstream crate has no such feature anymore; it depends on dash-network directly) and added an explicit dash-network dep for direct Network::Mainnet/Testnet references. - dash-spv/src/network/constants.rs: kept pw2's HEAD block in full (MAINNET_DNS_SEEDS, TESTNET_DNS_SEEDS, TESTNET_FIXED_PEERS, MAINNET_P2P_PORT, TESTNET_P2P_PORT) and rewrote the two seed-peer helper functions to call `dash_network_seeds::addresses( dash_network::Network::Mainnet|Testnet)` — `Network` moved out of dash_network_seeds. - dash-spv/src/network/discovery.rs: kept pw2's full behavior — the match-on-Network arm that picks DNS-seed/port/embedded-list tuples per network and the testnet TESTNET_FIXED_PEERS fallback loop — rather than v0.42-dev's simpler `network.dns_seeds()` / generic path, because pw2's code additionally defends against DNS failure on testnet with a coded-in peer list (PR #658). Fallout from #679: - key-wallet/src/account/account_type.rs: removed two now-dead `_ => Err(InvalidNetwork)` arms in IdentityAuthenticationEcdsa / IdentityAuthenticationBls derivation-path construction. Network is no longer `#[non_exhaustive]` once re-exported from the standalone dash-network crate, and all four variants were already covered, so the wildcard arm became `unreachable_patterns` under -D warnings. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
6 tasks
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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
Carves the
Networkenum out ofdashcoreinto a new dependency-lightdash-networkcrate, so that leaf crates (wallets, seed lists, light clients, tools) can depend on the Dash network identity without pulling in the full protocol library.This PR intentionally ships as a prerequisite to other work that wants a tiny
Networkdep — notably thedash-network-seedscrate in the open seed-list work. Landing this first lets that crate drop its own localNetworkenum.What's in the new
dash-networkcrateNetworkenum (unchanged variants; still#[non_exhaustive])magic()/from_magic()— bothconst fnDisplay/FromStr, with canonical lowercase output and\"main\"/\"test\"/\"dev\"aliases on parse. NewParseNetworkErrorreplaces the oldStringerror type.v20_activation_height()— the only numeric-only per-network helper that doesn't needBlockHashdefault_p2p_port()— new helper, mirrors Dash Core's defaults (9999 / 19999 / 19799 / 19899)serde(lowercase rename) andbincodefeature flagsZero runtime deps when those features are off.
dashcorecompatibilitydashcorere-exportsNetworkandParseNetworkErrorat their previous paths so downstream consumers are unaffected:The one method that had to move is
known_genesis_block_hash— it returns aBlockHash(which lives indashcore), andNetworkis now a foreign enum, so inherent impls on it fromdashcoreare impossible. It becomes a free function:Three call sites updated:
dash/src/sml/masternode_list/from_diff.rs,dash/src/sml/masternode_list_engine/mod.rs,dash-spv/src/client/lifecycle.rs.Because
Networkis#[non_exhaustive]and is now foreign todashcore, a handful ofmatch network { ... }sites insidedashcoreneeded wildcard arms added (address.rs,blockdata/constants.rs,consensus/params.rs,crypto/key.rs,ffi/network.rs,sml/llmq_type/network.rs). Behavior is unchanged for the four known variants.Test plan
cargo build --workspacecargo clippy --workspace --all-targets -- -D warningscargo fmt --all --checkcargo test -p dash-network --all-features— 7 unit + 4 doctests, all passing (covers magic round-trip, case-insensitiveFromStr, display, activation heights, default ports)cargo test -p dashcore --lib— 531 passed, 0 failedcargo test -p dash-spv --lib— 374 passed, 0 failedpython3 .github/scripts/ci_config.py verify-groupspasses (dash-networkadded tocoregroup)🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
New Features
dash-networklibrary module for centralized network configuration and protocol handling.Refactor
dash-network.