Skip to content

fix: preserve USDT application identity across backup merges - #179

Merged
ben-kaufman merged 1 commit into
masterfrom
fix/usdt-backup-merge-order
Oct 24, 2026
Merged

ben-kaufman merged 1 commit into
masterfrom
fix/usdt-backup-merge-order

Conversation

@ben-kaufman

Copy link
Copy Markdown
Collaborator

Companion: synonymdev/bitkit-ios#1118, synonymdev/bitkit-android#1681

Restoring a snapshot with an application payment ID followed by a snapshot containing only the chain-discovered alias currently rejects the second snapshot. Independent recovery records can arrive in either order, so this can block restoration of a valid signed transfer.

Description

  • Retain the explicit application ID and signed plan when an incoming record is only its hash-based chain alias with no plan.
  • Preserve conflict rejection for two distinct application IDs. The merge remains atomic, and chain outcomes are reconciled separately.
  • Exercise both orders across restart, repeat imports, verify one confirmed transfer and one submitted operation, and reject a conflicting application ID without changing the stored snapshot.

Preview

N/A: internal backup-merge correction with no public API or wire-format change.

QA Notes

The new regression fails unchanged production with InvalidBackup and passes the candidate. Focused signed-operation backup/recovery coverage passed 102 tests with one ignored fork-dependent test. The isolated four-thread main USDT suite passed 81 tests with one ignored test. The matched unchanged-production baseline passed the other 80 cases and failed only the new merge-order regression.

Final cargo fmt --all -- --check and cargo clippy --locked --lib --tests passed. All 94 complete Clippy diagnostics match the actual unchanged base after normalizing checkout paths and source positions. No lint suppression or baseline file was added. ./build.sh all passed: iOS device/simulator, all four Android ABIs and Python bindings. Swift type-checking passed without diagnostics. Android :lib:check passed with an empty ktlint report and seven dependency-version warnings in the byte-identical base Gradle manifest. Generated Kotlin bindings are byte-identical to bindings independently generated from unchanged production. Native release libraries passed stripping and 16 KB alignment checks; only local Maven output was produced. The optional Python formatter (yapf) was unavailable; no generated sources or release checksum changes are committed.

The final isolated complete Rust suite finished with 652 passed, 11 failed and 27 ignored. All eleven failures are external Blocktank staging HTTP failures also reproduced on unchanged production. The initial run had an additional nine quote-expiry failures and one route-selection failure; none recurred in the matched candidate/base USDT runs or this final complete run. Their original transient cause remains unproven. This is not a clean full-suite claim.

Native apps need compatible artifacts containing this correction before the independent-backup changes are ready. No package release, live funds or wallet reset is part of this PR.

@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. Lets restore_backup accept a chain-discovered alias record after the application-ID record for the same user operation has been restored, so the order of the two snapshots no longer matters. Reviewed c393f8e, full tier.

What I checked, and 5 candidates I ruled out

Read in full: src/modules/usdt/backup.rs, the new test in src/modules/usdt/tests/backup.rs
Call sites traced: the usdt_transfers hash lookups in backup.rs and store.rs (find_transfer_by_hash, the history settlement insert)
Ran locally: cargo test --locked --lib modules::usdt at the head, 102 passed, 1 ignored. With backup.rs reverted to master, the new test fails at line 534 with InvalidBackup, so it pins the fix.
CI: usdt-tests, SwiftPM and the manifest checks green; Android and iOS Bindgen still queued when I looked. The diff touches no exported type.

Ruled out

  • Two application IDs for one operation get merged: the skip needs item.transfer.id == hash, so an incoming record with its own application ID still reaches InvalidBackup; the test covers that case and checks that the stored snapshot does not change.
  • A signed plan gets dropped: the skip also needs item.plan.is_none(). An alias row written by history sync has no plan, and an incoming record with a plan still fails.
  • Newer chain status in the alias gets lost: the stored row's block and status are left as they are, and refresh_transfers reconciles them from the chain. The test asserts Confirmed after both orders.
  • The skip bypasses require_refund_owner: the skipped item writes nothing, so there is no refund claim to check.
  • Alias and application record in the same snapshot: the pre-transaction hashes set already rejects a duplicate operation hash within one backup.

Merge confidence: 5/5, the change is covered by recovery_snapshots_merge_chain_aliases_in_either_order_after_restart, which fails without it. That assumes the queued Bindgen jobs go green.

@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

Full review: diff, 2 files.

Findings: None.

QA for c393f8e:

Test 1: Orchestra backup merge across restart ✅ Rust assertions verified both import orders preserve the application identity and signed bridge plan without a second submission.

I exercised application and chain-alias snapshots in both orders across restart, then repeated both imports. Assertions checked one transfer, the unchanged signed plan, Base destination, recipient and tracking ticket, and one submitted operation after recovery against the mock chain.

Check 1: Native library build ✅ The unmodified reviewed head builds with cargo build --locked --lib.

I built the native library before adding the additional test harness.

Check 2: Existing exact-head regression evidence ✅ The ordinary-transfer merge regression and USDT suite are covered by the linked report.

I relied on the existing test report at the reviewed head for ordinary-transfer merge ordering, conflicting-ID rejection and rollback, and the USDT suite. That report also verifies the new regression fails without this correction. I did not repeat those cases or run the ignored fork-dependent test.

No device run: this PR specifies Rust regression coverage and has no app UI change. Native-app recovery journeys and adoption of compatible artifacts are not verified by this result.


Reviewed by gpt-6.1-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.

utAck

@ovitrif

ovitrif commented Oct 24, 2026

Copy link
Copy Markdown
Collaborator

Approved and green, ready for you to merge and release.

@coreyphillips coreyphillips 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.

Two independent reviews, nothing blocking a merge.

@ben-kaufman
ben-kaufman merged commit a66511f into master Oct 24, 2026
6 checks passed
@ben-kaufman
ben-kaufman deleted the fix/usdt-backup-merge-order branch October 24, 2026 15:17
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