Repository navigation
fix(core): release waitForTransaction observers and port viem's wait, retry and cache fixes - #82
Merged
Merged
Conversation
The `count > retryCount` branch rejected with a timeout and did not return. When the same block callback then found the transaction confirmed or replaced, or failed to fetch the block, `done` ran a second time. Its second `_unwatch()` found exactly one other listener on the shared block watcher and ran the poll cleanup, so that wait and every later wait on the client joined a dead watcher and never settled. The second emit could also reject a newer wait on the same txId with the stale error. Return after the timeout, let `done` run at most once, and make an observer's `unwatch` a no-op once its own listener is gone. A throwing `onReplaced` still rejects the wait with its error instead of leaving it pending behind the new flag.
A second wait on the same txId joins the first one's observer and has no driver of its own. `done` emitted to every listener but removed only the driver's, so the joiner stayed in `listenersCache` with its callbacks, and a third wait on that txId joined the stale list, never started a block watcher and never settled. `observe` also never deleted a key whose list became empty, so every finished wait left one key per txId, and a stopped watcher left its `cleanupCache` entry. The settling emit settles every wait on the txId, so `done` now deletes the whole key after it. `observe`'s `unwatch` deletes the key and its cleanup entry when the last listener leaves, then runs the cleanup. A wait that registers on the same key afterwards starts its own driver.
The `timeout` timer only rejected the caller's promise. It was never cleared, and it did not unwatch the block watcher or remove the observer, so a wait whose `getblockcount` never succeeded (blocked egress, an offline page) kept one poll and its closures alive until the page or process ended: block errors go to an unset `onError`, and the `retryCount` count only grows in a block callback. On expiry the driver now calls `done` with a timeout rejection, which stops the watcher and settles and removes every wait on the txId, as the `retryCount` timeout already does. A wait that joined another wait removes only itself. Each wait passes `observe` a `resolve` and `reject` that clear its timer, so every exit path clears it.
…Transaction Add two cases that fail when a guard of the fix is removed: - The driver's `timeout` fires while its first callback still retries `getBlock`. A new wait on the same txId then starts its own observer. When the stale retries end with `BlockNotFoundError`, the new wait stays pending and later resolves. Without the `finished` flag in `done`, the stale emit rejects it. - The `retryCount` timeout settles a driver and a joiner that both set `timeout`. No timer remains. Without `clearTimeout` in the `reject` wrapper, both timers stay pending. Also explain why `onBlockNumber` may use `done` before its declaration and why `done` may delete the wait key directly, and correct the changeset: `done` ran a second time, and a newer wait on the same txId settled with a stale error instead of never settling.
🦋 Changeset detectedLatest commit: 6d51caa The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
When the wait that drove the shared observer timed out, its timer called the shared `done`, so every wait that had joined it on the same txId rejected too, even one with a longer timeout or with none. On `main` each timer rejected only its own promise. Each wait now settles through its own `settle`, which clears its own timer and removes only its own listener. The timeout settles only that wait. The observer fn returns its unwatch of the shared block watcher as its cleanup, so `observe` runs it when the last wait leaves. The cleanup also ends the observer, so an in-flight callback cannot emit to a newer wait on the same txId. `done` still settles at most once, and it no longer unwatches or deletes the key itself.
…h the same options The observer key held only the client and the txId, so a wait joined any wait on the same txId and ran with the first wait's closure: its `confirmations`, `pollingInterval`, `retryCount`, `retryDelay` and `senderAddress`. A wait for 3 confirmations that joined a wait for 1 resolved at the first confirmation. The key now also holds these options, as viem does since 2.57.0. A `retryDelay` function cannot be compared and is left out. `timeout` is left out because each wait owns its timer. The spec's `waitId` helper builds the new key, and the mock chain can report confirmations that grow with the tip.
… observer The observer cleanup sets `finished`, so a block callback that is still in flight when the last wait leaves cannot emit to a newer wait that starts a new observer on the same key. The existing test for this gives the newer wait other options than the first one, so since the options are part of the key the two waits never share one, and that test passes without the flag. Add a copy where the newer wait has the same options as the first one. It fails when the cleanup does not set `finished`.
The replacement search scans the current block for a transaction that spends one of the awaited transaction's inputs. The awaited transaction spends them too, so when a provider listed it in a block before `getrawtransaction` reported it mined, the search matched it and called `onReplaced` with the transaction itself. viem skips the awaited hash for the same reason (60951cb0, viem@2.57.3); the search now skips the awaited txid, and a later callback finds the transaction mined. A replacement that `getrawtransaction` reported with 0 or no confirmations passed the confirmations check and resolved the wait at once. It now keeps the wait polling, as viem's `!receipt.blockNumber` check does. A replacement that is tracked into a later block then resolves the wait through the normal paths, which did not call `onReplaced`. That already lost the callback for a replacement that needed more confirmations. The found replacement is now kept, and every resolve reports it first.
`withRetry` started each attempt without awaiting it: the first one from the promise executor, every later one from `retry`. A throw from `shouldRetry` or from the `delay` function rejected that attempt, and nothing handled the rejection, so the returned promise never settled. Port viem's fix (f05ae7ef, viem@2.55.11): `retry` returns the next attempt, and the first attempt passes a rejection to `reject`. The inner functions get explicit `Promise<void>` return types, which the now circular calls need. viem's abort `signal` is not ported.
The `count > retryCount` branch rejects with `WaitForTransactionReceiptTimeoutError` once a wait has run `retryCount + 1` block callbacks. Every callback counted, also those after the transaction was mined, so a wait for more confirmations than the budget had left rejected while the transaction was confirming. With the default `retryCount` of 10, a wait for 6 confirmations rejected when the transaction was mined at block 106 after a start at block 100. A callback now counts only when the tracked transaction, or the replacement it tracks, has no `blockhash` after the callback. A mined transaction waits for its confirmations without a budget, and an unmined one still rejects on the 12th block with the default `retryCount`. viem dropped this budget for a default timeout (aadeada3). Here it stays for unmined transactions, where it is the only default limit. The new tests pin the unmined count, and that a burst of missed blocks counts only the callback that runs: the later callbacks of the burst return at `retrying` before they reach the count.
Port viem's `LruMap` fix (94248ab6, viem@2.47.5): - `set` deletes a key before it sets it again, so the key becomes the newest. Before, `Map.set` kept its old place, and the next eviction could drop the key that was just set. - `get` moves a key to the newest place also when its value is `undefined`, and uses `super.delete`. - The eviction reads the first key through `super.keys()`, as viem does for a stale iterator of a Map subclass on iOS 18 JavaScriptCore, and checks `firstKey !== undefined`, so an empty-string key is evicted. The explicit return types stay for `isolatedDeclarations`. No Node test can show the `super.keys()` change, because the bug is in JavaScriptCore only.
Since a replacement with no confirmations keeps the wait polling, the tracked transaction can be a replacement that later leaves the chain. The next replacement search then started from that replacement, found the reason against it and reported it as `replacedTransaction`. When a cancel was listed in a block that was reorged out and a fee bump of the cancel was mined, `onReplaced` reported `repriced` against the cancel, so a caller treated a cancelled transfer as a success. The reason is now found against the awaited transaction, parsed from `txHex`, and `onReplaced` reports that transaction as `replacedTransaction`. The search itself still starts from the tracked transaction, whose inputs the next replacement spends.
Bitcoin Core answers `getrawtransaction` for a mempool transaction without `confirmations` and without `blockhash`. The test for a replacement that is not confirmed yet used only `confirmations: 0`. Run that test for both answers. The mock chain gets an option that leaves `confirmations` out of the answer for an unconfirmed transaction. With the check narrowed to `confirmations === 0`, the new case fails.
Once the tracked transaction had a `blockhash`, a block callback no longer counted against the `retryCount` budget. When `getblockstats` answers without `height` and the wait needs more than 1 confirmation, the confirmations check can never pass, so the wait polled with no end unless the caller set `timeout`. Before the budget change it rejected on the 12th block with the default `retryCount`. Both `getblockstats` reads now record the height of the transaction's block, and a callback also counts while that height is unknown. Such a wait rejects on the 12th block again, and a wait whose block height is known still waits for every confirmation.
…sets The `retryCount` doc said `@default 6 (exponential backoff)`, but the default is 10 and `retryDelay` is a constant 3_000 ms. It also did not say that `retryCount` is the block budget. Document both uses, when a block callback counts, and the `retryDelay` default. In the changesets, say that a throwing `retryDelay` function of `waitForTransaction` hung the wait before the `withRetry` fix, that a `retryDelay` function is not part of the observer key, and that the `LruMap` fix matters only above 8192 deduplicated requests in flight.
The replacement search skipped only the tracked transaction. Once a replacement with no confirmations was tracked, the search started from it. When that replacement left the chain and the awaited transaction was mined after all, the search matched the awaited transaction as a replacement of the replacement, and `onReplaced` reported it as its own replacement with `repriced`. The search now skips both the tracked and the awaited txid, each computed once per search. When it finds the awaited transaction in the block, the wait drops the tracked replacement and tracks the awaited transaction again, so the next callback looks it up by `txId` and resolves with it without `onReplaced`. Without that step the wait kept looking up the replacement that had left the chain.
The block budget skips a callback once the height of the tracked transaction's block is known. Both `getblockstats` reads record that height, but only the read in the lookup path had a test. A found replacement is mined without such a lookup, so for it the height comes from the read in the `blockhash` path. Removing that record kept the spec green. Add a test: the original leaves the mempool at block 106, and its replacement is mined in that block. With `confirmations: 6`, the wait resolves at block 111 with the replacement and calls `onReplaced` once. Without the record in the `blockhash` path, it rejects at block 111.
Since the search skips the awaited txid, it parsed `txHex` before every block scan, also when `getrawtransaction` had answered with the hex and no replacement was found. An invalid `txHex` for a transaction that the node knows then rejected the wait at the first unconfirmed callback. Before, such a wait resolved once the transaction was mined. The scan now compares each block transaction's id with the `txId` string and with the tracked transaction's id, and `txHex` is parsed only when the scan has found a replacement, for its reason and for the `replacedTransaction` that `onReplaced` reports.
The search skips a block transaction whose id is the awaited `txId`. `getId()` gives a txid in lower case, but a caller can pass `txId` in upper case: Bitcoin Core takes either case, and bigmi passes `txId` to the node unchanged. For such a caller the skip did not match, so a mined awaited transaction was reported as a replacement of its replacement. Compare with `txId` in lower case, computed once per wait. What is sent to the node does not change. The mock chain gets an option that finds a txid in either case in `getrawtransaction`.
chybisov
changed the title
Merged
chybisov
added a commit
to lifinance/sdk
that referenced
this pull request
Oct 6, 2026
@bigmi/core 0.9.3 (lifinance/bigmi#82) fixes waitForTransaction, which the Bitcoin provider uses for every transaction. In 0.9.2 a wait whose block budget ran out could stop the shared block watcher, so a resumed route and every later wait on the same client never settled. Finished waits also kept their observers. 0.9.3 also stops reporting the awaited transaction as its own replacement and compares a replacement with the awaited transaction, so a fee bump of a cancel stays a cancel. Resume without re-signing relies on a resumed wait that settles, so the provider now requires 0.9.3.
2 of 3 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.
Which Linear task is linked to this PR?
None. Found in the memory-leak review of the LI.FI SDK (
@lifi/sdk-provider-bitcoinwaits for every Bitcoin transaction withwaitForTransaction), then extended after a comparison with viem.Why was it implemented this way?
Lifetime bugs in
waitForTransactionandobserveFour bugs leak listeners, keys or a running block poll, and one of them can freeze every later wait on a client:
count > retryCountbranch calleddone(reject)without areturn, so the same block callback could calldoneagain (when it then found the transaction confirmed or replaced, or failed to fetch the block). The second_unwatch()ran the shared block poll's cleanup while another wait on the same client still listened, so that wait and every later wait on the client never settled; it could also reject a newer wait on the same txId with the stale error.donenow runs at most once, the timeout branch returns, and an observer'sunwatchis a no-op once its own listener is gone.listenersCache. A second wait on the same txId (for example a resumed run) joined the first one's listener list; the settling emit reached both but removed only the driver, so the joiner's callbacks stayed forever and a third wait on that txId joined them and never settled. Each wait now removes its own listener when it settles, andobservedeletes a key and itscleanupCacheentry and runs the cleanup (which leaves the shared block watcher) when the last listener leaves.listenersCachekey per txId. Every finished wait left its key behind (about 185 bytes each, without limit in a long-lived process). Fixed by 2.timeoutoption did not end the wait. Its timer only rejected the promise: it was never cleared and the block watcher kept polling, so a wait whosegetblockcountnever succeeded polled until the page or process ended. The timeout now rejects and removes only its own wait, so another wait on the same txId with a longer timeout or none keeps waiting, and every way a wait settles clears its timer.Fixes ported from viem, adapted to Bitcoin
bigmi's observer, polling, retry and cache helpers come from viem. A comparison with viem 2.57.3 found fixes viem made after bigmi copied them. Each one is reproduced by a test on
mainfirst:confirmations: 3that joined a wait with 1 resolved at 1 confirmation. The key now includesconfirmations,pollingInterval,retryCount, a numericretryDelayandsenderAddress(timeoutis per wait, see 4).getrawtransactionstill reported the transaction unconfirmed butgetblockalready listed it (lagging or load-balanced nodes), the input-overlap search matched the transaction itself and calledonReplacedwith reasonrepriced. The search now skips the awaited txid (and a replacement it already tracks). A replacement thatgetrawtransactionreports without confirmations keeps the wait polling, a replacement is always compared with the awaited transaction (so a fee bump of a cancel is stillcancelled), andonReplacedfires once, when the wait resolves with the replacement. viem still drops the callback when a replacement resolves in a later block; bigmi reports it.withRetrysettles whenshouldRetryordelaythrows (viem f05ae7ef). The throw rejected an internal attempt that nothing handled, so the returned promise stayed pending; a throwingretryDelayfunction ofwaitForTransactionhung the wait.retryCountalso counted the blocks after the transaction was mined, so a wait for 6 confirmations rejected with a timeout while the transaction was confirming. viem removed its block budget for a 180 s default timeout, which does not fit 10-minute blocks; bigmi keeps the budget for an unmined transaction (same block as before) and stops it once the mined height is known.LruMapevicts the least recently used key (viem 94248ab6). A key that was set again kept its old place and an empty-string key was never evicted. This matters only above 8192 in-flight deduplicated requests.The public API and every default stay the same. Changes callers can see: a joined wait keeps its own timeout and options; a replacement without confirmations resolves when it is confirmed, not at once; a mined transaction waits for all its confirmations;
withRetryrejects instead of hanging. The two changesets describe each fix.Tests (
waitForTransaction.spec.ts,observe.spec.ts,withRetry.spec.ts,lru.spec.ts; an in-memory chain with realbitcoinjs-libtransactions and fake timers) fail without their fix and pass here; each fix was also checked by reverting it. The LI.FI SDK's use (txId,txHex,senderAddress,onReplaced, default options) was probed before and after:onReplacednever fires more often, a cancel is still reported ascancelled, and the only timing change is that a replacement reported without confirmations resolves once it is confirmed.Not in this PR (follow-ups): an in-flight block callback can keep its RPC retries for up to
retryCount × retryDelayafter the last wait leaves (bounded, also in viem); the mined-transaction path does not re-fetch a transaction whose block was reorged out (also in viem); the fallback transport'srankloop never stops, and an HTTP response body read is not covered by the request timeout (both also in viem).Visual showcase (Screenshots or Videos)
Not applicable (no UI change).
Checklist before requesting a review