feat: bump rust-dashcore to mempool support (1af96dd)#3390
feat: bump rust-dashcore to mempool support (1af96dd)#3390lklimek wants to merge 9 commits intorefactor/sdk-rpitit-fetch-traitsfrom
Conversation
Update all rust-dashcore dependencies (dashcore, dash-spv, dash-spv-ffi, key-wallet, key-wallet-manager, dashcore-rpc) to commit 9959201593826d. Also unify rs-platform-encryption which was pinned to a different older rev. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Remove `create_unsigned_payment_transaction`, `FeeLevel`, and `TransactionError` usage — all removed from the upstream WalletInfoInterface trait in rust-dashcore v0.42-dev. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
lklimek
force-pushed
the
feat/mempool-support
branch
from
422ecb4 to
6d9efa9
Compare
Update all 6 rust-dashcore workspace dependencies to commit 1af96dd02f12576ba0727476e39ff78aca0a1e13 which adds MempoolManager (8th parallel sync manager) with bloom filter based mempool monitoring. Remove "std" feature from rs-dpp dashcore dependency as it was eliminated upstream. Adapt to upstream API changes: - Network::Dash renamed to Network::Mainnet - DMNState.service changed from SocketAddr to Option<SocketAddr> - WalletInfoInterface gained mark_instant_send_utxos method - WalletTransactionChecker::check_core_transaction gained update_balance param Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
lklimek
force-pushed
the
feat/mempool-support
branch
from
6d9efa9 to
2414799
Compare
The cherry-pick of 6d9efa9 included 548 lines of unrelated test code in consensus_params_update/mod.rs due to --theirs conflict resolution. Restore the file to its base branch state. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Human-reviewed, on top of some old commit as v3.1-dev breaks dash-evo-tool |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
A thorough dependency bump with consistent Network::Dash→Network::Mainnet rename across 53 files and correct Option adaptation in most paths. The two substantive issues are: (1) the incremental validator-set update path silently ignores service removal (Some(None)), creating a divergence from fresh validator-set construction that could cause inconsistent state after restart; and (2) the SocketAddr→Option change in MasternodeStateV0 breaks bincode serialization format for persisted PlatformState without a versioned migration, acknowledged by the PR's 2 known snapshot test failures.
Reviewed commit: 2af8cac
🟡 4 suggestion(s) | 💬 2 nitpick(s)
1 additional finding
🟡 suggestion: platform_http_port changes never trigger validator set update
packages/rs-drive-abci/src/execution/platform_events/core_based_updates/update_masternode_list/update_state_masternode_list/v0/mod.rs (lines 144-147)
The condition at lines 144-147 checks pose_ban_height, service, and platform_p2p_port but does NOT include platform_http_port. The update_masternode_in_validator_sets function (line 75-76) correctly handles platform_http_port changes, but for HTTP-port-only diffs this code path is never reached. The comment on line 142 says "these 3 fields are the only fields useful for validators" but platform_http_port is a 4th validator-relevant field that's handled inside the function but gated out at the call site.
💡 Suggested change
if state_diff.pose_ban_height.is_some()
|| state_diff.service.is_some()
|| state_diff.platform_p2p_port.is_some()
|| state_diff.platform_http_port.is_some()
{
🤖 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 `packages/rs-drive-abci/src/execution/platform_events/core_based_updates/update_masternode_list/update_state_masternode_list/v0/mod.rs`:
- [SUGGESTION] lines 67-69: Some(None) service diff leaves stale validator IP/port and diverges from fresh construction
When `dmn_state_diff.service` is `Some(None)` (service address removed by Core), the pattern `if let Some(Some(address))` at line 67 silently skips the update. The validator retains its old `node_ip` and `core_port`. Meanwhile, `ValidatorV0::new_validator_if_masternode_in_state()` (validator/v0/mod.rs:40) returns `None` for masternodes with `service: None`, so any fresh validator-set construction would exclude this masternode.
This creates an asymmetry: incrementally-updated sets retain the validator with stale data, while rebuilt sets (e.g., after restart or new quorum construction) exclude it. If a node restarts while a quorum is active, its validator set could differ from live nodes that processed the diff incrementally.
Since upstream rust-dashcore now explicitly allows `service: None` for masternodes, this path is reachable in production. Consider removing the member from the validator set when `Some(None)` is received.
- [SUGGESTION] lines 144-147: platform_http_port changes never trigger validator set update
The condition at lines 144-147 checks `pose_ban_height`, `service`, and `platform_p2p_port` but does NOT include `platform_http_port`. The `update_masternode_in_validator_sets` function (line 75-76) correctly handles `platform_http_port` changes, but for HTTP-port-only diffs this code path is never reached. The comment on line 142 says "these 3 fields are the only fields useful for validators" but `platform_http_port` is a 4th validator-relevant field that's handled inside the function but gated out at the call site.
In `packages/rs-drive-abci/src/platform_types/masternode/v0/mod.rs`:
- [SUGGESTION] lines 97-101: SocketAddr → Option<SocketAddr> breaks bincode serialization format without migration
Changing `service` from `SocketAddr` to `Option<SocketAddr>` in `MasternodeStateV0` (line 101) breaks binary compatibility. `MasternodeStateV0` is part of the serialization chain: `PlatformStateForSaving` (V0 and V1) → `Masternode::V0(MasternodeV0)` → `MasternodeStateV0`. Both `PlatformStateForSavingV0` and `PlatformStateForSavingV1` contain `BTreeMap<Bytes32, Masternode>` which includes this struct.
Bincode encodes `Option<T>` with a leading discriminant byte that bare `T` does not have, so previously-serialized platform state will fail to deserialize. The PR acknowledges 2 snapshot test failures (`should_deserialize_state_stored_in_version_0_from_testnet` and `should_deserialize_state_stored_in_version_8_from_devnet`).
If this ships alongside a mandatory re-sync or consensus upgrade that discards old state, this is acceptable. If nodes must read pre-existing persisted state, a `MasternodeStateV1` or a new `PlatformStateForSavingV2` with migration logic is needed. Please confirm intended upgrade path.
In `packages/rs-drive-abci/tests/strategy_tests/masternodes.rs`:
- [SUGGESTION] lines 249-253: No tests exercise service: None path
All test masternode generators construct `service: Some(SocketAddr::from_str(...).unwrap())` (line 250), and the IP update path uses `.expect("service address")` (line 356). There are zero tests with `service: None` anywhere in the test suite. This leaves the new `Option<SocketAddr>` behavior untested in: validator creation (`new_validator_if_masternode_in_state` returning None), validator set updates (the `Some(None)` diff path), and serialization round-trips. Adding at least one test that creates a masternode with `service: None` would validate the new code path.
| if let Some(Some(address)) = dmn_state_diff.service { | ||
| validator.node_ip = address.ip().to_string(); | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: Some(None) service diff leaves stale validator IP/port and diverges from fresh construction
When dmn_state_diff.service is Some(None) (service address removed by Core), the pattern if let Some(Some(address)) at line 67 silently skips the update. The validator retains its old node_ip and core_port. Meanwhile, ValidatorV0::new_validator_if_masternode_in_state() (validator/v0/mod.rs:40) returns None for masternodes with service: None, so any fresh validator-set construction would exclude this masternode.
This creates an asymmetry: incrementally-updated sets retain the validator with stale data, while rebuilt sets (e.g., after restart or new quorum construction) exclude it. If a node restarts while a quorum is active, its validator set could differ from live nodes that processed the diff incrementally.
Since upstream rust-dashcore now explicitly allows service: None for masternodes, this path is reachable in production. Consider removing the member from the validator set when Some(None) is received.
💡 Suggested change
| if let Some(Some(address)) = dmn_state_diff.service { | |
| validator.node_ip = address.ip().to_string(); | |
| } | |
| if let Some(maybe_address) = dmn_state_diff.service { | |
| match maybe_address { | |
| Some(address) => { | |
| validator.node_ip = address.ip().to_string(); | |
| validator.core_port = address.port(); | |
| } | |
| None => { | |
| // Service removed — remove validator from set | |
| // to match new_validator_if_masternode_in_state behavior | |
| validator_set.members_mut().remove(pro_tx_hash); | |
| return; | |
| } | |
| } | |
| } |
source: ['claude-general', 'codex-general', 'claude-security-auditor', 'codex-security-auditor', 'claude-rust-quality', 'codex-rust-quality']
🤖 Fix this 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 `packages/rs-drive-abci/src/execution/platform_events/core_based_updates/update_masternode_list/update_state_masternode_list/v0/mod.rs`:
- [SUGGESTION] lines 67-69: Some(None) service diff leaves stale validator IP/port and diverges from fresh construction
When `dmn_state_diff.service` is `Some(None)` (service address removed by Core), the pattern `if let Some(Some(address))` at line 67 silently skips the update. The validator retains its old `node_ip` and `core_port`. Meanwhile, `ValidatorV0::new_validator_if_masternode_in_state()` (validator/v0/mod.rs:40) returns `None` for masternodes with `service: None`, so any fresh validator-set construction would exclude this masternode.
This creates an asymmetry: incrementally-updated sets retain the validator with stale data, while rebuilt sets (e.g., after restart or new quorum construction) exclude it. If a node restarts while a quorum is active, its validator set could differ from live nodes that processed the diff incrementally.
Since upstream rust-dashcore now explicitly allows `service: None` for masternodes, this path is reachable in production. Consider removing the member from the validator set when `Some(None)` is received.
| pub struct MasternodeStateV0 { | ||
| /// Masternode's network service address. | ||
| #[bincode(with_serde)] | ||
| pub service: SocketAddr, | ||
| pub service: Option<SocketAddr>, |
There was a problem hiding this comment.
🟡 Suggestion: SocketAddr → Option breaks bincode serialization format without migration
Changing service from SocketAddr to Option<SocketAddr> in MasternodeStateV0 (line 101) breaks binary compatibility. MasternodeStateV0 is part of the serialization chain: PlatformStateForSaving (V0 and V1) → Masternode::V0(MasternodeV0) → MasternodeStateV0. Both PlatformStateForSavingV0 and PlatformStateForSavingV1 contain BTreeMap<Bytes32, Masternode> which includes this struct.
Bincode encodes Option<T> with a leading discriminant byte that bare T does not have, so previously-serialized platform state will fail to deserialize. The PR acknowledges 2 snapshot test failures (should_deserialize_state_stored_in_version_0_from_testnet and should_deserialize_state_stored_in_version_8_from_devnet).
If this ships alongside a mandatory re-sync or consensus upgrade that discards old state, this is acceptable. If nodes must read pre-existing persisted state, a MasternodeStateV1 or a new PlatformStateForSavingV2 with migration logic is needed. Please confirm intended upgrade path.
source: ['claude-general', 'claude-security-auditor', 'claude-rust-quality', 'claude-ffi-engineer']
🤖 Fix this 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 `packages/rs-drive-abci/src/platform_types/masternode/v0/mod.rs`:
- [SUGGESTION] lines 97-101: SocketAddr → Option<SocketAddr> breaks bincode serialization format without migration
Changing `service` from `SocketAddr` to `Option<SocketAddr>` in `MasternodeStateV0` (line 101) breaks binary compatibility. `MasternodeStateV0` is part of the serialization chain: `PlatformStateForSaving` (V0 and V1) → `Masternode::V0(MasternodeV0)` → `MasternodeStateV0`. Both `PlatformStateForSavingV0` and `PlatformStateForSavingV1` contain `BTreeMap<Bytes32, Masternode>` which includes this struct.
Bincode encodes `Option<T>` with a leading discriminant byte that bare `T` does not have, so previously-serialized platform state will fail to deserialize. The PR acknowledges 2 snapshot test failures (`should_deserialize_state_stored_in_version_0_from_testnet` and `should_deserialize_state_stored_in_version_8_from_devnet`).
If this ships alongside a mandatory re-sync or consensus upgrade that discards old state, this is acceptable. If nodes must read pre-existing persisted state, a `MasternodeStateV1` or a new `PlatformStateForSavingV2` with migration logic is needed. Please confirm intended upgrade path.
| if let Some(Some(address)) = dmn_state_diff.service { | ||
| validator.node_ip = address.ip().to_string(); |
There was a problem hiding this comment.
💬 Nitpick: core_port not updated when service address changes
When service changes to a new address (e.g., from 1.2.3.4:1234 to 5.6.7.8:5678), only node_ip is updated at line 68 (validator.node_ip = address.ip().to_string()) but core_port is not updated from address.port(). The validator's core_port field will become stale if a masternode changes its service port. This is pre-existing (not introduced by this PR), but the refactoring here is a good opportunity to fix it.
💡 Suggested change
| if let Some(Some(address)) = dmn_state_diff.service { | |
| validator.node_ip = address.ip().to_string(); | |
| if let Some(Some(address)) = dmn_state_diff.service { | |
| validator.node_ip = address.ip().to_string(); | |
| validator.core_port = address.port(); | |
| } |
source: ['claude-rust-quality']
| state: DMNState { | ||
| service: SocketAddr::from_str(format!("1.0.{}.{}:1234", i / 256, i % 256).as_str()) | ||
| .unwrap(), | ||
| service: Some( | ||
| SocketAddr::from_str(format!("1.0.{}.{}:1234", i / 256, i % 256).as_str()) | ||
| .unwrap(), | ||
| ), |
There was a problem hiding this comment.
🟡 Suggestion: No tests exercise service: None path
All test masternode generators construct service: Some(SocketAddr::from_str(...).unwrap()) (line 250), and the IP update path uses .expect("service address") (line 356). There are zero tests with service: None anywhere in the test suite. This leaves the new Option<SocketAddr> behavior untested in: validator creation (new_validator_if_masternode_in_state returning None), validator set updates (the Some(None) diff path), and serialization round-trips. Adding at least one test that creates a masternode with service: None would validate the new code path.
source: ['claude-general', 'claude-rust-quality']
🤖 Fix this 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 `packages/rs-drive-abci/tests/strategy_tests/masternodes.rs`:
- [SUGGESTION] lines 249-253: No tests exercise service: None path
All test masternode generators construct `service: Some(SocketAddr::from_str(...).unwrap())` (line 250), and the IP update path uses `.expect("service address")` (line 356). There are zero tests with `service: None` anywhere in the test suite. This leaves the new `Option<SocketAddr>` behavior untested in: validator creation (`new_validator_if_masternode_in_state` returning None), validator set updates (the `Some(None)` diff path), and serialization round-trips. Adding at least one test that creates a masternode with `service: None` would validate the new code path.
| [dependencies] | ||
| # Cryptography | ||
| dashcore = { git = "https://github.com/dashpay/rust-dashcore", rev = "2bd764fdc071828939d9dc8dd872c39bae906b04" } | ||
| dashcore = { git = "https://github.com/dashpay/rust-dashcore", rev = "9959201593826def0ad1f6db51b2ceb95b68a1ca" } |
There was a problem hiding this comment.
💬 Nitpick: rs-platform-encryption dashcore rev diverges from workspace
This crate pins dashcore to rev 9959201... while all 6 workspace-level deps use 1af96dd.... Since this is a workspace member (root Cargo.toml line 45), Cargo unifies the dep — Cargo.lock confirms only one dashcore entry at 1af96dd. The local rev is effectively ignored but misleading to anyone reading the manifest. Consider switching to dashcore = { workspace = true } for consistency.
💡 Suggested change
| dashcore = { git = "https://github.com/dashpay/rust-dashcore", rev = "9959201593826def0ad1f6db51b2ceb95b68a1ca" } | |
| dashcore = { workspace = true } |
source: ['claude-general', 'claude-rust-quality']
Adds notify_wallet_addresses_changed() to DashSpvClient for triggering mempool bloom filter rebuild when wallets are added or addresses registered after initial SPV connection. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Incremental review of 1 new commit (d9de0fd) since prior review at 2af8cac. This push bumps the rust-dashcore workspace rev from 1af96dd0 to c451a1c6 (filter rebuild API). The change touches only Cargo.toml -- 6 lines swapping the git rev hash for all 6 workspace dependencies. No code changes, no new API adaptations. Prior findings status: All 5 prior findings from the initial review remain applicable -- they concern code logic (service Option handling, core_port updates, test coverage) that is untouched by this rev bump. In particular, the rs-platform-encryption/Cargo.toml rev divergence nitpick (finding d09a8c70d24f) is still present -- it remains pinned to 9959201 while the workspace now uses c451a1c6. No new findings from this push.
Reviewed commit: d9de0fd
Updates rust-dashcore from c451a1c6 to b03252af which includes: - bloom filter staleness detection via monitor_revision polling - key-wallet-manager merged into key-wallet (upstream #503) - IS lock deduplication fix - transaction status events Adapts platform to the key-wallet-manager → key-wallet merge by re-exporting key_wallet as key_wallet_manager for backward compat. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
The incremental delta cleanly removes the key-wallet-manager workspace dependency after it was merged into key-wallet upstream. The backward-compatibility alias (pub use key_wallet as key_wallet_manager) preserves crate-level naming but silently changes internal module paths, and the README wasn't updated to match. The now-empty manager feature flag is a no-op that should be cleaned up. No blocking issues.
Reviewed commit: be2082b
🟡 3 suggestion(s) | 💬 1 nitpick(s)
2 additional findings
🟡 suggestion: README uses stale module paths and lists removed dependency
packages/rs-platform-wallet/README.md (lines 24-25)
The code example on line 24 imports key_wallet_manager::wallet_manager::WalletManager, but key-wallet-manager was merged into key-wallet and the wallet_manager module was renamed to manager. The actual example file (basic_usage.rs) was correctly updated to use key_wallet::manager::WalletManager.
Lines 108 and 113 also reference key-wallet-manager as a separate dependency, which no longer exists in the workspace.
Note: The README is not modified in this PR, but this is newly broken by the key-wallet-manager removal in this PR's incremental commit.
💡 Suggested change
Update line 24 to:
```rust
use key_wallet::manager::WalletManager;
And update lines 108 and 113 to remove references to key-wallet-manager as a standalone dependency.
</details>
</details>
<details>
<summary>💬 <b>nitpick:</b> build_ios.sh references non-existent key-wallet-manager directory/crate</summary>
`packages/rs-sdk-ffi/build_ios.sh (lines 81-98)`
Line 81 includes `$RUST_DASHCORE_DIR/key-wallet-manager/src` in the hash computation and line 98 tries to `cargo clean -p key-wallet-manager`. Both are harmless due to `2>/dev/null`/`|| true`, but they're dead references now that key-wallet-manager was merged into key-wallet upstream. Pre-existing issue, not introduced by this delta.
*Pre-existing issue, not introduced by this PR.*
<details>
<summary>💡 Suggested change</summary>
```suggestion
Remove `key-wallet-manager` from both the `find` path list (line 81) and the `cargo clean` loop (line 98).
🤖 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 `packages/rs-platform-wallet/README.md`:
- [SUGGESTION] lines 24-25: README uses stale module paths and lists removed dependency
The code example on line 24 imports `key_wallet_manager::wallet_manager::WalletManager`, but `key-wallet-manager` was merged into `key-wallet` and the `wallet_manager` module was renamed to `manager`. The actual example file (`basic_usage.rs`) was correctly updated to use `key_wallet::manager::WalletManager`.
Lines 108 and 113 also reference `key-wallet-manager` as a separate dependency, which no longer exists in the workspace.
*Note: The README is not modified in this PR, but this is newly broken by the `key-wallet-manager` removal in this PR's incremental commit.*
In `packages/rs-platform-wallet/Cargo.toml`:
- [SUGGESTION] line 36: Empty `manager = []` feature is a no-op
The `manager` feature was changed from `["key-wallet-manager"]` to `[]` but is still in `default`. It now only gates the `key_wallet_manager` backward-compat alias in `lib.rs` via `#[cfg(feature = "manager")]`. However, the example (`basic_usage.rs`) is also cfg-gated on this feature yet imports directly from `key_wallet` (which is unconditional), so the gate is misleading.
Consider either:
1. Removing the `manager` feature entirely and making the re-export unconditional, or
2. Having `manager` gate something meaningful (e.g., `manager = ["key-wallet/manager"]` if such a feature exists upstream).
In `packages/rs-platform-encryption/Cargo.toml`:
- [SUGGESTION] line 11: rs-platform-encryption pins a divergent dashcore rev, causing duplicate compilation
This crate pins dashcore to rev `9959201` via a direct git dependency instead of using `workspace = true`. The workspace now points to `b03252af`. Cargo.lock shows 7 entries at the old rev alongside 23 at the new rev — meaning dashcore, dashcore-private, and dashcore_hashes each compile twice.
This is a **pre-existing issue** not introduced by this PR, but the gap widened with this bump. Aligning the rev (or switching to `dashcore.workspace = true`) would eliminate the duplicate builds.
Co-authored-by: Pasta Lil Claw <pasta+claw@dashboost.org>
2 participants
Summary
Bump rust-dashcore to
c451a1c6(on top of PR #558 mempool support) for dash-evo-tool integration.42eb1d69→c451a1c6"std"feature from rs-dpp dashcore dependency (eliminated upstream)impl_wasm_conversions→impl_wasm_conversions_serde)rust-dashcore changes included
MempoolManagerwith bloom filter supportnotify_wallet_addresses_changed()API for external filter rebuildBased on
aa86b74f(before PR #3376) to avoid RPITIT Send regression.Test plan
cargo check --all-features --workspacepasses🤖 Co-authored by Claudius the Magnificent AI Agent