fix(platform-wallet): gate ADDR-09 watermark invalidation inside the reconcile seam#4005
Conversation
QuantumExplorer
requested review from
ZocoLini,
lklimek,
llbartekll and
shumkov
as code owners
|
Warning Review limit reached
Next review available in: 7 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
✅ Review complete (commit 9351733) |
There was a problem hiding this comment.
Code Review
The address-funds transfer path is patched correctly, but the sibling identity-to-address transfer wrapper at platform_wallet.rs:342-344 still has the same optimistic-absolute-seed vs incremental-delta double-count — IdentityCreditTransferToAddressesTransitionAction emits AddBalanceToAddress for every recipient, so a recipient owned by the calling wallet reproduces the exact bug this PR set out to fix. The reconcile/invalidate atomicity race is real but inherited unchanged from the merged #4004 asset-lock pattern (which documents acceptance of the same transient window), so it does not block this PR.
1 blocking | 1 nitpick(s)
Source: reviewers: reviewer-claude-general-4005-1783237422 (opus), reviewer-codex-general-4005-1783237422 (gpt-5.5), reviewer-claude-security-auditor-4005-1783237422 (opus), reviewer-codex-security-auditor-4005-1783237422 (gpt-5.5), reviewer-claude-rust-quality-4005-1783237422 (opus), reviewer-codex-rust-quality-4005-1783237422 (gpt-5.5); verifier: verifier-4005-1783237422 (opus).
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 `packages/rs-platform-wallet/src/wallet/platform_wallet.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/platform_wallet.rs:342-344: Same double-count on identity-to-address transfer to an owned recipient
`transfer_credits_to_addresses_with_external_signer` calls `reconcile_address_infos` without invalidating the sync watermark, so it hits the exact ADDR-09-sibling bug this PR set out to close. `IdentityCreditTransferToAddressesTransitionAction::into_high_level_drive_operations` (packages/rs-drive/src/state_transition_action/action_convert_to_operations/identity/identity_credit_transfer_to_addresses_transition.rs:42-49) emits `AddressFundsOperationType::AddBalanceToAddress` — a DELTA in Drive's recent-address-balance-changes tree — for every entry of `recipient_addresses`. When the caller passes one of this wallet's own addresses as a recipient (a straightforward, public-API-reachable scenario: identity → own platform address), the seam writes the proof-attested absolute post-balance into the provider's `found` seed but leaves the incremental watermark stale. The next incremental BLAST pass re-applies that `AddToCredits` delta on top of the already-absolute seed → `X + delta` over-count, identical to the mechanism transfer.rs now guards against. The narrower `platform_addresses/transfer.rs` guard does not cover this wrapper, so the PR's stated scope (invalidate on same-wallet transfer credit) is incomplete. Apply the same reconcile-credited-owned-output gate + `invalidate_sync_watermark()` here (or fold the decision into `reconcile_address_infos`), keyed on the recipient set overlapping with wallet-owned addresses in the returned `AddressInfos`.
In `packages/rs-platform-wallet/src/wallet/platform_addresses/transfer.rs`:
- [NITPICK] packages/rs-platform-wallet/src/wallet/platform_addresses/transfer.rs:230-236: P2sh output addresses are silently dropped from the invalidation gate
`output_addresses` is built via `filter_map` that only keeps `PlatformAddress::P2pkh` and drops `PlatformAddress::P2sh`. This is correct today because `PlatformAddressBalanceEntry.address` is a `PlatformP2PKHAddress` and wallets do not own P2sh outputs, so the invariant holds — but the coupling is enforced silently by the filter rather than by a type check, `debug_assert!`, or doc comment. If wallet-owned P2sh support is ever added, an owned P2sh change output would reintroduce the same double-count with no signal. Consider a `debug_assert!(matches!(addr, PlatformAddress::P2pkh(_)))` in the collector or a comment on the collector documenting the coupling to `PlatformAddressBalanceEntry`'s P2PKH-only shape.
| let output_addresses: BTreeSet<PlatformP2PKHAddress> = outputs | ||
| .keys() | ||
| .filter_map(|addr| match addr { | ||
| PlatformAddress::P2pkh(hash) => Some(PlatformP2PKHAddress::new(*hash)), | ||
| _ => None, | ||
| }) | ||
| .collect(); |
There was a problem hiding this comment.
💬 Nitpick: P2sh output addresses are silently dropped from the invalidation gate
output_addresses is built via filter_map that only keeps PlatformAddress::P2pkh and drops PlatformAddress::P2sh. This is correct today because PlatformAddressBalanceEntry.address is a PlatformP2PKHAddress and wallets do not own P2sh outputs, so the invariant holds — but the coupling is enforced silently by the filter rather than by a type check, debug_assert!, or doc comment. If wallet-owned P2sh support is ever added, an owned P2sh change output would reintroduce the same double-count with no signal. Consider a debug_assert!(matches!(addr, PlatformAddress::P2pkh(_))) in the collector or a comment on the collector documenting the coupling to PlatformAddressBalanceEntry's P2PKH-only shape.
source: ['claude']
There was a problem hiding this comment.
Resolved in this update — P2sh output addresses are silently dropped from the invalidation gate 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.
…reconcile seam Follow-up to the ADDR-09 asset-lock top-up double-credit fix (#4004), generalizing it to every flow that credits a wallet-owned address. Any transition that credits an address does so via an on-chain `AddBalanceToAddress` op — a DELTA (`AddToCredits`) in Drive's recent-address-balance-changes tree — while inputs are recorded as absolute `SetBalanceToAddress` ops. When `reconcile_address_infos` commits the proof-attested ABSOLUTE post-balance into the provider's `found` seed without advancing the incremental sync watermark, the next incremental BLAST pass re-applies the recent delta on top of the already-absolute seed → `X + delta` — the ADDR-09 double-count. #4004 patched this caller-side for the asset-lock top-up only; review of the first cut of this change found the same live bug in three more flows. Fix at the seam instead of per caller: - `reconcile_address_infos` now takes a `credited_outputs` set (built with the new `credited_outputs_set` helper) naming the addresses the transition credited via deltas. When a COMMITTED entry matches, the seam calls `invalidate_sync_watermark()` INSIDE its provider-lock critical section — so no sync pass can interleave between the seed commit and the invalidation with the stale watermark (closing the race window the caller-side pattern left open) — forcing the next pass to full-scan-reconcile. Entries skipped as stale/unchanged do not fire the gate: a sync already applied that credit and advanced the watermark past it. Callers audited and updated: - `transfer` — outputs are credited; a same-wallet output (change address or transfer-to-self) was the double-count shape (CONFIRMED). - `fund_from_asset_lock` — recipients are credited; replaces #4004's caller-side unconditional invalidation. - `transfer_credits_to_addresses` — recipients are credited; the primary use is consolidating identity credits into the wallet's OWN addresses, so this path double-counted too (CONFIRMED, unfixed before this change). - `register_from_addresses` — the optional refund output is credited (CONFIRMED mechanism; reachable when the output is owned). - `top_up_from_addresses` and `withdraw` — pass an empty set: they only drain inputs (absolute ops, idempotent under re-apply) and have no change output today; comments direct any future change output through the gate. Cross-wallet transfers remain unaffected: each wallet's provider tracks only its own wallet (`from_wallets([self.wallet_id])`), so the recipient wallet never receives an optimistic absolute seed write; its own sync applies the delta exactly once. Adds three seam-level tests (gate fires on committed credited output and keeps the seed; watermark preserved for input-only reconciliation; watermark preserved when the credited entry was skipped as already applied) alongside #4004's provider-level `invalidate_sync_watermark_forces_full_scan_keeps_seed`. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
QuantumExplorer
force-pushed
the
claude/angry-goldberg-37ea17
branch
from
e3174c5 to
9351733
Compare
QuantumExplorer
changed the title
There was a problem hiding this comment.
Code Review
Both prior findings resolved. The blocking identity-to-address double-count is fixed by threading credited_outputs from recipient_addresses.keys() into reconcile_address_infos (platform_wallet.rs:344-370), and the P2SH-drop nitpick is addressed by the credited_outputs_set helper's docstring documenting the P2PKH-only wallet invariant (platform_addresses/mod.rs:44-63). Bonus: the invalidation gate now lives inside reconcile_address_infos under the provider write lock (wallet.rs:369-384), closing the reconcile/invalidate atomicity race that was previously out-of-scope. No new in-scope defects in the latest delta.
Prior finding reconciliation:
- FIXED:
transfer_credits_to_addresses_with_external_signernow passes recipient credited outputs intoreconcile_address_infos, so owned identity-to-address recipients invalidate the watermark inside the reconcile critical section. - OUTDATED: the P2SH-filter nitpick is covered by the shared
credited_outputs_sethelper's doc comment documenting the P2PKH-only wallet-owned address invariant.
Source: reviewers claude general opus, codex general gpt-5.5, claude security-auditor opus, codex security-auditor gpt-5.5, claude rust-quality opus, codex rust-quality gpt-5.5; verifier claude opus.
…reconcile seam (#4005) Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
2 participants
Issue being fixed or feature implemented
Follow-up to the ADDR-09 asset-lock top-up double-credit fix (#4004), generalizing it to every flow that credits a wallet-owned address.
Any transition that credits an address does so via an on-chain
AddBalanceToAddressop — a DELTA (AddToCredits) in Drive's recent-address-balance-changes tree — while inputs are recorded as absoluteSetBalanceToAddressops. Whenreconcile_address_infoscommits the proof-attested ABSOLUTE post-balance into the provider'sfoundseed without advancing the incremental sync watermark, the next incremental BLAST pass re-applies the recent delta on top of the already-absolute seed →X + delta— the ADDR-09 double-count.#4004 patched this caller-side for the asset-lock top-up only. An adversarial review of the first cut of this PR (which patched only
transfer, also caller-side) confirmed the same live bug in more flows and a race window in the caller-side pattern itself:transfer(this PR's original target) — a same-wallet output (change address, or a transfer to one of the wallet's own addresses) double-counts. CONFIRMED.transfer_credits_to_addresses— recipients are delta-credited, and the primary use of this flow is consolidating identity credits into the wallet's own DIP-17 addresses. CONFIRMED, previously unfixed.register_from_addresses— the optional refund-style output is delta-credited; reachable whenever it lands on an owned address. CONFIRMED mechanism.reconcile_address_infosreturns (the fix(platform-wallet): reflect asset-lock top-up balance once, not doubled #4004 caller-side pattern) leaves a gap between the seam releasing the provider lock and the invalidation re-acquiring it, in which a ~15s-cadence background sync could run incrementally with the stale watermark and transiently commit 2×.Verified NOT affected: cross-wallet transfers (each wallet's provider tracks only its own wallet via
from_wallets([self.wallet_id]); the recipient wallet's own sync applies the delta exactly once), withdrawals andtop_up_from_addresses(input-only drains — absolute ops, idempotent under re-apply; no change output today).What was done?
Moved the gate into the seam instead of patching callers one by one:
PlatformAddressWallet::reconcile_address_infosnow takes acredited_outputs: &BTreeSet<PlatformP2PKHAddress>parameter naming the addresses the transition credited via deltas (built with the newcredited_outputs_sethelper). When a committed entry matches, the seam callsinvalidate_sync_watermark()inside its provider-lock critical section — no sync pass can interleave between the seed commit and the invalidation, closing the race window — forcing the next BLAST pass to full-scan-reconcile.transfer(outputs),fund_from_asset_lock(recipients; replaces fix(platform-wallet): reflect asset-lock top-up balance once, not doubled #4004's caller-side unconditional invalidation),transfer_credits_to_addresses(recipients),register_from_addresses(optional output),top_up_from_addresses+withdraw(empty set, with comments directing any future change output through the gate).Option) so a future caller cannot silently forget the ADDR-09 analysis — every call site must state its credited-output set.PlatformPaymentAddressProvider::invalidate_sync_watermark, whose single call site is now the seam gate.How Has This Been Tested?
cargo test -p platform-wallet --lib— 242 passed, 0 failed.invalidate_sync_watermark_forces_full_scan_keeps_seedstill covers the invalidation effect itself.cargo clippy -p platform-wallet --all-targets— clean;cargo check -p platform-wallet-ffi— compiles (no FFI surface change).cargo fmt --all.Breaking Changes
None on-chain / consensus.
reconcile_address_infos(apubRust API onPlatformAddressWallet, not exposed over FFI) gains a required parameter; all in-repo callers updated.Checklist:
🤖 Generated with Claude Code