chore(deps): bump platform to v3.1.0-dev.8 (9e00c8b894)#862
Conversation
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- DocumentQuery: add select/group_by/having fields (15 sites) - Signer: migrate both impls to async generic Signer<K>; await callsites - identity ST API: top_up_identity/put_to_platform/try_from_identity_* renamed - shielded build path now async (build_shield_credit/transition) - key-wallet 0.43: TransactionBuilder, ManagedAccount*, AddressProvider, run()/stop() Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- backend-e2e: await now-async try_from_identity_with_signer - spv unit test: per-wallet synced_height replaces global filter_committed_height - core/mod.rs: drop redundant closure call after removing ? early returns - cargo +nightly fmt --all Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
✅ Review complete (commit d510bcd) |
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (2)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughSDK dependency updates align the codebase with trait signature changes in ChangesSDK async trait integration and SPV account refactoring
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
QA audit (Marvin) — 1 CRITICAL blocker, fixes in progressFull audit traced the behaviour-ambiguous sites. Not mergeable as-is — one critical regression, proven by the branch's own failing unit test. 🔴 QA-001 (CRITICAL) — "Clear SPV Data" no longer triggers rescan-from-genesis —
|
…001) key-wallet 0.43's update_wallet_synced_height is forward-only monotonic, so update_wallet_synced_height(id, 0) was a silent no-op for any synced wallet — the genesis rescan was skipped after a data clear, leaving stale/zero balances. Write metadata.synced_height/last_processed_height directly to force the rewind. Test now asserts the real invariant: post-clear the wallet is height 0 AND wallets_behind() flags it for rescan. cargo test --lib green (468 passed). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
build_shield_transition runs its CPU-heavy Halo2 proving synchronously inline before its first await, so awaiting it on a tokio worker starved the runtime. Drive the build via spawn_blocking + block_on so proving stays off the async worker pool. Also document the intentional behaviour changes flagged by review: change-address peek now bumps monitor revision (QA-005), current_balances filter is lossless for current usage (QA-004), build_unsigned fee-coverage check now fails earlier (QA-006). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Large platform v3.1.0-dev.8 migration with careful TODO markers and explicit smoke-test gating on the behavior-ambiguous wallet/SPV/shielded paths. The mechanical adaptations (async Signer awaits, DocumentQuery field expansion, TransactionBuilder reshape, identity ST renames) look consistent. I found no verified blocking regression, but there are a few non-blocking follow-ups worth addressing or explicitly covering in the planned smoke run.
🟡 3 suggestion(s) | 💬 1 nitpick(s)
1 additional finding(s) omitted (not in diff).
🤖 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 `src/backend_task/core/mod.rs`:
- [SUGGESTION] src/backend_task/core/mod.rs:744-748: Consider add_to_state=true now that the API exposes the choice — current `false` preserves a latent issue
The new `next_change_address(account_xpub, add_to_state)` makes the address-pool registration explicit. With `add_to_state=false`, the address-pool path in key-wallet generates a new change address (when no pre-generated unused change address exists) without inserting it into `addresses` / `address_index` / `script_pubkey_index` (`address_pool.rs:507-516`). Because DET's SPV registration walks `account.all_addresses()` (`src/context/wallet_lifecycle.rs:518`), and that returns only entries actually in `self.addresses`, a freshly-derived change address used in a send is not added to the SPV watch set.
Important context for the author and reviewers: this is NOT a fresh regression. The prior code (`wallet_account.derive_change_address(change_index)`) was an immutable `&` borrow and did not register either. The new API simply makes the trade-off visible. Two observations:
1. The comment correctly notes parity with the prior behavior, and the PR description marks this for smoke testing. If smoke runs confirm that subsequent UTXO scans recover the change output (because the wallet records its own outgoing tx outputs), the existing `false` is defensible.
2. If the smoke test reveals the change output is not picked up by SPV until a manual recovery/rescan, the simplest fix is `add_to_state=true`. Re-using the same change address across retries inside the `loop` would still work (the address-pool returns the same unused entry on subsequent calls until it's marked used).
At minimum, the outcome of this smoke check should be documented in the PR before merge, since this site sits right on the boundary the new API was designed to clarify.
In `src/model/wallet/mod.rs`:
- [SUGGESTION] src/model/wallet/mod.rs:2662-2671: pending_addresses() and current_balances() silently drop conversion failures — possible silent sync hang
Both `pending_addresses()` (2662-2671) and `current_balances()` (2730-2743) use `filter_map(... .ok())` to drop entries where `PlatformAddress::try_from(core_address)` fails. The SDK consumes `pending_addresses()` to learn what is still to be resolved, but `has_pending()` (2724-2728) iterates `self.pending` directly — so any entry that fails conversion never reaches the SDK, never gets `on_address_found` / `on_address_absent`, never gets inserted into `self.resolved`, and `has_pending()` stays `true` forever. Result: silent sync hang with no error and no log line.
In practice the conversion should not fail (each `pending` entry came from `derive_address_at_index`, which already round-tripped `PlatformAddress::try_from` at insertion), but the silent-drop branch is the kind of invariant-breakage that is very hard to diagnose later. At minimum, log at `error!` when conversion fails and either force-resolve the offending index (so the loop makes progress) or `unwrap()` to surface the invariant violation. The same applies to `current_balances()` at 2738-2742, where the silent skip on a missing-from-`pending` index would corrupt the SDK's view of the stored baseline for incremental catch-up.
In `src/spv/manager.rs`:
- [SUGGESTION] src/spv/manager.rs:1188-1198: Cancellation path can block forever if client.stop() errors without flipping the run flag
The prior code used `run_cancel.cancel()` — a synchronous, infallible signal — that guaranteed `run()` would unwind. The new path now relies on `client.stop().await` to flip the client's internal running flag, then `(&mut run_future).await` to let `run()` observe it. If `stop()` returns an error in a path that did not actually flip the flag, the subsequent unconditional `(&mut run_future).await` blocks indefinitely, hanging the SPV worker on shutdown or network switch. The error from `stop()` is only logged at `warn!` level today.
Two straightforward defenses: (a) wrap the post-stop `(&mut run_future).await` in a `tokio::time::timeout` and drop/abort on expiry, or (b) when `stop()` errored, return `Outcome::RunCompleted(Err(...))` immediately without awaiting `run_future`. Adding a `TODO(v3.1-bump)` here — matching the four already-flagged sites — would also give the smoke-test pass a hook to verify the cancellation contract explicitly.
| let change_addr = account | ||
| .next_change_address(Some(&account_xpub), false) | ||
| .map_err(|e| TaskError::WalletPaymentFailed { | ||
| detail: format!("Failed to derive change address: {e}"), | ||
| })?; |
There was a problem hiding this comment.
🟡 Suggestion: Consider add_to_state=true now that the API exposes the choice — current false preserves a latent issue
The new next_change_address(account_xpub, add_to_state) makes the address-pool registration explicit. With add_to_state=false, the address-pool path in key-wallet generates a new change address (when no pre-generated unused change address exists) without inserting it into addresses / address_index / script_pubkey_index (address_pool.rs:507-516). Because DET's SPV registration walks account.all_addresses() (src/context/wallet_lifecycle.rs:518), and that returns only entries actually in self.addresses, a freshly-derived change address used in a send is not added to the SPV watch set.
Important context for the author and reviewers: this is NOT a fresh regression. The prior code (wallet_account.derive_change_address(change_index)) was an immutable & borrow and did not register either. The new API simply makes the trade-off visible. Two observations:
- The comment correctly notes parity with the prior behavior, and the PR description marks this for smoke testing. If smoke runs confirm that subsequent UTXO scans recover the change output (because the wallet records its own outgoing tx outputs), the existing
falseis defensible. - If the smoke test reveals the change output is not picked up by SPV until a manual recovery/rescan, the simplest fix is
add_to_state=true. Re-using the same change address across retries inside theloopwould still work (the address-pool returns the same unused entry on subsequent calls until it's marked used).
At minimum, the outcome of this smoke check should be documented in the PR before merge, since this site sits right on the boundary the new API was designed to clarify.
source: ['claude', 'codex']
| fn pending_addresses(&self) -> impl Iterator<Item = (Self::Tag, Self::Address)> + '_ { | ||
| self.pending | ||
| .iter() | ||
| .filter(|(index, _)| !self.resolved.contains(index)) | ||
| .map(|(index, (key, _))| (*index, key.clone())) | ||
| .collect() | ||
| .filter_map(|(index, (_, core_address))| { | ||
| PlatformAddress::try_from(core_address.clone()) | ||
| .ok() | ||
| .map(|pa| (*index, pa)) | ||
| }) | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: pending_addresses() and current_balances() silently drop conversion failures — possible silent sync hang
Both pending_addresses() (2662-2671) and current_balances() (2730-2743) use filter_map(... .ok()) to drop entries where PlatformAddress::try_from(core_address) fails. The SDK consumes pending_addresses() to learn what is still to be resolved, but has_pending() (2724-2728) iterates self.pending directly — so any entry that fails conversion never reaches the SDK, never gets on_address_found / on_address_absent, never gets inserted into self.resolved, and has_pending() stays true forever. Result: silent sync hang with no error and no log line.
In practice the conversion should not fail (each pending entry came from derive_address_at_index, which already round-tripped PlatformAddress::try_from at insertion), but the silent-drop branch is the kind of invariant-breakage that is very hard to diagnose later. At minimum, log at error! when conversion fails and either force-resolve the offending index (so the loop makes progress) or unwrap() to surface the invariant violation. The same applies to current_balances() at 2738-2742, where the silent skip on a missing-from-pending index would corrupt the SDK's view of the stored baseline for incremental catch-up.
source: ['claude']
✅ QA re-verify (Marvin) — both blockers resolved at the mechanism levelRe-checked commits QA-001 (CRITICAL) — CONFIRMED-FIXED ( QA-003 (MEDIUM) — CONFIRMED-FIXED ( Remaining pre-merge gates (external to code):
Co-authored by Claudius the Magnificent AI Agent |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Latest delta (commits 6d1dfe9 and 84ab1a5) addresses two QA items cleanly: clear_data_dir now rewinds per-wallet SPV sync metadata with a regression test, and shielded Halo2 proving is offloaded onto the blocking pool via spawn_blocking + Handle::block_on. No new defects in the delta. Three prior findings from the cumulative PR remain valid and carried forward (change-address peek not registering state, SPV cancellation hang on stop() error, dead highest_found field). One prior finding (PlatformAddress conversion drops in pending_addresses/current_balances) is now mitigated by the explanatory comment plus the P2PKH-only construction invariant; dropped as outdated.
🟡 2 suggestion(s) | 💬 1 nitpick(s)
1 additional finding(s) omitted (not in diff).
🤖 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 `src/spv/manager.rs`:
- [SUGGESTION] src/spv/manager.rs:1194-1200: SPV cancellation can hang indefinitely when client.stop() returns Err
Carried forward from the prior review — still valid in the current source. On cancellation the code calls `client.stop().await`, logs any error via `tracing::warn!`, and then unconditionally awaits `(&mut run_future).await` to let `run()` observe the stop flag and unwind. The whole termination guarantee rides on `stop()` having flipped the client's internal running flag before it returned. If `stop()` returns `Err` before clearing that flag (e.g. an internal guard-acquisition failure or shutdown handshake error in the SDK), `run_future` has no other cancellation signal to observe and this await blocks forever — stalling SPV shutdown, network switches, and clear_data_dir on the exact error path where graceful shutdown matters most. The previous design used `run_cancel.cancel()`, a synchronous infallible signal that guaranteed unwind. Treat a `stop()` error as a hard cancellation: either `tokio::select!` the final await against `stop_token` again with a short timeout, or skip the await entirely on `Err` and let `run_future` drop. At minimum, verify upstream `DashSpvClient::stop()` always clears the running flag before any fallible work.
In `src/backend_task/core/mod.rs`:
- [SUGGESTION] src/backend_task/core/mod.rs:750-754: Change-address peek with add_to_state=false leaves newly derived change unregistered
Carried forward from the prior review — still valid. This site still calls `next_change_address(Some(&account_xpub), false)`. In key-wallet 0.43, `next_change_address` delegates to `address_pool::next_unused(..., add_to_state)` which will derive a brand-new change address whenever the internal pool has no pre-generated unused entry. With `add_to_state = false`, that fresh address is returned but not inserted into the account's internal address pool, so the transaction can pay change to an address the wallet has never reserved/registered for monitoring. The wallet can then miss tracking its own change output until some later rescan or unrelated address-generation step discovers it. The new TODO comment correctly explains why the existing `true` path has an unwanted monitor-revision side effect, but does not address this correctness risk — and because this PR migrates DET onto the new explicit `add_to_state` API, closing this hole is part of the migration surface. Either flip to `true` and accept the bloom-filter rebuild, or take a different non-mutating peek that still registers the address before use.
| let change_addr = account | ||
| .next_change_address(Some(&account_xpub), false) | ||
| .map_err(|e| TaskError::WalletPaymentFailed { | ||
| detail: format!("Failed to derive change address: {e}"), | ||
| })?; |
There was a problem hiding this comment.
🟡 Suggestion: Change-address peek with add_to_state=false leaves newly derived change unregistered
Carried forward from the prior review — still valid. This site still calls next_change_address(Some(&account_xpub), false). In key-wallet 0.43, next_change_address delegates to address_pool::next_unused(..., add_to_state) which will derive a brand-new change address whenever the internal pool has no pre-generated unused entry. With add_to_state = false, that fresh address is returned but not inserted into the account's internal address pool, so the transaction can pay change to an address the wallet has never reserved/registered for monitoring. The wallet can then miss tracking its own change output until some later rescan or unrelated address-generation step discovers it. The new TODO comment correctly explains why the existing true path has an unwanted monitor-revision side effect, but does not address this correctness risk — and because this PR migrates DET onto the new explicit add_to_state API, closing this hole is part of the migration surface. Either flip to true and accept the bloom-filter rebuild, or take a different non-mutating peek that still registers the address before use.
| let change_addr = account | |
| .next_change_address(Some(&account_xpub), false) | |
| .map_err(|e| TaskError::WalletPaymentFailed { | |
| detail: format!("Failed to derive change address: {e}"), | |
| })?; | |
| let change_addr = account | |
| .next_change_address(Some(&account_xpub), true) | |
| .map_err(|e| TaskError::WalletPaymentFailed { | |
| detail: format!("Failed to derive change address: {e}"), | |
| })?; |
source: ['codex']
There was a problem hiding this comment.
Pull request overview
This PR bumps the pinned Dash Platform Rust dependencies (dash-sdk and rs-sdk-trusted-context-provider) to v3.1.0-dev.8 and applies the required API migrations across DET, primarily around async Signer<K>, updated identity transition APIs, updated SPV/key-wallet APIs, and expanded DocumentQuery fields.
Changes:
- Updated dependency pins to platform commit
9e00c8b894and refreshed the lockfile accordingly. - Migrated signing/state-transition call sites to the new async + generic
Signer<K>and updated identity state transition APIs. - Adapted SPV + wallet/address sync code to key-wallet
0.43API changes (per-wallet sync height, address provider associated types/iterators, client run/stop lifecycle) and updatedDocumentQueryinitialization to includeselect/group_by/having.
Reviewed changes
Copilot reviewed 26 out of 27 changed files in this pull request and generated 15 comments.
Show a summary per file
| File | Description |
|---|---|
tests/backend-e2e/z_broadcast_st_tasks.rs |
Awaits updated async identity update transition builder. |
src/ui/wallets/shield_screen.rs |
Uses async shield build API and relies on upstream/offloaded proving. |
src/ui/tokens/view_token_claims_screen.rs |
Initializes new DocumentQuery fields. |
src/spv/tests.rs |
Updates regression test to key-wallet 0.43 per-wallet sync checkpoint semantics. |
src/spv/manager.rs |
Adapts SPV manager to key-wallet 0.43 client lifecycle and per-wallet sync height reset behavior. |
src/model/wallet/mod.rs |
Updates Signer<K> to async and reshapes AddressProvider implementation to new associated types/iterator API. |
src/model/qualified_identity/mod.rs |
Updates identity signer implementation to async Signer<IdentityPublicKey>. |
src/context/wallet_lifecycle.rs |
Updates managed-account APIs (managed_account_type(), all_addresses()). |
src/backend_task/wallet/fetch_platform_address_balances.rs |
Adjusts address sync result typing and highest-found index computation. |
src/backend_task/update_data_contract.rs |
Awaits updated async transition creation API. |
src/backend_task/shielded/bundle.rs |
Makes shield build async and offloads proof generation to blocking pool; updates broadcast flow. |
src/backend_task/platform_info.rs |
Initializes new DocumentQuery fields for withdrawal queries. |
src/backend_task/identity/top_up_identity.rs |
Migrates to top_up_identity_with_private_key API. |
src/backend_task/identity/register_identity.rs |
Migrates to put_to_platform_and_wait_for_response_with_private_key + async retry reconstruction. |
src/backend_task/identity/register_dpns_name.rs |
Initializes new DocumentQuery fields. |
src/backend_task/identity/refresh_loaded_identities_dpns_names.rs |
Initializes new DocumentQuery fields. |
src/backend_task/identity/load_identity.rs |
Initializes new DocumentQuery fields. |
src/backend_task/identity/load_identity_from_wallet.rs |
Initializes new DocumentQuery fields. |
src/backend_task/identity/load_identity_by_dpns_name.rs |
Initializes new DocumentQuery fields. |
src/backend_task/identity/discover_identities.rs |
Initializes new DocumentQuery fields. |
src/backend_task/identity/add_key_to_identity.rs |
Awaits updated async transition creation API. |
src/backend_task/document.rs |
Initializes new DocumentQuery fields for document operations. |
src/backend_task/dashpay/contact_requests.rs |
Initializes new DocumentQuery fields for DPNS lookup. |
src/backend_task/core/mod.rs |
Adapts transaction building + managed-account change/receive address APIs and builder reshaping. |
src/backend_task/contract.rs |
Initializes new DocumentQuery fields for keyword search contract fetch. |
Cargo.toml |
Bumps platform git revisions for dash-sdk and rs-sdk-trusted-context-provider. |
Cargo.lock |
Updates the dependency graph for the new platform commit (dashcore/key-wallet/orchard/etc.). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| fn pending_addresses(&self) -> impl Iterator<Item = (Self::Tag, Self::Address)> + '_ { | ||
| self.pending | ||
| .iter() | ||
| .filter(|(index, _)| !self.resolved.contains(index)) | ||
| .map(|(index, (key, _))| (*index, key.clone())) | ||
| .collect() | ||
| .filter_map(|(index, (_, core_address))| { | ||
| PlatformAddress::try_from(core_address.clone()) | ||
| .ok() | ||
| .map(|pa| (*index, pa)) | ||
| }) |
| self.stored_balances | ||
| .iter() | ||
| .filter_map(|(index, _key, funds)| { | ||
| let (_, core_address) = self.pending.get(index)?; | ||
| let pa = PlatformAddress::try_from(core_address.clone()).ok()?; | ||
| Some((*index, pa, *funds)) | ||
| }) |
Testnet smoke (Marvin) — core behaviour GREEN, one test-infra regressionLive testnet, serial backend-e2e on the bump branch. Functionally PASS (dashcore 0.42→0.43 behaviour preserved)
🟠 New regression (test-infrastructure only) — being fixed
Not validated
Remaining pre-merge work
Co-authored by Claudius the Magnificent AI Agent |
Correction: the "stack overflow" was a missed run-flag, not a regressionFollow-up to the smoke report above. The earlier SIGABRT was operator error in the smoke run, not a defect:
Net: the dashcore 0.42→0.43 behaviour is sound on testnet, and there is no stack regression. Full serial suite re-run (with the required env var) is in progress for end-to-end confirmation. Co-authored by Claudius the Magnificent AI Agent |
✅ Full backend-e2e suite GREEN — zero new regressions from the bumpFull serial run, live testnet, documented Tally: 55 passed / 4 failed / 0 ignored (647 s). The 4 failures — all pre-existing or environmental (bisected against
|
| Test | Bucket |
|---|---|
tc_037, tc_043 |
KNOWN-PREEXISTING — documented "incoming contact request not associated with sender" |
tc_014_wallet_platform_lifecycle |
Pre-existing test-data/funding artifact (AddressesNotEnoughFunds) — reproduces on v1.0-dev |
tc_018_fund_platform_address_from_asset_lock |
Environmental — InstantLock proof propagation timeout (broadcast succeeded); reproduces on v1.0-dev |
None are caused by the bump. The dashcore 0.42→0.43 InstantLock/SPV path did not regress.
Priority tests (exercise the changed code) — all PASS
SPV sync, send/receive (coin-selection/fee/change via reworked TransactionBuilder), is-ours/address derivation, balance reporting (TC001/TC002), identity registration (async Signer), top-up asset lock (tc005), withdrawal.
Remaining manual gates (coverage gaps, not failures)
- Shielded lifecycle (QA-003 / orchard 0.13) — BLOCKED here: shielded ops are feature-gated OFF on this testnet. Needs a shielded-enabled network. (Offload statically re-verified correct.)
- Clear-SPV-Data rescan (QA-001) — unit-verified; manual GUI check pending.
Net
Code-complete; build/clippy/lib green; QA-audited + re-verified; runtime-validated on testnet with zero new regressions to the extent this network allows. Outstanding items are the two manual gates above.
Co-authored by Claudius the Magnificent AI Agent
…et clone, test name) - C: SPV cancellation no longer awaits run_future when client.stop() errors — on Err the run flag may not have flipped, so awaiting could hang shutdown; treat stop()-Err as a hard cancel and drop the future. - D: distinguish JoinError cancellation from panic in the shield offload error. - E: pass Arc<RwLock<Wallet>> into the shield offload and take the read guard on the blocking thread instead of deep-cloning the whole Wallet (incl. seed). - F: rename test_clear_data_dir_resets_filter_committed_height → test_clear_data_dir_rewinds_wallet_synced_height. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Latest delta (d510bcd) fixes prior finding #1 (SPV cancellation hang): src/spv/manager.rs:1192-1208 now matches client.stop() and only awaits run_future on Ok, treating Err as hard cancellation with a warn log. The shielded offload (src/backend_task/shielded/bundle.rs:140-179) also distinguishes JoinError::is_cancelled from a panic and passes Arc<RwLock> instead of deep-cloning. Prior findings #2 (change-address peek with add_to_state=false) and #3 (highest_found dead state) remain STILL VALID: the new TODO comment at src/backend_task/core/mod.rs:726-734 explicitly acknowledges that the pool is not advanced but does not change runtime behavior, and the derived change address is still used as the real change output on line 771 (not just for fee estimation). Carrying both forward.
🟡 1 suggestion(s) | 💬 1 nitpick(s)
1 additional finding(s) omitted (not in diff).
🤖 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 `src/backend_task/core/mod.rs`:
- [SUGGESTION] src/backend_task/core/mod.rs:749-755: Change-address derived with add_to_state=false is used as the real change output, leaving it unregistered for monitoring
Carried forward from the prior review and re-validated at d510bcd3. `account.next_change_address(Some(&account_xpub), false)` is called at line 750-751, and the returned address is then passed to `builder.set_change_address(change_addr.clone())` at line 771 and broadcast as the actual change output — this is not just a fee-estimation peek. In key-wallet 0.43, `next_change_address` forwards `add_to_state` to `address_pool::next_unused`; when no pre-generated unused internal address remains, the fallback `generate_address_at_index(next_index, key_source, add_to_state)` only inserts the new address into the managed account's pool when `add_to_state` is `true`. The new comment at lines 726-734 acknowledges the pool is not advanced and argues the monitor-revision bump is acceptable for bloom-filter rebuild reasons, but a bloom rebuild does not help if the address is not in the wallet's tracked set; the monitoring concern is unresolved. Two consecutive sends before SPV sees the first change tx can also derive the same change index and reuse it. The TODO(v3.1-bump) tag is appropriate, but as long as the runtime behavior persists this stays a correctness/privacy risk.
| let utxos: Vec<_> = account.utxos.values().cloned().collect(); | ||
| let change_index = account.get_next_change_address_index().unwrap_or(0); | ||
| (utxos, change_index) | ||
| let change_addr = account | ||
| .next_change_address(Some(&account_xpub), false) | ||
| .map_err(|e| TaskError::WalletPaymentFailed { | ||
| detail: format!("Failed to derive change address: {e}"), | ||
| })?; | ||
| (utxos, change_addr) |
There was a problem hiding this comment.
🟡 Suggestion: Change-address derived with add_to_state=false is used as the real change output, leaving it unregistered for monitoring
Carried forward from the prior review and re-validated at d510bcd. account.next_change_address(Some(&account_xpub), false) is called at line 750-751, and the returned address is then passed to builder.set_change_address(change_addr.clone()) at line 771 and broadcast as the actual change output — this is not just a fee-estimation peek. In key-wallet 0.43, next_change_address forwards add_to_state to address_pool::next_unused; when no pre-generated unused internal address remains, the fallback generate_address_at_index(next_index, key_source, add_to_state) only inserts the new address into the managed account's pool when add_to_state is true. The new comment at lines 726-734 acknowledges the pool is not advanced and argues the monitor-revision bump is acceptable for bloom-filter rebuild reasons, but a bloom rebuild does not help if the address is not in the wallet's tracked set; the monitoring concern is unresolved. Two consecutive sends before SPV sees the first change tx can also derive the same change index and reuse it. The TODO(v3.1-bump) tag is appropriate, but as long as the runtime behavior persists this stays a correctness/privacy risk.
| let utxos: Vec<_> = account.utxos.values().cloned().collect(); | |
| let change_index = account.get_next_change_address_index().unwrap_or(0); | |
| (utxos, change_index) | |
| let change_addr = account | |
| .next_change_address(Some(&account_xpub), false) | |
| .map_err(|e| TaskError::WalletPaymentFailed { | |
| detail: format!("Failed to derive change address: {e}"), | |
| })?; | |
| (utxos, change_addr) | |
| let utxos: Vec<_> = account.utxos.values().cloned().collect(); | |
| let change_addr = account | |
| .next_change_address(Some(&account_xpub), true) | |
| .map_err(|e| TaskError::WalletPaymentFailed { | |
| detail: format!("Failed to derive change address: {e}"), | |
| })?; | |
| (utxos, change_addr) |
source: ['claude', 'codex']
Summary
Bumps
dash-sdkandrs-sdk-trusted-context-providerfrom platform pin54048b9352tov3.1-devHEAD9e00c8b894(v3.1.0-dev.8).This is a prerequisite step ahead of the v0.10-alpha release. It is an L-sized, semantic migration (537 commits / 300 files upstream), dominated by an async
Signer<K>rework, the reworked identity state-transition API, and a rust-dashcore/key-wallet0.42 → 0.43sideways move — not a mechanical version bump.What changed (27 files, +414 / −439)
DocumentQuerygainedselect/group_by/havingSigneris nowasync+ genericSigner<K>withsign_create_witness#[async_trait]+async fn; every.sign()caller now.awaittop_up_identity_with_private_key,put_to_platform_and_wait_for_response_with_private_key,try_from_identity_with_private_key,try_from_identity_with_signer_and_private_key(async),sign_external_with_options(async)TransactionBuilderreshapedset_selection_strategy/set_current_height/add_inputs+build_unsigned(); removed old string-classificationManagedAccount*/WalletManagermanaged_account_type(),next_change_address,next_receive_address,ManagedAccountTraitimportAddressProviderassociated typesTag = AddressIndex,Address = PlatformAddress; iterator returns + callbacks reshapedWalletManagerper-walletsynced_height,DashSpvClient<W,N,S>3 params,run()/stop()lifecycleVerification
cargo build --all-featurescargo clippy --all-features --all-targets -- -D warnings(lib + bins + kittest + e2e + backend-e2e)cargo +nightly fmt --all#[ignore]test suites not executed — see pre-merge gates below.The dashcore bump is a divergent move (~90 commits theirs we lacked; 1 squashed commit ours they lack), so compile-clean ≠ behaviour-equal. The compiler cannot catch regressions in these areas — they require a runtime smoke test:
0.12 → 0.13shielded pathBehaviour-ambiguous sites flagged for audit (
TODO(v3.1-bump))src/spv/manager.rs:822— global filter-height → per-walletsynced_heightreset on data clear; verify rescan-from-genesis still fires.src/ui/wallets/shield_screen.rs:328— Halo2 proof gen moved offspawn_blockingonto the async worker (builder is now async); watch for runtime starvation.backend_task/core/mod.rs— change-address: index-peek →next_change_address(…, add_to_state=false).model/wallet/mod.rscurrent_balances— now skips stored balances whose index left the pending window.Pre-merge gates
platform-wallet-storagepersister crate) is still open againstv3.1-dev. This bump is necessary-but-not-sufficient if the upcoming architecture work (PR feat: platform-wallet backend rewrite (spec + implementation) #860) requires that crate specifically. This PR does not unblock feat: platform-wallet backend rewrite (spec + implementation) #860 on its own.Co-authored by Claudius the Magnificent AI Agent
Summary by CodeRabbit
Bug Fixes
Chores