Skip to content

fix: overlap USDT send preflight reads - #177

Merged
ben-kaufman merged 1 commit into
masterfrom
fix/usdt-send-preflight
Oct 20, 2026
Merged

ben-kaufman merged 1 commit into
masterfrom
fix/usdt-send-preflight

Conversation

@ben-kaufman

Copy link
Copy Markdown
Collaborator

USDT sends currently wait for nonce, account authorization and balance reads in sequence. Run these independent reads concurrently under the existing operation lock to reduce pre-signing latency without caching their results or adding RPC calls.

Description

The final nonce, authorization, balance, bridge, gas and expiry checks remain in place. Signing, durable recording, acknowledged backup and broadcast keep their existing order. No public API, binding or version change.

QA Notes

  • Added a barrier regression that requires the three reads to overlap and verifies exactly one submitted operation. It fails on the previous implementation and passes with this change.
  • Local Arbitrum-fork contract test passes, covering actual USDT fee collection and atomic rollback of a failed bridge.
  • Full suite: 651 passed, 27 ignored; 11 external Blocktank tests failed identically on unchanged master (fab768db).
  • cargo fmt --check passes. cargo clippy has the same 40 warnings as unchanged master, with no added warnings.

This removes sequential network waits; it does not establish an end-to-end payment or merchant-unlock latency guarantee.

@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. Runs the nonce, EIP-7702 authorization and balance reads in send_with_deadline concurrently with tokio::try_join!, under the existing operation lock, and adds a barrier test that needs all three reads to be in flight at once. Reviewed 2285d83, full tier, reasoned from the code and CI (not built locally).

What I checked, and 3 candidates I ruled out

Read in full: send_with_deadline and the read helpers in src/modules/usdt/wallet.rs, Rpc::call_with_deadline and reserve_request in src/modules/usdt/rpc.rs, the mock RPC and the new test in src/modules/usdt/tests.rs
CI: usdt-tests, SwiftPM and Cargo/Android manifests green at this head. iOS and Android Bindgen were still pending, and the PR does not change any public type or export.

Ruled out

  • Rate limiter under concurrency: reserve_request holds a shared tokio::sync::Mutex only long enough to claim a slot. The three reads (four requests, because authorization() makes two) fit inside the 20-request burst, and a dropped future in try_join! only uses up a slot it had already claimed.
  • Error precedence: before this change, a stale nonce returned QuoteExpired before the balance was read. Now, if the nonce is stale and the balance is also short, the call can return InsufficientBalance instead. Both errors stop the send before signing, and neither is retried, so no money moves either way. Every check that ran before still runs before derive_owner_key.
  • Test validity: the barrier counts only eth_call and eth_getCode. Before the barrier is armed, send sends only eth_chainId, so the three requests that reach it are the getNonce call, the getCode call and the balanceOf call. With the old sequential code, the first read waits at the barrier and the 10 s timeout fails the test, which matches what the PR says.

Merge confidence: 5/5, usdt-tests green and the overlap is covered by send_preflight_reads_overlap_before_signing.

@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

Findings: None.

QA for 2285d83:

Test 1: Preflight overlap ✅ The barrier regression passed and submitted once.

Checked with the repository regression. Keeping the new test and substituting the base implementation made it time out at the barrier, as expected.

Test 2: Send safeguards and recovery ✅ The USDT module checks passed.

Checked with cargo test --locked --lib modules::usdt, including nonce, authorization, balance, expiry, backup and recovery checks.

Test 3: Deployed contracts ✅ Fee collection and atomic bridge rollback passed.

Ran the repository's ignored Arbitrum-fork test, including successful bridging and helper-allowance revocation.

Test 4: Swift simulator send ✅ A 1 USDT payment confirmed; repeating its quote ID caused no second debit.

An iOS 18.5 test harness linked to the library built at this head checked the fee cap, principal-plus-fee balance debit, and identical operation on repeat. This uses the repository fork provider and a fixture backup callback.

simulator2x.2x.mp4

Full review of fab768db..2285d83, covering both changed files. Native Bitkit UI journeys and end-to-end payment latency were not tested.

The earlier code review also found no defects at this head.


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

@ben-kaufman
ben-kaufman merged commit 4ad4229 into master Oct 20, 2026
6 checks passed
@ben-kaufman
ben-kaufman deleted the fix/usdt-send-preflight branch October 20, 2026 12:16
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.

3 participants