Repository navigation
fix(platform-wallet): owe a chain truncation's cursor to the DashPay backfill record - #5484
Conversation
…backfill record `PlatformWalletInfo::truncate_above` (brought to v5.1-dev with the rust-dashcore update, #5307) lowers the in-memory scan cursor to the fork height, while the host keeps the replaced branch's cursor because `ChainTruncated` is not mirrored to the persister. Since #5026 made the DashPay backfill coverage durable, a contact registered at or above the fork height is forward-covered at the lowered cursor, so its record round stored coverage with no cursor beside the host's stale one. On relaunch the coverage suppressed the rewind and the scan resumed past the new branch's blocks, which were never tested with the contact's addresses watched. Neither branch had the bug alone: v5.1 coverage was process-local and v5.0 had no truncation override. A truncation that lowers the cursor is now noted like key-wallet's account-add rewind: the rewind barrier is armed, so advances emitted on the replaced branch (queued or already projected) are not stored after the fork, and the lowered cursor is owed to the next DashPay backfill record round, which writes it as a rewind beside the coverage. A refused or non-lowering truncation notes nothing. The private helper is renamed `note_account_add_rewind` -> `note_in_memory_rewind` to cover both. Tests would have caught this in CI: ✖ before the fix, ✔ after: - should_rescan_a_new_contacts_blocks_after_a_restart_that_follows_a_chain_truncation (relaunch resumed at 1000, expected 999) - a_chain_truncation_alone_is_owed_to_the_next_record_round (no record round lowered the host after the fork) - advances_from_before_a_chain_truncation_are_dropped_after_the_new_branch_climbs_past_them (a replaced-branch advance survived the commit: Some(1000), expected None) - should_note_a_truncation_only_when_it_lowers_the_scan_cursor (rewind epoch 0, expected 1) a_new_branch_advance_the_host_accepted_pays_a_chain_truncations_debt passes before and after: it guards against the fix over-lowering the host once a new-branch advance has paid the debt (it fails if the debt is owed in the wrong epoch). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configuration
You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
|
Waiting for bot review — coderabbitai not yet · thepastaclaw ✓. Wait for the missing reviews, or a writer can post |
|
✅ Final review complete — no blockers (commit 7ad0146) · triage: normal |
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Phase 1 + Phase 2
Verified the complete three-file diff at 7ad0146 against the supplied reviewer evidence, surrounding persistence and epoch guards, and pinned/current upstream truncation implementations; no actionable in-scope defects were found. Lowering truncations correctly arm the rewind barrier and owe the cursor in the new epoch, while refused or non-lowering truncations leave bookkeeping unchanged. This was a static review: current CI reports successful Rust wallet tests, Swift build/tests and schema checks, and Kotlin build/tests; PR Hygiene remains pending.
Review provenance
Source: reviewer 1: gemini-3.8-flash-high (agent: phase1-reviewer, role: general); reviewer 2: gemini-3.8-flash-high (agent: phase1-reviewer, role: rust-quality); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 7: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
normalbygpt-6.1-sol(effort low) — The change involves nontrivial wallet cursor, epoch, and persistence bookkeeping, but does not itself change consensus, funds movement, cryptography, key handling, network deserialization, or storage migrations. - Phase 1 reviewers:
gemini-3.8-flash-high— general (completed, effort high); agentphase1-reviewer,gemini-3.8-flash-high— rust-quality (completed, effort high); agentphase1-reviewer - Phase 1 model:
gemini-3.8-flash-high— antigravity quota: weekly 71% left, 5h 100% left - Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— platform-versioning (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort high); agentphase2-reviewer
|
Bots are done — your move: post |
Issue being fixed or feature implemented
After merging v5.0-dev into v5.1-dev (#5481), DashPay backfill coverage could be persisted beside a stale host cursor following a chain truncation. After a restart, the blocks of the new branch above the fork could then go unscanned for a newly registered contact.
This is a semantic conflict between two changes that are each correct alone, and git merged them without a textual conflict:
PlatformWalletInfo::truncate_above. When the wallet follows a fork, it lowers the in-memory scan cursor to the fork height, for example 1000 → 999 at the last chain lock.ChainTruncatedis not mirrored to the persister (TODO platform-wallet: persist chain truncation (reorg) reported by key-wallet #5380), so the host keeps 1000.DashPayBackfillRecord). It also added the rewind barrier and owed-cursor bookkeeping that every in-memory lowering has to go through. The account-add rewind and the manual rescan reset already do.truncate_aboveskipped that bookkeeping. Here is how the bug plays out:floorandowedare bothNone.covered_from = 999with no cursor, next to the host's 1000.A second effect: advances emitted on the replaced branch, whether queued or already projected, were not invalidated. Once the new branch climbed back past their height they could still be stored, ahead of the new branch's rows.
Neither branch had the bug alone. Before the merge, v5.1 coverage was process-local, so a relaunch re-armed the contact. v5.0 has no
truncate_aboveoverride, so v5.0-dev is not affected.What was done?
PlatformWalletInfo::truncate_abovenow records the cursor it was at, forwards to key-wallet with?, and then notes a lowering truncation the same way as the account-add rewind:ChainTruncatedis emitted. Replaced-branch advances that are still queued are dropped as stale when projected. Already-projected ones carry a superseded epoch and are stripped at commit.note_account_add_rewindis renamed tonote_in_memory_rewind, with its doc generalized to cover both lowerings. The six account-add call sites behave exactly as before.ChainTruncatedarm comment incore_bridge.rsno longer says the stored sync height stays at the replaced branch's value.Behaviour change, accepted. On hosts that store the backfill record durably, the first DashPay sweep after a fork runs one record-only round that lowers the host's
synced_heightto the fork height. It skips this if a new-branch advance has already paid the debt. The write only ever lowers the host cursor, so the cost is extra rescanning, never less coverage. This also narrows #5380's stale-cursor window for DashPay coverage that was recorded before the fork. #5380 itself is not fixed: stored rows above the fork,last_processed_heightand record demotions are left as they are.There are no FFI/JNI, schema, API or consensus changes.
Interaction with #4740 (open, base v5.1-dev, currently conflicting): its diff does not touch
truncate_aboveor the renamed private helper. Its registration-time checkpoint restore calls the publicadd_managed_account, which reaches the same helper after its rebase. The two changes don't overlap or duplicate each other. Itsaccount_generationhunk inplatform_wallet_traits.rsalready conflicts with v5.1-dev, independent of this PR.How Has This Been Tested?
New tests, all written before the fix:
payments.rsshould_rescan_a_new_contacts_blocks_after_a_restart_that_follows_a_chain_truncation: host accepted 1000 → engineWalletManager::truncate_above(999)at chain lock 999 → contact at 999 → forward-covered record round → relaunch from the host's cursor and recordleft: 1000, right: 999"the relaunched scan must test the new branch's block 1000 with the contact watched"payments.rsa_chain_truncation_alone_is_owed_to_the_next_record_round: only pre-fork coverage; the debt alone runs a record-only round that writes 999core_bridge.rsadvances_from_before_a_chain_truncation_are_dropped_after_the_new_branch_climbs_past_them: replaced-branch advances, both projected and queued, are dropped after the new branch climbs past them (adapter level, engine truncation path)left: Some(1000), right: None"projected before the truncation: stripped at commit"platform_wallet_traits.rsshould_note_a_truncation_only_when_it_lowers_the_scan_cursor: refused / at-cursor truncation notes nothing; a lowering truncation arms and owesleft: 0, right: 1payments.rsa_new_branch_advance_the_host_accepted_pays_a_chain_truncations_debt: guard against over-loweringThe queued-advance variant is tested at the adapter level. In the manager-level tests the background adapter makes the "queued → committed alone after the new branch climbs" interleaving impossible to force deterministically. The end-to-end test models the reconcile-side consequence instead: the host cursor came from a pre-fork advance in the current epoch.
Mutation checks on the fix:
Results on this branch:
cargo test -p platform-wallet: 1351 lib tests, plus the integration tests and doctests, all pass (1 test is ignored, as it already was).cargo clippy -p platform-wallet --all-targetsis clean (one existingdppmanifest warning).cargo fmt --all -- --checkis clean.Breaking Changes
None.
Checklist:
structure.rs, regeneratedgrovedb-structure.json, and checked the structure viewer link posted on this pull requestFor repository code-owners and collaborators only
🤖 Generated with Claude Code
PR Hygiene ·
7ad0146/self-reviewedrs-platform-wallet(packages/rs-platform-wallet/src/changeset/core_bridge.rs,packages/rs-platform-wallet/src/wallet/identity/network/payments.rs,packages/rs-platform-wallet/src/wallet/platform_wallet_traits.rs) — HashEngineering or ZocoLini or llbartekll or romchornyiWhen every merge requirement is met, the
PR Hygienecheck passes. Reviewer limits do not block merging; other required GitHub checks and protections still apply.