Found-008: `LockNotifyHandler::notify_waiters()` drops lock events arriving in `wait_for_proof`'s check/await gap (concurrent asset-lock builds stall on FinalityTimeout)

[lklimek]

User Story

As a Platform application developer using rs-platform-wallet to fund concurrent asset-lock-based identity operations (e.g. registering multiple identities in parallel, or topping up several identities at once), I need AssetLockManager::wait_for_proof to reliably wake when an IS-lock or ChainLock event arrives at LockNotifyHandler, so that concurrent asset-lock builds from the same wallet complete without stalling on FinalityTimeout.

Today, any caller driving N ≥ 2 concurrent asset-lock builds against a single wallet hits a deterministic missed-wakeup race that turns a 5-second proof wait into a 5-minute FinalityTimeout.

Current Behavior

Failing e2e pin at packages/rs-platform-wallet/tests/e2e/cases/al_001_concurrent_asset_lock_builds.rs:299 (AL-001 in tests/e2e/TEST_SPEC.md):

• 2 concurrent top_up_identity_with_funding tasks against the same Core-funded wallet
• Both tasks acquire UTXOs cleanly (the historical coin-selection race is closed by PR fix(platform-wallet): auto_select_inputs honors Σ inputs == Σ outputs #3554 + fix: case-insensitive .dash + atomic state on broadcast failure #3585's OutpointReservations)
• Both tasks broadcast their asset-lock transactions
• Task 1's wait_for_proof enters its check-then-await loop, observes no proof yet, drops the wallet-manager read lock, and proceeds to tokio::select! { _ = self.lock_notify.notified() => ..., _ = sleep(remaining) => ... }
• The IS-lock event for task 1's transaction arrives at LockNotifyHandler::on_sync_event between the state check and the registration of the notified() future
• Notify::notify_waiters() runs against an empty waker list and discards the event — it does not store a permit
• Task 1's notified() future then registers for an event that already fired
• Task 1 sleeps until FinalityTimeout (no second IS-lock event for the same transaction)

Failure fingerprint FinalityTimeout(<txid>) is identical across v48, v49, v50, v51 — no run-to-run drift. Reproduces deterministically under 14-thread test parallelism.

Reproduction

On branch feat/rs-platform-wallet-e2e (PR #3549):

cargo test -p platform-wallet --test e2e \
  -- --include-ignored --test-threads=14 --nocapture \
  al_001_concurrent_asset_lock_builds

Requires PLATFORM_WALLET_E2E_BANK_CORE_GATE and the Wave-E Core-funded test wallet harness. Expected: panic at al_001_concurrent_asset_lock_builds.rs:299 with task 1 panicked: FinalityTimeout(<txid>).

Root Cause

Bug site: packages/rs-platform-wallet/src/wallet/asset_lock/lock_notify_handler.rs:30

impl EventHandler for LockNotifyHandler {
    fn on_sync_event(&self, event: &dash_spv::sync::SyncEvent) {
        if matches!(event, ...InstantLockReceived... | ...ChainLockReceived...) {
            self.notify.notify_waiters();   // ← drops events for late-registering waiters
        }
    }
}

Affected caller: packages/rs-platform-wallet/src/wallet/asset_lock/sync/proof.rs:367-418 (wait_for_proof). The structural problem in the check-then-await loop:

1. loop { (line 367)
2. Read state under wallet_manager.read().await lock (lines 371-380)
3. Match record.context for InstantSend / InChainLockedBlock (lines 384-403)
4. Drop the read lock; compute remaining deadline (line 406)
5. tokio::select! — register self.lock_notify.notified() future (line 413)

If an IS-lock or ChainLock event fires between step 3 (state check finished, no proof yet) and step 5 (future registered with Notify), notify_waiters() runs through the empty waker list and the wakeup is permanently lost. There is no second event for the same transaction.

This is the canonical mis-use of tokio::sync::Notify:

• Notify::notify_waiters() — wakes only currently-registered waiters; does NOT store a permit
• Notify::notify_one() — stores a pending permit when no waiter is registered; the next notified().await claims it immediately

The same race surface appears at proof.rs:324 (the chainlock variant) — same notified().await => continue pattern, same bug.

Affected Versions

• v3.1-dev HEAD (today) — confirmed
• All prior versions since LockNotifyHandler was introduced (search returned 0 issues / 0 PRs touching notify_waiters or LockNotifyHandler)

Cross-References

• TEST_SPEC entry: packages/rs-platform-wallet/tests/e2e/TEST_SPEC.md Found-008 (lines ~2117-2138) and AL-001 (lines ~1539-1611)
• AL-001 fingerprint history: v48/v49/v50/v51 — identical
• Failing pin: al_001_concurrent_asset_lock_builds.rs:299
• NOT fixed by PR feat: identity registration with asset-lock proofs #3634: PR feat: identity registration with asset-lock proofs #3634's commit 885a1be3 removed the masternodeSyncEnabled=false FFI knob, fixing a different bug (IS/CL events never reaching LockNotifyHandler at all when SPV managers were disabled). The Found-008 race surface — events that do reach LockNotifyHandler but are dropped during the wait_for_proof check/await window — is unchanged by feat: identity registration with asset-lock proofs #3634. Verified by diff inspection: lock_notify_handler.rs:30 and the wait_for_proof loop in proof.rs are not modified in PR feat: identity registration with asset-lock proofs #3634.

Severity

HIGH — asset-lock proof flow is on the critical path of every identity registration and every identity top-up. A single missed wakeup converts a 5-second proof wait into a 5-minute hang. Any production caller (Dash Mobile, third-party apps) that drives concurrent identity operations from the same wallet hits this.

Notes

The fix is well-understood (the spec discusses Option A: notify_waiters() → notify_one() with permit storage; Option B: replace Notify with tokio::sync::broadcast). Per repo conventions for issue filing, no fix proposal in this issue body — leaving the implementation choice to the implementer.

[shumkov]

There's a third fix worth considering alongside Options A and B in the issue body — the canonical Tokio Notify pattern: arm the Notified future before the state check via enable().

loop {
    let notified = self.lock_notify.notified();
    tokio::pin!(notified);
    notified.as_mut().enable();   // registers this waiter NOW

    // state check happens with the waiter already armed —
    // any notify_waiters() between here and the await is
    // captured by the pinned future
    if let Some(proof) = check_state() {
        return Ok(proof);
    }

    let remaining = deadline.saturating_duration_since(Instant::now());
    tokio::select! {
        _ = &mut notified => continue,
        _ = sleep(remaining) => return Err(timeout),
    }
}

Tradeoff comparison vs the issue's two options:

• Option A (notify_one + permit): correct for the missed-wakeup race, but breaks multi-waiter semantics. A single CL event today wakes every parked wait_for_proof (one per outpoint); notify_one would wake only one. Fine for single-flow callers; wrong for concurrent multi-identity / multi-top-up callers (exactly the scenario AL-001 exercises).
• Option B (replace with broadcast): correct, preserves multi-waiter semantics, but a heavier API swap with downstream churn (LockNotifyHandler becomes a publisher, every wait_for_proof site needs a subscriber, lagging-receiver policy needs picking).
• Option C (Notified::enable()): preserves the existing Notify::notify_waiters() semantics (every armed waiter wakes), closes the missed-wakeup race, no API change to LockNotifyHandler or callers outside wait_for_proof / wait_for_chain_lock. ~10 lines per loop site. Available since tokio 1.13 (we're on 1.51).

Applied as a follow-up commit in #3634 — both wait_for_proof and wait_for_chain_lock patched the same way. The AL-001 e2e pin in #3549 should observe the race-free behavior once both PRs land.

Cross-reference: see the "Phase 3.5" section of #3634 for the wider context (this fix is bundled into the asset-lock catch-up + retry-hardening sweep, since Phase 3.5's new catchUpStuckAssetLocks flow spawns up to 4 concurrent wait_for_proof calls at app launch and would have directly amplified the AL-001 race surface in single-wallet flows).