chore(deps): bump platform to 2cc4cdcf8 (rust-dashcore 309fac8)#834
Conversation
Adapts dash-evo-tool to rust-dashcore 309fac8 API changes shipped via platform rev 2cc4cdcf85ce62b437031671336bf622b3aafaa5. API surface changes handled: - `MasternodeListEngine::feed_qr_info` signature changed: now takes `&QRInfoBlockData` rather than an `Option<F>` height-lookup closure. Both call sites in `masternode_list_diff_screen.rs` now pre-populate the bundle via a new `build_qr_info_block_data` helper that resolves hash->height from the screen's cache/Core RPC and height->hash for cycle boundary blocks via `get_block_hash` (Core RPC). - `MasternodeListEngine::rotated_quorums_per_cycle` is now a `BTreeMap<BlockHash, BTreeMap<u16, QualifiedQuorumEntry>>` (was a `Vec`). `render_last_commitments` now destructures the inner map directly and the quorum index comes from the map key. - `AddressProvider::on_address_found` / `on_address_absent` are now `async fn` (via the trait's `#[async_trait]`). `WalletAddressProvider` is annotated with `#[async_trait]` and both callbacks made `async`. - `sml::quorum_validation_error::ClientDataRetrievalError` was removed upstream. The only DET uses were inside the feed_qr_info closure that is now replaced wholesale; the import is gone. No behavioural change - this is pure API adaptation. All existing SPV fixes on this branch (DB fallback for start_spv, atomic BIP44 pool extension, SPV restart on mid-session import) continue to behave identically and their tests still pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
At rust-dashcore 309fac8, WalletManager gained an independent filter_committed_height field. FiltersManager::new() reads filter_committed_height() for its "already synced" guard; the field is no longer derived from synced_height. Previously clear_data_dir called update_synced_height(0), which pre-bump effectively reset both fields via the trait's default impl. Post-bump it only resets synced_height, leaving filter_committed_height at its stale previous value. FiltersManager then logs "Filters already synced to N" and skips the rescan, producing zero balance after a Clear SPV Data. Switch clear_data_dir to update_filter_committed_height(0), which unconditionally sets the field and only bumps synced_height upward if the incoming height is larger — exactly the semantics we need for a rescan-floor reset. Adds a regression test asserting filter_committed_height is lowered to 0 after clear_data_dir(). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
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 35 minutes and 56 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 (3)
📝 WalkthroughWalkthroughUpdates dependency git revisions; convert wallet address provider callbacks to async; change SPV clear logic to reset Changes
Sequence Diagram(s)sequenceDiagram
participant UI as "UI (Screen)"
participant Engine as "MasternodeListEngine"
participant Cache as "block_height_cache"
participant Core as "Core RPC"
rect rgba(200,230,255,0.5)
UI->>Engine: request feed_qr_info_and_get_dmls(...)
Engine->>Engine: enumerate referenced QRInfo hashes
Engine->>Cache: lookup hash heights
alt cache hit
Cache-->>Engine: return height
else cache miss
Engine->>Core: get block height by hash
Core-->>Engine: return height
Engine->>Cache: insert resolved height
end
Engine-->>UI: call feed_qr_info(&QRInfoBlockData)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 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 |
Review GateCommit:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/spv/manager.rs`:
- Around line 821-830: The code currently swallows failures from
self.wallet.try_write() when resetting
WalletManager::update_filter_committed_height(0) and still returns Ok(()) after
clearing SPV data; instead, propagate the error so the SPV clear operation fails
if the lock/update fails. Change the match to return an Err (or map the
PoisonError to the existing error type) when try_write() returns Err(_) rather
than only logging, ensuring the caller of the SPV clear routine sees the failure
and no success is returned when update_filter_committed_height could not be
applied.
🪄 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: 23d1f699-8628-4ce3-878f-9f51db8afcec
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
Cargo.tomlsrc/model/wallet/mod.rssrc/spv/manager.rssrc/spv/tests.rssrc/ui/tools/masternode_list_diff_screen.rs
There was a problem hiding this comment.
Pull request overview
Updates the pinned Dash Platform (dash-sdk) revision and adapts Dash Evo Tool to upstream Wallet/SPV and SML masternode list engine API changes, including a targeted SPV reset fix for the new filter_committed_height state.
Changes:
- Bump
dash-sdk/ platform git rev (and resultingCargo.lockupdates). - Adapt
WalletAddressProviderto upstreamAddressProviderbecoming#[async_trait]with async callbacks. - Update MasternodeListDiff QRInfo processing to the new
feed_qr_info(&QRInfoBlockData)API and fix “Clear SPV Data” to resetfilter_committed_height, with a regression test.
Reviewed changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| Cargo.toml | Bumps Dash Platform/dash-sdk pin to 2cc4cdcf8…. |
| Cargo.lock | Locks updated dependency graph for the new platform + rust-dashcore revisions. |
| src/model/wallet/mod.rs | Applies async-trait adaptation for AddressProvider callbacks. |
| src/ui/tools/masternode_list_diff_screen.rs | Refactors QRInfo height/hash resolution to build QRInfoBlockData and adapts to updated engine iteration/signatures. |
| src/spv/manager.rs | Resets filter_committed_height during SPV data clear to ensure rescans aren’t skipped. |
| src/spv/tests.rs | Adds regression test ensuring clear_data_dir() resets filter_committed_height. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- spv/manager.rs: propagate try_write() failure in clear_data_dir instead of logging-and-continuing. A contended or poisoned RwLock would have left filter_committed_height stale in memory while the on-disk data was wiped, re-triggering the exact skipped-rescan bug this PR is meant to fix. - ui/tools/masternode_list_diff_screen.rs: use get_block_hash_and_cache helper in build_qr_info_block_data so both engine block_container and caches are consulted/populated consistently; drops redundant direct RPC call and redundant block_height_cache write. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/ui/tools/masternode_list_diff_screen.rs`:
- Around line 4255-4263: When build_qr_info_block_data fails the code only sets
ui_state.error and returns, leaving self.task.pending set and the pending
spinner stuck; replace the early-return error handling to call the existing
display_message(...) with MessageType::Error (which will clear self.task.pending
and set ui_state.error) instead of only assigning ui_state.error. Apply the same
change in the feed_qr_info_and_get_dmls error branch so both
build_qr_info_block_data and feed_qr_info_and_get_dmls use display_message to
clear pending and record the error.
🪄 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: 4a07e44f-6c7e-47a1-aa0d-c5cb0b201edb
📒 Files selected for processing (2)
src/spv/manager.rssrc/ui/tools/masternode_list_diff_screen.rs
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 6 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Error paths in `feed_qr_info_and_get_dmls` and the `MnListFetchedQrInfo` handler returned early after setting `ui_state.error` without clearing `self.task.pending`, leaving the pending spinner stuck after a failure. Clear `self.task.pending = None` on every error return so the UI is always returned to an idle state. Also move `build_qr_info_block_data` above the `insert_mn_list_diff` calls in the `MnListFetchedQrInfo` handler so a failure does not leave the diff cache partially populated. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 6 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if let Ok(mut storage_guard) = self.storage.lock() { | ||
| *storage_guard = None; | ||
| } | ||
|
|
||
| if let Ok(mut client_guard) = self.spv_client.write() { | ||
| *client_guard = None; | ||
| } | ||
|
|
||
| if let Ok(mut request_guard) = self.request_tx.lock() { | ||
| *request_guard = None; | ||
| } | ||
|
|
||
| if let Ok(mut wallet_map) = self.det_wallets.write() { | ||
| wallet_map.clear(); | ||
| } | ||
|
|
||
| // Reset the in-memory WalletManager's synced_height so the next SPV session | ||
| // scans filters from genesis instead of the stale height from the previous run. | ||
| match self.wallet.try_write() { | ||
| Ok(mut wm) => { | ||
| wm.update_synced_height(0); | ||
| } | ||
| Err(_) => { | ||
| tracing::warn!("Failed to reset WalletManager synced_height during SPV data clear"); | ||
| } | ||
| // Reset the in-memory WalletManager's filter_committed_height so the next | ||
| // SPV session scans filters from genesis instead of the stale height from the | ||
| // previous run. We reset filter_committed_height (not synced_height) because at | ||
| // rust-dashcore 309fac8 these became independent fields — FiltersManager::new() | ||
| // reads filter_committed_height() for its "already synced" guard. | ||
| // | ||
| // This must succeed before we wipe the on-disk data; otherwise the in-memory | ||
| // height would stay stale while on-disk filters are gone, re-triggering the | ||
| // skipped-rescan bug this clear is meant to prevent. | ||
| { | ||
| let mut wm = self.wallet.try_write().map_err(|e| { | ||
| format!( | ||
| "Failed to reset WalletManager filter_committed_height during SPV data clear: {e}" | ||
| ) | ||
| })?; | ||
| wm.update_filter_committed_height(0); | ||
| } |
There was a problem hiding this comment.
clear_data_dir() now returns Err if self.wallet.try_write() fails, but this lock attempt happens after clearing storage/spv_client/request_tx and det_wallets. If try_write fails, the function exits early with those fields already cleared, leaving SpvManager in a partially-reset state (and without resetting filter_committed_height or touching on-disk data). To keep the operation atomic, acquire the wallet write lock before mutating other state (or defer clearing det_wallets/client fields until after the lock succeeds), or otherwise roll back the partial in-memory changes on this error path.
| // Build this BEFORE inserting diffs into the cache so a failure does not | ||
| // leave the cache partially populated. |
There was a problem hiding this comment.
The comment says building block_data before inserting diffs avoids leaving "the cache" partially populated on failure, but build_qr_info_block_data() itself populates block_height_cache/block_hash_cache via get_*_and_cache. Consider clarifying the comment to refer specifically to the mn_list_diff/diff caches (or adjust the flow if the intent is truly to avoid any cache mutation before success).
| // Build this BEFORE inserting diffs into the cache so a failure does not | |
| // leave the cache partially populated. | |
| // Build this BEFORE inserting mn_list_diff entries so a failure does not | |
| // leave the diff-related caches partially populated. Note that | |
| // build_qr_info_block_data() may still warm block hash/height caches. |
Summary
Bumps the Dash Platform pin to
2cc4cdcf8(which pulls inrust-dashcore 309fac8) and adapts DET to the upstreamWalletInterface/ SPV API changes. Extracted from PR #830 to keep that PR focused on the bug-2 pool-extension fix.Why now
PR #830 originally carried the platform bump, the bug-2 pool-extension fix, and a
clear_data_dirreset fix in a single branch. The bump is a pure dependency update with associated API adaptations; mixing it with a behavioural SPV bug fix makes the diff harder to review and raises the blast radius of any revert. This PR carries only the bump and the oneclear_data_dirchange that is directly tied to the newfilter_committed_heightfield introduced by the bump.What's in this PR
Commit 1 —
chore(deps): bump platform to 2cc4cdcf8 (rust-dashcore 309fac8)Pure API adaptation. No behavioural change.
Cargo.toml/Cargo.lock— pin bump.src/model/wallet/mod.rs—AddressProvidertrait became#[async_trait]; adapted.src/ui/tools/masternode_list_diff_screen.rs:QualifiedQuorumEntryiteration changed from&QualifiedQuorumEntryto(&u16, &QualifiedQuorumEntry)tuples.feed_qr_infosignature changed from closure-based to&QRInfoBlockDatastruct — helperbuild_qr_info_block_dataextracted.SdkError::Protocol(e)wrapping for upstreamProtocolError.Commit 2 —
fix(spv): reset filter_committed_height on Clear SPV DataA direct consequence of the bump. Ships here (not in #830) because pre-bump it would be a no-op.
WalletManager::update_filter_committed_heightwas a default trait impl wrappingupdate_synced_height. Resettingsynced_heightto0was sufficient for rescan.filter_committed_heightis an independent field and is whatFiltersManager::new()reads for its "already synced" guard. Leavingsynced_height = 0whilefilter_committed_heightstayed stale meant the "Clear SPV Data" rescan was silently skipped.SpvManager::clear_data_dirnow callswm.update_filter_committed_height(0)instead ofupdate_synced_height(0).test_clear_data_dir_resets_filter_committed_height.Files (effective diff vs v1.0-dev)
Cargo.toml/Cargo.locksrc/model/wallet/mod.rsAddressProviderasync-trait adaptationsrc/ui/tools/masternode_list_diff_screen.rs+115/-QualifiedQuorumEntrytuple-iter +feed_qr_infoAPI adaptationsrc/spv/manager.rsclear_data_dir— resetfilter_committed_heightsrc/spv/tests.rstest_clear_data_dir_resets_filter_committed_heightTest plan
Automated (all green)
cargo build --all-features— cleancargo clippy --all-features --all-targets -- -D warnings— cleancargo +nightly fmt --all -- --check— cleancargo test --all-features --workspace --lib— 453 / 453 passManual on testnet
&QRInfoBlockDatapath.AddressProviderasync-trait change didn't regress anything).Related
🤖 Co-authored by Claudius the Magnificent AI Agent
Summary by CodeRabbit
Release Notes
Bug Fixes
Improvements
Chores