fix(wallets): use selector ceiling for asset-lock Max - #937
Conversation
Query the live asset-lock TransactionBuilder for the largest credit output its final-input coin selection accepts, and reuse that ceiling for Max and pre-send validation across Shield and identity wallet-funding flows. Keep snapshot balances display-only and add real-selector regressions for unconfirmed funds that inflate the UI subtotal. Fixes #929 Co-Authored-By: Codex GPT-5 <noreply@openai.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe change replaces snapshot-based asset-lock maximums with dry-run builder-derived ceilings. Generation and UTXO-composition-aware caches deliver these values to identity funding and Core send flows for Max calculations, validation, loading states, and retry handling. Wallet-task plumbing, builder integration, and removal of timeout-based errors complete the foundation. ChangesAsset-lock maximum flow
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant SendScreen
participant AssetLockBalanceCache
participant AppContext
participant WalletBackend
participant TransactionBuilder
SendScreen->>AssetLockBalanceCache: request maximum for (generation, final_funds, utxo_revision)
alt cache miss or stale
AssetLockBalanceCache->>AppContext: enqueue GetAssetLockMaxAmount task
AppContext->>WalletBackend: call asset_lock_max_amount
WalletBackend->>TransactionBuilder: dry-run candidate amounts
TransactionBuilder-->>WalletBackend: builder-derived ceiling
WalletBackend-->>AppContext: BackendTaskSuccessResult
AppContext-->>AssetLockBalanceCache: store result
end
AssetLockBalanceCache-->>SendScreen: current cached amount or stale-for-display amount
SendScreen->>SendScreen: validate_asset_lock_amount(requested, reserve, cached_max)
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1⚔️ Resolve merge conflicts 💡
📝 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 |
… probe Independent architecture audit of 3493f71 found the Max/validation dry-run probe read `synced_height()` while the real `create_asset_lock_proof` path reads `last_processed_height()` — a different, independently-advanced watermark. Diverging by enough blocks could sweep a live in-flight UTXO reservation via the coinbase-maturity/TTL logic, a real side effect from a supposedly read-only probe. Route the probe through the same accessor the real path uses. Also give `AssetLockBalanceCache` a per-wallet publish generation so a cached Max ceiling is rejected and re-queried after the wallet snapshot changes, instead of only on screen refresh. The hand-duplicated `ASSET_LOCK_FEE_PER_KB` constant stays: verified against the pinned `rs-platform-wallet` source that the real path's `DEFAULT_FEE_PER_KB` is `pub(super)` and not reachable from DET, and that DET's existing `FeeRate::normal()` usage elsewhere (ordinary Core sends) is an unrelated crate-level constant that only coincidentally matches today. No importable shared source exists yet; comment updated to say so plainly and track it as an upstream ask. Co-Authored-By: Codex GPT-5 <noreply@openai.com> Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…a seeded probe The prior implementation binary-searched [0, MAX_MONEY] against the real selection strategy — correct, but ~34-51 real TransactionBuilder builds per Max query, each holding the wallet's global async write lock. A first attempt at trimming this hand-derived a "68-byte" padding correction by adding placeholder credit outputs until tests passed. Rejected on review: independently checked against the vendored `key-wallet` coin-selector and could only confirm a single 34-byte change-output assumption in `branch_and_bound_with_size`'s fallback path, not two, and DET's own `create_asset_lock_proof` never touches `TransactionBuilder` directly (it's wrapped inside `platform-wallet`'s `AssetLockManager`), so there was no local ground truth to confirm the number generalized. A constant reverse-engineered against RED tests until they pass is exactly the failure mode this whole review has been catching. Replaced with a seed-then-bisect approach that never assumes anything about the real strategy's internal fee/size formula: - One `SelectionStrategy::All` drain call gives a fast, verified-correct upper bound (the builder already drops the change address for `All` before sizing). - Exponential search downward from that seed using the real default strategy (`BranchAndBound`, matching `create_asset_lock_proof`), then a short bisection to the exact boundary. Typically ~15 calls instead of ~34-51, all against the real code, none against a guessed formula. - `TooManyInputs` on the seed call (more spendable UTXOs than fit one transaction — `All` can never succeed there) falls back to the original full-range search unchanged, preserving old behavior for that edge case. All three regression tests pass with assertions unchanged, proving the seeded result is exact: wallet_backend::payments::tests::asset_lock_max_excludes_unconfirmed_funds_counted_by_snapshot wallet_backend::payments::tests::asset_lock_max_uses_last_processed_height_when_sync_watermarks_diverge ui::wallets::send_screen::tests::core_asset_lock_max_and_validation_use_builder_quote Co-Authored-By: Codex GPT-5 <noreply@openai.com> Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
lklimek
added
the
claudius-review
|
⛔ Blockers found — Sonnet deferred (commit 8ea190d) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
src/ui/identities/add_new_identity_screen/mod.rs (1)
1061-1088: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated wallet-balance validation logic across screens.
The seed-hash lookup →
asset_lock_balance.get→validate_asset_lock_amount→ banner-on-error sequence here is nearly identical totop_up_identity_screen/mod.rs'stop_up_identity_clicked(lines 481-519), and the two have already started to drift (this one usesread_recover()silently, top-up explicitly handles a busy lock with its own banner). Extracting a shared helper (e.g. infunding_common.rs) would prevent further divergence in this validation/error-messaging path.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/ui/identities/add_new_identity_screen/mod.rs` around lines 1061 - 1088, Extract the wallet-balance validation sequence from the add-identity handler and top_up_identity_clicked into a shared helper in funding_common.rs. The helper should perform seed-hash lookup, asset_lock_balance retrieval, validate_asset_lock_amount, and the corresponding warning-banner behavior, including explicit handling of a busy wallet lock; update both callers to reuse it and preserve their existing success and rejection flows.src/ui/wallets/send_screen.rs (1)
641-677: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate eligibility guard between
request_asset_lock_max_amountandrender_asset_lock_balance_status.Both functions repeat the identical
selected_source == CoreWallet && destination_kind in {Shielded, Identity}check. Extracting a shared helper avoids the two conditions drifting apart (e.g., a future flow change updated in one place but not the other, silently breaking the request/status pairing).♻️ Proposed extraction
+ fn asset_lock_max_amount_applies(&self) -> bool { + matches!(self.selected_source, Some(SourceSelection::CoreWallet)) + && matches!( + self.destination_kind(), + Some(AddressKind::Shielded | AddressKind::Identity) + ) + } + fn request_asset_lock_max_amount(&mut self) -> Option<BackendTask> { - if !matches!(self.selected_source, Some(SourceSelection::CoreWallet)) - || !matches!( - self.destination_kind(), - Some(AddressKind::Shielded | AddressKind::Identity) - ) - { + if !self.asset_lock_max_amount_applies() { return None; } let seed_hash = self.selected_wallet_seed_hash?; let snapshot_generation = self.app_context.snapshot_generation(&seed_hash); self.asset_lock_balance .ensure_requested(seed_hash, snapshot_generation) } fn render_asset_lock_balance_status(&mut self, ui: &mut Ui) { let Some(seed_hash) = self.selected_wallet_seed_hash else { return; }; - if !matches!(self.selected_source, Some(SourceSelection::CoreWallet)) - || !matches!( - self.destination_kind(), - Some(AddressKind::Shielded | AddressKind::Identity) - ) - { + if !self.asset_lock_max_amount_applies() { return; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/ui/wallets/send_screen.rs` around lines 641 - 677, Extract the shared eligibility condition from request_asset_lock_max_amount and render_asset_lock_balance_status into a helper on the same type, then call that helper from both functions. Preserve the existing CoreWallet source and Shielded/Identity destination requirements and each function’s current behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/ui/identities/add_new_identity_screen/by_using_unused_balance.rs`:
- Around line 102-121: Route the task returned by
asset_lock_balance.ensure_requested through each screen’s existing end-of-frame
batching instead of merging it directly with AppAction::BackendTask. In
src/ui/identities/add_new_identity_screen/by_using_unused_balance.rs:102-121,
enqueue it in AddNewIdentityScreen’s pending_tasks; in
src/ui/identities/top_up_identity_screen/by_using_unused_balance.rs:93-113,
enqueue it through TopUpIdentityScreen’s pending_backend_tasks_action path.
Preserve the existing loading and retry behavior.
In `@src/ui/wallets/send_screen.rs`:
- Around line 634-639: Update asset_lock_max_amount to distinguish a failed
asset-lock balance lookup from an in-progress or missing cache entry. When the
cached state has mark_loading_failed, return an error directing the user to
retry; preserve the existing “still being checked” message for loading states,
and keep successful balance handling unchanged.
In `@src/wallet_backend/payments.rs`:
- Around line 208-241: Narrow the wallet-manager write guard in
asset_lock_max_amount so it only retrieves/clones the required account, managed
account state, and current height; ensure the guard is dropped before calling
asset_lock_max_amount_from_account. Preserve the existing TaskError mappings and
use owned/cloned data as needed so the search runs without the exclusive lock.
---
Nitpick comments:
In `@src/ui/identities/add_new_identity_screen/mod.rs`:
- Around line 1061-1088: Extract the wallet-balance validation sequence from the
add-identity handler and top_up_identity_clicked into a shared helper in
funding_common.rs. The helper should perform seed-hash lookup,
asset_lock_balance retrieval, validate_asset_lock_amount, and the corresponding
warning-banner behavior, including explicit handling of a busy wallet lock;
update both callers to reuse it and preserve their existing success and
rejection flows.
In `@src/ui/wallets/send_screen.rs`:
- Around line 641-677: Extract the shared eligibility condition from
request_asset_lock_max_amount and render_asset_lock_balance_status into a helper
on the same type, then call that helper from both functions. Preserve the
existing CoreWallet source and Shielded/Identity destination requirements and
each function’s current behavior.
🪄 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 Plus
Run ID: 2cf77879-9c09-4a5f-bfdb-aafdaa08bae2
📒 Files selected for processing (17)
CHANGELOG.mdsrc/backend_task/error.rssrc/backend_task/mod.rssrc/backend_task/wallet/mod.rssrc/context/mod.rssrc/model/asset_lock.rssrc/model/mod.rssrc/ui/identities/add_new_identity_screen/by_using_unused_balance.rssrc/ui/identities/add_new_identity_screen/mod.rssrc/ui/identities/top_up_identity_screen/by_using_unused_balance.rssrc/ui/identities/top_up_identity_screen/mod.rssrc/ui/state/asset_lock_balance.rssrc/ui/state/mod.rssrc/ui/wallets/send_screen.rssrc/wallet_backend/mod.rssrc/wallet_backend/payments.rssrc/wallet_backend/snapshot.rs
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review — 19 findings (3 HIGH, 11 MEDIUM, 4 LOW, 1 INFO), 4 blocking
Three specialist reviewers went over this independently — security, project consistency, and adversarial QA. They converged, which is the part worth paying attention to.
The diagnosis is right and the direction is right. Replacing a display-only snapshot with a builder-derived ceiling is the correct fix for #929, and several details are genuinely well done: module placement obeys the strictest parts of CLAUDE.md (AssetLockBalanceCache renders nothing and correctly lives in ui/state/; model/asset_lock.rs is truly stateless), TaskError::AssetLockBalanceQueryFailed satisfies all seven error-message rules, no secret material is touched on the new path, both bisections provably terminate with no overflow and no unwrap, and — the nicest property in the whole PR — the algorithm only ever returns an amount it observed the builder accept. That means even if BranchAndBound turns out to be non-monotone, a bad seed can only make Max too low, never reintroduce the over-promise. The seed-then-bisect rewrite over the earlier hand-derived padding constant was the right call.
Unfortunately the implementation also ships two new regressions and leaves a fourth asset-lock flow on the old contract.
🔴 Blocking (4)
| ID | Finding | Where |
|---|---|---|
| SEC-001 | TooManyInputs → Rejected { available: 0 } seeds the full-range fallback's upper bound, so it returns high = 0. The wallets routed into that fallback are exactly the fragmented-UTXO wallets guaranteed to hit it — they get Max = 0 and cannot shield or fund an identity at all. Worse than the pre-PR behaviour. Untested path. |
payments.rs:89,133-158 |
| RUST-001 | The ceiling cache discards its loaded value on every snapshot publish (which fires on every SyncHeightAdvanced) and has no in-flight guard. The identity funding step blanks itself during sync; if generation outruns probe latency the ceiling never settles and the step is permanently gated on "Checking the available amount…". Meanwhile probes pile onto the wallet's global write lock. |
ui/state/asset_lock_balance.rs:29-68 |
| SEC-004 | The probe models an empty ScriptBuf credit output where the real path pays a ~25-byte script → smaller modelled tx → lower fee → quote above the real ceiling. Small magnitude, wrong sign, and it lands exactly at the boundary Max targets. Plus a hand-copied fee constant with no link to upstream. |
payments.rs:30-92 |
| PROJ-003 | CHANGELOG promises "Max always produces an amount that goes through" — an absolute guarantee the code cannot make, omitting the still-broken Fund Platform Address flow, and re-using the word "spendable" this PR spent effort demoting. Cheapest of the four to fix. | CHANGELOG.md:49-55 |
🟠 Also worth your time before this ships
PROJ-001 (HIGH) — Core → Platform-address funding is the same asset-lock bug, unmigrated. send_core_to_platform → FundPlatformAddressFromWalletUtxos → create_asset_lock_proof, yet Max and validation still read .spendable(). It isn't in the declared out-of-scope list, so #929 reproduces there verbatim while the CHANGELOG implies coverage. Fix it or scope it out loudly — fixing three of four routes silently makes the fourth look intentional.
Then: SEC-003 (ReceiveDeposit bypasses the new ceiling on the same dispatch), SEC-002 (dry-run reservation isolation rests on an unasserted upstream Clone contract, with release_reservation on 1 of ~15 exit paths — a cheap unit test settles it either way), RUST-004 (a background probe failure raises a sticky global banner and can rewind an in-flight identity wizard out of WaitingForAssetLock — one-line fix via should_suppress_backend_task_error), CODE-001 (both backend regression tests build their "real selector" reference from the probe's own constants, so neither drift surface can ever fail CI), CALL-001 (the funding screens still headline "You can use X DASH" three lines above a Max that now offers less), PROJ-002, RUST-002, RUST-003, RUST-005. All are inline.
Four LOW/INFO items are in the full report rather than inline: SEC-005 (the per-wallet generation counter restarts at 1 after forget_wallet, so a stale higher ceiling can match again — a global AtomicU64 closes this and RUST-003 together), PROJ-004 (ceiling enforcement is UI-only; MCP/CLI get no pre-check), CALL-002 (get_core_balance's new carve-out list is trustworthy-looking and already wrong; plus a name collision between the screen helper and the backend probe), DOC-001 (funding_common helpers still named and documented as taking spendable), PROJ-005 (user-stories catalog).
On verification honesty
The Bash tool was non-functional in this sandbox, so nothing was compiled, executed, linted or dependency-audited — every finding above is static analysis, and CI remains the compilation and test backstop. The upstream key-wallet / platform-wallet source isn't present either, so four upstream-contract questions could not be discharged: the real DEFAULT_FEE_PER_KB, the real credit-output script size, ManagedCoreFundsAccount's Clone semantics, and BranchAndBound monotonicity. Findings resting on those are explicitly marked unverified in-line rather than asserted as fact. I'd rather hand you a labelled uncertainty than a confident-sounding guess.
Requesting changes on the four blocking items. Everything else is a follow-up, and the underlying architecture doesn't need rethinking — it needs its edges finished.
🤖 Reviewed by Claudius the Magnificent — security-engineer-smythe (opus), project-reviewer-adams (opus), qa-engineer-marvin (sonnet). Full report: review-report/report.html.
There was a problem hiding this comment.
🟠 RUST-002 · MEDIUM — a read-only quote holds the wallet-manager write lock across an unbounded probe loop
(payments.rs:214-241, search at :133-158)
asset_lock_max_amount holds wallet.wallet_manager().write().await for the entire duration of the search even though it mutates nothing — it clones the account and probes copies. It takes a write guard purely because it reaches for get_wallet_and_info_mut.
The search length isn't bounded by a small constant either. The typical path is cheap (drain seed plus a couple of probes), but both AssetLockDrainSeed::TooManyInputs and the exhausted-doubling path fall through to full_range_asset_lock_max_amount — a binary search over 0..=MAX_MONEY, roughly 51 iterations, each a full deep clone of ManagedCoreFundsAccount including its whole UTXO map. Combined with RUST-001's re-probe on every snapshot publish, that write guard ends up contended against the very sync path producing the events that trigger the probe. A background UI convenience query can stall wallet event processing on a large wallet, repeatedly.
Also: MAX_DRAIN_SEARCH_DOUBLINGS: u32 = 40 is named but undocumented. Nothing tells the next maintainer why 40, or that 2^40 doesn't span MAX_MONEY — so the fallback is reachable by design, not by accident, and right now it reads like dead code.
Fix: use a read accessor if the upstream API offers one; otherwise take the write guard only long enough to clone the account and read the height, then run the search unlocked. Consider also hoisting the per-candidate account clone out of the loop and reusing one clone across probes — contingent on SEC-002's isolation question being settled first. And add a one-line comment on MAX_DRAIN_SEARCH_DOUBLINGS explaining the bound and that exhaustion is expected and handled.
There was a problem hiding this comment.
Real, but this is the same lock-hold-cost item already raised and accepted as a non-blocking follow-up earlier in this PR's own review. Deferring for the same reason (responsiveness/stall risk, not correctness) — will fold your specific narrow-the-guard-scope suggestion into that follow-up.
🤖 Co-authored by Claudius the Magnificent AI Agent
There was a problem hiding this comment.
🟠 RUST-005 · MEDIUM — the request/status block and the validate/report block are copy-pasted across three screens
(add_new_identity_screen/by_using_unused_balance.rs:102-126; top_up_identity_screen/by_using_unused_balance.rs:93-117; plus a third variant in send_screen.rs)
Two verbatim duplications and a third partial one.
Block A — 25 lines, byte-identical apart from the selected_wallet/wallet field name: resolve seed hash → read snapshot_generation → ensure_requested → failed branch with label and "Retry available amount check" button → not-loaded branch with "Checking the available amount…". send_screen carries a third, structurally different variant of the same logic split across request_asset_lock_max_amount and render_asset_lock_balance_status. That structural difference is not incidental — it is precisely why the three screens behave differently when the ceiling isn't loaded (RUST-001). The copies have already diverged once.
Block B — ~28 lines, verbatim: validate_asset_lock_amount(amount, 0, max_amount) → match error { Overflow => max_amount, ExceedsMaximum { maximum_amount_duffs } => maximum_amount_duffs } → MessageBanner::set_global(..) with the identical "You can transfer up to {} right now." string, duplicated between add_new_identity_screen/mod.rs:1061-1085 and top_up_identity_screen/mod.rs:481-505, with a Result<_, String> variant of the same shape in send_screen.rs:1675-1686.
Note the Overflow => max_amount fallback is hand-written at all four sites — and is unreachable at three of them, since they pass reserve_duffs = 0 and amount.checked_add(0) cannot overflow. The variant has no test coverage. An error type whose every consumer must write the same collapse is the wrong shape.
Fix:
- Extract Block A into one helper on
AssetLockBalanceCache(orfunding_common) taking&mut Uiand returning a smallReady(u64) | Pending | Failedenum — each screen then decides layout only. This is also the cheapest way to land RUST-001's stale-while-revalidate change once rather than three times. - Collapse Block B by having the model yield the user-facing maximum directly — e.g. make
AssetLockAmountErroralways carrymaximum_amount_duffs— so no caller re-derives it. Then delete theOverflowarm, or cover it with a test.
There was a problem hiding this comment.
Agreed this is duplicated across the three funding screens, and your diagnosis that it's directly why the three screens drifted in behavior is exactly right. Deferring the full extraction as a larger refactor rather than doing it under this PR's scope, but the round of fixes just pushed did bring the three screens' patterns back into alignment with each other, which reduces (doesn't eliminate) the risk you're flagging.
🤖 Co-authored by Claudius the Magnificent AI Agent
github-actions
Bot
removed
the
claudius-review
Fixes four confirmed issues in the Max-ceiling probe from CI review of PR #937 (github-actions[bot]): - A wallet with more spendable UTXOs than fit in one transaction (TooManyInputs) collapsed the Max quote to a hard 0 instead of finding the largest amount achievable from an in-cap subset. The drain-ceiling seed and the full-range fallback now both route through an input-cap-aware search: a LargestFirst probe finds a real achievable seed, nudged by one well-known P2PKH input's fee as a starting point, then verified/refined against the actual default (BranchAndBound) strategy via real builder calls — never a guessed formula. - The drain-ceiling probe's placeholder credit-output script was 0 bytes where the real path produces a ~25-byte P2PKH script, skewing the fee estimate (and therefore the quoted ceiling) in the unsafe direction. - The asset-lock balance cache discarded its last loaded value on every wallet-snapshot generation bump and had no in-flight guard, so during active sync the funding screens could get stuck on "Checking the available amount..." indefinitely. It now keeps serving the last loaded value while a refresh runs in the background (stale-while-revalidate) and never dispatches a second probe while one is already in flight. - SnapshotStore::publish cloned the whole WalletSnapshot (transaction/UTXO history) inside its rcu closure instead of bumping an Arc refcount, on every wallet event. Generation is now assigned once via a dedicated per-wallet counter before entering the closure, which only clones the Arc; an ordering guard prevents a stale generation from overwriting a newer one under retry. New test: asset_lock_max_uses_an_in_cap_subset_when_the_wallet_has_too_many_utxos (517 UTXOs, verifies the quoted max builds within the input cap and one duff more does not). Co-Authored-By: Codex Sol (gpt-5.6-sol) <noreply@openai.com> Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…iling Extends the #929 Max fix to every screen/funding-method that dispatches through the same asset-lock builder chokepoint but was left reading the display-only snapshot balance, per CI review of PR #937: - Fund Platform Address (Core wallet -> Platform address) now computes Max and validates the pre-send amount against the builder ceiling, same as Shield and Identity destinations already did. - The ReceiveDeposit funding method on both Create Identity and Top Up Identity now shares the same ceiling cache as UseWalletBalance, since both dispatch through the identical wallet-level asset-lock builder downstream. Removes the separate funding_address_balance_duffs-based check that could accept an amount the builder would reject. - Pre-selection affordability gates (wallet_can_afford_creation, wallet_balance_can_afford_top_up, and the funding-method-availability checks) now read the same ceiling as the "not enough Dash" banner they claim to match, instead of the older, larger snapshot figure. An unloaded ceiling does not block the option. - The "You can use X DASH" balance headline on both funding screens now reads the builder ceiling once loaded, falling back to the snapshot figure only while it's still loading -- it no longer shows a number larger than what Max/validation will actually accept. - A failed background asset-lock-max probe no longer raises a duplicate global error banner or resets in-flight wizard step state (confirmed: without this, a probe failure invoked step_after_task_failure on the identity screens and reset send_status on the send screen, mid-wizard). should_suppress_backend_task_error now suppresses it on all three screens; the existing inline "could not be checked" / Retry UI is the right surface for this background query. Extends core_asset_lock_max_and_validation_use_builder_quote with a Platform-destination case (Max computation and exact-boundary rejection). Co-Authored-By: Codex Sol (gpt-5.6-sol) <noreply@openai.com> Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The entry claimed Max "always produces an amount that goes through" and omitted the Fund Platform Address flow (now also fixed), and reused "spendable" -- the exact term the code comments demote in favor of the builder-verified ceiling. Names all four fixed flows, drops the absolute claim, and mentions the new checking/retry UI states. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…nce' into fix/929-max-shield-snapshot-balance
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/wallet_backend/snapshot.rs (1)
456-474: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winKeep snapshot generations monotonic across wallet forget/re-import.
forget_walletremovesgenerations[*seed_hash], so a re-imported wallet can publish generation1again.AssetLockBalanceCachecaches asset-lock max amounts byseed_hashandsnapshot_generation; after the first wallet loaded that generation, a re-import with the same generation will re-use the staleGetAssetLockMaxAmountresult instead of refetching for the freshly published state.Keep the per-wallet counter across
forget_wallet, or use a global monotonic counter, so a seed hash never reuses an old generation value.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/wallet_backend/snapshot.rs` around lines 456 - 474, Update forget_wallet so it no longer removes the seed_hash entry from generations, preserving the per-wallet generation counter across forget and re-import; alternatively, use an established global monotonic counter. Ensure a seed hash never republishes an earlier snapshot_generation value while retaining removal of the other wallet state.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/wallet_backend/snapshot.rs`:
- Around line 456-474: Update forget_wallet so it no longer removes the
seed_hash entry from generations, preserving the per-wallet generation counter
across forget and re-import; alternatively, use an established global monotonic
counter. Ensure a seed hash never republishes an earlier snapshot_generation
value while retaining removal of the other wallet state.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 3a35c7a9-ac8e-4adc-aa6a-03af74533d07
📒 Files selected for processing (11)
CHANGELOG.mdsrc/ui/identities/add_new_identity_screen/by_receive_deposit.rssrc/ui/identities/add_new_identity_screen/by_using_unused_balance.rssrc/ui/identities/add_new_identity_screen/mod.rssrc/ui/identities/top_up_identity_screen/by_receive_deposit.rssrc/ui/identities/top_up_identity_screen/by_using_unused_balance.rssrc/ui/identities/top_up_identity_screen/mod.rssrc/ui/state/asset_lock_balance.rssrc/ui/wallets/send_screen.rssrc/wallet_backend/payments.rssrc/wallet_backend/snapshot.rs
🚧 Files skipped from review as they are similar to previous changes (6)
- CHANGELOG.md
- src/ui/identities/add_new_identity_screen/by_using_unused_balance.rs
- src/wallet_backend/payments.rs
- src/ui/state/asset_lock_balance.rs
- src/ui/identities/top_up_identity_screen/mod.rs
- src/ui/wallets/send_screen.rs
…ore#918 The asset-lock max-amount seed-then-bisect probes deliberately walk toward the wallet's near-total balance, which is exactly the regime where key-wallet's BranchAndBound find_exact_match lacks an undershoot/feasibility prune (filed upstream, not fixed in this repo). Document the accepted risk at the shared dry-run chokepoint pending the upstream fix, per user direction, rather than bounding the probe here. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ment reservation sharing SEC-001: asset_lock_max_amount held the wallet-manager write lock across its entire synchronous seed-then-bisect search, causing SnapshotStore's try_read() to fall back to carry-forward publishes that bump the generation and discard the probe's own result -- a self-sustaining invalidation loop. Shrink the lock to just the initial read. CODE-002: document why dry_run_asset_lock_amount_with_strategy's release_reservation calls are load-bearing (cloning ManagedCoreFundsAccount shares its live ReservationSet) and stop calling the path "read-only" when it mutates shared reservation state. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…nd stuck in-flight requests SEC-004: a wallet whose snapshot-generation counter restarts (e.g. after SnapshotStore::forget_wallet + re-registration) was permanently frozen out of future asset-lock-max queries once the cache had observed a higher generation, comparing forever against the stale high-water mark. SEC-005: an in-flight request whose result never routed back to store()/ mark_loading_failed() permanently blocked every future query for that wallet regardless of how far the real snapshot generation advanced. Confirmed trigger: in both identity receive-deposit flows, ensure_requested() marks the probe in flight, then a same-frame AppAction replacement (the end-of-frame tracked-lock/receive-address batch) discards that dispatch before it ever reaches the backend -- the probe never runs, and nothing ever calls store()/mark_loading_failed() to release it. Fix: a snapshot-generation change now always supersedes stale in-flight/ failed tracking for the wallet -- a higher generation clears the stale markers while preserving the last displayable amount (the refresh-in- progress case), and a lower generation (a genuine counter restart) also discards the now-untrustworthy cached amount. Equal-generation requests still dedupe, and a stale response for an old generation is still ignored once a newer generation has superseded it. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
… address, fix trapped loading state
SEC-002: send_screen's Max/validation for a Core -> Identity send reserved
no fee at all (missing AddressKind::Identity match arm), and the Platform/
Identity submit-time validations checked the raw probe ceiling instead of
the same fee-reserved ceiling their own display path already computes --
a manually typed amount (not using the Max button) could pass validation
with no fee headroom. Both submit paths now recompute the same fee their
display arm uses (identity top-up fee, Platform funding-transition fee),
rounding the credits->duffs conversion up so the reserve is never
understated.
SEC-003: the ReceiveDeposit funding method's Max ceiling used the wallet-
wide live-builder probe instead of the specific deposit address's balance,
letting the input accept more than actually arrived at that address.
Added receive_deposit_ceiling_duffs() (funding_common.rs) bounding by
min(wallet probe, deposit-address balance); UseWalletBalance is unaffected.
PROJ-002: the receive-deposit flow's "Choose a different funding method"
escape hatch was unreachable while the probe was loading, had failed, or
the wallet seed hash was momentarily unavailable -- contradicting its own
doc comment ("the user is never trapped"). The button now renders on every
path through the FundsReceived step, in both identity screens.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ttest route
CODE-001: the offline kittest harness never populates AssetLockBalanceCache
(no real backend), so send_core_to_platform's new pre-flight probe check
("still being checked") short-circuited before the test's actual balance
assertion, and that assertion's expected wording was stale besides (from
before this PR's validate_asset_lock_amount rewrite).
Added a #[cfg(feature = "testing")] hook, seed_asset_lock_max_amount_for_test,
that seeds the cache via its own public ensure_requested/store API (no
reaching into private fields), threaded an optional seed amount through
assert_route(), and updated the one affected test to assert the current
"You can transfer up to ..." message. Audited every other CoreWallet-sourced
kittest route: none of them hit the Platform/Shielded/Identity destination
guard that triggers the probe, so no other test needed seeding.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
PROJ-003: the CHANGELOG's "Max now matches..." entry (3rd attempt) still
didn't match the shipped UI: it used an ASCII "..." instead of the actual
unicode ellipsis, named a generic "Retry" button instead of the real label
("Retry available amount check"), and omitted both the Identity-destination
Send-screen case (SEC-002) and the receive-deposit address bound (SEC-003)
now that they're fixed. Corrected against the actual strings in
send_screen.rs and both by_receive_deposit.rs/by_using_unused_balance.rs
pairs (all 5 call sites verified consistent).
PROJ-006: added SND-017 documenting the live-builder-verified Max/amount-
check behavior across Shield, Fund Platform Address, send-to-Identity, and
identity funding -- this had no user story despite being a materially
different mechanism from SND-014's Core-to-Core Max (simple network-fee
subtraction). IDN-014's existing acceptance criteria (deposit-bounded
prefill, never-trapped funding-method switch) already documented the
*intended* behavior correctly; Batch 3 of this remediation made the code
finally live up to it, so IDN-014 itself needed no changes.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…st display PROJ-001: both identity funding screens' dispatch-time validation called validate_asset_lock_amount(amount, 0, max_amount) -- a literal zero fee reserve -- even though their own Max button (and the equivalent send_screen paths fixed in 64cd544) already reserve the fee for display. This is a regression against origin/v1.0-dev, not just an unfinished threading: the pre-PR code at this exact dispatch point did reserve the fee (max_amount_after_fee_reserve(spendable_duffs, fee_credits)) and this PR's migration deleted that check. A manually typed amount between the fee- reserved display ceiling and the raw builder ceiling could pass validation and commit to an under-funded identity creation/top-up that then fails on Platform after the asset lock is already on L1. Both sites now recompute the same fee their display path uses (estimate_identity_create/estimate_identity_topup), rounding credits->duffs up so the reserve is never understated. AssetLockAmountError::Overflow is now reachable at these sites as a side effect (previously dead with reserve always 0). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
… its dispatch PROJ-007: SyncHeightAdvanced/ChainLockProcessed carry no UTXO deltas but still bump the snapshot generation, and Batch 2's generation-supersession fix (d02fd53) treats every bump as a reason to cancel and restart the live-builder probe -- during active sync this can mean the (potentially slow, per rust-dashcore#918) probe never completes. Debounce on the wallet's spendable balance (AssetLockBalanceCache now tracks request_spendable_duffs alongside request_generation): a generation bump with no spendable-balance change is a no-op for dispatch purposes, while a generation regression still forces a reload as before. PROJ-008: a probe dispatch merged via `action |= AppAction::BackendTask(..)` could be silently overwritten by a later same-frame action (BitOrAssign is last-write-wins), with no recovery on an idle wallet whose generation never advances. Both identity funding screens (add_new_identity_screen, top_up_identity_screen) now collect the probe dispatch, tracked-asset-lock fetches, and receive-address generation into a single end-of-frame AppAction::BackendTasks(..., Concurrent) batch via new funding_common helpers (can_append_concurrent_backend_tasks, append_concurrent_backend_tasks) instead of relying on overwrite semantics. Fixed call sites: both by_receive_deposit.rs and both by_using_unused_balance.rs (dispatch moved to the parent mod.rs render loop, gated on which funding method actually rendered this frame). Independently reviewed the full diff and re-ran fmt/test/clippy myself before committing (Codex Sol's own commit attempt failed on the sandboxed worktree's read-only git metadata, as expected). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ce cache, invalidate on network switch Holistic review of the full PR #937 diff found 3 blocking regressions inside this PR's own remediation commits, invisible to per-commit review. All 3 independently re-verified by the coordinator (direct code reads + diff review) before and after this fix. SEC-003: ReceiveDeposit dispatch-time validation checked only the wallet-wide asset-lock ceiling, contradicting the deposit-address bound CHANGELOG.md and user story SND-017 both promise unconditionally (and that the display-path Max button already enforced). Both identity screens now compute the ceiling through one shared `available_ceiling_duffs()` helper used by both the Max button and dispatch validation, so the two paths cannot diverge again. Debounce/dedup logic (SEC-004 + PROJ-008 + QA-001, all describing two flaws in `AssetLockBalanceCache::ensure_requested`): - The debounce signal (spendable = confirmed+unconfirmed) could not see a UTXO's unconfirmed-to-confirmed/InstantLock transition, since the builder only accepts confirmed-or-instantlocked inputs (require_final_inputs) -- exactly the transition the ReceiveDeposit flow depends on. Replaced with a real final-funds subtotal (`asset_lock_final_funds_duffs`, mirroring the builder's own eligibility filter) computed at snapshot-publish time and read atomically alongside the generation via a new `AppContext::asset_lock_probe_snapshot()` accessor -- this also eliminates the torn-read TOCTOU window between the old two separate `snapshot_generation()`/`snapshot_balance()` calls. - A same-generation dedup check could mask a genuine debounce-signal change once `loaded` was already set for that generation. `loaded`/ `in_flight`/`failed` now key on the `(generation, signal)` pair instead of generation alone. SEC-005: both identity screens kept their asset-lock ceiling cache across a network switch (WalletSeedHash is network-independent) -- a regression against the pre-PR live per-network snapshot_balance() read. Both screens now get explicit change_context handling (reset_for_network_switch(), mirroring WalletSendScreen) plus refresh()/refresh_on_arrival() overrides that invalidate the cache. Independently reviewed the full 9-file diff by hand and re-ran the verification sweep myself with forced genuine recompiles (fmt clean, 7/7 asset_lock_balance tests incl. 2 new regressions, 43/43 identities tests, 22/22 send_screen tests, 12/12 kittest, clippy clean) before committing -- Codex's own commit attempt failed on the sandboxed worktree's read-only git metadata, as expected. Noted, not blocking: unifying both identity screens onto the shared available_ceiling_duffs() helper means top_up_identity_screen lost a more specific "wallet is busy" banner for a rare lock-contention case, falling back to the generic "still being checked" message like add_new_identity_screen already did -- a minor UX nuance, not a correctness regression (PROJ-004, already tracked as non-blocking). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…h a timeout Holistic review (SEC-001): the asset-lock ceiling probe now runs automatically on every funding-screen render, converting the accepted upstream BnB algorithmic-complexity risk (dashpay/rust-dashcore#918) from rare/user-initiated into routine/automatic/uncancellable/remotely re-triggerable. Rust cannot forcibly kill a spawn_blocking thread, so this adds cooperative cancellation instead: a CancellationToken checked between search-loop iterations, cancelled by a 5s timer. On timeout the caller gets a distinguishable TaskError with an actionable message rather than an indefinite silent wait. Deliberately scoped: does not touch AssetLockBalanceCache (a separate, concurrent commit rewrote that file) or the accepted-risk annotation itself. Cancel-on-navigate-away and single-flight-per-wallet dedup are deferred follow-ups using the same token, not built here. The timeout message wording is draft and needs a copy pass before shipping. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The Max-fix entry described the safe-Max check but not its new worst-case bound (5s timeout, added to contain an accepted upstream BnB algorithmic-complexity risk). Documents the user-visible outcome for very large wallets now that the fix is complete. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@CHANGELOG.md`:
- Around line 66-75: Revise the changelog entry’s sendability claims to describe
Max and validation as using a builder-derived, point-in-time ceiling, without
guaranteeing the transaction will remain sendable. Remove or soften “can
actually send,” “stay in agreement,” and “never” absolute wording while
preserving the listed flow coverage and deposit-address cap behavior.
In `@src/ui/identities/top_up_identity_screen/by_receive_deposit.rs`:
- Around line 174-182: Update the asset-lock failure handling around is_failed
and invalidate_one to emit the error through MessageBanner once when the failure
state is recorded, while preserving duplicate-banner suppression. Keep the
existing retry and “Choose a different funding method” buttons and return
behavior unchanged, replacing the inline ui.label notification with the
established MessageBanner mechanism.
🪄 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 Plus
Run ID: 20944da4-eaac-498c-a58f-bfbbe81f89b4
📒 Files selected for processing (19)
CHANGELOG.mddocs/user-stories.mdsrc/backend_task/error.rssrc/context/mod.rssrc/context/test_support.rssrc/ui/identities/add_new_identity_screen/by_receive_deposit.rssrc/ui/identities/add_new_identity_screen/by_using_unused_balance.rssrc/ui/identities/add_new_identity_screen/mod.rssrc/ui/identities/funding_common.rssrc/ui/identities/top_up_identity_screen/by_receive_deposit.rssrc/ui/identities/top_up_identity_screen/by_using_unused_balance.rssrc/ui/identities/top_up_identity_screen/mod.rssrc/ui/mod.rssrc/ui/state/asset_lock_balance.rssrc/ui/wallets/send_screen.rssrc/wallet_backend/mod.rssrc/wallet_backend/payments.rssrc/wallet_backend/snapshot.rstests/kittest/send_screen.rs
💤 Files with no reviewable changes (2)
- src/ui/identities/add_new_identity_screen/by_using_unused_balance.rs
- src/ui/identities/top_up_identity_screen/by_using_unused_balance.rs
🚧 Files skipped from review as they are similar to previous changes (7)
- src/wallet_backend/mod.rs
- src/ui/identities/add_new_identity_screen/by_receive_deposit.rs
- src/wallet_backend/payments.rs
- src/context/mod.rs
- src/ui/identities/top_up_identity_screen/mod.rs
- src/ui/identities/add_new_identity_screen/mod.rs
- src/ui/wallets/send_screen.rs
| if self.asset_lock_balance.is_failed(&seed_hash) { | ||
| ui.label("The available amount could not be checked."); | ||
| if ui.button("Retry available amount check").clicked() { | ||
| self.asset_lock_balance.invalidate_one(&seed_hash); | ||
| } | ||
| if ui.button("Choose a different funding method").clicked() { | ||
| self.reset_to_choose_funding(); | ||
| } | ||
| return action; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Route the failed-check notification through MessageBanner.
This new user-facing error path only renders an inline label. Emit the error through MessageBanner once when the failure state is recorded, while retaining the retry button and duplicate-banner suppression.
As per coding guidelines, UI errors must use MessageBanner.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/ui/identities/top_up_identity_screen/by_receive_deposit.rs` around lines
174 - 182, Update the asset-lock failure handling around is_failed and
invalidate_one to emit the error through MessageBanner once when the failure
state is recorded, while preserving duplicate-banner suppression. Keep the
existing retry and “Choose a different funding method” buttons and return
behavior unchanged, replacing the inline ui.label notification with the
established MessageBanner mechanism.
Source: Coding guidelines
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The builder-derived ceiling now covers the intended funding flows and most prior findings are fixed, but this head still has three implementation blockers: stale ceilings remain authoritative after wallet-input changes, dry-run probes race real builders through the shared reservation set, and the five-second timer cannot stop or bound an active selector call. The changelog also promises guarantees contradicted by those paths; two smaller message and terminology issues remain.
Validated blockers were found in the Codex precheck. Sonnet is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— rust-quality (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 4 blocking | 🟡 1 suggestion(s) | 💬 1 nitpick(s)
🤖 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/ui/state/asset_lock_balance.rs`:
- [BLOCKING] src/ui/state/asset_lock_balance.rs:43-64: Wallet-input changes can leave a stale ceiling authoritative
When `final_funds_duffs` changes, `ensure_requested` starts a replacement query but deliberately retains `state.loaded`. `get()` therefore keeps returning the previous ceiling, and every affected flow uses that value not only for display but also for Max and dispatch-time validation. If final funds fall from 1 DASH to 0.5 DASH, a cached 0.9-DASH ceiling can still authorize a transaction while the refresh runs. The debounce key also uses only the aggregate final-funds sum, so a newer snapshot with different UTXO count, value distribution, outpoints, or reservations but the same subtotal is ignored indefinitely even though those properties affect BranchAndBound, fees, and the 500-input cap. Keep stale data display-only, expose a separate current authoritative value for validation, and key refreshes on selector-relevant input state rather than only the subtotal.
In `src/wallet_backend/payments.rs`:
- [BLOCKING] src/wallet_backend/payments.rs:502-537: Dry-run probes can clear reservations owned by real sends
`ManagedCoreFundsAccount::clone` copies the UTXO map but shares the live `ReservationSet` through `Arc<Mutex<_>>`. The probe drops the wallet-manager guard before running `set_funding` through `build_unsigned`, even though the pinned key-wallet implementation explicitly requires that sequence to run under one uninterrupted wallet-lock hold. A probe can observe an input as free, a real builder can then select and reserve the same input under the wallet lock, and the probe can subsequently reserve and unconditionally release that outpoint. Because `ReservationSet` is an ownership-free `HashMap`, the release deletes the real transaction's reservation too, allowing a later build to reuse an in-flight input. Run probes in the same synchronization domain as real builders or use an upstream quote path with a detached, non-reserving reservation set.
- [BLOCKING] src/wallet_backend/payments.rs:522-543: The five-second timer does not bound an active selector call
The timer only cancels a cooperative token, while `asset_lock_max_amount` still awaits the `spawn_blocking` handle before checking that token. Cancellation is checked between dry-run builds, but the known exponential work occurs inside a single synchronous `build_unsigned()` call, and the pinned BranchAndBound recursion has no cancellation check. One selector invocation can therefore run far beyond five seconds, leaving the UI query in flight and occupying a blocking-pool thread until it finishes. An async timeout around the join handle would at least restore the UI promptly, but the blocking work would continue in the background; fully enforcing the bound requires a cancellable or bounded upstream selector, or avoiding unbounded BranchAndBound on this automatic path.
In `CHANGELOG.md`:
- [BLOCKING] CHANGELOG.md:66-82: The changelog overpromises absolute sendability
The entry says Max matches what the wallet can actually send, Max and validation stay in agreement, and the large-wallet check is capped at a few seconds. The cache currently validates against stale ceilings after wallet-input changes, and the timeout still waits for an active BranchAndBound call to finish. A builder quote is also inherently a point-in-time result whose reservation state can change before dispatch. Rephrase the changelog and SND-017 as a builder-derived point-in-time ceiling that reduces over-limit attempts, and claim a bounded wait only after the backend actually returns at the deadline.
In `src/backend_task/error.rs`:
- [SUGGESTION] src/backend_task/error.rs:238-242: Timeout guidance recommends an action the UI still blocks
After this error, each screen marks the quote request as failed, and all affected dispatch paths still require `asset_lock_balance.get(...)` to return a loaded ceiling. Entering a smaller amount cannot bypass that prerequisite, so the suggested action cannot send anything. Direct users to the visible Retry control or to wait and retry the check.
| let generation_restarted = snapshot_generation < state.request_generation; | ||
| let final_funds_changed = final_funds_duffs != state.request_final_funds_duffs; | ||
| if generation_restarted || final_funds_changed { | ||
| state.request_generation = snapshot_generation; | ||
| state.request_final_funds_duffs = final_funds_duffs; | ||
| state.in_flight = None; | ||
| state.failed = None; | ||
| if generation_restarted { | ||
| state.loaded = None; | ||
| } | ||
| } else if snapshot_generation != state.request_generation { | ||
| return None; | ||
| } | ||
| let request_key = (snapshot_generation, final_funds_duffs); | ||
| if state.in_flight == Some(request_key) | ||
| || state.loaded.is_some_and(|(generation, final_funds, _)| { | ||
| (generation, final_funds) == request_key | ||
| }) | ||
| || state.failed == Some(request_key) | ||
| { | ||
| return None; | ||
| } |
There was a problem hiding this comment.
🔴 Blocking: Wallet-input changes can leave a stale ceiling authoritative
When final_funds_duffs changes, ensure_requested starts a replacement query but deliberately retains state.loaded. get() therefore keeps returning the previous ceiling, and every affected flow uses that value not only for display but also for Max and dispatch-time validation. If final funds fall from 1 DASH to 0.5 DASH, a cached 0.9-DASH ceiling can still authorize a transaction while the refresh runs. The debounce key also uses only the aggregate final-funds sum, so a newer snapshot with different UTXO count, value distribution, outpoints, or reservations but the same subtotal is ignored indefinitely even though those properties affect BranchAndBound, fees, and the 500-input cap. Keep stale data display-only, expose a separate current authoritative value for validation, and key refreshes on selector-relevant input state rather than only the subtotal.
source: ['codex']
There was a problem hiding this comment.
Resolved in 33622bb — Wallet-input changes can leave a stale ceiling authoritative no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
| let (managed_account, account, current_height) = { | ||
| let wallet_manager = wallet.wallet_manager().read().await; | ||
| let (key_wallet, info) = wallet_manager | ||
| .get_wallet_and_info(&wallet_id) | ||
| .ok_or(TaskError::WalletStateInconsistent)?; | ||
| let account = key_wallet | ||
| .get_bip44_account(DEFAULT_BIP44_ACCOUNT) | ||
| .ok_or(TaskError::WalletStateInconsistent)? | ||
| .clone(); | ||
| let current_height = asset_lock_builder_height(&info.core_wallet); | ||
| let managed_account = info | ||
| .core_wallet | ||
| .accounts | ||
| .standard_bip44_accounts | ||
| .get(&DEFAULT_BIP44_ACCOUNT) | ||
| .ok_or(TaskError::WalletStateInconsistent)? | ||
| .clone(); | ||
| (managed_account, account, current_height) | ||
| }; | ||
|
|
||
| let cancellation_token = CancellationToken::new(); | ||
| let timer_token = cancellation_token.clone(); | ||
| tokio::spawn(async move { | ||
| tokio::time::sleep(ASSET_LOCK_PROBE_TIMEOUT).await; | ||
| timer_token.cancel(); | ||
| }); | ||
|
|
||
| let probe_token = cancellation_token.clone(); | ||
| let result = tokio::task::spawn_blocking(move || { | ||
| asset_lock_max_amount_from_account( | ||
| &managed_account, | ||
| &account, | ||
| current_height, | ||
| &probe_token, | ||
| ) | ||
| }) |
There was a problem hiding this comment.
🔴 Blocking: Dry-run probes can clear reservations owned by real sends
ManagedCoreFundsAccount::clone copies the UTXO map but shares the live ReservationSet through Arc<Mutex<_>>. The probe drops the wallet-manager guard before running set_funding through build_unsigned, even though the pinned key-wallet implementation explicitly requires that sequence to run under one uninterrupted wallet-lock hold. A probe can observe an input as free, a real builder can then select and reserve the same input under the wallet lock, and the probe can subsequently reserve and unconditionally release that outpoint. Because ReservationSet is an ownership-free HashMap, the release deletes the real transaction's reservation too, allowing a later build to reuse an in-flight input. Run probes in the same synchronization domain as real builders or use an upstream quote path with a detached, non-reserving reservation set.
source: ['codex']
There was a problem hiding this comment.
Resolved in 33622bb — Dry-run probes can clear reservations owned by real sends no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
| let cancellation_token = CancellationToken::new(); | ||
| let timer_token = cancellation_token.clone(); | ||
| tokio::spawn(async move { | ||
| tokio::time::sleep(ASSET_LOCK_PROBE_TIMEOUT).await; | ||
| timer_token.cancel(); | ||
| }); | ||
|
|
||
| let probe_token = cancellation_token.clone(); | ||
| let result = tokio::task::spawn_blocking(move || { | ||
| asset_lock_max_amount_from_account( | ||
| &managed_account, | ||
| &account, | ||
| current_height, | ||
| &probe_token, | ||
| ) | ||
| }) | ||
| .await?; | ||
| if cancellation_token.is_cancelled() { | ||
| return Err(TaskError::AssetLockMaxAmountTimedOut { | ||
| seed_hash: *seed_hash, | ||
| }); | ||
| } |
There was a problem hiding this comment.
🔴 Blocking: The five-second timer does not bound an active selector call
The timer only cancels a cooperative token, while asset_lock_max_amount still awaits the spawn_blocking handle before checking that token. Cancellation is checked between dry-run builds, but the known exponential work occurs inside a single synchronous build_unsigned() call, and the pinned BranchAndBound recursion has no cancellation check. One selector invocation can therefore run far beyond five seconds, leaving the UI query in flight and occupying a blocking-pool thread until it finishes. An async timeout around the join handle would at least restore the UI promptly, but the blocking work would continue in the background; fully enforcing the bound requires a cancellable or bounded upstream selector, or avoiding unbounded BranchAndBound on this automatic path.
source: ['codex']
There was a problem hiding this comment.
Resolved in 33622bb — The five-second timer does not bound an active selector call no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
| - **"Max" now matches what your Core wallet can actually send**: pressing | ||
| "Max" when shielding DASH, funding a Platform address, sending directly to | ||
| an identity, or funding an identity (creating or topping up, from your | ||
| wallet balance or a received deposit) could suggest an amount larger than | ||
| the wallet could actually send, so the transaction was rejected no matter | ||
| how you adjusted it. Max and the amount check now ask the wallet directly | ||
| what it can send instead of estimating from an on-screen balance, so the | ||
| two stay in agreement, and both reserve room for the fee. Funding from a | ||
| received deposit is also now capped by what actually arrived at that | ||
| deposit address, never by unrelated funds elsewhere in the wallet. While | ||
| the check is running, the amount field shows "Checking the available | ||
| amount…"; if it fails, "The available amount could not be checked." | ||
| appears with a "Retry available amount check" button, and you can still | ||
| switch to a different funding method at any point. For a very large | ||
| wallet, this check is capped at a few seconds; if it can't finish in | ||
| time, the amount field explains that and suggests sending a smaller | ||
| amount or trying again shortly. |
There was a problem hiding this comment.
🔴 Blocking: The changelog overpromises absolute sendability
The entry says Max matches what the wallet can actually send, Max and validation stay in agreement, and the large-wallet check is capped at a few seconds. The cache currently validates against stale ceilings after wallet-input changes, and the timeout still waits for an active BranchAndBound call to finish. A builder quote is also inherently a point-in-time result whose reservation state can change before dispatch. Rephrase the changelog and SND-017 as a builder-derived point-in-time ceiling that reduces over-limit attempts, and claim a bounded wait only after the backend actually returns at the deadline.
source: ['codex']
| /// The non-broadcasting asset-lock builder probe exceeded its bounded wait. | ||
| #[error( | ||
| "We couldn't work out the maximum amount you can send from this wallet in time. Try sending a smaller amount, or wait a few minutes and try again." | ||
| )] | ||
| AssetLockMaxAmountTimedOut { seed_hash: WalletSeedHash }, |
There was a problem hiding this comment.
🟡 Suggestion: Timeout guidance recommends an action the UI still blocks
After this error, each screen marks the quote request as failed, and all affected dispatch paths still require asset_lock_balance.get(...) to return a loaded ceiling. Entering a smaller amount cannot bypass that prerequisite, so the suggested action cannot send anything. Direct users to the visible Retry control or to wait and retry the check.
| /// The non-broadcasting asset-lock builder probe exceeded its bounded wait. | |
| #[error( | |
| "We couldn't work out the maximum amount you can send from this wallet in time. Try sending a smaller amount, or wait a few minutes and try again." | |
| )] | |
| AssetLockMaxAmountTimedOut { seed_hash: WalletSeedHash }, | |
| /// The non-broadcasting asset-lock builder probe exceeded its bounded wait. | |
| #[error( | |
| "We couldn't check how much this wallet can send in time. Select \"Retry available amount check\", or wait a few minutes and try again." | |
| )] | |
| AssetLockMaxAmountTimedOut { seed_hash: WalletSeedHash }, |
source: ['codex']
There was a problem hiding this comment.
Resolved in 33622bb — Timeout guidance recommends an action the UI still blocks no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
| .saturating_sub(fee_credits) | ||
| } |
There was a problem hiding this comment.
💬 Nitpick: Funding helpers still describe builder ceilings as spendable balances
max_amount_after_fee_reserve and spendable_covers_minimum still name their inputs spendable_duffs and document them as wallet subtotals, while current callers also pass builder-derived ceilings and deposit-address bounds. The statement that the value never exceeds what the wallet can actually send is also too strong for a cached point-in-time quote. Rename these inputs around an available ceiling and describe the helper as domain-neutral arithmetic so future callers are not encouraged to restore DetWalletBalance::spendable() as an authoritative funding gate.
source: ['codex']
…ation and reservation-TOCTOU gaps Round-5 remediation of PR #937, following a bot review (thepastaclaw) that found three unresolved issues in round 4's fixes: - Remove the CancellationToken/5s-timeout workaround added to bound the asset-lock probe's worst-case wait against the upstream BranchAndBound O(2^N) risk (rust-dashcore#918). The root cause is now fixed upstream (rust-dashcore#919's feasibility/undershoot prune, confirmed present on the dash-evo-tool integration branch platform is being re-pinned to in a follow-up commit) so the client-side workaround is unnecessary complexity. A TODO(upstream- pin) comment flags that this assumes the pin bump lands with it. - Close a stale-cache-serves-validation gap: AssetLockBalanceCache deduped re-queries only on the wallet's aggregate spendable subtotal, so a UTXO composition change that didn't move the subtotal could leave pre-send validation trusting a stale builder-verified ceiling. WalletSnapshot now tracks a canonical signature of eligible asset-lock inputs and a revision counter that advances only when that exact set changes; validation call sites (Shield, Core->Identity, Fund Platform Address, Create/Top- Up Identity) now require a quote matching the current UTXO revision via a new get_current(), while display keeps its existing stale-while-revalidate behavior unchanged. - Close a reservation-release TOCTOU race: the probe previously held only a brief read lock to clone the account, then ran its whole dry-run search with no lock held at all, racing against real sends (which correctly hold a write lock across their own select- reserve). A concurrent real send and probe iteration could both reserve the same UTXO via the shared ReservationSet, and the probe's cleanup could then release the real send's live reservation out from under it. The probe now holds an owned wallet-manager write guard for its entire search, matching the real-send path's locking discipline. Independently re-verified: all touched test suites pass with real test names visible in the log (payments, asset_lock_balance, send_screen, identities, snapshot), clippy clean, no test silently removed beyond the one now-obsolete pre-cancellation test. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…-prune fix Re-pin dash-sdk, rs-sdk-trusted-context-provider, platform-wallet, and platform-wallet-storage from 288a6cae4f9653d6085d2b3d6c7410210a0c95ba to a18bd1586858ef680124e150caad6a7dc21d0b64 (feat/platform-wallet- storage-rehydration tip), whose key-wallet dependency now tracks rust-dashcore's dash-evo-tool integration branch (rev 34f0921e) — confirmed to include rust-dashcore#919's suffix-sum feasibility/ undershoot prune for BranchAndBound coin selection, closing #918. This makes the prior commit's TODO(upstream-pin) assumption real: the asset-lock probe's worst-case search is now algorithmically bounded upstream, matching why this PR's earlier client-side timeout workaround was removed instead of kept. The initially-targeted branch tip (4ca05f51) did not compile — a signature drift between key-wallet's build_asset_lock_with_signer and platform-wallet's own call site — so this bump waited for that to be fixed upstream before landing here. Verified: full workspace test suite (2472 passed, 0 failed, +2 from round 5's own new tests, no other count changes), doc tests (7 passed), cargo fmt clean, clippy --all-features --all-targets clean. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…entry The "upstream search-complexity risk... fixed at its source" sentence was internal-implementation language that broke this entry's otherwise plain, user-observable-behavior style (per CLAUDE.md's error-message/ i18n-ready string conventions, which this changelog entry otherwise follows closely). Removed; the entry now ends on the last user-visible behavior (Checking/Retry banner), same as before the timeout workaround was ever added. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/ui/identities/top_up_identity_screen/mod.rs (1)
584-622: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winUnit mismatch between the Max reserve here and the dispatch validation.
available_ceiling_duffsis in duffs, butestimate_identity_topup()returns credits and is passed straight intomax_amount_after_fee_reserve— whereastop_up_identity_clicked(Line 531) converts the same estimate with.div_ceil(CREDITS_PER_DUFF)before validating against the same ceiling. The resultingmaxis then fed toAmount::dash_from_credits, treating a duffs value as credits. Ifmax_amount_after_fee_reservedoes not itself normalise units, the Max button reserves ~1000× the intended fee and displays the wrong amount, and Max/validation disagree on the same ceiling.🐛 Likely fix
- let estimated_fee = fee_estimator.estimate_identity_topup(); - let max_with_fee_reserved = - max_amount_after_fee_reserve(available_ceiling_duffs, estimated_fee); + let estimated_fee = fee_estimator.estimate_identity_topup(); + let estimated_fee_duffs = estimated_fee.div_ceil(CREDITS_PER_DUFF); + let max_with_fee_reserved = + max_amount_after_fee_reserve(available_ceiling_duffs, estimated_fee_duffs);#!/bin/bash # What units does the helper expect, and how do other callers pass the fee? rg -nP --type=rust -B4 -A20 'fn max_amount_after_fee_reserve' rg -nP --type=rust -C3 'max_amount_after_fee_reserve\('🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/ui/identities/top_up_identity_screen/mod.rs` around lines 584 - 622, Update the max-amount calculation in the funding amount setup to convert estimate_identity_topup() from credits to duffs using the same ceil conversion as top_up_identity_clicked before passing it to max_amount_after_fee_reserve. Keep available_ceiling_duffs, the helper’s expected units, and Amount::dash_from_credits consistent so the Max button and dispatch validation use the same fee-reserved ceiling.src/ui/state/asset_lock_balance.rs (1)
80-141: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftResults are matched on
snapshot_generationalone, so a superseded reply can be labelled with the new request's key.
ensure_requestedcan re-arm at the same generation whenfinal_funds_duffsorutxo_revisionchange (your own test at Line 380 does exactly this, and aforget_wallet-driven generation restart makes the number recur in production). When that happens,in_flightalready holds the new triple, but a late reply from the superseded probe still carries generation 7 — it passes thein_flight_generation == snapshot_generationcheck, and the amount is then written intoloadedtagged with the new triple.get_currentwill happily serve that stale ceiling for the new composition, which is the exact failure mode this PR closes elsewhere.mark_loading_failedmislabels the same way.Carry the full request identity (or a monotonic per-cache request id) through the task and its result so
store/mark_loading_failedcan reject replies that do not belong to the current in-flight request.#!/bin/bash # What identity does the task/result envelope carry today? rg -nP --type=rust -C6 'GetAssetLockMaxAmount|AssetLockMaxAmount\s*\{'🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/ui/state/asset_lock_balance.rs` around lines 80 - 141, Update the wallet query task/result flow and the AssetLockBalance methods store and mark_loading_failed to carry and validate the full request identity, including snapshot_generation, final_funds_duffs, and utxo_revision, or a monotonic request ID. Reject replies whose identity differs from the current in_flight request, including same-generation requests re-armed with changed funds or revision, and only clear in_flight or update loaded/failed after an exact match.src/ui/identities/add_new_identity_screen/mod.rs (1)
1135-1167: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDispatch-time warning doesn't distinguish a failed probe from a still-loading one.
See consolidated comment below (shared root cause with
send_screen.rs).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/ui/identities/add_new_identity_screen/mod.rs` around lines 1135 - 1167, Update the dispatch path around current_validation_ceiling_duffs so it distinguishes an unavailable/failed validation probe from a probe that is still loading. Show the “still being checked” warning only for the loading state, and handle the failed-probe state with its appropriate failure behavior instead of treating both as None.
🧹 Nitpick comments (2)
src/wallet_backend/snapshot.rs (1)
97-99: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winConsider storing a fingerprint rather than the full eligible-input vector.
asset_lock_input_signatureretains one(OutPoint, u64)entry per eligible UTXO in every published snapshot, plus a second copy inSnapshotRevision, andpublishdoes a full-vector compare + clone on every wallet event (andcarried_forward_stateclones it again). For wallets with large UTXO sets this is a per-event allocation on the publish hot path the module header advertises as cheap. A 64/128-bit fingerprint of the sorted pairs gives the same change-detection semantics with a constant-size field.♻️ Sketch
- asset_lock_input_signature: Vec<(OutPoint, u64)>, + /// Order-independent fingerprint of the eligible `(outpoint, value)` set. + asset_lock_input_fingerprint: u64,
asset_lock_final_inputswould then hash the sorted pairs (e.g. viaDefaultHasher/blake3) and return(total, fingerprint), andpublishcompares twou64s.Also applies to: 750-768
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/wallet_backend/snapshot.rs` around lines 97 - 99, Replace the full `asset_lock_input_signature` vector with a constant-size fingerprint of the sorted eligible `(OutPoint, u64)` pairs. Update `asset_lock_final_inputs` to return the total and fingerprint, then adjust `SnapshotRevision`, `publish`, and `carried_forward_state` to store and compare the fingerprint while preserving the existing change-detection semantics.src/ui/identities/add_new_identity_screen/mod.rs (1)
1897-2033: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing test: stale
utxo_revisionrejection at identity-registration dispatch.
send_screen.rsaddedcore_asset_lock_validation_rejects_quote_for_stale_utxo_compositionto proveget_current-backed validation rejects a quote computed for a different UTXO composition.current_validation_ceiling_duffshere exercises the exact same gate forregister_identity_clicked, but no equivalent test exists in this file.🧪 Suggested test sketch (mirrors the send_screen.rs test)
#[test] fn register_identity_dispatch_rejects_quote_for_stale_utxo_composition() { // seed asset_lock_balance with `ensure_requested(seed_hash, gen, final_funds, stale_revision)` // where stale_revision != current asset_lock_probe_snapshot revision, then `store(...)`. // Assert register_identity_clicked(FundingMethod::UseWalletBalance) returns AppAction::None // and the warning banner text mentions "still being checked". }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/ui/identities/add_new_identity_screen/mod.rs` around lines 1897 - 2033, Add a test in the funding_method_tests module covering register_identity_clicked with FundingMethod::UseWalletBalance when asset_lock_balance was stored using a stale utxo_revision. Seed the quote via ensure_requested with a revision different from the current asset_lock_probe_snapshot, store the ceiling, then assert dispatch returns AppAction::None and the warning banner contains “still being checked,” mirroring the existing stale-composition validation test.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/ui/identities/add_new_identity_screen/mod.rs`:
- Around line 1135-1167: Update the dispatch path around
current_validation_ceiling_duffs so it distinguishes an unavailable/failed
validation probe from a probe that is still loading. Show the “still being
checked” warning only for the loading state, and handle the failed-probe state
with its appropriate failure behavior instead of treating both as None.
In `@src/ui/identities/top_up_identity_screen/mod.rs`:
- Around line 584-622: Update the max-amount calculation in the funding amount
setup to convert estimate_identity_topup() from credits to duffs using the same
ceil conversion as top_up_identity_clicked before passing it to
max_amount_after_fee_reserve. Keep available_ceiling_duffs, the helper’s
expected units, and Amount::dash_from_credits consistent so the Max button and
dispatch validation use the same fee-reserved ceiling.
In `@src/ui/state/asset_lock_balance.rs`:
- Around line 80-141: Update the wallet query task/result flow and the
AssetLockBalance methods store and mark_loading_failed to carry and validate the
full request identity, including snapshot_generation, final_funds_duffs, and
utxo_revision, or a monotonic request ID. Reject replies whose identity differs
from the current in_flight request, including same-generation requests re-armed
with changed funds or revision, and only clear in_flight or update loaded/failed
after an exact match.
---
Nitpick comments:
In `@src/ui/identities/add_new_identity_screen/mod.rs`:
- Around line 1897-2033: Add a test in the funding_method_tests module covering
register_identity_clicked with FundingMethod::UseWalletBalance when
asset_lock_balance was stored using a stale utxo_revision. Seed the quote via
ensure_requested with a revision different from the current
asset_lock_probe_snapshot, store the ceiling, then assert dispatch returns
AppAction::None and the warning banner contains “still being checked,” mirroring
the existing stale-composition validation test.
In `@src/wallet_backend/snapshot.rs`:
- Around line 97-99: Replace the full `asset_lock_input_signature` vector with a
constant-size fingerprint of the sorted eligible `(OutPoint, u64)` pairs. Update
`asset_lock_final_inputs` to return the total and fingerprint, then adjust
`SnapshotRevision`, `publish`, and `carried_forward_state` to store and compare
the fingerprint while preserving the existing change-detection semantics.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: e02cae49-b3c1-4ac4-b070-a75e390bb36c
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (11)
CHANGELOG.mdCargo.tomlsrc/backend_task/error.rssrc/context/mod.rssrc/ui/identities/add_new_identity_screen/mod.rssrc/ui/identities/top_up_identity_screen/mod.rssrc/ui/state/asset_lock_balance.rssrc/ui/wallets/send_screen.rssrc/wallet_backend/mod.rssrc/wallet_backend/payments.rssrc/wallet_backend/snapshot.rs
💤 Files with no reviewable changes (1)
- src/backend_task/error.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- CHANGELOG.md
…nguish failed from loading, fingerprint UTXO composition Round-6 remediation of PR #937, following a fresh CodeRabbit review of round 5's own diff. Four findings independently verified against the code before fixing (a fifth, a claimed unit mismatch in top_up_identity_screen.rs, was verified to be a false positive and is untouched — max_amount_after_fee_reserve's duffs/credits split is intentional and correct). - Close a stale-probe-reply mismatch race: AssetLockBalanceCache's store()/mark_loading_failed() matched an incoming reply against the cache's *current* in-flight request using only snapshot_generation. Because ensure_requested can legitimately re-arm at the same generation when UTXO composition changes (existing, tested behavior), a superseded probe's stale reply could be silently mislabeled as the current request's result once it arrived late - reopening the exact stale-ceiling-authorizes-a-send risk round 5 closed, via a different mechanism. Every probe now carries a monotonic per-cache request_id threaded through WalletTask, BackendTaskContext, and BackendTaskSuccessResult, so a reply can only be applied to the exact request it belongs to. - Distinguish a permanently failed probe from one still loading at dispatch time, consistently across Send, Create Identity, and Top Up Identity - previously all three always showed "still being checked" even when the cache had already given up and was waiting on an explicit Retry. - Add the stale-UTXO-composition dispatch-rejection test to both identity screens, mirroring send_screen.rs's existing coverage. - Replace snapshot.rs's per-event Vec<(OutPoint, u64)> UTXO signature (cloned and compared on every wallet event) with a constant-size, order-independent u64 fingerprint of the same set. Independently re-verified: full diff read against the actual code (not the diff's own claims), all touched test suites re-run for real through the verification wrapper, no test silently removed or weakened. Extended the round-6 fingerprint-order test with a distinct-sets assertion (two different UTXO sets must not fingerprint identically) that the initial diff omitted. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Reviewed the 5 "outside diff range" findings from the latest review (round 5's diff) individually against the code before acting on any of them: Fixed in
Not fixed — verified false positive: All four fixes independently re-verified (diff read against actual code, not the fix's own claims) before committing; full targeted test suites re-run for real, no test silently removed or weakened. |
…k-hold change Round 5 fixed the reservation-release TOCTOU race by holding the wallet manager's write lock for the probe's full search, but introduced two second-order regressions a holistic review caught: the freshness signal never accounted for reservations (SEC-001), and the unbounded lock hold froze the same freshness recompute it should have protected while also stalling real sends (SEC-002). A third finding showed the Send screen's AppAction::BitOrAssign overwrite could silently drop a probe dispatch or navigation action queued in the same frame (PROJ-001), and a fourth showed a dropped probe reply left the UI stuck loading with no retry affordance (SEC-004). - snapshot.rs: asset_lock probes now clone the live ManagedCoreFundsAccount (sharing its Arc<Mutex<..>> ReservationSet) and drive it through the real TransactionBuilder::set_funding/build_unsigned path, so a concurrent send's reservation is excluded from the probe exactly as it would be from a real build - not a parallel reimplementation of the eligibility predicate. Replaces the DefaultHasher-based fingerprint with exact AssetLockInputState comparison. - payments.rs: adds ProbeDeadline, checked between each individually- bounded builder call across the search functions, bounding worst-case wallet-lock hold to ~5s; on expiry returns a proven-safe lower bound and marks the quote is_partial rather than fabricating a value. - asset_lock_balance.rs: AssetLockBalanceCache now validates replies against the OBSERVED input composition returned by the probe rather than dispatch-time snapshot metadata, so freshness is correct even when background recompute lags behind. Adds a 15s in-flight reply deadline that redispatches with a fresh request_id and offers Retry while still loading, so a dropped reply can no longer wedge the UI. - funding_common.rs / ui/mod.rs / send_screen.rs: relocates the concurrent-task-append helper out of ui::identities so the Send screen's probe-dispatch and confirmation-dialog call sites route around AppAction::BitOrAssign's overwrite instead of through it. - Bundled cheap fixes: aligns send_core_to_shielded's Overflow wording with the other three dispatch-validation sites, corrects CHANGELOG.md and docs/user-stories.md to scope the "Max" fix to the Simple builder-driven form (the Advanced manual-input Platform-address path remains governed by the Core inputs the user selects), adds coverage for cache invalidation and reservation-aware revision changes. Independently verified: read the reservation-aware predicate against the pinned key-wallet checkout to confirm SEC-001 is fully closed (not partially), confirmed the new tests reproduce each original bug against pre-round-7 code, and re-ran the full workspace suite for real (2487 passed, 0 failed - exactly baseline 2480 + 7 new tests). fmt and clippy clean. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…d-snapshot-balance # Conflicts: # Cargo.lock # Cargo.toml
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The builder-derived quote now covers the intended asset-lock flows, and the previous reservation-release race, unbounded selector path, and unusable timeout guidance are fixed. This exact head still permits stale reservation state to authorize dispatch, can retain a wallet's exclusive lock beyond the advertised deadline, and can mis-correlate requests after navigation; the release documentation also overstates a potentially partial point-in-time quote. Changes are required before final review.
Validated blockers were found in the Codex precheck. Sonnet is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— rust-quality (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 4 blocking | 🟡 1 suggestion(s)
1 additional finding(s) omitted (not in diff).
2 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
🤖 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/wallet_backend/snapshot.rs`:
- [BLOCKING] src/wallet_backend/snapshot.rs:802-857: Wallet-input changes can leave a stale ceiling authoritative
Dispatch validation compares the cached quote only with `asset_lock_inputs` from the last published wallet snapshot. A real transaction build can reserve inputs after that snapshot without publishing a new snapshot or advancing its input revision, so another funding flow continues to accept the old quote even though the real builder now excludes those inputs. The contention fallback preserves the same problem: when either `try_state()` or `try_asset_lock_final_input_state()` is unavailable, `recompute()` republishes the prior input composition with a newer generation, while `AssetLockBalanceCache::ensure_requested()` intentionally ignores generation-only changes. Reservation changes must advance a directly observed revision, unavailable observations must invalidate validation, or quote validation and transaction construction must share one live wallet-lock operation.
- [SUGGESTION] src/wallet_backend/snapshot.rs:236-290: Asset-lock input discovery runs transaction builders on every wallet event
`observe_asset_lock_inputs_locked()` clones all eligible UTXOs, constructs a value map, partitions the UTXOs into 500-input batches, clones the managed account for every batch, and runs `TransactionBuilder::build_unsigned()`. `SnapshotStore::recompute()` performs this synchronously while holding the non-blocking wallet state guard, and `EventBridge::on_wallet_event()` invokes `recompute()` for every wallet event, including `SyncHeightAdvanced` and `ChainLockProcessed`. Catch-up synchronization therefore repeatedly performs work proportional to the wallet's UTXO count inside a callback whose documented contract is to remain non-blocking, while also delaying wallet-manager writers. Move this discovery out of the event callback, cache it behind a meaningful input/reservation revision, or obtain reservation-aware inputs through an upstream constant-time snapshot.
In `src/wallet_backend/payments.rs`:
- [BLOCKING] src/wallet_backend/payments.rs:551-579: The observation mutex can extend the wallet write lock past its deadline
The probe acquires the wallet-manager write guard and starts its five-second deadline before calling `asset_lock_final_input_state()`. That call waits indefinitely on the process-wide `ASSET_LOCK_INPUT_OBSERVATION_LOCK`, and the deadline is not checked until the subsequent amount search. A snapshot observation for another fragmented wallet can hold this mutex while running transaction builders over every 500-input batch, so the probe can retain its own wallet's exclusive manager lock for longer than five seconds and block real sends or state updates. Do not wait for the global observation mutex while holding a wallet-manager write guard; use per-wallet serialization or return a retryable partial result when the observation lock is unavailable.
In `src/ui/state/asset_lock_balance.rs`:
- [BLOCKING] src/ui/state/asset_lock_balance.rs:58-63: Per-screen request IDs can attach an old reply to a new screen
Every newly constructed `AssetLockBalanceCache` starts its request counter at zero, while backend results and errors are routed to whichever screen is visible when they arrive. If screen A dispatches request 1, the user navigates away, and a newly constructed funding screen B dispatches request 1 for the same wallet and snapshot generation, A's late failure matches B's in-flight request and marks B failed. B's actual later result is then ignored because the in-flight entry was cleared. A late partial success can similarly replace and discard B's fresher result. Include an application-global unique request token or a per-cache instance nonce in the correlation key.
| let wallet = self.resolve_wallet(seed_hash).await?; | ||
| let wallet_id = wallet.wallet_id(); | ||
| // Every dry run performs one reservation read→reserve→release cycle. | ||
| // Keep them all under the same exclusive boundary as real builds. | ||
| let wallet_manager = Arc::clone(wallet.wallet_manager()).write_owned().await; | ||
| let deadline = ProbeDeadline::after(timeout); | ||
| let (managed_account, account, current_height) = { | ||
| let (key_wallet, info) = wallet_manager | ||
| .get_wallet_and_info(&wallet_id) | ||
| .ok_or(TaskError::WalletStateInconsistent)?; | ||
| let account = key_wallet | ||
| .get_bip44_account(DEFAULT_BIP44_ACCOUNT) | ||
| .ok_or(TaskError::WalletStateInconsistent)? | ||
| .clone(); | ||
| let current_height = asset_lock_builder_height(&info.core_wallet); | ||
| let managed_account = info | ||
| .core_wallet | ||
| .accounts | ||
| .standard_bip44_accounts | ||
| .get(&DEFAULT_BIP44_ACCOUNT) | ||
| .ok_or(TaskError::WalletStateInconsistent)? | ||
| .clone(); | ||
| (managed_account, account, current_height) | ||
| }; | ||
|
|
||
| tokio::task::spawn_blocking(move || { | ||
| let observed_inputs = | ||
| asset_lock_final_input_state(&managed_account, &account, current_height)?; | ||
| let amount_duffs = asset_lock_max_amount_from_account_until( |
There was a problem hiding this comment.
🔴 Blocking: The observation mutex can extend the wallet write lock past its deadline
The probe acquires the wallet-manager write guard and starts its five-second deadline before calling asset_lock_final_input_state(). That call waits indefinitely on the process-wide ASSET_LOCK_INPUT_OBSERVATION_LOCK, and the deadline is not checked until the subsequent amount search. A snapshot observation for another fragmented wallet can hold this mutex while running transaction builders over every 500-input batch, so the probe can retain its own wallet's exclusive manager lock for longer than five seconds and block real sends or state updates. Do not wait for the global observation mutex while holding a wallet-manager write guard; use per-wallet serialization or return a retryable partial result when the observation lock is unavailable.
source: ['codex']
| /// Async fetch state for asset-lock maximum amounts, keyed by wallet. | ||
| #[derive(Default)] | ||
| pub struct AssetLockBalanceCache { | ||
| states: BTreeMap<WalletSeedHash, FetchState>, | ||
| next_request_id: u64, | ||
| } |
There was a problem hiding this comment.
🔴 Blocking: Per-screen request IDs can attach an old reply to a new screen
Every newly constructed AssetLockBalanceCache starts its request counter at zero, while backend results and errors are routed to whichever screen is visible when they arrive. If screen A dispatches request 1, the user navigates away, and a newly constructed funding screen B dispatches request 1 for the same wallet and snapshot generation, A's late failure matches B's in-flight request and marks B failed. B's actual later result is then ignored because the in-flight entry was cleared. A late partial success can similarly replace and discard B's fresher result. Include an application-global unique request token or a per-cache instance nonce in the correlation key.
source: ['codex']
There was a problem hiding this comment.
Resolved in 9a5105d — Per-screen request IDs can attach an old reply to a new screen no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
| fn observe_asset_lock_inputs_locked( | ||
| managed_account: &ManagedCoreFundsAccount, | ||
| account: &Account, | ||
| current_height: u32, | ||
| ) -> Result<AssetLockInputState, BuilderError> { | ||
| let candidates: Vec<_> = managed_account | ||
| .utxos | ||
| .values() | ||
| .filter(|utxo| { | ||
| (utxo.is_confirmed || utxo.is_instantlocked) && utxo.is_spendable(current_height) | ||
| }) | ||
| .cloned() | ||
| .collect(); | ||
| let values: BTreeMap<_, _> = candidates | ||
| .iter() | ||
| .map(|utxo| (utxo.outpoint, utxo.value())) | ||
| .collect(); | ||
| let mut observed = Vec::with_capacity(candidates.len()); | ||
|
|
||
| for batch in candidates.chunks(ASSET_LOCK_INPUT_OBSERVATION_BATCH_SIZE) { | ||
| let mut dry_run_account = managed_account.clone(); | ||
| dry_run_account.utxos = batch | ||
| .iter() | ||
| .cloned() | ||
| .map(|utxo| (utxo.outpoint, utxo)) | ||
| .collect(); | ||
| let result = TransactionBuilder::new() | ||
| .set_fee_rate(FeeRate::new(0)) | ||
| .set_current_height(current_height) | ||
| .set_selection_strategy(SelectionStrategy::All) | ||
| .set_special_payload(TransactionPayload::AssetLockPayloadType( | ||
| AssetLockPayload::new(vec![TxOut { | ||
| value: 1, | ||
| script_pubkey: ScriptBuf::new(), | ||
| }]), | ||
| )) | ||
| .set_funding(&mut dry_run_account, account) | ||
| .require_final_inputs() | ||
| .build_unsigned(); | ||
| match result { | ||
| Ok((transaction, _)) => { | ||
| observed.extend(transaction.input.iter().filter_map(|input| { | ||
| values | ||
| .get(&input.previous_output) | ||
| .map(|value| (input.previous_output, *value)) | ||
| })); | ||
| dry_run_account.release_reservation(&transaction); | ||
| } | ||
| Err(BuilderError::CoinSelection(SelectionError::NoUtxosAvailable)) => {} | ||
| Err(source) => return Err(source), | ||
| } | ||
| } | ||
|
|
||
| Ok(AssetLockInputState::from_inputs(observed)) | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: Asset-lock input discovery runs transaction builders on every wallet event
observe_asset_lock_inputs_locked() clones all eligible UTXOs, constructs a value map, partitions the UTXOs into 500-input batches, clones the managed account for every batch, and runs TransactionBuilder::build_unsigned(). SnapshotStore::recompute() performs this synchronously while holding the non-blocking wallet state guard, and EventBridge::on_wallet_event() invokes recompute() for every wallet event, including SyncHeightAdvanced and ChainLockProcessed. Catch-up synchronization therefore repeatedly performs work proportional to the wallet's UTXO count inside a callback whose documented contract is to remain non-blocking, while also delaying wallet-manager writers. Move this discovery out of the event callback, cache it behind a meaningful input/reservation revision, or obtain reservation-aware inputs through an upstream constant-time snapshot.
source: ['codex']
…d-snapshot-balance # Conflicts: # CHANGELOG.md
…and send path - Send screen: a user-confirmed send is no longer dropped when a same-frame navigation action wins; merge_confirmation_action restores the pre-existing |= precedence, and append_concurrent_backend_tasks warns instead of silently dropping a task batch. - The asset-lock probe's ProbeDeadline now bounds the UTXO-composition observation too (checked between batches), starts when the blocking work starts rather than before it is scheduled, and the observation clones the account once instead of once per batch. - A contended or failed asset-lock observation during a snapshot recompute carries forward only the composition field: fresh balance/UTXOs/addresses still publish, with debug logging on the fallback path. - Max now reads the same current-composition accessor as dispatch validation (send screen and both identity funding screens), so Max can never offer an amount validation would refuse; CHANGELOG and SND-017 now describe the bounded revalidation window instead of an unconditional guarantee. Refs #929. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…probe's per-frame re-dispatch loop Two defects in the asset-lock Max probe machinery: - The input-composition observation treated a batch whose eligible funds sit at or below the 546-duff dust threshold as a fatal builder error, turning an everyday near-drained wallet into a permanent "could not be checked" state whose Retry deterministically fails. Classify the insufficient-funds outcomes as ordinary results (matching the dry-run helpers' classification) and keep the batch's eligible candidates in the composition key, so such wallets resolve to a current zero quote. - AssetLockBalanceCache had no backoff when a reply's observed composition cannot match the published snapshot key (deadline-expiry marker, carried-forward stale key), re-dispatching the probe every UI frame and repeatedly seizing the process-global observation lock for up to 5s per probe. After one automatic re-probe, a second consecutive mismatched reply now marks the entry failed and stops automatic dispatch until the composition actually changes or the user retries. The suppression is composition-keyed, not generation-keyed, so SPV event churn cannot re-arm it. Co-Authored-By: Claude Fable <noreply@anthropic.com>
…c comment Round-11 verification (PR #937) found asset_lock_final_input_state's doc comment still claimed the observed composition excludes reserved outpoints. Since 9a5105d's SEC-001 fix, a sub-dust batch's InsufficientFunds arm cannot filter reserved outpoints the way the success path does, so one may transiently enter the key. Harmless (the key only drives quote-match detection, never a spent amount) but the doc was wrong. Clarify both the match arm and the function doc. 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.
Preliminary review — Codex only
The exact head improves sub-dust handling and stops persistent composition mismatches from causing per-frame probe dispatches, but four in-scope blockers remain. Reservation changes can still leave an obsolete quote authoritative, the global observation mutex can defeat the wallet-lock deadline, screen-local request IDs can misroute late replies, and the release notes promise stronger freshness and maximality guarantees than the implementation provides. The event-path builder workload and funding-helper terminology also remain valid non-blocking findings.
Validated blockers were found in the Codex precheck. Sonnet is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— rust-quality (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 4 blocking
5 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
🤖 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/wallet_backend/snapshot.rs`:
- [BLOCKING] src/wallet_backend/snapshot.rs:295-305: Wallet-input changes can leave a stale ceiling authoritative
The insufficient-funds arm records every candidate in the batch, including inputs that `TransactionBuilder::set_funding` excluded as reserved. A batch can therefore produce the same composition key across a real reservation change: for example, two free 250-duff inputs plus reserved inputs remain sub-dust both before and after one of the 250-duff inputs is reserved, so this arm records the entire batch both times. A larger free input in another batch lets the full-wallet builder use those small inputs and makes the earlier quote higher, but after the reservation the real builder excludes one of them. Because the published composition remains unchanged, `AssetLockBalanceCache::get_current` still accepts the obsolete higher quote and dispatch validation can authorize an amount the builder now rejects. Filter the insufficient batch through the live reservation set, include a reservation revision in the key, or validate and construct the transaction under one authoritative wallet operation.
| Err(BuilderError::CoinSelection(SelectionError::InsufficientFunds { .. })) | ||
| | Err(BuilderError::InsufficientFunds { .. }) => { | ||
| // A sub-dust batch is an ordinary near-drained state, not a | ||
| // failure; its eligible candidates still shape the real | ||
| // builder's cross-batch choices, so they stay in the key. | ||
| // Unlike the success branch above, this arm cannot filter out | ||
| // an already-reserved outpoint, so one may transiently enter | ||
| // `observed`. That is safe: this key only ever drives quote | ||
| // *match* detection (see the doc comment below), never the | ||
| // amount `TransactionBuilder` reports or actually spends. | ||
| observed.extend(batch.iter().map(|utxo| (utxo.outpoint, utxo.value()))); |
There was a problem hiding this comment.
🔴 Blocking: Wallet-input changes can leave a stale ceiling authoritative
The insufficient-funds arm records every candidate in the batch, including inputs that TransactionBuilder::set_funding excluded as reserved. A batch can therefore produce the same composition key across a real reservation change: for example, two free 250-duff inputs plus reserved inputs remain sub-dust both before and after one of the 250-duff inputs is reserved, so this arm records the entire batch both times. A larger free input in another batch lets the full-wallet builder use those small inputs and makes the earlier quote higher, but after the reservation the real builder excludes one of them. Because the published composition remains unchanged, AssetLockBalanceCache::get_current still accepts the obsolete higher quote and dispatch validation can authorize an amount the builder now rejects. Filter the insufficient batch through the live reservation set, include a reservation revision in the key, or validate and construct the transaction under one authoritative wallet operation.
source: ['codex']
There was a problem hiding this comment.
Resolved in this update — Wallet-input changes can leave a stale ceiling authoritative no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
…d-snapshot-balance # Conflicts: # CHANGELOG.md
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
At exact head 8ea190d, all six indexed findings remain valid; the latest merge does not change the relevant implementations. Four blockers remain around stale quote authorization, deadline-defeating lock contention, cross-screen reply correlation, and inaccurate release-note guarantees. The targeted asset-lock cache tests pass, but they do not exercise cross-screen request-ID reuse or the sub-dust reservation-composition case.
Validated blockers were found in the Codex precheck. Sonnet is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— rust-quality (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 4 blocking
6 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
3 participants
TL;DR
Pressing Max when shielding DASH, funding a Platform address, creating an identity, or topping up an identity from your Core wallet balance — whether from your wallet balance or a received deposit — could suggest an amount larger than the wallet could actually send. The transaction was then rejected, no matter how you adjusted the amount, because the number the UI showed wasn't the number the wallet's real balance-selection logic was actually working from. Max and the amount check now both ask the wallet directly what it can send, so the two stay in agreement across every one of those flows.
User story
As a DET wallet user, I want the Max button on Shield / Fund Platform Address / Create Identity / Top Up Identity to give me an amount that actually sends, so that I don't hit a confusing "insufficient funds" error after already confirming the send.
Scenario
Actual behavior (before this PR): After receiving funds that hadn't fully settled yet (for example, right after withdrawing from a Platform identity), or with a wallet holding an unusually large number of small UTXOs, pressing Max on Shield, Fund Platform Address, or funding an identity from the Core wallet filled in an amount that looked correct and passed the on-screen check — then failed when actually sent, with a technical error the app never explained. Lowering the amount by trial and error was the only way through.
Expected behavior (after this PR): Max always reflects what the wallet can actually send right now, across all four of those flows, and stays in agreement with dispatch-time validation for every one of them (including the deposit-address-scoped ReceiveDeposit method). If the real amount is temporarily lower than the wallet's headline balance (e.g. funds still settling), or the wallet's UTXO composition changes between the last check and the moment you send, Max and the send both reflect that instead of over-promising. If the real amount is capped by how many inputs a single transaction can hold, Max reflects that too, and the send succeeds on the first try. While the wallet is being asked, the amount field shows "Checking the available amount…"; if that check fails, "The available amount could not be checked." appears with a "Retry available amount check" button, and you can still switch to a different funding method at any point.
Detailed discussion
Fixes #929.
Root cause:
send_screen.rs's Max computation and its pre-send validation forSourceSelection::CoreWalletasset-lock flows (Shield-from-Core, Core→Identity top-up) both readAppContext::snapshot_balance(..).spendable()— a display-only push snapshot (confirmed + unconfirmed) that the codebase's own A04 fund-safety invariant says must never feed coin selection. Because Max and validation shared that same wrong source, validation could never catch what Max over-shot; both agreed with each other and disagreed with the real coin selector, which is only consulted later, insidecreate_asset_lock_proof, after the user had already committed to the send. The same pattern was present in the identity "fund from wallet balance" screens (Create Identity, Top Up Identity).Fix: added
WalletBackend::asset_lock_max_amount()— a read-only counterpart tocreate_asset_lock_proofthat queries the liveTransactionBuilderfor the largest credit output it will actually accept, cleaning up its own dry-run UTXO reservations afterward. Max and validation for Shield, Core→Identity top-up, Create Identity from wallet balance, and Top Up Identity from wallet balance now consume this builder-derived ceiling (via an async per-wallet UI cache,AssetLockBalanceCache) instead of the display-only snapshot.DetWalletBalance::spendable()'s doc comment was corrected to state plainly that it is display-only. A newTaskError::AssetLockBalanceQueryFailedvariant surfaces probe failures with a plain, actionable message ("Wait a moment and try again") rather than raw internals.Follow-up hardening (post-review): an independent architecture audit of the initial implementation found it was a hand-rebuilt parallel path rather than routing through the real chokepoint, and two of its duplicated parameters had already drifted:
synced_height()while the realcreate_asset_lock_proofpath readslast_processed_height()— a different, independently-advanced watermark. Diverging by enough blocks could sweep a live in-flight UTXO reservation via the coinbase-maturity/TTL logic — a real side effect from a supposedly read-only probe. Fixed: the probe now uses the same accessor the real path uses, with a regression test proving it (asset_lock_max_uses_last_processed_height_when_sync_watermarks_diverge).AssetLockBalanceCache) only invalidated on screen refresh, not on new blocks or balance changes, so validation could run against a stale ceiling. Fixed:WalletSnapshotnow carries a per-wallet publish generation, and the cache rejects stale-generation results and re-queries.ASSET_LOCK_FEE_PER_KB) is a hand-copied duplicate of the real path'sDEFAULT_FEE_PER_KB— verified against the pinnedrs-platform-walletsource that this constant ispub(super)with no public accessor, so DET genuinely cannot import or derive it today. Left in place with an explicitTODO(upstream)rather than faked closed; tracked as a follow-up for upstream to export the constant or expose a shared ceiling-quote primitive.Simplification (post-review, round 2): the initial ceiling query binary-searched
[0, MAX_MONEY]against the real builder — correct, but up to ~34-51 realTransactionBuilderbuilds per Max query, each holding the wallet's global async write lock. A first attempt at trimming this added a hand-derived "68-byte" padding correction to a single-call drain probe, tuned by adding placeholder outputs until tests passed — rejected on review: independently checked against the vendored coin-selector and could only verify a single 34-byte change-output assumption in its fallback path, not two, with no local ground truth to confirm the larger number generalized (DET's own code never touchesTransactionBuilderdirectly for the real send — it's wrapped insideplatform-wallet'sAssetLockManager). Replaced instead with a seed-then-bisect approach that assumes nothing about the real strategy's internal fee formula: oneSelectionStrategy::Alldrain call gives a fast, structurally-verified upper bound (the builder already drops the change output for a drain), then a short exponential-search-then-bisect against the real default strategy (BranchAndBound, matchingcreate_asset_lock_proof) finds the exact boundary — typically ~15 calls instead of ~34-51, all against real code, none against a guessed formula. A wallet with more spendable UTXOs than fit in one transaction (where the drain seed itself is rejected) falls back to the original full-range search unchanged, preserving prior behavior for that edge case. All three regression tests pass with their assertions unchanged, proving the seeded result is exact.Closing the gaps found by automated CI review (round 3): an automated code review of this PR found 14 issues; 9 confirmed real and independently re-verified (diff-read, not just trusted) before fixing, 5 deferred with reasons noted below:
LargestFirstprobe finds a real, builder-verified achievable seed (nudged by one well-known P2PKH input's fee as a starting point only — never trusted, always re-verified against the real default strategy via real builder calls). New test with 517 UTXOs proves a non-zero max that builds within the input cap, and one duff more does not.AssetLockBalanceCachediscarded its last loaded value on every wallet-snapshot generation bump and had no in-flight-request guard, so during active sync the funding screens could get stuck on "Checking the available amount…" indefinitely. Now stale-while-revalidate: the last loaded value stays displayed while a refresh runs in the background, and a second probe never dispatches while one is already in flight.Maxon Shield reads the DISPLAY-ONLY snapshot balance, not the coin selector's set — over-shoots and the asset lock always fails #929 bug, in a fourth call site none of the prior rounds scoped. Now uses the same builder ceiling as Shield and Identity destinations.UseWalletBalance— now shares the same ceiling cache.SnapshotStore::publish's own generation-tracking fix (round 1, above) had turned a cheapArcrefcount clone into a full deep-clone of the wallet's transaction/UTXO history on every wallet event — a real hot-path regression introduced by this PR itself. Generation is now assigned via a dedicated counter before entering the publish closure, which only clones theArc.step_after_task_failureon the identity screens and resetsend_statuson the send screen, mid-registration. All three screens now suppress the global banner for this specific background task; the existing inline "could not be checked" / Retry UI is the right surface.Closing the gaps found by a holistic full-diff review (round 4): with rounds 1-3 landed, a full-diff review (not per-commit — specifically looking for cross-commit interaction bugs the incremental rounds couldn't see) found 4 more blocking issues, each independently reproduced against current code before fixing:
receive_deposit_ceiling_duffs()— the deposit-address-scoped bound the Max button itself already used — contradicting this PR's own "the two stay in agreement" guarantee for that one funding method. Fixed: Create Identity and Top Up Identity now share oneavailable_ceiling_duffs()helper for both display and dispatch validation, structurally preventing the two paths from diverging again.AssetLockBalanceCache::ensure_requested) had two flaws of its own. (a) Its debounce signal (spendable = confirmed + unconfirmed) can't distinguish an unconfirmed UTXO from the same UTXO once confirmed or InstantLocked — but the real builder only accepts confirmed-or-instantlocked inputs, so a same-signal balance change (a confirmation landing) could be silently ignored. (b) A same-generation dedup check could mask a genuine signal change reached via a real two-read race:snapshot_generation()andsnapshot_balance()are two independent reads that can straddle a background publish — reproduced with a standalone test (ensure_requestedat a given generation,store, then a secondensure_requestedat the same generation with a changed signal silently returnedNoneinstead of re-dispatching) before fixing. Fixed: the debounce key is now(generation, final_funds_duffs), wherefinal_funds_duffsis the confirmed-or-instantlocked total computed once at snapshot-publish time, and both fields are read from one snapshot load — eliminating the race at its source.WalletSeedHashis network-independent, so a mainnet-probed ceiling could survive into a Testnet session.WalletSendScreenalready handled this;AddNewIdentityScreenandTopUpIdentityScreendid not. Fixed: both now invalidate the cache onchange_context/refresh/refresh_on_arrival, matching the send screen.dashpay/rust-dashcore#918— BranchAndBound has no feasibility prune for near-total-balance targets) from rare/user-initiated into routine, automatic, and remotely re-triggerable by anyone who knows a receive address and sends dust to add a UTXO. Rust cannot forcibly kill an in-flightspawn_blockingsearch, so this added cooperative cancellation instead: every search-loop function checked aCancellationTokenbetween iterations, cancelled by a 5-second timer started alongside the probe. On timeout the caller got a distinguishable error with a plain, actionable message instead of an unbounded wait. This was explicitly a bounding workaround, not a fix for the underlying complexity — see round 5 below for what replaced it.Deferred from round 4 (reviewed, not fixed here): cancelling the probe when the user navigates away, and deduplicating concurrent probes for the same wallet — both real, both able to reuse the round-4
CancellationTokenmechanism, neither required to close the round-4 findings. (Cancel-on-navigate-away and probe dedup remain open; theCancellationTokenmechanism itself was removed in round 5 below, so any future work here needs a fresh mechanism.)Closing the gaps found by a bot review of the reconciled whole (round 5): after round 4 landed,
thepastaclaw's review of the current tip found three further issues. All three were independently re-verified by reading the actual pinned dependency source (not taken on trust) before fixing:CancellationTokenwas only checked between search-loop iterations — never during a single call into the realTransactionBuilder. The one call most likely to hit the exponentialBranchAndBoundcase is exactly the one the timeout couldn't interrupt, so the "bound worst-case wait" claim didn't hold for a genuinely pathological wallet. Rather than build a bespoke fallback search algorithm to patch around this, the actual upstream fix was confirmed to already exist:dashpay/rust-dashcore#919adds a suffix-sum feasibility/undershoot prune toBranchAndBounditself, closing#918at the algorithmic level (verified directly against the vendored source — the exactBnbSearch/suffix-array/BNB_NODE_BUDGETcode, with a test explicitly asserting "what actually fixes feat(identity): pending DPNS registration indicator, hint-text sizing, and social-profile fixes #918 is the feasibility bound, not the node budget"). With that confirmed present on thedashpay/platformbranch this repo now pins to, the round-4 timeout workaround —ASSET_LOCK_PROBE_TIMEOUT, theCancellationTokenthreaded through every search function, and the distinguishable timeout error and its UI handling — was removed entirely as unnecessary complexity now that the thing it was working around is fixed at the source.AssetLockBalanceCache::ensure_requesteddeduped re-queries on the wallet's aggregate spendable subtotal only, so a UTXO composition change that didn't move that subtotal left pre-send validation trusting a ceiling computed before the change — exactly the class of bug this PR exists to fix, reintroduced by the cache's own debounce logic. Fixed:WalletSnapshotnow tracks a canonical signature of eligible asset-lock inputs and a revision counter that advances only when that exact set changes; a newget_current()requires a quote matching the current UTXO revision for dispatch-time validation, while display keeps the existing stale-while-revalidate behavior unchanged (verified: all of round 3's anti-thrash/debounce tests still pass unmodified).key-walletsource directly:ReservationSetis a bareArc<Mutex<HashMap<OutPoint,u32>>>with no owner/session tag, andrelease()matches by outpoint alone. Real sends correctly hold the wallet-manager write lock across their whole select→reserve sequence; the probe previously took only a brief read lock to clone the account, then ran its whole dry-run search — potentially many iterations, each capable of reserving and releasing outpoints in the same shared reservation set — with no lock held at all. A concurrent real send and a probe iteration could both reserve the same UTXO, and the probe's own cleanup could then release the real send's still-live reservation out from under it. Fixed: the probe now acquires an owned wallet-manager write guard before cloning the account and holds it for the entire dry-run search, matching the real-send path's own locking discipline (new testasset_lock_max_probe_waits_for_wallet_manager_write_lockproves genuine lock contention, not a superficial check).The
dashpay/platformpin bump needed to make the first finding's fix real (288a6cae4f...→a18bd1586858ef680124e150caad6a7dc21d0b64, thefeat/platform-wallet-storage-rehydrationbranch tip) initially failed to compile — a signature drift betweenkey-wallet'sbuild_asset_lock_with_signerandplatform-wallet's own call site, introduced by the same branch'srust-dashcorebump — and had to wait for that to be fixed upstream (indashpay/platform, not this repo) before landing here.github-actions[bot]'s earlier review on this PR was filed against the very first pre-remediation commit and is now stale — every item it raised maps to a fix already landed in rounds 1-4.Closing the gaps found by a fresh CodeRabbit review of round 5's own diff (round 6): CodeRabbit's review of the round-5 commits found 5 issues, all embedded in the review body ("outside diff range" — no resolvable GitHub thread), each independently verified against the code before deciding what to do with it. Four were real and fixed:
AssetLockBalanceCache::store()/mark_loading_failed()matched an incoming reply against the cache's current in-flight request using onlysnapshot_generation— becauseensure_requestedcan legitimately re-arm at the same generation when UTXO composition changes (existing, tested behavior since round 4), a superseded probe's stale reply, arriving late, could be silently applied to the newer request's identity: reopening the exact stale-ceiling-authorizes-a-send risk round 5 closed, via a different mechanism. Fixed: every probe now carries a monotonic per-cacherequest_id, threaded throughWalletTask::GetAssetLockMaxAmount,BackendTaskContext, andBackendTaskSuccessResult, so a reply can only ever be applied to the exact request it belongs to.send_screen.rs's existing coverage.snapshot.rscloned a fullVec<(OutPoint, u64)>UTXO signature on every wallet event. Replaced with a constant-size, order-independentu64fingerprint of the same set; the round-6 diff's own fingerprint test only proved order-independence, so a distinctness assertion (two different UTXO sets must not collide) was added before committing.One finding (a claimed unit mismatch between duffs and credits in
top_up_identity_screen.rs's Max computation) was verified to be a false positive —max_amount_after_fee_reserve(spendable_duffs, fee_credits)deliberately takes a duffs first argument and a credits second argument by its own signature and doc comment; the flagged call site is correct as written, and the suggested "fix" would have introduced a real ~1000x under-reservation bug. Left untouched, with the reasoning recorded on the PR.Closing the gaps found by a holistic 3-agent review of the full diff (round 7): with rounds 1-6 landed, a fresh security/project-consistency/QA review of the complete diff (25 commits at the time) found 4 more blocking issues — all second-order effects of round 5's own lock-widening commit, plus a pre-existing gap in the Send screen that the two identity screens had already been hardened against. Each was independently re-verified against the actual pinned dependency source before fixing, not taken on trust:
asset_lock_final_inputs's eligibility check didn't consult the sharedReservationSet, so a real send's live reservation didn't move the tracked UTXO composition — meaningget_current()could still authorize dispatch against a ceiling the real builder would now reject, the exact failure the round-5 revision counter exists to prevent, reopened via a different gap in the same mechanism. Fixed at the root rather than patched: the probe now clones the liveManagedCoreFundsAccount(itsReservationSetis a sharedArc<Mutex<...>>, so the clone sees the same live reservations) and drives it through the actualTransactionBuilder::set_funding/build_unsignedpath — the same code a real send runs — instead of a parallel reimplementation of the eligibility rule. Independently confirmed by readingset_funding's reservation-filtering logic directly against the pinnedkey-walletcheckout.SnapshotStore::recompute(which needs the same lock) for the same duration — stalling the very freshness recompute the lock-widening was meant to help. Fixed: a deadline is now checked between each individually-bounded builder call (an in-flight call itself still can't be interrupted — no change there from what's achievable), bounding the worst-case lock hold to ~5 seconds; on expiry the probe returns a already-proven-safe lower bound and marks the quote partial rather than blocking indefinitely or fabricating a number.AppAction::BitOrAssignis a pure overwrite, not a merge — if a Max-probe dispatch and a confirmation-dialog action landed in the same UI frame, one could silently clobber the other. The two identity screens (Create Identity, Top Up Identity) already had a concurrent-task-preserving helper for exactly this; the Send screen never got it, since the pattern predates this PR touching that screen. Fixed: the helper is now shared (relocated out ofui::identitiesintoui::mod) and applied at the Send screen's probe-dispatch and confirmation-dialog call sites too.Bundled in the same pass: the round-6
DefaultHasher-based UTXO fingerprint (deterministic, zero-keyed, not the right primitive for a funds-safety equality check even as defense-in-depth) was replaced with exact input-set comparison;send_core_to_shielded's over-maximum error wording was aligned with the other three dispatch-validation sites; and this PR's own CHANGELOG/user-stories entries were corrected to scope the "Max" fix to the Simple builder-driven funding form — the Advanced manual-input Platform-address path remains governed by the Core inputs the user selects, not this ceiling.Closing the gaps found by two more holistic full-diff reviews (rounds 8-11):
Round 8 — a fresh 3-agent (security/project-consistency/QA) review of the complete diff (28 commits at the time) found 4 more blocking issues, each either a new regression round 7's own fix introduced or an incomplete fix for a round-6 finding:
ProbeDeadlinedidn't bound the phase that costs the most: the observation/clone work ran first, unbounded, before the deadline clock even started.get()vsget_current()), leaving a narrow window where Max could offer an amount validation would immediately reject; the CHANGELOG/user-stories entries overclaimed an absolute "always agree" guarantee the code didn't provide.All four fixed in round 9 (which also resynced the branch with
v1.0-dev's then-current tip — oneCHANGELOG.mdconflict, resolved by keeping both sides' entries), each independently re-verified against source before being accepted rather than taken on the fixing agent's own report.Round 10 — a second independent whole-diff review, this time of round 9's own fixes, explicitly briefed to re-derive every safety claim from source rather than trust the prior verification pass. It confirmed round 9's four fixes were themselves correct, but found 2 new blocking issues in the machinery round 9 built to make them work:
Both fixed in round 11: the dust-balance outcome is now treated as an ordinary result — mirroring a classification already used elsewhere in the same file — instead of an error; the cache now caps automatic re-dispatch at one transient retry before requiring an explicit Retry, with the suppression keyed to the wallet's actual UTXO composition (not sync-driven churn) so it can't be defeated by ordinary sync activity. Both fixes were independently re-verified from source, including tracing every consumer of the affected data to confirm the one flagged edge case (a since-reserved outpoint transiently entering the observed composition) can only ever cause a stale mismatch — never an inflated Max amount or a send targeting an already-reserved outpoint.
Both rounds' remaining non-blocking findings — documentation gaps, code duplication the fixes moved rather than removed, message-wording inconsistencies, and a handful of pre-existing residuals already accepted as follow-up debt in round 8 (a transitively mutable dependency pin, an undocumented process-global mutex, uneven adoption of a shared helper) — were reviewed and intentionally left out of this PR's scope: none are required to close #929 or any explicit acceptance criterion.
Deferred (reviewed, not fixed here):
ReservationSetshares state viaArc<Mutex<...>>, so the release is load-bearing, not dead code. Not a bug.TransactionBuilderrather than the real entry point, since the real path has no unit-test-reachable seam (wrapped several layers insideplatform-wallet'sAssetLockManager) — a documented limitation, not fixable without a backend-e2e-level test.(The wallet-manager write-lock hold during the ceiling search — previously listed here as an accepted, deferred follow-up — is no longer deferred: round 5 implements it, as the fix for the reservation-release race above.)
Out of scope, left as-is:
src/ui/dashpay/send_payment.rsstill reads.spendable()for ordinary (non-asset-lock) sends — that path doesn't hit the final-input rule the asset-lock builder does, needs different plumbing, and is a separate follow-up.availableexceedsrequired, andMaxreliably produces the failing amount #909 (Send Core→Core Max) is a distinct, already-triaged upstream defect indashpay/rust-dashcore's coin selection (tracked at rust-dashcore#911) — untouched here.Testing
wallet_backend::payments::tests::asset_lock_max_excludes_unconfirmed_funds_counted_by_snapshot: constructs a wallet with a confirmed and an unconfirmed UTXO and asserts the builder-derived ceiling excludes the unconfirmed value — a direct reproduction of the reported scenario.wallet_backend::payments::tests::asset_lock_max_uses_last_processed_height_when_sync_watermarks_diverge: proves the probe uses the real path's height watermark, not a diverging one.wallet_backend::payments::tests::asset_lock_max_uses_an_in_cap_subset_when_the_wallet_has_too_many_utxos: 517-UTXO wallet (over the per-tx input cap) proves Max is a real, non-zero, builder-verified amount instead of collapsing to 0, and one duff more is rejected.wallet_backend::payments::tests::asset_lock_max_probe_waits_for_wallet_manager_write_lock(round 5): proves the probe genuinely blocks on a concurrent real-send write-lock holder and only proceeds once it's released, closing the reservation-release race.model::asset_lock::tests::builder_ceiling_validation_reserves_operation_feefor the stateless ceiling/reserve arithmetic.ui::wallets::send_screen::tests::core_asset_lock_max_and_validation_use_builder_quotecovering Shield, Core→Identity, and Core→Platform-address Max, exact-ceiling acceptance, and one-duff-over rejection.ui::wallets::send_screen::tests::core_asset_lock_validation_rejects_quote_for_stale_utxo_composition(round 5): drives the real send-dispatch path and confirms it rejects a quote whose UTXO revision is stale rather than authorizing the send.ui::state::asset_lock_balance::tests::asset_lock_balance_cache_requeries_when_unconfirmed_funds_become_finaland..._requeries_same_generation_after_loaded_signal_changes: reproduce round 4's two debounce-cache flaws before the fix, then prove them closed.ui::state::asset_lock_balance::tests::asset_lock_balance_cache_blocks_stale_validation_after_utxo_composition_change(round 5): showsget()still returns a stale display value (correct, stale-while-revalidate) whileget_current()correctly refuses to authorize a send against it.wallet_backend::snapshot::tests::asset_lock_probe_snapshot_reads_generation_and_final_funds_togetherand..._final_funds_tracks_confirmation_and_instant_lock: prove generation and the confirmed-or-instantlocked total are read from one atomic snapshot load.ui::identities::add_new_identity_screen::tests::receive_deposit_dispatch_rejects_amount_above_deposit_address_balanceand..._network_switch_and_refresh_invalidate_asset_lock_balance: round 4's ReceiveDeposit and network-switch fixes, the latter driving the realScreen::change_contextentry point.ui::state::asset_lock_balance::tests::asset_lock_balance_cache_rejects_superseded_reply_at_same_generation(round 6): reproduces the stale-probe-reply race directly — dispatches a superseded request, then a superseding one at the same generation, and proves the superseded reply's late arrival cannot taint the superseding request's state.backend_task::tests::backend_task_context_preserves_asset_lock_request_identity,ui::wallets::send_screen::tests::core_asset_lock_dispatch_distinguishes_failed_probe_from_loading,ui::identities::add_new_identity_screen::tests::register_identity_dispatch_distinguishes_failed_probe_from_loadingand..._rejects_quote_for_stale_utxo_composition,ui::identities::top_up_identity_screen::tests::top_up_dispatch_distinguishes_failed_probe_from_loadingand..._rejects_quote_for_stale_utxo_composition(round 6): the request-id correlation, failed-vs-loading messaging, and stale-composition dispatch-rejection coverage added to both identity screens.wallet_backend::snapshot::tests::asset_lock_input_fingerprint_is_independent_of_utxo_order(round 6, extended after independent review to also assert two genuinely different UTXO sets don't collide): proves the new fingerprint is order-independent without being degenerate.wallet_backend::payments::tests::asset_lock_max_probe_deadline_bounds_the_wallet_manager_write_lock(round 7): with a zero-duration deadline, proves the write lock is released within 1 second and the returned quote is marked partial — reproduces round 5's unbounded-lock-hold regression before the fix.wallet_backend::snapshot::tests::asset_lock_input_revision_changes_when_an_eligible_utxo_is_reserved(round 7): reserves an eligible UTXO via the sharedReservationSetand proves the tracked input composition changes — reproduces the reservation-blind freshness gap directly.ui::state::asset_lock_balance::tests::asset_lock_balance_cache_validates_the_observed_not_dispatched_composition(round 7): stores a reply whose observed composition differs from what was dispatched and provesget_current()authorizes only against the observed value.ui::state::asset_lock_balance::tests::asset_lock_balance_cache_redispatches_and_offers_retry_after_reply_deadline(round 7): lets an in-flight request's deadline expire and proves it redispatches with a new request id and offers Retry while still loading.ui::wallets::send_screen::tests::same_frame_probe_and_confirmation_dispatch_are_both_preserved(round 7): dispatches a Max probe and a confirmation action in the same frame and proves neither is silently dropped byAppAction::BitOrAssign.ui::wallets::send_screen::tests::confirmed_send_action_survives_same_frame_navigation(round 9): proves a same-frame Max-probe dispatch and a confirmed send no longer clobber each other.wallet_backend::payments::tests::asset_lock_observation_reports_incomplete_when_the_deadline_has_expiredand..._asset_lock_probe_deadline_bounds_the_observation_phase(round 9): prove the observation phase itself is now deadline-bounded, not just the phases after it.wallet_backend::snapshot::tests::contended_asset_lock_observation_keeps_the_fresh_balance_and_utxos(round 9): holds the real global observation lock during a live recompute and proves an observation failure no longer discards the balance/UTXO fields alongside it.ui::wallets::send_screen::tests::max_button_offers_nothing_while_the_builder_quote_is_revalidating(round 9): proves Max and validation now read the same accessor rather than two accessors with different freshness guarantees.wallet_backend::snapshot::tests::asset_lock_observation_treats_a_sub_dust_balance_as_observed_not_failed(round 11): a single 300-duff UTXO now yields a real, current zero quote instead of a permanent "could not be checked".ui::state::asset_lock_balance::tests::asset_lock_balance_cache_stops_redispatching_after_persistent_composition_mismatchand..._retry_rearms_a_mismatch_suppressed_wallet(round 11): prove a persistent composition mismatch stops automatic re-dispatch after one transient retry, and that Retry still re-arms it.cargo fmt --all -- --checkclean and scopedcargo clippy/cargo testfor every touched module passing at each round, each independently re-run and re-verified from source rather than trusted from the fixing agent's own report; full-workspace sweep left to CI per this repo's own local-vs-CI policy.cargo fmt --all -- --checkclean;cargo test --all-features --workspace— 2487 tests, 0 failed (2177 unit + 310 kittest/e2e/legacy/mcp-auth/backend-e2e, +7 from round 7's own new tests, no other unexpected count swings);cargo test --doc --all-features --workspace— 7 passed (5 + 2 compile-fail), unchanged from round 5;cargo clippy --all-features --all-targets -- -D warningsclean.🤖 Co-authored by Claudius the Magnificent AI Agent
Summary by CodeRabbit
Bug Fixes
User Experience