fix(platform-wallet): height-pin address balances so delta replay cannot double-count (ADDR-09)#4019
Conversation
…not double-count ADDR-09: after "Top Up from Core", the topped-up platform address showed exactly 2x its on-chain balance, durable across automatic syncs, manual Clear+resync, and app restart. The watermark-invalidation fixes (#4004 / #4005) forced a full rescan but could not stop the rescan itself from replaying the on-chain AddToCredits delta on top of an absolute that already included it. Root cause: two sources of truth for an address balance — proof-attested ABSOLUTES (ST results, trunk/branch scan elements) and the recent/compacted balance-change DELTA stream — were reconciled through global height cursors, and any path that threaded the wrong height re-applied deltas already baked into an absolute. Fix: every absolute now carries its height pin. `AddressFunds` gains `as_of_height` — the Platform block height the balance is current AS OF: - a delta recorded at block B applies only when `B > pin` (otherwise it is already included); applying advances the pin. The recent loop gates per entry; the compacted loop gates PER OPERATION (block-aware ops carry their own heights, so a pin inside a compacted range drops exactly the ops the absolute already includes). - trunk/branch scan absolutes are pinned at the scan checkpoint height; absence-zeroing entries too. - `BroadcastStateTransition` gains `*_with_metadata` variants exposing the quorum-authenticated `metadata.height` (previously verified and discarded), and every address/identity transition helper returns the proof height alongside its `AddressInfos`. - `reconcile_address_infos(infos, as_of_height, ctx)` pins ST-attested absolutes; freshness is decided by pin ordering (nonce only breaks same-block ties) — a later pin wins even when it revises the balance DOWNWARD, which both protects against stale-node scans and self-heals rows poisoned by the old bug. The now-redundant `invalidate_sync_watermark` / `credited_outputs` machinery is removed. - the pin round-trips through persistence: sqlite V002 migration adds `platform_addresses.as_of_height`; the FFI `AddressBalanceEntryFFI` carries it; Swift stores it in `PersistentPlatformAddress .lastSeenHeight` (previously a dead column). Pin 0 = "unknown provenance" (legacy rows): deltas apply as before and any pinned absolute supersedes them. Verified on testnet simulator: fresh 0.05 DASH top-up reflected exactly once and stable through automatic syncs, Clear+full-rescan, and restart; all four wallet addresses match proof-verified on-chain balances exactly; the funding pin (379731) survived a rescan whose snapshot was older (379728) — the stale-scan race the pin is designed to win. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
QuantumExplorer
requested review from
ZocoLini,
lklimek,
llbartekll and
shumkov
as code owners
|
Warning Review limit reached
Next review available in: 44 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 (13)
📝 WalkthroughWalkthroughThis PR threads a proof-committed block height through platform address transitions and persists it as ChangesAddress balance height pin
Estimated code review effort: 4 (Complex) | ~75 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 4dd2098) |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## v4.1-dev #4019 +/- ##
=============================================
+ Coverage 58.79% 87.21% +28.41%
=============================================
Files 15 2642 +2627
Lines 1966 328287 +326321
=============================================
+ Hits 1156 286330 +285174
- Misses 810 41957 +41147
🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/rs-platform-wallet-storage/tests/sqlite_persist_roundtrip.rs (1)
295-310: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRound-trip test doesn't assert
as_of_heightsurvives persistence.The fixtures set
as_of_height: 111/as_of_height: 222, but the post-reload assertions never checkrows[0].funds.as_of_height/rows[1].funds.as_of_height. Since this test exists specifically to validate the new height-pin persistence, a regression in the column read/write path could pass silently.✅ Proposed fix
assert_eq!(rows[0].address, addr1); assert_eq!(rows[0].funds.balance, 500); assert_eq!(rows[0].funds.nonce, 1); + assert_eq!(rows[0].funds.as_of_height, 111); assert_eq!(rows[1].address, addr2); assert_eq!(rows[1].funds.balance, 1500); + assert_eq!(rows[1].funds.nonce, 2); + assert_eq!(rows[1].funds.as_of_height, 222);Also applies to: 333-339
🤖 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 `@packages/rs-platform-wallet-storage/tests/sqlite_persist_roundtrip.rs` around lines 295 - 310, The sqlite_persist_roundtrip.rs round-trip test is not verifying that as_of_height survives persistence. Update the assertions in the existing reload check for the PlatformAddressBalanceEntry rows so they explicitly validate rows[0].funds.as_of_height and rows[1].funds.as_of_height match the fixture values set in the test, using the same round-trip assertion block that already checks nonce and balance.
🧹 Nitpick comments (2)
packages/rs-platform-wallet/src/wallet/platform_addresses/sync.rs (1)
200-354: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest gap: no coverage for a non-zero
absent_as_of_heightlanding in an emitted entry.Every test passes
0forabsent_as_of_height, so the pinning behavior added in this PR (lines 86-93) is only exercised with the "legacy" value. A test assertingentries[0].funds.as_of_height == <non-zero checkpoint>for the absent-zeroing path would directly cover the new field.✅ Suggested additional test
#[test] fn absent_previously_funded_address_pins_to_checkpoint_height() { let mut before = BTreeMap::new(); before.insert(tag(0), funds(294_627_247_940, 5)); let found = BTreeMap::new(); let mut absent = BTreeSet::new(); absent.insert((tag(0), p2pkh(1))); let entries = compute_address_balance_diff(&before, &found, &absent, 12_345); assert_eq!(entries.len(), 1); assert_eq!(entries[0].funds.as_of_height, 12_345); }🤖 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 `@packages/rs-platform-wallet/src/wallet/platform_addresses/sync.rs` around lines 200 - 354, Add coverage for the new absent-entry height pinning in compute_address_balance_diff by extending the absent-zeroing path test around absent_previously_funded_address_emits_zero_entry (or adding a nearby test) to pass a non-zero absent_as_of_height and assert the emitted entry’s AddressFunds.as_of_height matches that checkpoint. This should exercise the branch that creates the zeroed AddressFunds for an address in the absent set, ensuring the new field is preserved in the emitted diff.packages/rs-sdk-ffi/src/identity/transfer_to_addresses.rs (1)
184-196: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider a brief comment documenting why
_proof_heightis dropped.Sibling call sites (
transfer.rs,top_up_from_asset_lock.rs) explain that this raw path only renders proof entries without persisting, so the height pin doesn't matter here. This site lacks that context, which could confuse a future reader into thinking the height was overlooked rather than intentionally discarded.✏️ Suggested comment
wrapper.runtime.block_on(async { + // The `_proof_height` pin matters to callers that PERSIST these + // absolutes (the platform-wallet reconcile seam); this raw FFI + // path only renders the transfer result, so it drops the height. let (address_infos, identity_balance, _proof_height) = identity🤖 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 `@packages/rs-sdk-ffi/src/identity/transfer_to_addresses.rs` around lines 184 - 196, Add a brief inline comment at the transfer path in transfer_to_addresses::transfer_credits_to_addresses explaining that _proof_height is intentionally discarded because this raw FFI path only renders proof entries and does not persist them, so the height pin is unnecessary. Mirror the context used in the sibling call sites transfer.rs and top_up_from_asset_lock.rs near the destructuring of the transfer_credits_to_addresses result.
🤖 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 `@packages/rs-sdk/src/platform/transition/transfer_to_addresses.rs`:
- Line 35: The return-tuple doc for transfer_to_addresses is outdated: it still
describes only AddressInfos and Credits, while the signature now returns a third
u64 from metadata.height. Update the doc block above transfer_to_addresses so
the third bullet clearly documents this proof-committed height and how callers
use it as the AddressFunds::as_of_height pin, instead of repeating the address
infos description.
---
Outside diff comments:
In `@packages/rs-platform-wallet-storage/tests/sqlite_persist_roundtrip.rs`:
- Around line 295-310: The sqlite_persist_roundtrip.rs round-trip test is not
verifying that as_of_height survives persistence. Update the assertions in the
existing reload check for the PlatformAddressBalanceEntry rows so they
explicitly validate rows[0].funds.as_of_height and rows[1].funds.as_of_height
match the fixture values set in the test, using the same round-trip assertion
block that already checks nonce and balance.
---
Nitpick comments:
In `@packages/rs-platform-wallet/src/wallet/platform_addresses/sync.rs`:
- Around line 200-354: Add coverage for the new absent-entry height pinning in
compute_address_balance_diff by extending the absent-zeroing path test around
absent_previously_funded_address_emits_zero_entry (or adding a nearby test) to
pass a non-zero absent_as_of_height and assert the emitted entry’s
AddressFunds.as_of_height matches that checkpoint. This should exercise the
branch that creates the zeroed AddressFunds for an address in the absent set,
ensuring the new field is preserved in the emitted diff.
In `@packages/rs-sdk-ffi/src/identity/transfer_to_addresses.rs`:
- Around line 184-196: Add a brief inline comment at the transfer path in
transfer_to_addresses::transfer_credits_to_addresses explaining that
_proof_height is intentionally discarded because this raw FFI path only renders
proof entries and does not persist them, so the height pin is unnecessary.
Mirror the context used in the sibling call sites transfer.rs and
top_up_from_asset_lock.rs near the destructuring of the
transfer_credits_to_addresses result.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: d718998a-7f7f-4e13-b225-f9f6c3268ba5
📒 Files selected for processing (43)
packages/rs-platform-wallet-ffi/src/persistence.rspackages/rs-platform-wallet-ffi/src/platform_address_types.rspackages/rs-platform-wallet-ffi/src/platform_addresses/wallet.rspackages/rs-platform-wallet-storage/migrations/V002__address_height_pin.rspackages/rs-platform-wallet-storage/src/sqlite/schema/platform_addrs.rspackages/rs-platform-wallet-storage/tests/sqlite_load_reconstruction.rspackages/rs-platform-wallet-storage/tests/sqlite_persist_roundtrip.rspackages/rs-platform-wallet-storage/tests/sqlite_structural_hardening.rspackages/rs-platform-wallet/src/changeset/changeset.rspackages/rs-platform-wallet/src/changeset/serde_adapters.rspackages/rs-platform-wallet/src/wallet/apply.rspackages/rs-platform-wallet/src/wallet/identity/network/register_from_addresses.rspackages/rs-platform-wallet/src/wallet/identity/network/top_up_from_addresses.rspackages/rs-platform-wallet/src/wallet/identity/network/transfer_to_addresses.rspackages/rs-platform-wallet/src/wallet/platform_addresses/fund_from_asset_lock.rspackages/rs-platform-wallet/src/wallet/platform_addresses/mod.rspackages/rs-platform-wallet/src/wallet/platform_addresses/provider.rspackages/rs-platform-wallet/src/wallet/platform_addresses/sync.rspackages/rs-platform-wallet/src/wallet/platform_addresses/transfer.rspackages/rs-platform-wallet/src/wallet/platform_addresses/wallet.rspackages/rs-platform-wallet/src/wallet/platform_addresses/withdrawal.rspackages/rs-platform-wallet/src/wallet/platform_wallet.rspackages/rs-sdk-ffi/src/address/transitions/top_up_from_asset_lock.rspackages/rs-sdk-ffi/src/address/transitions/transfer.rspackages/rs-sdk-ffi/src/address/transitions/withdraw.rspackages/rs-sdk-ffi/src/address_sync/mod.rspackages/rs-sdk-ffi/src/identity/create_from_addresses.rspackages/rs-sdk-ffi/src/identity/top_up_from_addresses.rspackages/rs-sdk-ffi/src/identity/transfer_to_addresses.rspackages/rs-sdk/src/platform/address_sync/mod.rspackages/rs-sdk/src/platform/address_sync/types.rspackages/rs-sdk/src/platform/transition/address_credit_withdrawal.rspackages/rs-sdk/src/platform/transition/broadcast.rspackages/rs-sdk/src/platform/transition/put_identity.rspackages/rs-sdk/src/platform/transition/top_up_address.rspackages/rs-sdk/src/platform/transition/top_up_identity_from_addresses.rspackages/rs-sdk/src/platform/transition/transfer_address_funds.rspackages/rs-sdk/src/platform/transition/transfer_to_addresses.rspackages/swift-sdk/Sources/SwiftDashSDK/Persistence/Models/PersistentPlatformAddress.swiftpackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/ManagedPlatformAddressWallet.swiftpackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Services/PlatformBalanceSyncService.swiftpackages/swift-sdk/SwiftExampleApp/TEST_PLAN.md
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Solid, well-scoped fix for ADDR-09. The per-address height pin is threaded end-to-end (SDK → provider → SQLite → Swift) with strong test coverage and clear rationale. One public-API serde gap should be closed so legacy blobs still deserialize; other agent findings are either false positives (the raw FFI debug path documents dropping the height on purpose) or speculative nits not caused by this PR.
Source: reviewers claude/opus + codex/gpt-5.5 for general, security-auditor, rust-quality, and ffi-engineer; verifier claude/opus; policy gate review_policy.enforce_backport_prereq_policy.
🟡 1 suggestion(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 `packages/rs-sdk/src/platform/address_sync/types.rs`:
- [SUGGESTION] packages/rs-sdk/src/platform/address_sync/types.rs:78-80: `AddressFunds::as_of_height` breaks serde compatibility for legacy blobs
`AddressFunds` derives `Serialize`/`Deserialize` behind the `serde` feature, and this PR adds `as_of_height` without `#[serde(default)]`. Any external consumer using `dash-sdk/serde` to persist `AddressFunds` directly will now fail to deserialize pre-PR blobs, even though the field's own doc-comment explicitly promises legacy-safe semantics (`0` = "unknown provenance"). The in-tree `changeset/serde_adapters.rs` correctly uses `#[serde(default)]` on its `Wire` mirror, but the public type does not — so anything not routed through that adapter breaks. Add `#[serde(default)]` to align the derive with the documented "pin = 0 for legacy" contract.
| /// `0` means "unknown provenance" (legacy rows persisted before the | ||
| /// pin existed): every delta applies, matching pre-pin behavior. | ||
| pub as_of_height: u64, |
There was a problem hiding this comment.
🟡 Suggestion: AddressFunds::as_of_height breaks serde compatibility for legacy blobs
AddressFunds derives Serialize/Deserialize behind the serde feature, and this PR adds as_of_height without #[serde(default)]. Any external consumer using dash-sdk/serde to persist AddressFunds directly will now fail to deserialize pre-PR blobs, even though the field's own doc-comment explicitly promises legacy-safe semantics (0 = "unknown provenance"). The in-tree changeset/serde_adapters.rs correctly uses #[serde(default)] on its Wire mirror, but the public type does not — so anything not routed through that adapter breaks. Add #[serde(default)] to align the derive with the documented "pin = 0 for legacy" contract.
| /// `0` means "unknown provenance" (legacy rows persisted before the | |
| /// pin existed): every delta applies, matching pre-pin behavior. | |
| pub as_of_height: u64, | |
| /// `0` means "unknown provenance" (legacy rows persisted before the | |
| /// pin existed): every delta applies, matching pre-pin behavior. | |
| #[cfg_attr(feature = "serde", serde(default))] | |
| pub as_of_height: u64, |
source: ['claude']
There was a problem hiding this comment.
Resolved in this update — AddressFunds::as_of_height breaks serde compatibility for legacy blobs 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.
…elpers The address/identity transition helpers now return the proof's committed block height alongside AddressInfos (the balance height pin). The wasm paths only render the proof entries, so the height is intentionally unused here. Fixes the macOS workspace-tests E0308s CI caught — verified this time with a full `cargo check --workspace --all-targets`. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Source: reviewers = claude ffi-engineer opus; claude general opus; claude rust-quality opus; claude security-auditor opus; codex ffi-engineer gpt-5.5; codex general gpt-5.5; codex rust-quality gpt-5.5; codex security-auditor gpt-5.5; verifier = claude opus; specialists = security-auditor, rust-quality, ffi-engineer; policy gate = review_policy.enforce_backport_prereq_policy.
Prior reconciliation: prior-1 STILL VALID at packages/rs-sdk/src/platform/address_sync/types.rs:80 — file unchanged in latest delta. Carried-forward: AddressFunds::as_of_height still lacks #[serde(default)]. New in latest delta (wasm-sdk destructuring of proof height): nothing to flag — the wasm surface is render-only. Cumulative: one moderate-confidence single-source suggestion from codex-security-auditor about the sync-path emit lacking pin-freshness enforcement, which would let a stale full-scan clobber a fresher commit_reconciliation row.
🟡 1 suggestion(s)
1 additional finding(s) omitted (not in diff).
1 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 `packages/rs-platform-wallet/src/wallet/platform_addresses/sync.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/platform_addresses/sync.rs:49-61: Sync-path diff emits without checking pin freshness against `before`
`compute_address_balance_diff` only skips a `found` entry when `before.get(&tag) == Some(&funds)` — an exact `AddressFunds` equality check. This means a `found` entry whose `as_of_height` is *below* the snapshot's pin still emits (the pin field differs, so the funds compare unequal) and gets persisted. The freshness guard the PR introduces in `commit_reconciliation` (provider.rs, per its doc block) is therefore not mirrored here. If the address trunk/branch scan returns a stale-but-valid proof — the SDK path disables metadata freshness checks and the recent catch-up query is best-effort — the resulting older-pinned funds will clobber a fresher `commit_reconciliation` row on disk. The lock-order argument in the comment at lines 169–174 protects against interleaved writes, not against stale inputs to the sync itself. Applying the same pin-freshness rule (`funds.as_of_height >= before.get(&tag).map(|f| f.as_of_height).unwrap_or(0)`) before emitting closes the gap and keeps the mitigation symmetrical across both writers. Single-source finding (codex-security-auditor only, moderate confidence); defense-in-depth rather than a proven exploit path.
Resolves the planned overlap with #3650 (Found-025): the height-pin delta gate now lives inside the extracted `apply_op` / `apply_change` seam, so the forward pass AND the end-of-pass unknown-address replay gate identically, and the gate is unit-testable without an Sdk. - `apply_op(op, current: AddressFunds, op_height)` gates per the pin: recent changes at or below it are already inside the pinned absolute; compacted operations gate per-op on their own heights. Applying advances the pin. - the call sites pass the height a change is recorded AS OF (recent: the entry's block height; compacted: the range end) instead of the pagination cursor — the cursor-based compacted filter is subsumed by per-address pins, which also fixes the fresh-address undercount the cursor filter caused on replay (a just-derived address's true base is 0 from genesis, so its older ops must apply). - `seed_base_balances` (#3650) becomes safe by construction: the seeded `current_balances()` funds now carry their pins, so the recent replay can no longer double-count a reconciled absolute routed through the defensive seed. - #3650's tests adapted from cursor to pin semantics (real block heights instead of 0-as-no-filter); new `apply_block_changes_drops_recent_delta_already_inside_pinned_absolute` covers the exact ADDR-09 shape on the pure seam. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Code Review
Incremental + cumulative review at cfa400c2. Both prior findings from b8eedda4 remain STILL VALID and are carried forward. The latest delta rewrites the SDK address-sync apply path; I found two additional suggestion-level issues there/at the adjacent standalone FFI surface, but no blockers.
Source: reviewers = claude general opus (ok); codex general gpt-5.5 (ok); claude security-auditor opus (ok); codex security-auditor gpt-5.5 (ok); claude rust-quality opus (ok); codex rust-quality gpt-5.5 (ok); claude ffi-engineer opus (ok); codex ffi-engineer gpt-5.5 (ok); verifier = claude opus (ok); specialists = security-auditor, rust-quality, ffi-engineer; policy gate = review_policy.enforce_backport_prereq_policy.
🟡 4 suggestion(s)
Prior Reconciliation
prior-1STILL VALID: Verified at packages/rs-sdk/src/platform/address_sync/types.rs:52,80 — struct still derives Serialize/Deserialize behind the serde feature and as_of_height still lacks #[serde(default)].prior-2STILL VALID: Verified at packages/rs-platform-wallet/src/wallet/platform_addresses/sync.rs:49-96 — the found loop skips only whenbefore.get(&tag) == Some(&funds)(exact equality) and the absent-zeroing loop only checks that before had non-default funds; neither branch compares the emitted pin to the snapshot pin.
Carried-Forward Prior Findings
- [SUGGESTION]
packages/rs-sdk/src/platform/address_sync/types.rs:78-80: AddressFunds::as_of_height breaks serde compatibility for legacy blobs
AddressFunds derives Serialize/Deserialize behind theserdefeature (line 52) but the newly addedas_of_heightfield has no#[serde(default)]. The doc-comment on lines 78–79 explicitly promises legacy-safe semantics (pin0= "unknown provenance", every delta applies), but any external consumer ofdash-sdk/serdethat persists AddressFunds directly (JS/Swift bindings via serde_json, downstream Rust caches, on-disk snapshots written pre-PR) will fail to deserialize with amissing field "as_of_height"error rather than loading with the documented pin-0 legacy contract. The in-treechangeset/serde_adapters.rs::Wiremirror already uses#[serde(default)], so the wallet-changeset path is fine — but the public SDK type is not routed through that adapter. Adding#[serde(default)]to the field aligns the derive with the documented contract. - [SUGGESTION]
packages/rs-platform-wallet/src/wallet/platform_addresses/sync.rs:48-96: compute_address_balance_diff can persist stale-pinned scan results over fresher rows
The found loop at lines 49-61 skips only whenbefore.get(&tag) == Some(&funds)— a byte-for-byte AddressFunds equality that includesas_of_height. A found entry whoseas_of_heightis older than the snapshot pin still compares unequal and is emitted, so a stale-but-valid trunk/branch scan proof can overwrite a freshercommit_reconciliationrow on disk. The absent-zeroing loop at lines 65-96 has the same shape — it emits a zero pinned atabsent_as_of_heightwithout checking that the scan checkpoint is newer than the snapshot pin.commit_reconciliationin provider.rs already applies the height-freshness rule (existing.as_of_height > entry.funds.as_of_height ⇒ stale); mirroring that rule here keeps both writers into the same rows consistent. The lock ordering at sync.rs:169-174 serializes concurrent writers but does not filter stale inputs to this writer. Skip the emit whenfunds.as_of_height < before.get(&tag).map(|f| f.as_of_height).unwrap_or(0)on the found branch, and whenabsent_as_of_height < …on the absent branch. Defense-in-depth against DAPI node lag / eclipse routing — the PR description itself calls out that a scan at height 379728 can arrive alongside a funding proof pinned at 379731 in the wild.
New Findings In Latest Delta / Current Head
- [SUGGESTION]
packages/rs-sdk/src/platform/address_sync/mod.rs:958-984: pending_unknown accumulates foreign-wallet changes without a hard cap on shared chains
The new unknown-address path clones every unmatched recent/compacted change intopending_unknownand the in-code comment explicitly leaves the buffer unbounded, relying on a one-shot warn at PENDING_UNKNOWN_WARN_THRESHOLD (1000). On a shared/busy chain the incoming stream is global: most changes seen during pagination belong to other wallets and remain buffered until the end-of-pass refresh, where they get discarded at theunknown_keysintersection. The comment correctly notes that a bounded-memory fix would need to re-query, not just stash keys, so replacing the design isn't trivial — but for mobile/embedded consumers, letting one wallet sync retain unbounded foreign owned-changes across a long catch-up window is a real memory-pressure risk (OwnedBalanceOp::Compactedcarries a Vec of arbitrary length). Consider either (a) tightening to a hard byte-count cap that triggers a mid-pass drain + refresh, or (b) draining pending_unknown after each pagination page rather than at end-of-pass so foreign changes are discarded promptly. The current tradeoff is defensible for well-behaved networks, so this is a hardening suggestion rather than a blocker — but the warn hook alone doesn't prevent the exhaustion it detects. - [SUGGESTION]
packages/rs-sdk-ffi/src/address_sync/types.rs:93-108: Standalone Swift batch sync (DashSDKFoundAddress) drops the new as_of_height pin
dash_sdk_sync_addresses_batch_with_resultreturnsDashSDKFoundAddressover the C boundary and Swift feeds those values back asknownBalanceson later calls. The C struct exposes only index/key/nonce/balance — the per-addressas_of_heightthat this PR introduces on AddressFunds is not carried across. When the Rust side rebuilds AddressFunds from FFI inputs it fillsas_of_height: 0, which per the field's own doc-comment means "unknown provenance ⇒ every delta applies". Standalone Swift consumers of this batch API therefore get pre-PR delta-replay semantics across persistence/restart, even though the platform-wallet FFI path (AddressBalanceEntryFFI) does round-trip the pin. Add aas_of_height: u64field to DashSDKFoundAddress (mirror it on the Swift Codable) so ADDR-09 replay protection extends to that surface too.
🤖 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-sdk/src/platform/address_sync/types.rs`:
- [SUGGESTION] packages/rs-sdk/src/platform/address_sync/types.rs:78-80: AddressFunds::as_of_height breaks serde compatibility for legacy blobs
AddressFunds derives Serialize/Deserialize behind the `serde` feature (line 52) but the newly added `as_of_height` field has no `#[serde(default)]`. The doc-comment on lines 78–79 explicitly promises legacy-safe semantics (pin `0` = "unknown provenance", every delta applies), but any external consumer of `dash-sdk/serde` that persists AddressFunds directly (JS/Swift bindings via serde_json, downstream Rust caches, on-disk snapshots written pre-PR) will fail to deserialize with a `missing field "as_of_height"` error rather than loading with the documented pin-0 legacy contract. The in-tree `changeset/serde_adapters.rs::Wire` mirror already uses `#[serde(default)]`, so the wallet-changeset path is fine — but the public SDK type is not routed through that adapter. Adding `#[serde(default)]` to the field aligns the derive with the documented contract.
In `packages/rs-platform-wallet/src/wallet/platform_addresses/sync.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/platform_addresses/sync.rs:48-96: compute_address_balance_diff can persist stale-pinned scan results over fresher rows
The found loop at lines 49-61 skips only when `before.get(&tag) == Some(&funds)` — a byte-for-byte AddressFunds equality that includes `as_of_height`. A found entry whose `as_of_height` is *older* than the snapshot pin still compares unequal and is emitted, so a stale-but-valid trunk/branch scan proof can overwrite a fresher `commit_reconciliation` row on disk. The absent-zeroing loop at lines 65-96 has the same shape — it emits a zero pinned at `absent_as_of_height` without checking that the scan checkpoint is newer than the snapshot pin. `commit_reconciliation` in provider.rs already applies the height-freshness rule (`existing.as_of_height > entry.funds.as_of_height ⇒ stale`); mirroring that rule here keeps both writers into the same rows consistent. The lock ordering at sync.rs:169-174 serializes concurrent writers but does not filter stale inputs to this writer. Skip the emit when `funds.as_of_height < before.get(&tag).map(|f| f.as_of_height).unwrap_or(0)` on the found branch, and when `absent_as_of_height < …` on the absent branch. Defense-in-depth against DAPI node lag / eclipse routing — the PR description itself calls out that a scan at height 379728 can arrive alongside a funding proof pinned at 379731 in the wild.
In `packages/rs-sdk/src/platform/address_sync/mod.rs`:
- [SUGGESTION] packages/rs-sdk/src/platform/address_sync/mod.rs:958-984: pending_unknown accumulates foreign-wallet changes without a hard cap on shared chains
The new unknown-address path clones every unmatched recent/compacted change into `pending_unknown` and the in-code comment explicitly leaves the buffer unbounded, relying on a one-shot warn at PENDING_UNKNOWN_WARN_THRESHOLD (1000). On a shared/busy chain the incoming stream is *global*: most changes seen during pagination belong to other wallets and remain buffered until the end-of-pass refresh, where they get discarded at the `unknown_keys` intersection. The comment correctly notes that a bounded-memory fix would need to re-query, not just stash keys, so replacing the design isn't trivial — but for mobile/embedded consumers, letting one wallet sync retain unbounded foreign owned-changes across a long catch-up window is a real memory-pressure risk (`OwnedBalanceOp::Compacted` carries a Vec of arbitrary length). Consider either (a) tightening to a hard byte-count cap that triggers a mid-pass drain + refresh, or (b) draining pending_unknown after each pagination page rather than at end-of-pass so foreign changes are discarded promptly. The current tradeoff is defensible for well-behaved networks, so this is a hardening suggestion rather than a blocker — but the warn hook alone doesn't prevent the exhaustion it detects.
In `packages/rs-sdk-ffi/src/address_sync/types.rs`:
- [SUGGESTION] packages/rs-sdk-ffi/src/address_sync/types.rs:93-108: Standalone Swift batch sync (DashSDKFoundAddress) drops the new as_of_height pin
`dash_sdk_sync_addresses_batch_with_result` returns `DashSDKFoundAddress` over the C boundary and Swift feeds those values back as `knownBalances` on later calls. The C struct exposes only index/key/nonce/balance — the per-address `as_of_height` that this PR introduces on AddressFunds is not carried across. When the Rust side rebuilds AddressFunds from FFI inputs it fills `as_of_height: 0`, which per the field's own doc-comment means "unknown provenance ⇒ every delta applies". Standalone Swift consumers of this batch API therefore get pre-PR delta-replay semantics across persistence/restart, even though the platform-wallet FFI path (AddressBalanceEntryFFI) does round-trip the pin. Add a `as_of_height: u64` field to DashSDKFoundAddress (mirror it on the Swift Codable) so ADDR-09 replay protection extends to that surface too.
| account_index, | ||
| address_index, | ||
| address: p2pkh, | ||
| // Absent in state ⇒ no balance and no nonce. Reset both. | ||
| // Absent in state ⇒ no balance and no nonce. Reset both. The | ||
| // absence was proven by the tree scan, so the zero is pinned | ||
| // at the scan's checkpoint height (`absent_as_of_height`) — | ||
| // a later re-credit carries a higher pin and wins. | ||
| funds: AddressFunds { | ||
| balance: 0, | ||
| nonce: 0, | ||
| as_of_height: absent_as_of_height, | ||
| }, | ||
| }); | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: compute_address_balance_diff can persist stale-pinned scan results over fresher rows
The found loop at lines 49-61 skips only when before.get(&tag) == Some(&funds) — a byte-for-byte AddressFunds equality that includes as_of_height. A found entry whose as_of_height is older than the snapshot pin still compares unequal and is emitted, so a stale-but-valid trunk/branch scan proof can overwrite a fresher commit_reconciliation row on disk. The absent-zeroing loop at lines 65-96 has the same shape — it emits a zero pinned at absent_as_of_height without checking that the scan checkpoint is newer than the snapshot pin. commit_reconciliation in provider.rs already applies the height-freshness rule (existing.as_of_height > entry.funds.as_of_height ⇒ stale); mirroring that rule here keeps both writers into the same rows consistent. The lock ordering at sync.rs:169-174 serializes concurrent writers but does not filter stale inputs to this writer. Skip the emit when funds.as_of_height < before.get(&tag).map(|f| f.as_of_height).unwrap_or(0) on the found branch, and when absent_as_of_height < … on the absent branch. Defense-in-depth against DAPI node lag / eclipse routing — the PR description itself calls out that a scan at height 379728 can arrive alongside a funding proof pinned at 379731 in the wild.
| account_index, | |
| address_index, | |
| address: p2pkh, | |
| // Absent in state ⇒ no balance and no nonce. Reset both. | |
| // Absent in state ⇒ no balance and no nonce. Reset both. The | |
| // absence was proven by the tree scan, so the zero is pinned | |
| // at the scan's checkpoint height (`absent_as_of_height`) — | |
| // a later re-credit carries a higher pin and wins. | |
| funds: AddressFunds { | |
| balance: 0, | |
| nonce: 0, | |
| as_of_height: absent_as_of_height, | |
| }, | |
| }); | |
| } | |
| // 1. Found-and-changed. | |
| for (&(tag, p2pkh), &funds) in found { | |
| if let Some(existing) = before.get(&tag) { | |
| // Skip when unchanged, or when the newly scanned pin is | |
| // older than the already committed pin — a stale-but-valid | |
| // proof must not clobber a fresher row. | |
| if existing == &funds || existing.as_of_height > funds.as_of_height { | |
| continue; | |
| } | |
| } | |
| let (wallet_id, account_index, address_index) = tag; | |
| entries.push(PlatformAddressBalanceEntry { | |
| wallet_id, | |
| account_index, | |
| address_index, | |
| address: p2pkh, | |
| funds, | |
| }); | |
| } | |
| // 2. Absent-but-previously-funded — zero out cached balances that the | |
| // new chain no longer knows about. | |
| for &(tag, p2pkh) in absent { | |
| // Absent at the checkpoint but re-found during incremental | |
| // catch-up — the find is newer (tip), so never zero it. | |
| if found.contains_key(&(tag, p2pkh)) { | |
| continue; | |
| } | |
| let Some(existing) = before.get(&tag) else { | |
| continue; | |
| }; | |
| // Only emit when we actually had non-default cached funds for the | |
| // address. An address that was already empty (or never cached) | |
| // doesn't need a zeroing write. | |
| if existing.balance == 0 && existing.nonce == 0 { | |
| continue; | |
| } | |
| // A stale absence proof must not clobber a row pinned by a newer | |
| // proof or transition reconciliation. | |
| if existing.as_of_height > absent_as_of_height { | |
| continue; | |
| } | |
| let (wallet_id, account_index, address_index) = tag; | |
| entries.push(PlatformAddressBalanceEntry { | |
| wallet_id, | |
| account_index, | |
| address_index, | |
| address: p2pkh, | |
| // Absent in state ⇒ no balance and no nonce. Reset both. The | |
| // absence was proven by the tree scan, so the zero is pinned | |
| // at the scan's checkpoint height (`absent_as_of_height`) — | |
| // a later re-credit carries a higher pin and wins. | |
| funds: AddressFunds { | |
| balance: 0, | |
| nonce: 0, | |
| as_of_height: absent_as_of_height, | |
| }, | |
| }); | |
| } |
source: ['claude', 'codex', 'prior']
There was a problem hiding this comment.
Good catch — taken in 78a16ce, with one extension: the suggestion as written guards only the persist diff, which would leave sync_finished's scratch merge (found.extend(...)) regressing the in-memory seed while disk holds the fresher row — a disk-vs-memory divergence in exactly the protected scenario. So the height-freshness rule now lands symmetrically in both writers:
compute_address_balance_diff: your two skips (older-pinned found entry; absence zeroing whose scan checkpoint is older than the committed pin), essentially as suggested.sync_finished: per-entry scratch merge keeping the committed entry when its pin is fresher.
Absent-removal stays unguarded in memory (absence carries no per-entry height at the provider layer); the diff guard protects the durable row, and the next pass reconstructs the in-memory entry from a full replay (base 0, pin 0 → every delta applies).
Worth noting the blast radius even unguarded was transient: the regressed row keeps its old pin, so the next pass on a healthy node re-applies the missing delta and self-heals within one cycle — but no reason to allow the window at all. Three new tests cover the stale-pin found skip, the stale-checkpoint absence skip, and the merge freshness rule (plus newer-pin paths still landing).
There was a problem hiding this comment.
Resolved in this update — compute_address_balance_diff can persist stale-pinned scan results over fresher rows 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.
…node scans Review follow-up (thepastaclaw on #4019): a full scan against a lagging node can produce a stale-but-valid absolute (older pin) for a row a fresher reconcile already committed — and both of the sync's writers would take it: `compute_address_balance_diff` emitted it to the durable rows, and `sync_finished`'s scratch merge overwrote the in-memory seed. The regression self-heals within one pass (the regressed row keeps its old pin, so the next replay re-applies the missing delta), but there is no reason to let it happen at all. Apply `commit_reconciliation`'s height-freshness rule to BOTH writers, symmetrically — guarding only the persist diff (as suggested) would leave memory regressing while disk holds the fresher row, i.e. a disk-vs-memory divergence in exactly the protected scenario: - `compute_address_balance_diff`: skip a found entry whose pin is older than the committed row's, and skip an absence zeroing whose scan checkpoint is older (on a lagging node an address funded seconds ago legitimately does not exist yet at the checkpoint). - `sync_finished`: merge scratch `found` per entry, keeping the committed entry when its pin is fresher. Absent-removal stays unguarded in memory (absence carries no per-entry height); the diff guard protects the durable row and the next pass reconstructs the in-memory entry from a full replay. Tests: stale-pin found skip, stale-checkpoint absence skip, and the scratch-merge freshness rule (plus the newer-pin paths still landing). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Review follow-up: the `transfer_credits_to_addresses` return doc still listed the old two elements (with a duplicated third bullet) after the signature grew the proof's committed block height. Document the third element as the balance height pin (`AddressFunds::as_of_height`) and what persisting callers must do with it; same staleness fixed on `TopUpAddress::top_up`, whose "returns AddressInfos" line predated the `(AddressInfos, u64)` tuple. Also qualify the pre-existing unresolved `AddressProvider::last_known_recent_block_height` intra-doc link in address_sync/types.rs (module path was missing, so rustdoc could not resolve it). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
CI caught AddressBalancePersistTests still using the pre-pin 6-tuple shapes of persistAddressBalances / loadCachedBalances. Update the entries and the loadedRow helper to the 7-tuple, give the fixtures realistic proof heights, and add an assertion that the pin round-trips into PersistentPlatformAddress.lastSeenHeight — so the new field's Swift persistence has direct coverage instead of just compiling. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Code Review
The delta from 78a16ce..a93f7ca is documentation-only in Rust (doc-comment clarifications on top_up_address / transfer_to_addresses and one intra-doc link fix on address_sync/types.rs) plus a Swift persist-test update to the 7-tuple shape asserting the pin round-trips into lastSeenHeight. No new Rust logic or FFI signatures changed. Prior-1 (AddressFunds::as_of_height lacks #[serde(default)]) and prior-3 (pending_unknown unbounded, intentionally deferred by author) remain STILL VALID and are carried forward. No new blockers.
Source: reviewers = claude general opus (ok); codex general gpt-5.5 (ok); claude security-auditor opus (ok); codex security-auditor gpt-5.5 (ok); claude rust-quality opus (ok); codex rust-quality gpt-5.5 (ok); claude ffi-engineer opus (ok); codex ffi-engineer gpt-5.5 (ok); verifier = claude opus (ok); specialists = security-auditor, rust-quality, ffi-engineer; policy gate = review_policy.enforce_backport_prereq_policy.
🟡 2 suggestion(s)
Prior Reconciliation
prior-1STILL VALID: verified atpackages/rs-sdk/src/platform/address_sync/types.rs:52,80;AddressFundsstill derives serde when the feature is enabled, andas_of_heightstill lacks#[serde(default)].prior-3STILL VALID: verified atpackages/rs-sdk/src/platform/address_sync/mod.rs:958-984;pending_unknownstill appends unmatched global changes into an intentionally unbounded buffer with only the one-shot threshold warning.
Carried-Forward Prior Findings
- [SUGGESTION]
packages/rs-sdk/src/platform/address_sync/types.rs:78-80: AddressFunds::as_of_height breaks serde compatibility for legacy blobs
AddressFunds derives Serialize/Deserialize behind theserdefeature (line 52), but the newas_of_heightfield on line 80 has no#[serde(default)]. The field's own doc-comment on lines 78–79 explicitly promises legacy-safe semantics — pin0= 'unknown provenance', every delta applies, matching pre-pin behavior. Without the serde default, any externaldash-sdk/serdeconsumer decoding a pre-PR persisted blob (JS/Swift wrappers, downstream Rust caches, on-disk snapshots) fails withmissing field "as_of_height"instead of loading the documented pin-0 fallback. The in-tree wallet-changeset path is safe becausechangeset/serde_adapters.rs::Wireuses#[serde(default)], but the public SDK type is not routed through that adapter. Adding the attribute aligns the derive with the documented contract. Carried forward from prior review; the latest delta touched this file only for an intra-doc link and did not address this. - [SUGGESTION]
packages/rs-sdk/src/platform/address_sync/mod.rs:958-984: pending_unknown accumulates foreign-wallet changes without a hard cap
The unknown-address branch clones every unmatched recent/compacted balance change intopending_unknownand the in-line comment (964–976) intentionally leaves the buffer unbounded, relying on a one-shot warn at PENDING_UNKNOWN_WARN_THRESHOLD (1000). The recent/compacted streams are global to the chain, so on a busy shared chain most entries belong to other wallets and remain buffered until the end-of-pass refresh intersects them withunknown_keys.OwnedBalanceOp::Compactedcarries aVecof arbitrary length, so for mobile/embedded consumers on a long catch-up window this is a real memory-pressure surface; a stale/eclipsing DAPI route could amplify it. The author's comment correctly notes that a bounded-memory fix must re-query rather than merely stash keys, so this remains a hardening suggestion — but the warn hook only observes the condition, it does not prevent exhaustion. Consider a hard byte-count cap that triggers a mid-pass drain + refresh, or drainingpending_unknownafter each pagination page rather than end-of-pass.
New Findings In Latest Delta
- None. The latest delta from
78a16ceatoa93f7cafonly updates Rust docs/intra-doc links and Swift persistence tests for the height-pin tuple.
🤖 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-sdk/src/platform/address_sync/types.rs`:
- [SUGGESTION] packages/rs-sdk/src/platform/address_sync/types.rs:78-80: AddressFunds::as_of_height breaks serde compatibility for legacy blobs
AddressFunds derives Serialize/Deserialize behind the `serde` feature (line 52), but the new `as_of_height` field on line 80 has no `#[serde(default)]`. The field's own doc-comment on lines 78–79 explicitly promises legacy-safe semantics — pin `0` = 'unknown provenance', every delta applies, matching pre-pin behavior. Without the serde default, any external `dash-sdk/serde` consumer decoding a pre-PR persisted blob (JS/Swift wrappers, downstream Rust caches, on-disk snapshots) fails with `missing field "as_of_height"` instead of loading the documented pin-0 fallback. The in-tree wallet-changeset path is safe because `changeset/serde_adapters.rs::Wire` uses `#[serde(default)]`, but the public SDK type is not routed through that adapter. Adding the attribute aligns the derive with the documented contract. Carried forward from prior review; the latest delta touched this file only for an intra-doc link and did not address this.
In `packages/rs-sdk/src/platform/address_sync/mod.rs`:
- [SUGGESTION] packages/rs-sdk/src/platform/address_sync/mod.rs:958-984: pending_unknown accumulates foreign-wallet changes without a hard cap
The unknown-address branch clones every unmatched recent/compacted balance change into `pending_unknown` and the in-line comment (964–976) intentionally leaves the buffer unbounded, relying on a one-shot warn at PENDING_UNKNOWN_WARN_THRESHOLD (1000). The recent/compacted streams are global to the chain, so on a busy shared chain most entries belong to other wallets and remain buffered until the end-of-pass refresh intersects them with `unknown_keys`. `OwnedBalanceOp::Compacted` carries a `Vec` of arbitrary length, so for mobile/embedded consumers on a long catch-up window this is a real memory-pressure surface; a stale/eclipsing DAPI route could amplify it. The author's comment correctly notes that a bounded-memory fix must re-query rather than merely stash keys, so this remains a hardening suggestion — but the warn hook only observes the condition, it does not prevent exhaustion. Consider a hard byte-count cap that triggers a mid-pass drain + refresh, or draining `pending_unknown` after each pagination page rather than end-of-pass.
There was a problem hiding this comment.
Code Review
Incremental + cumulative review at 4dd20980. The latest delta a93f7caf..4dd20980 is a broad v4.1-dev merge and does not change the ADDR-09 height-pin files. Both prior findings remain STILL VALID and are carried forward; the verifier also kept one additional cumulative current-head suggestion around stale absence clearing in the in-memory provider state.
Source: reviewers = claude general opus (ok); codex general gpt-5.5 (ok); claude security-auditor opus (ok); codex security-auditor gpt-5.5 (ok); claude rust-quality opus (ok); codex rust-quality gpt-5.5 (ok); claude ffi-engineer opus (ok); codex ffi-engineer gpt-5.5 (ok); verifier = claude opus (ok); specialists = security-auditor, rust-quality, ffi-engineer; policy gate = review_policy.enforce_backport_prereq_policy.
3 suggestion(s)
Prior Reconciliation
prior-1STILL VALID:packages/rs-sdk/src/platform/address_sync/types.rs:52still derives serde behind theserdefeature, andas_of_heightat line 80 still lacks#[serde(default)]; the latest delta did not modify this file.prior-2STILL VALID:packages/rs-sdk/src/platform/address_sync/mod.rs:958-984still appends unmatched global changes intopending_unknownwith only the one-shot warning threshold and no hard cap; the latest delta did not modify this file.
Carried-Forward Prior Findings
- [SUGGESTION]
packages/rs-sdk/src/platform/address_sync/types.rs:78-80: AddressFunds::as_of_height breaks serde compatibility for legacy blobs
AddressFundsderivesSerialize/Deserializebehind theserdefeature, but the newas_of_height: u64field has no#[serde(default)]. The field docs define pin0as the legacy/unknown-provenance fallback, so externaldash-sdk/serdeconsumers decoding pre-PR blobs can fail withmissing field "as_of_height"instead of loading the documented pin-0 behavior. The in-tree wallet changeset adapter is safe, but the public SDK type is not routed through that adapter. - [SUGGESTION]
packages/rs-sdk/src/platform/address_sync/mod.rs:958-984: pending_unknown accumulates foreign-wallet changes without a hard cap
The unknown-address branch clones every unmatched recent/compacted balance change intopending_unknownuntil the end-of-pass refresh. Recent/compacted streams are global to the chain, so busy shared-chain catch-up can buffer mostly foreign-wallet changes;OwnedBalanceOp::Compactedcan also carry an arbitrary-lengthVec. The existing warning observes the condition but does not bound memory. A byte-count cap with mid-pass drain/refresh, or per-page draining, would close the hardening gap.
New Findings In Latest Delta
- None. The latest delta is a merge from
v4.1-dev; it does not touch the two carried-forward ADDR-09 files.
Additional Cumulative Current-Head Finding
- [SUGGESTION]
packages/rs-platform-wallet/src/wallet/platform_addresses/provider.rs:840-847: Stale absence pass still clears in-memory found entries even when durable rows are pin-fresher
Insync_finished, thefoundmerge enforces height-pin freshness, but the following absent-removal is not pin-guarded: every address inaccount_scratch.absentis removed fromaccount_state.found, andon_address_absenthas already zeroed the managed-account balance cache. In the stale-node race this PR targets, disk keeps the fresher pinned row, but the active in-memory session can lose the funded address until reload or a later repair pass. The code comment documents this tradeoff, so this remains a suggestion rather than a blocker; consider skipping removal when the committed pin is fresher than the pass, mirroring the found-merge rule.
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-sdk/src/platform/address_sync/types.rs`:
- [SUGGESTION] packages/rs-sdk/src/platform/address_sync/types.rs:78-80: AddressFunds::as_of_height breaks serde compatibility for legacy blobs
`AddressFunds` derives `Serialize`/`Deserialize` behind the `serde` feature (types.rs:52), but the new `as_of_height: u64` field at line 80 has no `#[serde(default)]`. The field's own doc-comment on lines 78-79 documents pin `0` as "unknown provenance" -- the legacy value for rows persisted before the pin existed. Without the serde default, any external `dash-sdk/serde` consumer decoding a pre-PR persisted blob fails instead of loading at the documented pin-0 fallback.
In `packages/rs-sdk/src/platform/address_sync/mod.rs`:
- [SUGGESTION] packages/rs-sdk/src/platform/address_sync/mod.rs:958-984: pending_unknown accumulates foreign-wallet changes without a hard cap
The unknown-address branch clones every unmatched recent/compacted balance change into `pending_unknown` until end-of-pass refresh. The recent/compacted streams are global to the chain, so on a busy shared chain most entries belong to foreign wallets and remain buffered indefinitely. The one-shot warning observes but does not bound memory.
In `packages/rs-platform-wallet/src/wallet/platform_addresses/provider.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/platform_addresses/provider.rs:840-847: Stale absence pass still clears in-memory found entries even when durable rows are pin-fresher
The found merge enforces height-pin freshness, but the absent-removal path is not pin-guarded. A stale-node absence can remove the in-memory found entry and zero the managed-account balance cache even though durable storage keeps the fresher pinned row.
| // Absence carries no per-entry height, so a stale pass's | ||
| // removal is NOT pin-guarded here; the persist diff skips | ||
| // the durable zero for fresher-pinned rows, and the next | ||
| // pass reconstructs the in-memory entry from a full | ||
| // replay (base 0, pin 0 → every delta applies). | ||
| for absent_addr in &account_scratch.absent { | ||
| account_state.found.remove(absent_addr); | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: Stale absence pass still clears in-memory found entries even when durable rows are pin-fresher
In sync_finished, the found merge at lines 832–838 correctly enforces height-pin freshness (the fresher committed entry wins). The following absent-removal at lines 845–847 is NOT pin-guarded: every address in account_scratch.absent is removed from account_state.found, and on_address_absent (line 788) has already zeroed the managed-account balance cache. In the stale-node race this PR is designed to handle, disk keeps the fresher pinned row (the persist diff at compute_address_balance_diff guards it), but the in-memory session loses the funded address. Spending/selection reads current_balances() from that same in-memory found map, so a valid funded address can disappear until reload or a later repair pass. The author's comment at lines 840–844 documents this as intentional — noting the next pass will reconstruct from full replay — so this is a hardening suggestion rather than a blocker; consider skipping the removal for entries whose committed pin is fresher than the pass, mirroring the found-merge rule.
source: ['codex']
…(SH-06 credit) Root cause of the on-device SH-06 gap (a confirmed unshield credit never reflected app-side): build_wallet_restore_entry hardcoded platform_address_balances to null/0, so persisted platform-address balances were never rehydrated into the Rust provider on load. After a restart the BLAST loop trusts its watermark but has an empty per-account balance map; the tree-scan re-pins each address at checkpoint_height and the #4019 ADDR-09 gate (op_height <= as_of_height) then discards the credit delta — the balance stays 0 forever. iOS is unaffected (its Swift loadCachedBalances feeds (balance, as_of_height) back), which is why the fix belongs in the Android load path only, not core. - PlatformWalletPersistenceHandler.buildWalletRestoreData now reads the wallet's platform_addresses Room rows into a new platformAddressBalances restore list (asOfHeight = lastSeenHeight). - NativePersistenceBridge gains PlatformAddressBalanceRestoreData +ing the WalletRestoreData field; JNI build_wallet_restore_entry marshals it into WalletRestoreEntryFFI.platform_address_balances via the staging-vecs discipline (AddressBalanceEntryFFI is Copy POD; sealed + freed like ignored_senders). - as_of_height round-trips verbatim, so the height-pin still prevents the #4019 double-count; a genuine 0 ("unknown provenance") self-heals to the first pinned absolute on the next sync. Round-trip unit test added. Not a distinct pre-restart bug: the persisted-balance map is the backstop that keeps a tracked address in the sync pending-set once it falls out of the live gap window; restoring it fixes both the post-restart and the out-of-window paths. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
2 participants
Issue being fixed or feature implemented
ADDR-09 behavioral QA (2026-07-06) showed the top-up double-count is not fixed by the watermark approach (#4004/#4005): after "Top Up from Core", the topped-up platform address showed exactly 2× its proof-verified on-chain balance, durable across automatic BLAST syncs, manual Clear+resync, and app restart. Ground truth: on-chain (via
getWithProof) held the single credit; the client double-counted.Root cause: two sources of truth — proof-attested absolutes (ST results, trunk/branch scan elements) and the recent/compacted delta stream — were reconciled through global height cursors. Any path that threaded the wrong height re-applied a delta that was already baked into an absolute. #4004/#4005 forced a full rescan via watermark invalidation, but the rescan itself replays the recent window and re-added the same delta.
What was done?
Every absolute balance now carries a height pin —
AddressFunds::as_of_height, the Platform block height the balance is current as of:address_sync): a delta at blockBapplies only whenB > pin; applying advances the pin. Recent loop gates per entry; compacted loop gates per operation (block-aware ops carry(height, credits), so a pin inside a compacted range drops exactly the already-included ops).checkpoint_height; absence-zeroing entries too.BroadcastStateTransitiongains*_with_metadatavariants exposing the quorum-authenticatedmetadata.height(previously verified then discarded); all address/identity transition helpers return it alongsideAddressInfos.reconcile_address_infos(infos, as_of_height, ctx)pins ST-attested absolutes; freshness = pin ordering (nonce breaks same-block ties only). A later pin wins even when it revises the balance downward — protecting against stale-node scans and self-healing rows poisoned by the old bug. The now-redundantinvalidate_sync_watermark/credited_outputsmachinery is deleted.V002migration addsplatform_addresses.as_of_height;AddressBalanceEntryFFI.as_of_height; Swift stores it inPersistentPlatformAddress.lastSeenHeight(previously a dead column). Pin0= "unknown provenance" (legacy rows): deltas apply as before and any pinned absolute supersedes them.How was it tested?
dash-sdk,platform-wallet,platform-wallet-storage,platform-wallet-ffi,rs-sdk-ffi, including newcommit_reconciliation_later_pin_revises_balance_downward(the healing property) andcommit_reconciliation_drops_stale_height.Breaking changes
None consensus-wise. SDK API: address/identity transition helpers now return the proof height (tuple extension);
reconcile_address_infostakesas_of_heightinstead ofcredited_outputs.Checklist
For repository code-owners and collaborators only
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes