feat: bump rust-dashcore to mempool support (1af96dd)#3390
feat: bump rust-dashcore to mempool support (1af96dd)#3390
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>
…HRTB Send issue Move the retry + closure logic from the default methods of Fetch, FetchMany, and FetchUnproved traits into standalone async fn helpers. This breaks the HRTB Send inference chain that prevents tokio::spawn from proving the resulting future is Send, without changing any call sites or removing #[async_trait]. Also remove stale #[async_trait::async_trait] from the bare FetchMany impl for ContenderWithSerializedDocument. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughDependency versions are pinned to a feature branch ( Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
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>
# Conflicts: # Cargo.lock # Cargo.toml # packages/rs-drive-abci/src/config.rs # packages/rs-platform-wallet/src/platform_wallet_info/wallet_info_interface.rs
|
📖 Book Preview built successfully. Download the preview from the workflow artifacts. Updated at 2026-03-23T10:50:27.990Z |
lklimek
changed the base branch from
refactor/sdk-rpitit-fetch-traits
to
v3.1-dev
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
♻️ Duplicate comments (2)
packages/rs-drive-abci/src/platform_types/masternode/v0/mod.rs (1)
97-101:⚠️ Potential issue | 🟠 MajorThis
V0shape change still needs an upgrade story.Changing
servicefromSocketAddrtoOption<SocketAddr>changes the bincode layout of an existingV0type. Previously persisted platform-state snapshots will deserialize against a different wire format unless this rollout is explicitly resync-only. Please version the stored state or add migration logic before merge, and document the intended upgrade path.🤖 Prompt for AI Agents
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` around lines 97 - 101, You changed MasternodeStateV0.service from SocketAddr to Option<SocketAddr>, which breaks bincode wire format for persisted V0 snapshots; add an explicit migration path: keep a legacy struct (e.g., MasternodeStateV0Legacy with service: SocketAddr), implement a custom Deserialize for MasternodeStateV0 that first attempts to decode the new shape and on failure decodes the legacy shape and converts it into the new shape (service -> Some(service)), and update any platform-state version identifier or add a stored schema version constant and document the upgrade/migration behavior (include symbols MasternodeStateV0 and MasternodeStateV0Legacy in comments/docs).packages/rs-drive-abci/src/execution/platform_events/core_based_updates/update_masternode_list/update_state_masternode_list/v0/mod.rs (1)
67-69:⚠️ Potential issue | 🟠 MajorService removals still leave stale validator endpoints.
DMNStateDiff.service = Some(None)now means Core explicitly removed the address, but this branch ignores that case and leaves the old validator entry intact. That makes incrementally updated validator sets diverge from freshly rebuilt ones, which now drop validators whoseserviceisNone; even when an address is present,core_portis still left stale.Suggested patch
validator_sets .iter_mut() .for_each(|(_quorum_hash, validator_set)| { + if matches!(dmn_state_diff.service, Some(None)) { + validator_set.members_mut().remove(pro_tx_hash); + return; + } + if let Some(validator) = validator_set.members_mut().get_mut(pro_tx_hash) { if let Some(maybe_ban_height) = dmn_state_diff.pose_ban_height { // the ban_height was changed validator.is_banned = maybe_ban_height.is_some(); } if let Some(Some(address)) = dmn_state_diff.service { validator.node_ip = address.ip().to_string(); + validator.core_port = address.port(); }🤖 Prompt for AI Agents
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` around lines 67 - 69, The branch only handles Some(Some(address)) and leaves stale endpoints when dmn_state_diff.service == Some(None); update the logic in update_state_masternode_list so it matches on dmn_state_diff.service with three cases: None (leave unchanged), Some(Some(address)) — set validator.node_ip = address.ip().to_string() and set validator.core_port = address.port() (or the correct field from address), and Some(None) — clear the endpoint by setting validator.node_ip to an empty string and validator.core_port to a zero/None-equivalent so the removed service is not left stale. Ensure you reference dmn_state_diff.service, address.ip()/address.port(), validator.node_ip and validator.core_port when making the changes.
🧹 Nitpick comments (2)
Cargo.toml (1)
50-54: Consider pinning to a specific rev before merging to a release branch.Using
branch = "feat/mempool-support"results in non-deterministic builds—different builds at different times may pull different commits. While this is acceptable for active development, Cargo best practices recommend pinning to a specific commit hash (rev = "...") for reproducible builds before merging to stable or release branches. Branch tracking is suitable for ongoing integration work but should be replaced with rev pinning to ensure build determinism in releases.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Cargo.toml` around lines 50 - 54, The Cargo.toml currently depends on the remote branch feat/mempool-support for the dashcore-related crates (dashcore, dash-spv, dash-spv-ffi, key-wallet, dashcore-rpc), which makes builds non-deterministic; update each dependency entry to pin to a specific commit by replacing the branch = "feat/mempool-support" with rev = "<commit-hash>" (use the exact commit SHA you intend to track for each of dashcore, dash-spv, dash-spv-ffi, key-wallet, and dashcore-rpc) so Cargo pulls a fixed revision for reproducible builds before merging to the release branch.packages/rs-sdk/src/platform/fetch_many.rs (1)
296-303: Renameobject_typeor pass the element type into the helper.
Ois theFetchManycontainer type, so this field now logs the map/container type rather than the fetched item type. That makes the trace metadata misleading during debugging. Either rename it tocontainer_type, or passstd::any::type_name::<Self>()down if you still want the element type.🪵 Minimal honest-field rename
- let object_type = std::any::type_name::<O>().to_string(); + let container_type = std::any::type_name::<O>(); tracing::trace!( request = ?request, response = ?response, ?address, retries, - object_type, + container_type, "fetched objects from platform" );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-sdk/src/platform/fetch_many.rs` around lines 296 - 303, The trace currently computes let object_type = std::any::type_name::<O>().to_string() but O is the FetchMany container type, so rename this variable to container_type (and update its usage in the tracing::trace! call) to avoid misleading metadata; locate the declaration of object_type in fetch_many.rs and change both the variable name and the trace field name from object_type to container_type (or alternatively replace the type_name::<O>() call with std::any::type_name::<Self>() if you prefer logging the element type).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In
`@packages/rs-drive-abci/src/execution/platform_events/core_based_updates/update_masternode_list/update_state_masternode_list/v0/mod.rs`:
- Around line 67-69: The branch only handles Some(Some(address)) and leaves
stale endpoints when dmn_state_diff.service == Some(None); update the logic in
update_state_masternode_list so it matches on dmn_state_diff.service with three
cases: None (leave unchanged), Some(Some(address)) — set validator.node_ip =
address.ip().to_string() and set validator.core_port = address.port() (or the
correct field from address), and Some(None) — clear the endpoint by setting
validator.node_ip to an empty string and validator.core_port to a
zero/None-equivalent so the removed service is not left stale. Ensure you
reference dmn_state_diff.service, address.ip()/address.port(), validator.node_ip
and validator.core_port when making the changes.
In `@packages/rs-drive-abci/src/platform_types/masternode/v0/mod.rs`:
- Around line 97-101: You changed MasternodeStateV0.service from SocketAddr to
Option<SocketAddr>, which breaks bincode wire format for persisted V0 snapshots;
add an explicit migration path: keep a legacy struct (e.g.,
MasternodeStateV0Legacy with service: SocketAddr), implement a custom
Deserialize for MasternodeStateV0 that first attempts to decode the new shape
and on failure decodes the legacy shape and converts it into the new shape
(service -> Some(service)), and update any platform-state version identifier or
add a stored schema version constant and document the upgrade/migration behavior
(include symbols MasternodeStateV0 and MasternodeStateV0Legacy in
comments/docs).
---
Nitpick comments:
In `@Cargo.toml`:
- Around line 50-54: The Cargo.toml currently depends on the remote branch
feat/mempool-support for the dashcore-related crates (dashcore, dash-spv,
dash-spv-ffi, key-wallet, dashcore-rpc), which makes builds non-deterministic;
update each dependency entry to pin to a specific commit by replacing the branch
= "feat/mempool-support" with rev = "<commit-hash>" (use the exact commit SHA
you intend to track for each of dashcore, dash-spv, dash-spv-ffi, key-wallet,
and dashcore-rpc) so Cargo pulls a fixed revision for reproducible builds before
merging to the release branch.
In `@packages/rs-sdk/src/platform/fetch_many.rs`:
- Around line 296-303: The trace currently computes let object_type =
std::any::type_name::<O>().to_string() but O is the FetchMany container type, so
rename this variable to container_type (and update its usage in the
tracing::trace! call) to avoid misleading metadata; locate the declaration of
object_type in fetch_many.rs and change both the variable name and the trace
field name from object_type to container_type (or alternatively replace the
type_name::<O>() call with std::any::type_name::<Self>() if you prefer logging
the element type).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 01300bfe-7ab1-4b3b-895f-87b4aed3ec89
⛔ Files ignored due to path filters (4)
.yarn/cache/@unrs-resolver-binding-darwin-arm64-npm-1.11.1-c78d4bd2cb-10.zipis excluded by!**/.yarn/**,!**/*.zip.yarn/cache/@unrs-resolver-binding-linux-x64-gnu-npm-1.11.1-93a00570de-10.zipis excluded by!**/.yarn/**,!**/*.zip.yarn/cache/fsevents-patch-19706e7e35-10.zipis excluded by!**/.yarn/**,!**/*.zipCargo.lockis excluded by!**/*.lock
📒 Files selected for processing (18)
Cargo.tomlpackages/rs-dpp/Cargo.tomlpackages/rs-dpp/src/lib.rspackages/rs-drive-abci/src/execution/platform_events/core_based_updates/update_masternode_identities/update_operator_identity/v0/mod.rspackages/rs-drive-abci/src/execution/platform_events/core_based_updates/update_masternode_list/update_state_masternode_list/v0/mod.rspackages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/mod.rspackages/rs-drive-abci/src/platform_types/masternode/v0/mod.rspackages/rs-drive-abci/src/platform_types/validator/v0/mod.rspackages/rs-drive-abci/tests/strategy_tests/masternode_list_item_helpers.rspackages/rs-drive-abci/tests/strategy_tests/masternodes.rspackages/rs-platform-wallet/Cargo.tomlpackages/rs-platform-wallet/examples/basic_usage.rspackages/rs-platform-wallet/src/lib.rspackages/rs-platform-wallet/src/platform_wallet_info/wallet_info_interface.rspackages/rs-platform-wallet/src/platform_wallet_info/wallet_transaction_checker.rspackages/rs-sdk/src/platform/fetch.rspackages/rs-sdk/src/platform/fetch_many.rspackages/rs-sdk/src/platform/fetch_unproved.rs
3 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
Summary by CodeRabbit
New Features
Improvements
Chores