feat(platform-wallet): report the balance a pooled build can actually spend - #4582
feat(platform-wallet): report the balance a pooled build can actually spend#4582romchornyi wants to merge 1 commit into
Conversation
… spend `core_wallet_get_balance` sums every funding account the wallet has — CoinJoin included — and never consults a reservation set. A host gating its amount entry on it therefore offers money the build then refuses, and the shortfall surfaces as CorePooledInsufficientFunds only after the user has committed to an amount. Support ticket 32081 is the shape of it: a wallet reading 94 DASH, of which 0.0054 was actually spendable, everything else on the CoinJoin account the send pool excludes by design. The same mismatch produces the asset-lock shortfall on the Transparent to Shielded path. `pooled_spendable_balance` answers with the accounts `finalize_transaction` funds from, resolved through the same `resolve_source_accounts` and the same source list, counting only UTXOs coin selection accepts. Hosts read it instead of mirroring the pooling rule themselves — the mirror is what drifted here. Reservations are not subtracted: key-wallet keeps each account's ReservationSet private, so reading it needs an accessor there and a pin bump. Documented at every layer. That part is transient — a reservation is released when its spend is processed, on a definitive rejection, at the TTL, or on restart — while the account-set difference is permanent and was the whole of the reported shortfall.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Team Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
llbartekll
left a comment
llbartekll
left a comment
There was a problem hiding this comment.
Reviewed the three files plus the key-wallet side (add_funding, select_coins_with_size, Utxo::is_spendable) to check the new figure really matches what selection accepts.
The diagnosis is right and the layering is right: .allSpendable → SEND_FUNDING_SOURCES genuinely keeps CoinJoin out, and spendable_utxos(height) genuinely matches select_coins_with_size, which filters candidates by is_spendable(current_height) on every strategy. So the CoinJoin over-report this targets is real and this fixes it.
Three things I think should be settled before merge, all of the same kind — the new function is a second hand-copy of finalize_transaction_with_options' funding loop, and it has already drifted from it in ways that reintroduce the over-report the PR exists to remove:
- Fee is not subtracted. The doc tells hosts to gate amount entry on this, but a build needs
amount + fee. A max-amount send entering the returned value verbatim fails with the sameCorePooledInsufficientFunds. account_of_typeis not checked, so accounts the build skips are still counted.- Single-family selectors (
.bip32,.coinJoin) returnOk(0)wherefinalizereturnsWalletNotFound— the mirror doesn't honourstrict.
Plus a read-only query holding the manager write lock, and the doc comments of finalize_transaction and core_wallet_tx_builder_use_only_added_inputs having been captured by the new functions in both Rust files.
On the untested point — I'd push back gently on shipping this one without a test specifically because the PR's own thesis is that an unpinned mirror of the pooling rule drifts. funded_wallet_manager_dual_standard(&[700_000], &[700_000]) is already in test_support and makes the core assertion about three lines; findings 2 and 3 would both have been caught by it.
Merge-order note in the description looks right and I agree this must not land first.
| let Some(managed) = info.core_wallet.accounts.funds_account_mut(&at) else { | ||
| continue; | ||
| }; | ||
| total += managed |
There was a problem hiding this comment.
The fee isn't subtracted, so this ceiling is still unspendable as an amount.
The doc above says hosts should gate amount entry on this, but the value returned is the gross sum of spendable UTXOs. A build needs amount + fee.
select_coins_with_size passes the total_available < target_amount check with the full sum, and then accumulate_coins_with_size can't cover target + fee — so a user tapping a max/"send all" button wired to this and entering the returned value verbatim gets exactly the CorePooledInsufficientFunds this PR is removing, just relocated from the CoinJoin edge to the max-amount edge.
With the 500-UTXO sweep landing alongside (#4548), the input count and therefore the fee can be well above dust, so this isn't a rounding concern.
Either subtract an estimated fee here, or state in the doc that the figure is pre-fee and hosts must reserve headroom — but given the failure mode this is meant to prevent, I'd rather it be handled here than left as a second thing for hosts to mirror.
| if !seen.insert(at) { | ||
| continue; | ||
| } | ||
| let Some(managed) = info.core_wallet.accounts.funds_account_mut(&at) else { |
There was a problem hiding this comment.
Diverges from finalize here: the account_of_type half of the check is missing.
finalize_transaction_with_options requires both to resolve before it funds an account:
let (Some(account), Some(managed)) = (
wallet.accounts.account_of_type(at),
info.core_wallet.accounts.funds_account_mut(&at),
) else { ... continue; };This counts an account when only funds_account_mut resolves. An AccountType present in info.core_wallet.accounts but absent from wallet.accounts is added to the total and then skipped by the build — an over-report of exactly the shape the PR is fixing.
The discarded _wallet binding on line 331 is where the other half went.
|
|
||
| let mut seen: HashSet<AccountType> = HashSet::new(); | ||
| let mut total: u64 = 0; | ||
| for &preference in sources { |
There was a problem hiding this comment.
Single-source selectors silently return 0 instead of the strict not-found error.
finalize_transaction_with_options sets strict = sources.len() == 1 and errors with WalletNotFound when the named account is missing — pinned by single_source_missing_account_still_errors (line 866). This loop applies the skip-missing rule unconditionally.
CoreAccountTypeFFI::funding_sources() returns a one-element list for BIP44, BIP32 and CoinJoin, so pooledSpendableBalance(accountType: .bip32) on a wallet with no BIP32 account returns Ok(0). The host renders "insufficient funds"; the matching finalize call would have said "no such account". Two different answers to the same selector.
That also makes the doc's "the same accounts finalize_transaction funds from" untrue for every single-family selector — worth either taking a strict flag or narrowing the doc.
| sources: &[AccountTypePreference], | ||
| source_index: u32, | ||
| ) -> Result<u64, PlatformWalletError> { | ||
| let mut manager = self.wallet_manager.write().await; |
There was a problem hiding this comment.
Write lock for a pure read.
Nothing here mutates. Read-only equivalents exist and are what the rest of the package uses:
WalletManager::get_wallet_and_info(key-wallet-manager/src/accessors.rs:29)ManagedAccountCollection::funds_account(key-wallet/src/managed_account/managed_account_collection.rs:581)
so this can be .read().await + get_wallet_and_info + funds_account, and the _wallet binding drops out.
It matters because of how this is meant to be called — gating amount entry means potentially one call per keystroke, each taking the exclusive manager lock against concurrent finalizers, broadcast reconciliation and SPV sync writers. Every other read-only path in the package already takes the read lock: sign_message.rs:149, broadcast.rs:136, wallet.rs:372/400, and transaction.rs:650 in this same file.
| let mut seen: HashSet<AccountType> = HashSet::new(); | ||
| let mut total: u64 = 0; | ||
| for &preference in sources { | ||
| for at in resolve_source_accounts(&info.core_wallet.accounts, preference, source_index) |
There was a problem hiding this comment.
This loop is a second copy of the one in finalize_transaction_with_options (lines ~434-447).
The PR's argument is that the host's hand-copy of the pooling rule drifted, so hosts should read it from the source of truth instead. But the new source of truth is itself a hand-copy of the funding loop — and it has already drifted twice before merge (the two comments above).
Extracting something like
fn resolved_funding_accounts(
accounts: &ManagedAccountCollection,
wallet: &Wallet,
sources: &[AccountTypePreference],
source_index: u32,
strict: bool,
) -> Result<Vec<AccountType>, PlatformWalletError>and driving both call sites from it makes the two impossible to desynchronise, and drops the duplicated dedup HashSet as a side effect. That seems like the depth the fix actually wants to sit at.
| /// is permanent, and was the whole of the shortfall in the report that | ||
| /// prompted it (support ticket 32081: 0.0054 DASH offered as spendable | ||
| /// against a 94 DASH balance, all of it CoinJoin). | ||
| pub async fn pooled_spendable_balance( |
There was a problem hiding this comment.
Worth a test — the fixtures are already there.
mod tests in this file (line 695) has what's needed:
funded_wallet_manager_dual_standard(&[700_000], &[700_000])— already used bypooled_send_spans_families_and_abandon_releases_all; assertingpooled_spendable_balance(&SEND_FUNDING_SOURCES, 0) == 1_400_000is about three lines.funded_wallet_manager_with_contactcovers the DashPay leg.
Nothing currently pins this function to finalize_transaction's account set, which is the one coupling the PR exists to enforce. Both divergences I flagged would be caught by that test.
| @@ -299,6 +299,61 @@ pub(crate) fn resolve_source_accounts( | |||
| impl<B: TransactionBroadcaster + ?Sized> CoreWallet<B> { | |||
| /// Consume a configured builder, atomically fund and reserve its selected | |||
There was a problem hiding this comment.
Doc comment got captured by the new function.
The insertion landed between finalize_transaction's doc comment and its signature, so "Consume a configured builder, atomically fund and reserve its selected inputs, then sign without holding the wallet-manager lock." is now the summary line of pooled_spendable_balance — a read-only getter that reserves and signs nothing — running straight on into the real description with no separating ///.
finalize_transaction (line 357) is left with no doc at all.
| /// each stay under the standard-transaction input limit needs this, or every | ||
| /// batch sees the whole account and fails with a too-many-inputs error. | ||
| /// | ||
| /// The balance a build funded by `account_type` could actually select from — the |
There was a problem hiding this comment.
Same capture here.
The doc block starting at line 629 ("Fund the build from the inputs core_wallet_tx_builder_add_inputs_from_outpoints supplied, and nothing else" … "fails with a too-many-inputs error") belongs to core_wallet_tx_builder_use_only_added_inputs, which now sits at line 674 with only the boilerplate # Safety note.
This one leaks further than the Rust-side twin: cbindgen emits these into the generated C header the Swift SDK imports, so the public header will describe a balance getter in terms of batched-drain funding, while use_only_added_inputs — new in the base PR, and whose entire rationale lived in that block — ships with no explanation at all.
3 participants
Issue being fixed or feature implemented
core_wallet_get_balancesums every funding account the wallet has — CoinJoin included — and neverconsults a reservation set. A host that gates its amount entry on it offers money the build then
refuses, and the shortfall surfaces as
CorePooledInsufficientFundsonly after the user has committedto an amount.
Ticket 32081 is the shape of it: a wallet reading 94 DASH, of which 0.0054 was actually
spendable — everything else sat on the CoinJoin account, which
SEND_FUNDING_SOURCESexcludes bydesign. The user was offered a 1 DASH send, entered it, and got
The same mismatch produces
asset lock coin selection is shorton Transparent → Shielded, with theidentical
available 538503.What was done?
CoreWallet::pooled_spendable_balance(sources, source_index), pluscore_wallet_pooled_spendable_balanceandManagedCoreWallet.pooledSpendableBalance(...).It resolves accounts through the same
resolve_source_accountsand the same source listfinalize_transactionuses, and counts only UTXOs coin selection accepts. Hosts read it instead ofmirroring the pooling rule — the mirror is exactly what drifted: the app's own comment claims
.allSpendablepools "the same set the home balance already totals", which was never true.Reservations are not subtracted. key-wallet keeps each account's
ReservationSetprivate, soreading it needs an accessor there and a pin bump. Documented at all three layers. That part is
transient — released when the spend is processed, on a definitive rejection, at the TTL, or on restart
— while the account-set difference is permanent and was the whole of the reported shortfall.
How Has This Been Tested?
cargo test -p platform-wallet --lib— 909 passed, 0 failed.dashpayTestnet target compiles against it with the hostside wired (fix(build): restore dashpay compilation dashwallet-ios#1095).
Not yet exercised against a live CoinJoin-heavy wallet: the testnet wallet built for this work has
since been swept, so the state has to be rebuilt to see the ceiling change.
Breaking Changes
None. New read-only entry point; no existing behaviour changes.
Merge order
This is one of three defects behind ticket 32081, and this one must not land first: it lowers the
ceiling the amount screen shows, so a user with mixed coins sees less. That is only safe once the
sweep works and can unlock the difference.
Based on #4548 rather than
v4.2-devto avoid conflicting intransaction.rs; rebase once that lands.Checklist:
For repository code-owners and collaborators only