fix(platform-wallet): wait indefinitely for asset-lock ChainLock finality#4006
Merged
Conversation
…lity A Platform funding operation (identity registration/top-up, platform- address top-up, shielded funding) that broadcast its asset lock but did not obtain an InstantSend proof fell back to a 180s ChainLock wait (CL_FALLBACK_TIMEOUT). On testnet, ChainLocks can take ~15min, so the bounded wait elapsed and the flow returned FinalityTimeout — surfaced to the user as "Top Up failed" even though the asset lock is committed on-chain and merely pending finality. A ChainLock is deterministic finality that will eventually cover any broadcast asset-lock tx, so there is no correct point at which to declare failure. Make the ChainLock wait unbounded: - wait_for_chain_lock / upgrade_to_chain_lock_proof / wait_for_proof / resume_asset_lock now take Option<Duration>; None waits indefinitely. - All user-facing funding flows pass None at their ChainLock fallback. - The 300s build/resume wait is kept as an InstantSend-preference window (not a finality timeout): on expiry it falls through to the unbounded ChainLock wait rather than failing. The shielded seed pool is the one deliberate exception: its FinalityTimeout is a pacing signal for the unconfirmed-ancestor stall, so it keeps a bounded Some(CL_FALLBACK_TIMEOUT) via a new cl_wait parameter on shielded_fund_from_asset_lock. CL_FALLBACK_TIMEOUT is now shielded- gated as its only remaining consumer. The two asset-lock resume/catch-up FFI entry points treat timeout_secs==0 as unbounded. No exported C symbol or signature changed. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
QuantumExplorer
requested review from
ZocoLini,
lklimek,
llbartekll and
shumkov
as code owners
Contributor
📝 WalkthroughWalkthroughAsset-lock proof waiting, resume, and shielded funding APIs are changed to accept ChangesOptional ChainLock fallback timeouts
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant AssetLockSync as "sync/proof.rs"
Caller->>AssetLockSync: wait_for_proof(out_point, timeout: Option<Duration>)
alt timeout is Some
AssetLockSync->>AssetLockSync: compute deadline, select notification vs sleep
AssetLockSync-->>Caller: FinalityTimeout on expiry
else timeout is None
AssetLockSync->>AssetLockSync: await notification indefinitely
AssetLockSync-->>Caller: proof resolved
end
Possibly related PRs
Suggested labels: `ready for final review` Suggested reviewers: `shumkov`, `lklimek`, `llbartekll`, `ZocoLini` 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/rs-platform-wallet/src/wallet/identity/network/registration.rs (1)
113-118: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winStale doc: "Idempotency note" still describes the bounded 180s wait.
The doc comment states "The IS→CL retry is bounded (180s waiting for ChainLock)." but the calls below (Line 186, 195, 247, 403-405, 441) now pass
Nonefor an unbounded wait. Compare withfund_from_asset_lock.rs, where the equivalent doc block was updated to describe the new unbounded semantics — this file's doc was missed.📝 Proposed doc fix
/// # Idempotency note /// - /// The IS→CL retry is bounded (180s waiting for ChainLock). If the - /// CL retry itself fails, the asset-lock stays tracked (cleanup + /// The IS→CL retry now waits for ChainLock indefinitely (no + /// timeout). If the process is killed or the caller otherwise + /// never observes completion, the asset-lock stays tracked (cleanup /// only runs on Platform success) so subsequent registration /// attempts can resume via `FromExistingAssetLock`.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/rs-platform-wallet/src/wallet/identity/network/registration.rs` around lines 113 - 118, Update the stale “Idempotency note” in registration.rs to match the current unbounded retry behavior: the comment still says the IS→CL retry is bounded to 180s, but the retry calls in the registration flow now pass None. Revise the doc block near the registration logic to describe the unbounded wait semantics, using the same wording style as the updated doc in fund_from_asset_lock.rs, and keep the note aligned with the retry path in the relevant registration functions.
🧹 Nitpick comments (2)
packages/rs-platform-wallet/src/wallet/asset_lock/orchestration.rs (1)
423-429: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated 300s IS-preference window magic number.
Duration::from_secs(300)is now hardcoded here and inbuild.rs(Line 395) as the InstantSend-preference window. SinceCL_FALLBACK_TIMEOUTalready exists as a named constant in this file for the analogous bounded-wait concept, consider adding a similarly named constant (e.g.IS_PREFERENCE_WINDOW: Duration = Duration::from_secs(300)) shared by both call sites, so the two windows can't silently drift apart in a future change.♻️ Proposed refactor
+/// InstantSend-preference window before falling back to an unbounded +/// ChainLock wait. Shared by `build.rs::create_funded_asset_lock_proof` +/// and `resolve_funding_with_is_timeout_fallback`. +pub(crate) const IS_PREFERENCE_WINDOW: Duration = Duration::from_secs(300); + match self - .resume_asset_lock(&out_point, Some(Duration::from_secs(300))) + .resume_asset_lock(&out_point, Some(IS_PREFERENCE_WINDOW)) .await🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/rs-platform-wallet/src/wallet/asset_lock/orchestration.rs` around lines 423 - 429, The InstantSend preference window is duplicated as a hardcoded 300-second duration, so add a named shared constant for it in the same area that defines CL_FALLBACK_TIMEOUT and use that constant at both call sites, including the resume_asset_lock path. Update the code that currently passes Duration::from_secs(300) so it references the new constant instead of embedding the magic number, keeping the analogous bounded-wait values centralized and consistent.packages/rs-platform-wallet/src/wallet/asset_lock/sync/proof.rs (1)
318-341: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate select/timeout logic — extract a shared helper.
The
match deadline { Some(dl) => {...} None => {...} }block is identical inwait_for_chain_lock(Lines 321-339) andwait_for_proof(Lines 551-569): compute remaining time, early-returnFinalityTimeoutif elapsed,tokio::select!between the armed notification and a timed sleep, or await the notification indefinitely when there's no deadline. Extracting this into a small shared helper (e.g.wait_on_notify_or_deadline(notified, deadline, out_point) -> Result<(), PlatformWalletError>) would remove the duplication and keep both call sites in sync if the pattern needs to change later (e.g. adding jitter/backoff).Also applies to: 548-571
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/rs-platform-wallet/src/wallet/asset_lock/sync/proof.rs` around lines 318 - 341, The deadline-wait logic is duplicated in both wait_for_chain_lock and wait_for_proof, so extract the shared notification/timeout handling into a small helper such as wait_on_notify_or_deadline. Move the compute-remaining, immediate FinalityTimeout return, tokio::select! between notified and sleep, and indefinite await behavior into that helper, then have both call sites pass their notified future, deadline, and out_point and handle the Result.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@packages/rs-platform-wallet/src/wallet/identity/network/registration.rs`:
- Around line 113-118: Update the stale “Idempotency note” in registration.rs to
match the current unbounded retry behavior: the comment still says the IS→CL
retry is bounded to 180s, but the retry calls in the registration flow now pass
None. Revise the doc block near the registration logic to describe the unbounded
wait semantics, using the same wording style as the updated doc in
fund_from_asset_lock.rs, and keep the note aligned with the retry path in the
relevant registration functions.
---
Nitpick comments:
In `@packages/rs-platform-wallet/src/wallet/asset_lock/orchestration.rs`:
- Around line 423-429: The InstantSend preference window is duplicated as a
hardcoded 300-second duration, so add a named shared constant for it in the same
area that defines CL_FALLBACK_TIMEOUT and use that constant at both call sites,
including the resume_asset_lock path. Update the code that currently passes
Duration::from_secs(300) so it references the new constant instead of embedding
the magic number, keeping the analogous bounded-wait values centralized and
consistent.
In `@packages/rs-platform-wallet/src/wallet/asset_lock/sync/proof.rs`:
- Around line 318-341: The deadline-wait logic is duplicated in both
wait_for_chain_lock and wait_for_proof, so extract the shared
notification/timeout handling into a small helper such as
wait_on_notify_or_deadline. Move the compute-remaining, immediate
FinalityTimeout return, tokio::select! between notified and sleep, and
indefinite await behavior into that helper, then have both call sites pass their
notified future, deadline, and out_point and handle the Result.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0ec05108-a7cc-4728-9828-1a5be7444da1
📒 Files selected for processing (10)
packages/rs-platform-wallet-ffi/src/asset_lock/sync.rspackages/rs-platform-wallet-ffi/src/shielded_send.rspackages/rs-platform-wallet/src/wallet/asset_lock/build.rspackages/rs-platform-wallet/src/wallet/asset_lock/orchestration.rspackages/rs-platform-wallet/src/wallet/asset_lock/sync/proof.rspackages/rs-platform-wallet/src/wallet/asset_lock/sync/recovery.rspackages/rs-platform-wallet/src/wallet/identity/network/registration.rspackages/rs-platform-wallet/src/wallet/platform_addresses/fund_from_asset_lock.rspackages/rs-platform-wallet/src/wallet/shielded/fund_from_asset_lock.rspackages/rs-platform-wallet/src/wallet/shielded/seed_pool.rs
5 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Issue being fixed or feature implemented
Found during ADDR-09 QA on SwiftExampleApp against testnet. A Platform identity top-up (asset lock
d407f18f:0) broadcast successfully and Core SPV synced to tip, but the top-up modal declared "Top Up failed" after ~6–8 min. The asset lock was genuinely committed on-chain and merely pending finality — not failed.Root cause: when an InstantSend proof doesn't propagate, the funding flow falls back to a bounded 180s ChainLock wait (
CL_FALLBACK_TIMEOUT). Testnet ChainLocks can take ~15 min, so the wait elapsed and the flow returnedFinalityTimeout, surfaced to the user as a hard failure.A ChainLock is deterministic finality that will eventually cover any broadcast asset-lock tx, so there is no correct point at which to declare failure. Per direction: there should be no timeout for chain locks at all.
What was done?
The ChainLock wait is now unbounded for every user-facing asset-lock funding flow.
wait_for_chain_lock,upgrade_to_chain_lock_proof,wait_for_proof,resume_asset_lock— now takeOption<Duration>, whereNonewaits indefinitely (the loop just parks on the SPVNotifyuntil the lock event arrives).Noneat their ChainLock fallback: identity register/top-up (registration.rs), platform-address top-up (platform_addresses/fund_from_asset_lock.rs, the reported flow), and user-initiated shielded funding (shielded_send.rs).Some(CL_FALLBACK_TIMEOUT). ItsFinalityTimeoutis a pacing signal for the unconfirmed-ancestor stall (batches chain ~25 unconfirmed change outputs; IS/CL proofs stop arriving until a block lands), not a finality failure.shielded_fund_from_asset_lockgained acl_wait: Option<Duration>parameter for this;CL_FALLBACK_TIMEOUTis nowshielded-gated as its only remaining consumer.timeout_secs == 0to unbounded. No exported C symbol or signature changed, so the iOS app picks this up as pure runtime behavior after an xcframework rebuild.Net effect: the top-up no longer reports a false "failed" — it stays pending finality until the ChainLock lands, then completes.
How Has This Been Tested?
cargo check -p platform-wallet --features shielded --all-targets— clean.cargo check -p rs-platform-wallet-ffi(default) and--features shielded --all-targets— clean (confirmed no dead-code warning aftershielded-gatingCL_FALLBACK_TIMEOUT).cargo clippy -p platform-wallet --features shielded --all-targets— clean.cargo test -p platform-wallet --features shielded asset_lock— 19 asset-lock unit tests pass.cargo fmt --allapplied.Not yet exercised end-to-end on-device: requires an xcframework rebuild (
build_ios.sh) to validate the top-up on the simulator against testnet.Breaking Changes
None. No consensus-affecting change and no FFI ABI change.
Checklist:
For repository code-owners and collaborators only
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes