fix(wallet): use mode-aware broadcast for platform funding in SPV mode#646
Merged
Conversation
fund_platform_address_from_wallet_utxos() called core_client.send_raw_transaction() directly, bypassing the mode-aware broadcast_raw_transaction() helper. This broke core-to-platform transfers in SPV mode where no RPC connection is available. Changes: - Replace direct RPC broadcast with self.broadcast_raw_transaction() which routes to SPV manager in SPV mode and Core RPC in RPC mode - Guard UTXO reload fallback with core_backend_mode() check: only attempt RPC-based reload_utxos in RPC mode; return error in SPV mode where wallet state is authoritative (matching register_identity.rs and top_up_identity.rs) - Remove unused dash_sdk::dashcore_rpc::RpcApi import Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
📝 WalkthroughWalkthroughRefactors wallet funding and UTXO reload flows: asset-lock txs are persisted before broadcast; broadcasts use an async, mode-aware path; failures trigger DB cleanup and mode-dependent recovery; reload_utxos signature changed to accept AppContext and return a changed flag; identity flows unified to reload-and-retry on UTXO changes. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant Wallet
participant DB
participant Broadcaster
participant CoreRPC
participant SPV
Caller->>Wallet: fund_platform_address_from_wallet_utxos(...)
Wallet->>Wallet: build asset-lock tx + metadata
Wallet->>DB: insert pre-broadcast asset-lock row
Wallet->>Broadcaster: broadcast_raw_transaction(tx) note right of Broadcaster: async, mode-aware
alt broadcast success
Broadcaster-->>Wallet: txid / success
Wallet->>Wallet: remove funded UTXOs, recalc balance
Wallet->>Wallet: wait_for_asset_lock_proof(txid)
Wallet->>CoreRPC: check/monitor finality (RPC) or rely on SPV sync
alt proof within timeout
Wallet-->>Caller: success
else timeout
alt RPC mode
Wallet->>Wallet: trigger async reload_utxos(app_context)
Wallet-->>Wallet: reload_utxos -> changed?
alt changed == true
Wallet->>Wallet: retry asset-lock flow
else changed == false
Wallet-->>Caller: return timeout/error
end
else SPV mode
Wallet->>SPV: log timeout, await next SPV sync reconciliation
Wallet-->>Caller: return timeout/info
end
end
else broadcast failure
Broadcaster-->>Wallet: error
Wallet->>DB: delete pre-broadcast asset-lock row
Wallet->>Wallet: cleanup finality tracking state
Wallet-->>Caller: return error
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/backend_task/wallet/fund_platform_address_from_wallet_utxos.rs (1)
68-75: Avoid panicking on a poisoned Core client lock.
expectwill abort the task on lock poisoning; prefer returning a structured error instead of panicking.🔧 Suggested change
- wallet - .reload_utxos( - &self - .core_client - .read() - .expect("Core client lock was poisoned"), + let core_client = self + .core_client + .read() + .map_err(|_| "Core client lock was poisoned")?; + wallet + .reload_utxos( + &core_client, self.network, Some(self), )Based on learnings, "For Bitcoin transactions in this project, amounts should be parameterized instead of hardcoded, and proper error handling should be used with
?instead ofunwrap()."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/backend_task/wallet/fund_platform_address_from_wallet_utxos.rs` around lines 68 - 75, The code currently panics on a poisoned RwLock by calling expect on self.core_client.read(); replace that with proper error propagation: acquire the read guard with self.core_client.read().map_err(|e| YourError::LockPoisoned("core_client", e))? (or convert the PoisonError into the crate's Result/Error type) and pass the referenced guard into reload_utxos (e.g., let core = self.core_client.read().map_err(...)?; .reload_utxos(&core, self.network, Some(self))). Also remove any other unwrap()/expect() in this call path and return errors with ? so the task can handle failures; if there are hardcoded amounts nearby, make them function parameters instead of constants so callers can supply amounts.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/backend_task/wallet/fund_platform_address_from_wallet_utxos.rs`:
- Around line 68-75: The code currently panics on a poisoned RwLock by calling
expect on self.core_client.read(); replace that with proper error propagation:
acquire the read guard with self.core_client.read().map_err(|e|
YourError::LockPoisoned("core_client", e))? (or convert the PoisonError into the
crate's Result/Error type) and pass the referenced guard into reload_utxos
(e.g., let core = self.core_client.read().map_err(...)?; .reload_utxos(&core,
self.network, Some(self))). Also remove any other unwrap()/expect() in this call
path and return errors with ? so the task can handle failures; if there are
hardcoded amounts nearby, make them function parameters instead of constants so
callers can supply amounts.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes a critical bug where fund_platform_address_from_wallet_utxos() was incompatible with SPV mode due to direct Core RPC usage instead of the mode-aware broadcast helper used by all other asset lock operations.
Changes:
- Removed unused
RpcApiimport since direct RPC calls are no longer used - Added mode-aware UTXO reload fallback that guards Core RPC operations behind
core_backend_mode()check, matching the established pattern inregister_identityandtop_up_identity - Replaced direct
core_client.send_raw_transaction()call with the mode-awarebroadcast_raw_transaction()helper to enable SPV mode support
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
fund_platform_address_from_wallet_utxos() called core_client.send_raw_transaction() directly, bypassing the mode-aware broadcast_raw_transaction() helper. This broke core-to-platform transfers in SPV mode where no RPC connection is available. Changes: - Replace direct RPC broadcast with self.broadcast_raw_transaction() which routes to SPV manager in SPV mode and Core RPC in RPC mode - Guard UTXO reload fallback with core_backend_mode() check: only attempt RPC-based reload_utxos in RPC mode; return error in SPV mode where wallet state is authoritative (matching register_identity.rs and top_up_identity.rs) - Store asset lock transaction in DB after broadcast so the SPV finality listener can retrieve it when processing InstantLock/ChainLock events (without this, the finality proof is never populated and the wait loop times out after 5 minutes) - Remove unused dash_sdk::dashcore_rpc::RpcApi import Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Change reload_utxos from (Client, Network, Option<AppContext>) to (&AppContext), returning Result<bool, String> (true = UTXOs changed) - SPV mode: no-op returning Ok(false) instead of error - RPC mode: acquire core_client internally with map_err (SEC-03 fix) - Callers skip retry when reload reports no changes - Replace inline tokio::select! timeout loop in fund_platform_address with shared wait_for_asset_lock_proof helper (SEC-04 fix) - Add mode-aware post-timeout recovery (RPC: refresh_wallet_info, SPV: tracing::warn about automatic reconciliation) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 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.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/model/wallet/utxos.rs (1)
170-219:⚠️ Potential issue | 🟡 MinorGuard against partial state if DB persistence fails.
self.utxosis mutated before DB drops/inserts; on DB error the function returnsErrbut the in-memory state is already updated. Consider applying DB changes first (or using a DB transaction) and only then mutatingself.utxos, or rolling back on failure.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/model/wallet/utxos.rs` around lines 170 - 219, The in-memory map self.utxos is updated before database persistence, risking partial state if db.drop_utxo or db.insert_utxo fails; change the flow so DB changes are applied first (use a DB transaction if supported) or prepare the full set of DB operations and execute them before mutating self.utxos, and only after all db.drop_utxo / db.insert_utxo calls succeed, apply the in-memory updates to self.utxos (or if you cannot transactionally commit the DB, implement rollback logic to revert self.utxos on any DB error). Ensure you reference and coordinate removed_outpoints, added_outpoints, new_utxo_map, db.drop_utxo and db.insert_utxo when reordering or wrapping in a transaction.
🤖 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/backend_task/wallet/fund_platform_address_from_wallet_utxos.rs`:
- Around line 87-105: transactions_waiting_for_finality entries are left behind
if broadcast_raw_transaction or store_asset_lock_transaction fail; update the
error paths in the fund_platform_address_from_wallet_utxos flow to remove the
pending entry before returning an error: after calling
self.broadcast_raw_transaction(&asset_lock_transaction).await and before
returning on error, and likewise after
self.db.store_asset_lock_transaction(...).map_err(...) failure, call the same
cleanup that removes the corresponding key from
transactions_waiting_for_finality (the map populated earlier for
asset_lock_transaction/seed_hash) so no stale entry remains; reference
transactions_waiting_for_finality, broadcast_raw_transaction,
store_asset_lock_transaction, asset_lock_transaction and seed_hash when locating
where to add the cleanup.
---
Outside diff comments:
In `@src/model/wallet/utxos.rs`:
- Around line 170-219: The in-memory map self.utxos is updated before database
persistence, risking partial state if db.drop_utxo or db.insert_utxo fails;
change the flow so DB changes are applied first (use a DB transaction if
supported) or prepare the full set of DB operations and execute them before
mutating self.utxos, and only after all db.drop_utxo / db.insert_utxo calls
succeed, apply the in-memory updates to self.utxos (or if you cannot
transactionally commit the DB, implement rollback logic to revert self.utxos on
any DB error). Ensure you reference and coordinate removed_outpoints,
added_outpoints, new_utxo_map, db.drop_utxo and db.insert_utxo when reordering
or wrapping in a transaction.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/backend_task/identity/register_identity.rssrc/backend_task/identity/top_up_identity.rssrc/backend_task/wallet/fund_platform_address_from_wallet_utxos.rssrc/model/wallet/utxos.rs
✅ Files skipped from review due to trivial changes (1)
- src/backend_task/identity/top_up_identity.rs
- CODE-01: Move store_asset_lock_transaction before broadcast to eliminate race where SPV finality listener fires before the DB row exists. Clean up DB row and finality tracking on broadcast failure. - CODE-02: Guard DB persistence in reload_utxos with `if changed` to skip empty-set iteration when nothing changed. - CODE-04: Renumber step comments sequentially (1-9, no gaps). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/backend_task/wallet/fund_platform_address_from_wallet_utxos.rs (1)
88-101:⚠️ Potential issue | 🟡 MinorClean up finality tracking if DB store fails.
If
store_asset_lock_transactionfails, thetransactions_waiting_for_finalityentry remains and can leak stale state. Please remove the entry on this error path as well.🧹 Suggested fix
- self.db - .store_asset_lock_transaction( - &asset_lock_transaction, - asset_lock_amount, - None, // No islock yet — SPV/ZMQ will update this - &seed_hash, - self.network, - ) - .map_err(|e| format!("Failed to store asset lock transaction: {}", e))?; + if let Err(e) = self.db.store_asset_lock_transaction( + &asset_lock_transaction, + asset_lock_amount, + None, // No islock yet — SPV/ZMQ will update this + &seed_hash, + self.network, + ) { + if let Ok(mut proofs) = self.transactions_waiting_for_finality.try_lock() { + proofs.remove(&tx_id); + } + return Err(format!("Failed to store asset lock transaction: {}", e)); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/backend_task/wallet/fund_platform_address_from_wallet_utxos.rs` around lines 88 - 101, The failure path after calling self.db.store_asset_lock_transaction(...) can leave a stale entry in transactions_waiting_for_finality; modify the error handling so that if store_asset_lock_transaction returns Err you first remove the pending entry (e.g. call the appropriate removal method on self.transactions_waiting_for_finality using the asset lock txid/seed_hash key) and then propagate the error (keeping the existing formatted error message). Ensure this cleanup happens inside the same error branch that currently maps the error to the "Failed to store asset lock transaction: {}" message so no stale state remains on early returns.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/backend_task/wallet/fund_platform_address_from_wallet_utxos.rs`:
- Around line 88-101: The failure path after calling
self.db.store_asset_lock_transaction(...) can leave a stale entry in
transactions_waiting_for_finality; modify the error handling so that if
store_asset_lock_transaction returns Err you first remove the pending entry
(e.g. call the appropriate removal method on
self.transactions_waiting_for_finality using the asset lock txid/seed_hash key)
and then propagate the error (keeping the existing formatted error message).
Ensure this cleanup happens inside the same error branch that currently maps the
error to the "Failed to store asset lock transaction: {}" message so no stale
state remains on early returns.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/backend_task/wallet/fund_platform_address_from_wallet_utxos.rssrc/model/wallet/utxos.rs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
fund_platform_address_from_wallet_utxos()calledcore_client.send_raw_transaction()directly, bypassing the mode-awarebroadcast_raw_transaction()helper used by every other asset lock caller (register_identity, top_up_identity, create_asset_lock)core_backend_mode()check, matching the established pattern inregister_identity.rsandtop_up_identity.rsstore_asset_lock_transaction()call after broadcast. Without this, the SPV finality listener cannot retrieve the transaction from the DB to process InstantLock/ChainLock events, causing the wait loop to always time out after 5 minutes (pre-existing bug, but now reachable with the SPV broadcast fix)Refactoring (review follow-up)
reload_utxossignature from(&Client, Network, Option<&AppContext>)to(&AppContext)— all three values came fromAppContextanyway. ReturnsResult<bool, String>wheretrue= UTXOs changed,false= no change (including SPV no-op). Callers skip retry when no changes detected.core_client.read()now usesmap_errinstead of.expect(), preventing panics on lock poisoningtokio::select!timeout loop infund_platform_address_from_wallet_utxoswith sharedwait_for_asset_lock_proof()helper. Added mode-aware post-timeout recovery (RPC: fire-and-forgetrefresh_wallet_info; SPV:tracing::warnabout automatic reconciliation)Review fixes
store_asset_lock_transactionbeforebroadcast_raw_transactionto eliminate race where SPV finality listener fires before the DB row exists. On broadcast failure, DB row is cleaned up with a TODO noting future status-column improvement.reload_utxoswithif changedto skip empty-set iterationTest plan
cargo clippy --all-features --all-targets -- -D warningspassescargo test --all-features --workspacepasses🤖 Generated with Claude Code
🤖 Co-authored by Claudius the Magnificent AI Agent
Summary by CodeRabbit
Bug Fixes
Improvements