Skip to content

Fix USDT backup identity and acknowledgement handling - #169

Merged
ben-kaufman merged 3 commits into
masterfrom
fix/usdt-backup-integrity
Oct 8, 2026
Merged

ben-kaufman merged 3 commits into
masterfrom
fix/usdt-backup-integrity

Conversation

@ben-kaufman

@ben-kaufman ben-kaufman commented Oct 8, 2026 •

Copy link
Copy Markdown
Collaborator

Restoring a USDT backup could assign one signed operation or refund receipt to multiple payments. Restore now canonicalizes operation/refund hashes and checks refund ownership inside the merge transaction, including pruned terminal records and seed-discovered payments. A conflict rolls back the whole restore. Live updates and restore recognize the same refund regardless of hash casing or an optional 0x prefix.

Unexpected Swift/Kotlin backup callback errors return BackupUnavailable. Pending-payment retries reuse an unchanged acknowledged snapshot within the wallet session; failed saves, changed snapshots and reopening still require acknowledgement before broadcast. The callback docs clarify that applications must back up subsequent application-only association changes separately.

Includes the rebuilt XCFramework checksum and updated generated Swift documentation for the unpublished 0.8.0-rc2. No schema changes, migrations or new public APIs.

Validation:

  • USDT suite: 85 passed, one opt-in fork test ignored. Covers conflicting restores, stored hash spellings, foreign callback errors, failed saves, rebroadcast, reopening and subsequent payments.
  • cargo fmt --check, git diff --check and Clippy completed with no introduced diagnostics. Existing Clippy warnings are outside the changed code.
  • iOS device/simulator XCFramework rebuilt and checksum verified; Swift constructor/export/restore smoke passed against the rebuilt static library.
  • Swift and Kotlin callback documentation regenerated and checked. Swift API shape is unchanged.
  • SwiftPM manifest passed. Full Android native packaging runs in CI.

@talosmachina talosmachina left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

No findings. Canonicalizes user-operation and refund hashes on restore, runs the refund-ownership check inside the restore transaction, maps foreign callback errors to BackupUnavailable, and skips re-uploading an unchanged snapshot already acknowledged in the current wallet session. Reviewed 57f96f7, full tier (wallet backup path).

What I checked, and 5 candidates I ruled out

Read in full: usdt/backup.rs, the changed regions of usdt/store.rs and usdt/wallet.rs, and the new tests in usdt/tests.rs and usdt/tests/backup.rs
Call sites traced: persist_backup (send path under operation lock, and recover_pending under refresh_pending_transfers' lock), refund_claimed (live refund path and both restore branches), every writer of user_operation_hash (wallet.rs and history.rs, both {:#x})
CI: usdt-tests, iOS Bindgen, SwiftPM and manifest checks green; Android Bindgen still running when this was posted. Not built here, CI is the gate.

Ruled out

  • Canonical form drifting from stored hashes: B256's Display without the alternate flag prints the full lowercase 0x hex, the same as the {:#x} every writer uses, so a backup from the current code canonicalizes to the stored hash column and existing != hash does not misfire.
  • Older backups now failing restore: master already parsed refund_tx as B256 before storing it, and user_operation_hash was always formatted from a B256, so the new parse cannot reject a snapshot this code produced.
  • Self-match in the seed-discovered branch: require_refund_owner is called with the row's pre-rename id, which refund_claimed excludes, so the record cannot conflict with itself.
  • Duplicate refund inside one snapshot: the in-memory pass does not catch it, but the second insert sees the first in the same transaction and the whole restore rolls back; the new test covers it on an empty wallet.
  • Stale acknowledgement after a failed or changed save: the cached hash is set only after persist returns Ok, and any restore or new record changes the snapshot bytes, so the callback runs again.

Merge confidence: 4/5, the change is covered by the new restore and retry tests, but Android Bindgen had not finished on this head.

@ovi-reviewer ovi-reviewer Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Verdict: ✅ Approve

Review: diff 8 files.

Findings:
1 inline (1 MEDIUM)


Reviewed by gpt-6-sol-high via gh-pr-review-loop skill
Commands: @ovi-reviewer review · test · retest

Comment thread src/modules/usdt/backup.rs
ovitrif
ovitrif previously approved these changes Oct 8, 2026
@ovitrif
ovitrif enabled auto-merge (squash) October 8, 2026 17:27

@ovi-reviewer ovi-reviewer Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Verdict: ✅ Approve

Reaudit: diff 1 file.
No new findings; the rest is in the review.

QA:
No reviewer test ran because the PR description reports only validation the author already completed and lists no journey or manual test.


Reviewed by gpt-6-sol-medium via gh-pr-review-loop skill
Commands: @ovi-reviewer review · test · retest

@coreyphillips

Copy link
Copy Markdown
Collaborator

Two independent reviews, nothing blocking a merge.

worth doing, does not block

  • Refund ownership check misses refund hashes stored without a 0x prefix (src/modules/usdt/store.rs:448). refund_claimed only lowercases both sides, so a refund hash stored without the 0x prefix never matches the canonical form now written. This PR canonicalizes new refund hashes in bridge_delivery and in restore_backup. Rows already on disk are left alone. The published v0.8.0-rc1 stored delivery.refund_tx exactly as the provider sent it, and its Orchestra outbound path was live. If the provider ever returned a hash without the prefix, that row stays that way. The refund-exclusivity check in update_delivery and in restore then lets a second payment claim the same receipt. The PR's own test feeds an unprefixed "AB".repeat(32) provider response, so this input is clearly considered possible. Confirmed on a scratch copy. I inserted a row whose orchestra.refund_tx is "ab".repeat(32) and called refund_claimed(conn, "other", &B256::repeat_byte(0xab).to_string()), which returned false. This is unlikely to hit anyone: it needs rc1 users and a provider that drops the prefix. Two cheap fixes: strip an optional 0x on both sides in the SQL at store.rs:448, or canonicalize stored refund_tx values once when the store opens.
  • Previously stored prefixless refunds can be assigned twice (src/modules/usdt/wallet.rs:743). Reopening an earlier database can let the same refund receipt settle two payments. The previous refund path stored prefixless hashes verbatim. The new path adds 0x, but refund_claimed only ignores case when comparing stored claims. I confirmed that its exact SQL query matches the old input and misses the canonicalized input. Normalize stored claims too. This affects retained prerelease databases.

nits

  • Cached-acknowledgement test never asserts that the rebroadcast happened (src/modules/usdt/tests/backup.rs:73). submission_and_recovery_wait_for_remote_backup turns the backup off, calls refresh_transfers, and only asserts that writes == 1. That check would also pass if the refresh never reached persist_backup. Adding assert_eq!(chain.state.lock().unwrap().operations.len(), 2) after the refresh would pin down the behaviour this PR adds: a retry reuses the unchanged acknowledged snapshot. On a scratch copy I confirmed the operation count goes from 1 to 2 there, so the code is right and only the assertion is missing.
  • UsdtBackup doc still implies persist runs on every automatic retry (src/modules/usdt/backup.rs:10). The trait doc says the callback saves recovery data before submission "including automatic retries". After this PR, a retry with an unchanged snapshot in the same wallet session skips the callback. Application-only association changes therefore have to be backed up by the app. The README now says this, but this doc comment is the text that ships in the generated Swift and Kotlin docs, and it says nothing about it. Updating it changes the generated bindings, so it may be better left to the next binding regeneration.

@ben-kaufman
ben-kaufman disabled auto-merge October 8, 2026 19:51
@ben-kaufman

Copy link
Copy Markdown
Collaborator Author

Thanks, addressed all three points in 9c273fa0. Refund ownership now compares hashes independently of case and the optional 0x prefix, for both live delivery updates and backup restore. No migration or rewrite of wallet data.

The existing refund test covers persisted prefixless claims through restart, pruning and restore. The retry test now asserts that the operation was rebroadcast while the acknowledged backup was reused. The callback docs are clarified, the Swift bindings are regenerated, and I checked the generated Kotlin docs too.

All 85 USDT tests pass, with the opt-in fork test skipped. Formatting passes and Clippy has no introduced diagnostics. The iOS device/simulator archive was rebuilt with its matching checksum, and the Swift backup smoke check passes.

@ben-kaufman
ben-kaufman enabled auto-merge (squash) October 8, 2026 20:00

@ovi-reviewer ovi-reviewer Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Verdict: ✅ Approve

Reaudit: diff 6 files.
No new findings; the rest is in the review.

QA:
No reviewer test ran; the PR description says the author completed tests, lint, checksum and manifest checks, and lists no journey or manual test.


Reviewed by gpt-6-sol-high via gh-pr-review-loop skill
Commands: @ovi-reviewer review · test · retest

@ovitrif ovitrif left a comment

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.

ack

@ben-kaufman
ben-kaufman merged commit bdce512 into master Oct 8, 2026
6 checks passed
@ben-kaufman
ben-kaufman deleted the fix/usdt-backup-integrity branch October 8, 2026 20:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants