feat(platform-wallet): watermark monotonic-merge#3647
Conversation
…accessor Make incremental-sync watermark updates monotonic so concurrent or out-of-order sync completions can never roll a watermark backward. `update_sync_state` now takes the per-field `max()` of the existing watermark and the incoming `AddressSyncResult` (height, timestamp, last_known_recent_block) instead of blind assignment. Add a `pub last_known_recent_block()` provider accessor and `PlatformAddressWallet::sync_watermark()` so callers can read the watermark without going through the `AddressProvider` trait. Includes an in-file unit-test module covering forward advance, backward rejection, per-field merge, and unconditional load-time overwrite. Carved out of #3549 (cluster G, SHA 59cba08 + merges) as a standalone production change. No e2e tests, no rust-dashcore bump: verified `cargo build`/`cargo clippy -p platform-wallet` clean on v3.1-dev against rust-dashcore 53130869 (the watermark logic uses only integer max() and pre-existing types). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR updates sync watermark handling in the platform address provider to use monotonic per-field merges during incremental updates, preventing watermark regression on out-of-order or stale sync completions. A new public ChangesWatermark Monotonicity Guarantee
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
lklimek
changed the title
Review GateCommit:
|
lklimek
added
the
ready for final review
|
Codex here. I reviewed this and I do not currently see the race that this PR is described as fixing in the active wallet call path.
The monotonic |
QuantumExplorer
left a comment
QuantumExplorer
left a comment
There was a problem hiding this comment.
Need the reason for this PR before merging it. Did we actually see something wrong?
Main goal is to add the sync_watermark() getter which I need to check sync progress. We cannot read it from the event, as the event only fires from the periodic PlatformAddressSyncManager pass — not from the direct sync_balances() path the tests (PA-007/PA-007b) actually call. Test: https://github.com/dashpay/platform/blob/7e57f7227a354fb547a58d2abe4727cdf62204fc/packages/rs-platform-wallet/tests/e2e/cases/pa_007_sync_watermark.rs |
|
✅ DashSDKFFI.xcframework built for this PR.
SwiftPM (host the zip at a stable URL, then use): .binaryTarget(
name: "DashSDKFFI",
url: "https://your.cdn.example/DashSDKFFI.xcframework.zip",
checksum: "d8c6a536cedda251de694a228f8d205f435e868e361d5d014c0d0614dc7beb45"
)Xcode manual integration:
|
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Confirmed one blocking issue: the new per-field max() merge in PlatformPaymentAddressProvider::update_sync_state breaks last_known_recent_block's sentinel-zero contract with the SDK, which treats 0 as 'fall back to RangeFrom (inclusive)'. Preserving an older non-zero value when a later sync returns 0 produces a stale RangeAfter cursor that can replay non-idempotent recent-tree AddToCredits operations. Several quality suggestions and nitpicks around the related apply_sync_state replay path, naming/docs of new sync_watermark, and a test-helper clippy nit are kept.
Reviewed commit: 9f5319d
🔴 1 blocking | 🟡 4 suggestion(s) | 💬 3 nitpick(s)
_Note: Inline posting via review_poster.py failed ( _gh(
File "/Users/claw/.openclaw/workspace/scripts/review_poster.py", line 23, in gh
raise RuntimeError(f"gh {' '.join(args)} failed: {result.stderr.strip()}")
RuntimeError: gh api /repos/dashpay/platform/pulls/3647/reviews --method POST --input - failed: gh: Unprocessable Entity (HTTP 422)
), so I posted the same verified findings as a top-level review body.
Verified findings
blocking: Per-field max() on last_known_recent_block breaks the SDK's sentinel-zero contract
packages/rs-platform-wallet/src/wallet/platform_addresses/provider.rs (line 430)
last_known_recent_block is not a free-standing monotonic counter — it is a paired compaction marker with explicit sentinel semantics in the SDK. In packages/rs-sdk/src/platform/address_sync/mod.rs:472-477, last_known_recent_block == 0 is the signal to fall back to RangeFrom(start_height) (inclusive), and > 0 switches the recent query to RangeAfter(last_known_recent_block) (exclusive). The SDK author's comment at line 720-721 confirms this: 'When the recent tree is empty (no address activity), this stays at 0 and the next sync falls back to RangeFrom (inclusive).'
The new per-field max() merge collapses that 3-state contract into a 2-state monotonic counter. A later sync that legitimately advances sync_height while the recent tree is empty (so result.last_known_recent_block == 0, set at line 722 of the SDK) will preserve the prior non-zero value. The provider then holds an inconsistent cursor pair like (sync_height=1100, last_known_recent_block=990). On the next sync, the SDK reads both values from the provider (lines 366 + 400 of the SDK), uses RangeAfter(990) for recent, and re-applies any recent-tree entries between 991..1100. The recent-phase path at line 690-695 is non-idempotent for AddToCredits operations (no height-based filtering, unlike the compacted phase at 627-636), so this can double-count credits.
The fix is to treat the watermark as a paired cursor: only adopt the incoming last_known_recent_block when the height watermark also advances (or use a tuple comparison). The unit tests in this PR mask the issue because they only exercise full forward / full backward / mixed-permutation results — never the realistic (advance height, zero recent_block) case.
pub(crate) fn update_sync_state(
&mut self,
result: &AddressSyncResult<PlatformAddressTag, PlatformP2PKHAddress>,
) {
// The (sync_height, last_known_recent_block) pair is a paired
// cursor: `last_known_recent_block == 0` is a sentinel telling
// the SDK to fall back to RangeFrom(start_height) on the next
// recent query. Per-field max() would silently preserve an
// older non-zero value while height advances, producing a
// stale RangeAfter cursor that can replay non-idempotent
// AddToCredits operations from the recent tree.
let incoming = (result.new_sync_height, result.new_sync_timestamp);
let current = (self.sync_height, self.sync_timestamp);
if incoming >= current {
self.sync_height = result.new_sync_height;
self.sync_timestamp = result.new_sync_timestamp;
self.last_known_recent_block = result.last_known_recent_block;
}
}
suggestion: apply_sync_state collapses Option None to 0 and overwrites unconditionally
packages/rs-platform-wallet/src/wallet/platform_addresses/wallet.rs (line 164)
PlatformAddressChangeSet documents sync_height, sync_timestamp, and last_known_recent_block as independent Options where None means 'no change' (changeset.rs:648-656), and Merge at changeset.rs:669-680 respects that contract. apply_sync_state breaks it: every missing field is converted to 0 and forwarded to set_stored_sync_state, which is an unconditional overwrite. Replaying a partial changeset (e.g. one that only advances height because the recent tree was empty for that sync) clobbers any previously-restored timestamp or recent-block watermark back to 0.
The in-memory effect after restore: sync_watermark() flips back to None, the next sync loses its RangeAfter cursor and falls back to a wider scan, and the relationship between update_sync_state's new monotonic merge and the restore path becomes inconsistent (one preserves prior fields, the other zeroes them).
Read the provider's current values first and only apply Some(_)-bearing fields; or change set_stored_sync_state to accept Options.
pub(crate) async fn apply_sync_state(
&self,
height: Option<u64>,
timestamp: Option<u64>,
last_known_recent_block: Option<u64>,
) {
if height.is_none() && timestamp.is_none() && last_known_recent_block.is_none() {
return;
}
let mut guard = self.provider.write().await;
if let Some(provider) = guard.as_mut() {
let current_height = dash_sdk::platform::address_sync::AddressProvider::last_sync_height(provider);
let current_timestamp = provider.last_sync_timestamp().unwrap_or(0);
let current_recent_block = provider.last_known_recent_block();
provider.set_stored_sync_state(
height.unwrap_or(current_height),
timestamp.unwrap_or(current_timestamp),
last_known_recent_block.unwrap_or(current_recent_block),
);
}
}
suggestion: Changeset emits raw result values, bypassing the new monotonic merge for the persisted watermark
packages/rs-platform-wallet/src/wallet/platform_addresses/sync.rs (line 66)
The new max() merge in update_sync_state only protects the in-memory provider state. The changeset persisted at lines 66-74 still uses the raw result.* values, computed before provider.update_sync_state(&result) runs. Today this is harmless because the provider write lock is held across the entire sync_balances call (line 29), so a stale-lower result cannot land after a higher one within a process lifetime — i.e. the very scenario the PR is defending against is currently prevented by the lock, not the merge. If the lock is ever relaxed for the SDK call (or multiple providers run concurrently), the persisted changeset will carry stale-lower values while the in-memory provider stays high, and on restart set_stored_sync_state (unconditional overwrite) loads the stale value back. Closing the loop is cheap: read the merged values back from the provider after update_sync_state and emit those into cs.
suggestion: sync_watermark conflates 'uninitialised provider', 'no observed block', and 'watermark==0'
packages/rs-platform-wallet/src/wallet/platform_addresses/wallet.rs (line 290)
The method collapses three genuinely distinct states (no provider, provider with no recent-block observation, provider with last_known_recent_block == 0) into a single None. The doc acknowledges this and calls it a convention, but the convention loses information for callers that want to distinguish 'wallet not initialized' from 'no recent block ever observed'. Since Option<P> already encodes 'uninitialised' and 0 is the SDK's meaningful sentinel for 'no recent block', returning Option<u64> mirroring the provider Option (None only when no provider, Some(0) for the sentinel) preserves information. Naming is also ambiguous — the provider tracks three watermarks (sync_height, sync_timestamp, last_known_recent_block) and sync_watermark doesn't disambiguate which one this exposes.
suggestion: No test exercises the realistic load-then-update sequence (or the blocking 'advance height, zero recent_block' case)
packages/rs-platform-wallet/src/wallet/platform_addresses/provider.rs (line 725)
The four new tests cover each method in isolation. The production sequence — set_stored_sync_state(persisted) → update_sync_state(result) — is never exercised end-to-end, and crucially the case driving the per-field-merge bug (a later sync legitimately returns last_known_recent_block == 0 while advancing height) is missing. A test along the lines of update_sync_state(sync_result(1000, _, 990)) followed by update_sync_state(sync_result(1100, _, 0)) would have surfaced the sentinel-collapse issue and locks the contract in place once fixed.
nitpick: last_known_recent_block() should be pub(crate), not pub
packages/rs-platform-wallet/src/wallet/platform_addresses/provider.rs (line 441)
The doc comment states this accessor exists so PlatformAddressWallet::sync_watermark (same crate) can read the value without going through the AddressProvider trait. pub(crate) is sufficient — exposing it as fully pub widens the public API beyond the stated motivation (external SDK consumers can already use AddressProvider::last_known_recent_block_height).
pub(crate) fn last_known_recent_block(&self) -> u64 {
self.last_known_recent_block
}
nitpick: Test helper triggers clippy::field_reassign_with_default
packages/rs-platform-wallet/src/wallet/platform_addresses/provider.rs (line 747)
sync_result constructs a default AddressSyncResult then reassigns fields. Clippy's field_reassign_with_default lint prefers the struct-update syntax Type { field: x, ..Default::default() }. The PR description claims cargo clippy -p platform-wallet --all-targets is clean — worth verifying the new tests are covered by that run.
fn sync_result(
height: u64,
timestamp: u64,
last_known_recent_block: u64,
) -> AddressSyncResult<PlatformAddressTag, PlatformP2PKHAddress> {
AddressSyncResult {
new_sync_height: height,
new_sync_timestamp: timestamp,
last_known_recent_block,
..AddressSyncResult::default()
}
}
nitpick: Rustdoc link target points at the sync module instead of the sync_balances method
packages/rs-platform-wallet/src/wallet/platform_addresses/wallet.rs (line 297)
[Self::sync_balances](super::sync) renders the display text 'Self::sync_balances' but resolves to the sync module, not the method. Drop the explicit URL ([Self::sync_balances] resolves to the method directly because both impls are on PlatformAddressWallet) or point the URL at the method.
/// across [`Self::sync_balances`] calls against the
|
replaced by #3723 with a smaller scope |
github-project-automation
Bot
moved this from In review / testing
to Done
in Platform team
3 participants
Issue being fixed or feature implemented
Incremental platform-address sync watermarks were updated by blind
assignment in
PlatformPaymentAddressProvider::update_sync_state. Whensync results complete out of order (a stale incremental scan finishing
after a newer one), the older result could roll the watermark
backward, causing the wallet to re-scan or under-report sync
progress.
Standalone carve-out of cluster G from #3549 (the e2e PR), extracted
so this correctness fix can land independently of the e2e test suite.
SHA of origin:
59cba08af5(+ merges).What was done?
update_sync_statenow merges each watermark field withmax()(
sync_height,sync_timestamp,last_known_recent_block) so thewatermark is monotonic non-decreasing regardless of completion order.
PlatformPaymentAddressProvider::last_known_recent_block()(
pub) — read-only mirror so wallet-level helpers can read thewatermark without going through the
AddressProvidertrait.PlatformAddressWallet::sync_watermark() -> Option<u64>(zero reported as
None, matching the existing "no storedwatermark" convention).
set_stored_sync_statedocumented as the unconditional load-timeoverwrite (monotonic invariant enforced at update-time, not load).
Only two production files changed:
provider.rs,wallet.rsunderrs-platform-wallet/src/wallet/platform_addresses/. No e2e tests,no rust-dashcore bump, no other clusters.
How Has This Been Tested?
#[cfg(test)]module: forward advance, full backwardrejection, per-field merge (advance + regression + tie), and
unconditional load-time overwrite.
cargo build -p platform-wallet— clean onv3.1-devagainstrust-dashcore
53130869(the pre-bump baseline; this cluster usesonly integer
max()and pre-existing types, so it does notdepend on the test(platform-wallet): e2e framework + full test suite — triage pins, Found-*/PA-* guards, fail-closed persist, Stage-2 merge #3549 rust-dashcore chainlock bump).
cargo clippy -p platform-wallet --all-targets— zero warnings.Breaking Changes
None. Additive accessors plus a stricter (monotonic) internal merge.
Checklist:
For repository code-owners and collaborators only
Carved out of #3549 (cluster G). 🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Tests