Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -293,6 +293,51 @@ impl PlatformAddressWallet {
.reconcile_address_infos(&address_infos, "fund from asset lock")
.await;

// ADDR-09: force the next BLAST sync to full-scan-reconcile
// instead of applying an incremental delta.
//
// `reconcile_address_infos` above set the provider's committed
// `found` seed to the proof-attested ABSOLUTE balance `X`, but the
// top-up is recorded on-chain as a DELTA (`AddBalanceToAddress` →
// `AddToCredits`) in Drive's recent-address-balance-changes tree,
// and we did NOT advance the incremental watermark. An incremental
// next pass would seed `result.found` from `current_balances()`
// (already `X`) and then re-apply that recent `AddToCredits(X)`
// delta from the stale watermark, landing at `X + X = 2X` — the
// ADDR-09 double-count. Zeroing the in-memory watermark makes
// `last_sync_timestamp()` return `None`, so the next pass full-scans
// (absolute seed from the tree, catch-up from the fresh checkpoint)
// and reconciles to the correct `X`. This is the automated
// equivalent of the manual Sync-tab "Clear" + "Sync Now".
//
// Unlike transfer/withdrawal (which also route through the seam),
// an asset-lock top-up credits its recipient with a pure additive
// delta and no offsetting input for that same address, so updating
// the `found` seed alone does not neutralize the re-applied delta —
// hence this path-specific watermark invalidation.
//
// DURABILITY: in-memory only. The persisted sync watermark cannot
// be reset to 0 through the normal changeset —
// `PlatformAddressChangeSet::merge` combines `sync_height` with
// `.max()` and the persister only fires `on_persist_sync_state_fn`
// when a component is `> 0` — so a durable zero would have to fight
// both the merge and the `> 0` gate. Instead we rely on the
// in-session BLAST cadence (~15s): the next pass full-scans,
// reconciles to `X`, and persists a correct FORWARD watermark, so
// durable state self-corrects within ~15s. The only residual gap is
// an app kill inside that ~15s window; a restart then resumes
// incremental sync from the stale persisted watermark and the
// double-count could briefly reappear until the next full rescan
// (or a manual Clear). That narrow window is accepted here rather
// than over-engineering a durable invalidation against the
// `.max()` merge / `> 0` gate.
{
let mut guard = self.provider.write().await;
if let Some(provider) = guard.as_mut() {
provider.invalidate_sync_watermark();
}
}
Comment on lines 293 to +339

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Suggestion: Watermark invalidation is not atomic with reconcile_address_infos

reconcile_address_infos at line 292-294 takes self.provider.write() internally, commits the reconciled found = X seed, and drops that guard before returning. The block at 334-339 then re-acquires self.provider.write() to call invalidate_sync_watermark. Meanwhile sync_balances in sync.rs:115 also acquires self.provider.write() for the entire sync round — including the SDK network call at lines 133-136 — and calls provider.last_sync_timestamp() before seeding before from current_balances(&*provider). tokio's RwLock is fair, so a background BLAST sync waiting on the lock during reconciliation can be granted the lock between the two acquisitions here. In that interleaving it will observe (a) the freshly committed found = X and (b) the still-non-zero watermark, take the incremental branch in sync_address_balances, re-apply the recent AddToCredits(X) delta, and persist X + X = 2X — the exact reading this PR claims to prevent. The subsequent invalidation still causes a full-scan reconcile within ~15s, so worst-case is a brief reappearance of the pre-PR bug rather than a regression, but the fix is not race-free the way the comment implies. A race-free variant would fold the watermark zeroing into the same critical section as the reconciliation commit — for example a fund-specific reconcile helper, or an invalidate_watermark_after: bool flag on reconcile_address_infos so transfer/withdrawal don't trigger it. Worth doing, since the whole point of this PR is to eliminate the ability to observe the double-count from a fresh top-up.

source: ['claude', 'codex']


if let Some(out_point) = tracked_out_point {
// Platform DID accept the top-up — propagating an Err
// here would misreport the protocol outcome, since the
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -520,6 +520,46 @@ impl PlatformPaymentAddressProvider {
}
}

/// Zero the incremental-sync watermark ONLY, so the next
/// `sync_balances` takes the full-scan branch — WITHOUT dropping the
/// cached `found` seed (unlike [`reset_sync_state`](Self::reset_sync_state),
/// which is the "Clear" flow).
///
/// WHY (ADDR-09): the asset-lock top-up path reconciles the
/// proof-attested ABSOLUTE balance `X` into both the managed account
/// and the provider's committed `found` seed, but the on-chain credit
/// is recorded as a DELTA (`AddBalanceToAddress` → `AddToCredits`) in
/// Drive's recent-address-balance-changes tree. If the next pass ran
/// INCREMENTALLY it would seed `result.found` from `current_balances()`
/// (already `X`) and then re-apply that recent `AddToCredits(X)` delta
/// from the stale watermark, landing at `X + X = 2X` — the ADDR-09
/// double-count. An optimistic absolute write is fundamentally
/// inconsistent with incremental delta re-application, so we force the
/// very next pass to full-scan-reconcile.
///
/// With `sync_timestamp == 0`, [`last_sync_timestamp`](Self::last_sync_timestamp)
/// returns `None`, which makes `sync_address_balances` choose the
/// full-scan branch: `result.found` is re-seeded ABSOLUTELY from the
/// tree (the `found` seed is only consulted on the incremental branch,
/// which is skipped), and incremental catch-up runs from the fresh
/// full-scan checkpoint rather than the stale height, so no recent
/// delta is re-applied. `last_known_recent_block` is zeroed too since
/// catch-up reads it as its recent-tree boundary.
///
/// The `found` seed is deliberately KEPT (not cleared): a full scan
/// ignores it as a seed, and preserving it means display and
/// `auto_select_inputs` budgeting keep the just-applied balance `X`
/// visible during the ~15s until the reconciling scan completes,
/// instead of the momentary zero `reset_sync_state` would show.
///
/// This is the in-memory equivalent of the manual Sync-tab
/// "Clear" + "Sync Now" that also fixes the double-count.
pub(crate) fn invalidate_sync_watermark(&mut self) {
self.sync_height = 0;
self.sync_timestamp = 0;
self.last_known_recent_block = 0;
}
Comment on lines 520 to +561

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💬 Nitpick: reset_sync_state and invalidate_sync_watermark duplicate the three-scalar zeroing

Both reset_sync_state (line 510) and the new invalidate_sync_watermark (line 557) zero the same three watermark fields (sync_height, sync_timestamp, last_known_recent_block). The two operations are semantically nested — Clear = invalidate watermark + drop found/absent + drop per_wallet_in_sync — so reset_sync_state could compose on top of invalidate_sync_watermark to keep the watermark reset single-sourced. If the watermark ever grows a fourth scalar, forgetting it in either function would silently reintroduce an ADDR-09-like bug or regress Clear. Not a blocker.

source: ['claude']


/// Diagnostic snapshot counts used by the read-only memory
/// explorer surface on
/// [`crate::manager::PlatformWalletManager::platform_address_provider_state_blocking`].
Expand Down Expand Up @@ -1822,4 +1862,49 @@ mod tests {
"reset must drop the cached `found` seed"
);
}

/// ADDR-09: after an asset-lock top-up reconciles an absolute balance,
/// the fund path calls `invalidate_sync_watermark` to force the next
/// BLAST pass into full-scan mode. Unlike `reset_sync_state`, it must
/// zero all three watermark scalars (so `last_sync_timestamp()` returns
/// `None`, the full-scan trigger) WITHOUT dropping the freshly
/// reconciled `found` seed — display and input budgeting rely on the
/// balance staying visible until the reconciling scan completes.
#[tokio::test]
async fn invalidate_sync_watermark_forces_full_scan_keeps_seed() {
let addr = p2pkh(1);
let mut provider = provider_with_one_funded_address(addr, funds(294_627_247_940, 5));

// Simulate a wallet mid-incremental-sync: non-zero watermark and a
// populated balance seed (the just-reconciled top-up balance `X`).
provider.set_stored_sync_state(10, 20, 30);
assert_eq!(provider.last_sync_height(), 10);
assert_eq!(provider.last_sync_timestamp(), Some(20));
assert_eq!(provider.last_known_recent_block(), 30);
assert_eq!(provider.current_balances().count(), 1);

provider.invalidate_sync_watermark();

// Watermark fully zeroed → SDK drops back to full-scan mode.
assert_eq!(provider.last_sync_height(), 0);
assert_eq!(
provider.last_sync_timestamp(),
None,
"invalidated watermark must report no last-sync timestamp so the \
next pass takes the full-scan branch (the ADDR-09 fix)"
);
assert_eq!(provider.last_known_recent_block(), 0);

// The reconciled `found` seed SURVIVES — a full scan ignores it as
// a seed, but keeping it means the balance `X` stays visible for
// display / input budgeting during the ~15s until the scan runs.
let seed: Vec<_> = provider.current_balances().collect();
assert_eq!(
seed.len(),
1,
"invalidate_sync_watermark must NOT drop the cached `found` seed"
);
assert_eq!(seed[0].1, addr);
assert_eq!(seed[0].2, funds(294_627_247_940, 5));
}
}
1 change: 1 addition & 0 deletions packages/swift-sdk/SwiftExampleApp/TEST_PLAN.md
Original file line number Diff line number Diff line change
Expand Up @@ -164,6 +164,7 @@ The app is a full multi-wallet client: `PlatformWalletManager` holds N wallets c
| ADDR-03 | Top up address from asset lock | Cross | Thorough | ✅ | `FundFromAssetLockPlatformAddressView` → `dash_sdk_address_top_up_from_asset_lock`. |
| ADDR-04 | Withdraw address credits → Core L1 | Cross | Thorough | ✅ | `WalletDetailView` → Platform Balance row **⋯ menu → Withdraw to Core** (sheet, `WithdrawPlatformAddressView`) → `ManagedPlatformAddressWallet.withdraw` → `platform_address_wallet_withdraw_to_address` (keychain-signed). Source = DIP-17 platform-payment account picker; the **full** account balance is withdrawn (no per-address amount, no change). Core L1 destination = own wallet (`core_wallet_next_receive_address`) or pasted external address, network-checked Rust-side. `coreFeePerByte` defaults to 1. Gated on the Core (SPV) wallet being initialized — shows a "Core not ready" state otherwise. Identity/address credit balance drops; L1 payout is pooled and processed asynchronously (no immediate txid). On success a DIP-17 resync runs. (Also reachable via the 🧪 debug builder *Settings → Platform State Transitions → Address → Withdraw Address Funds (raw)* → `dash_sdk_address_withdraw_funds`, which pastes a raw 64-char private key.) |
| ADDR-06 | Display / share your Platform receive address | Platform | Common | ✅ | "Receive Dash" sheet → **Platform** tab (`ReceiveAddressView`, `ReceiveAddressTab.platform`, "Your Platform Address"): QR + bech32m DIP-17 address + Copy. The receive counterpart to the credit-transfer / top-up funding paths. |
| ADDR-09 | Top-up balance reflects exactly once (no double-credit) | Cross | Thorough | ✅ | Regression guard for the top-up double-credit bug. Run `ADDR-03` ("Top Up from Core", `FundFromAssetLockPlatformAddressView`, `dash_sdk_address_top_up_from_asset_lock`) on a Core-funded wallet, then **wait through at least one automatic BLAST platform-address sync (~15s)** and re-read the `WalletDetailView` Platform Balance. **Pass:** the balance increases by the topped-up amount **exactly once** and stays there — no manual Sync-tab "Clear" + "Sync Now" needed. **Fail (the bug):** balance shows ~2× the top-up until a manual Clear+resync. Root cause was in `rs-platform-wallet`: the fund path reconciled the proof-attested *absolute* balance into the provider but left the incremental sync watermark stale, so the next *incremental* BLAST pass re-applied the on-chain `AddBalanceToAddress` (`AddToCredits`) *delta* on top → 2×. Fixed by `PlatformPaymentAddressProvider::invalidate_sync_watermark()` (called from `fund_from_asset_lock` after `reconcile_address_infos`), which forces the next pass to full-scan-reconcile — the automated equivalent of Clear+resync. Needs a **Funded Core wallet** fixture; the wallet's Core (SPV) balance must be non-zero to build the asset lock. |

### 4.4 DPNS (usernames) — `Domain=DPNS`

Expand Down
Loading