lwwallet: gap-fill skipped heights on next TipBlock event - #524
Conversation
|
@claude review this |
|
Claude finished @ellemouton's task in 3m 33s —— View job Code Review
The fix is structurally correct and well-reasoned. A few things to look at, ranging from a real accuracy bug to nits. 1. Comment claims
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request addresses a bug in EsploraChainService where transient failures during block processing could lead to permanently skipped heights. The implementation introduces a gap-filling mechanism that ensures all blocks between the last delivered height and the current tip are processed sequentially. Additionally, a comprehensive regression test was added using a new mock Esplora server to verify recovery from transient failures. The review feedback suggests enhancing the robustness of notification delivery by adding context cancellation checks to the select blocks, preventing potential hangs if the notification channel is full during shutdown.
| select { | ||
| case s.notifications <- chain.FilteredBlockConnected{ | ||
| Block: &blockMeta, | ||
| RelevantTxs: relevantTxs, | ||
| }: | ||
|
|
||
| case <-s.quit: | ||
| return | ||
| return false | ||
| } |
There was a problem hiding this comment.
The select block should also include a case for ctx.Done() to ensure that the goroutine can exit if the context is cancelled, even if the s.quit channel hasn't been closed yet. This prevents potential hangs if the notification channel is full during shutdown.
| select { | |
| case s.notifications <- chain.FilteredBlockConnected{ | |
| Block: &blockMeta, | |
| RelevantTxs: relevantTxs, | |
| }: | |
| case <-s.quit: | |
| return | |
| return false | |
| } | |
| select { | |
| case s.notifications <- chain.FilteredBlockConnected{ | |
| Block: &blockMeta, | |
| RelevantTxs: relevantTxs, | |
| }: | |
| case <-ctx.Done(): | |
| return false | |
| case <-s.quit: | |
| return false | |
| } |
| select { | ||
| case s.notifications <- chain.BlockConnected(blockMeta): | ||
| case <-s.quit: | ||
| return | ||
| return false | ||
| } |
There was a problem hiding this comment.
Similar to the previous notification send, this select block should also handle ctx.Done() to avoid blocking indefinitely if the context is cancelled while the notification channel is full.
| select { | |
| case s.notifications <- chain.BlockConnected(blockMeta): | |
| case <-s.quit: | |
| return | |
| return false | |
| } | |
| select { | |
| case s.notifications <- chain.BlockConnected(blockMeta): | |
| case <-ctx.Done(): | |
| return false | |
| case <-s.quit: | |
| return false | |
| } |
Add a reproducer for the "Esplora failures can make lwwallet permanently skip blocks" finding (darepo-client#369). EsploraChainService.processTipEvent fetches the raw block via Esplora when at least one address is being watched. When that single HTTP request fails (transient 5xx, timeout), the function returns without emitting FilteredBlockConnected or BlockConnected for the affected height. The shared TipPoller has already advanced its cached tipHeight past that block and will not re-emit the same TipBlock event, so the height is permanently stranded between btcwallet's view and the chain tip. Future TipBlock events deliver later heights but never re-walk the missed one, and btcwallet's sync height eventually advances past it via BlockConnected on a subsequent block. The new test builds real wire.MsgBlock values per height (so /block/:hash/raw can serve bytes that deserialize and hash-verify under EsploraClient's integrity check) and injects a single 502 on the raw-block fetch for height 101. It then advances the chain 100->103 and asserts every advanced-through height surfaces as a BlockConnected notification. The test is gated on t.Skip pending the gap-fill fix in the next commit; without it, the test fails with observed=[102 103] (101 missing) within the drain deadline. The follow-up commit removes the skip together with the fix that re-walks any heights between s.bestBlock and the latest TipBlock event.
Fix EsploraChainService.processTipEvent so a transient
GetRawBlock failure no longer permanently strands a height between
btcwallet's view and the chain tip (darepo-client#369).
Previously, on a /block/:hash/raw failure the function would log and
return without sending FilteredBlockConnected or BlockConnected.
TipPoller had already advanced its cached tipHeight past that block
and never re-emits the same TipBlock event, so the missed height was
permanently lost and btcwallet would eventually be advanced past it
by a later block's BlockConnected without ever scanning the skipped
height for relevant transactions. The previous per-component poll
loop (replaced by the shared TipPoller) owned its own bestBlock and
naturally retried the same height on the next tick; this restores
the equivalent invariant on the subscriber side.
The chain service now treats s.bestBlock.Height as its canonical
delivery cursor:
- processTipEvent snapshots s.bestBlock.Height and the watched
pkScript set under the lock, drops duplicates
(event.Height <= lastDelivered), and walks any gap between
lastDelivered and event.Height-1 via fillGap before processing
the live event.
- fillGap re-fetches hash + header for each intermediate height
(both LRU-cached on EsploraClient, so the warm path costs no
extra network round trips) and delegates per-height emission to
deliverHeight. Work is bounded by defaultMaxGapFillPerTipEvent
(256, documented at the const) so a deep outage doesn't hold
the handleTipEvents goroutine for longer than one poll
interval.
- deliverHeight extracts the GetRawBlock -> filter -> emit pair
and advances s.bestBlock only on full success. Any failure
(raw-block fetch error, shutdown mid-send) leaves s.bestBlock
at the last successfully delivered height, so the next
TipBlock event re-walks the unfinished stretch.
The same watched-scripts snapshot is passed to gap-fill and the live
event so every height processed in one invocation sees the same
watch set.
Re-enables the regression test added in the previous commit by
removing its t.Skip; the test now passes (101, 102, 103 all
delivered after one transient 502 on height 101's raw-block fetch).
d64db23 to
c2fee6a
Compare
|
Thanks @claude and @gemini-code-assist for the reviews. Pushed a squashed update (force-pushed onto the same two commits) addressing the substantive items: Addressed
Skipped (with rationale)
Local checks: |
|
Thanks for the update, @ellemouton. The changes look solid and address the points raised in the review. Specifically, adding |
|
Claude finished @ellemouton's task in 1m 7s —— View job Claude finished reviewing @ellemouton's updates — Follow-up Review
All four addressed items look correct. Quick verification: #1 — fillGap comment ( #3 — Test ordering ( #5 nit — Gemini ×2 — The skipped items have good rationale. The pre-existing double- LGTM. The fix is correct, the test is strong, and the follow-up changes address all the substantive feedback. |
The original regression test for the "Esplora failures permanently
skip blocks" finding only exercised the raw-block failure path
through fillGap. Three sibling branches were still uncovered:
1. A header fetch failure inside fillGap. Reachable in production
when blockHeaderCache has been evicted for a height that the
chain service still needs to back-fill.
2. The per-event walk cap. A pathological gap (deep Esplora
outage with no Rescan) must bound a single processTipEvent
invocation; the cursor advances by exactly the cap and the
next TipBlock event picks up the remainder.
3. Duplicate / out-of-order events. A subscribe-time race or
a future retry path may hand the chain service an event at
or below the cursor; processTipEvent must short-circuit
before any HTTP fetch or notification send.
Add direct-invocation tests for each branch. To exercise the cap
without revealing 256+ heights of HTTP traffic, introduce the
WithMaxGapFillPerTipEvent functional option on
NewEsploraChainService — production callers leave the default
in place. Extend the rawBlockStubChain test fixture with a
header-failure injection knob symmetric to the existing raw-block
one, plus small helpers (seedCursor, tipEventFor, watchOne,
requireCursor, requireNoNotification) shared across the new tests.
Roasbeef
left a comment
Roasbeef
left a comment
There was a problem hiding this comment.
LGTM added on some additional tests
🛼
2 participants
Summary
Fix the regression reported in #369: a transient
/block/:hash/rawfailure inside
EsploraChainService.processTipEventcan permanentlystrand a height between btcwallet's view and the chain tip.
When the shared
TipPollerwas introduced, it became the single sourceof truth for the chain tip and only emits each height once. The
previous per-component poll loop owned its own
bestBlockcursor andwould naturally retry the same height on the next tick after a fetch
failure. That invariant was lost on the subscriber side: a
GetRawBlockfailure inside
processTipEventreturned without sendingFilteredBlockConnectedorBlockConnected, and TipPoller neverre-emits the same
TipBlockevent. btcwallet's sync height eventuallyadvanced past the skipped block via a later
BlockConnectedwithoutever scanning it for relevant transactions.
This PR restores the cursor on the subscriber side.
EsploraChainServicenow treats
s.bestBlock.Heightas its canonical delivery cursor andwalks any gap between it and the latest
TipBlockevent beforeprocessing the live event.
Changes
lwwallet: add regression test for transient block-fetch tip skip— the reproducer (gated ont.Skipso this commit alone does not introduce a failing test).lwwallet: gap-fill skipped heights on next TipBlock event— the fix and the corresponding un-skip.Mechanics of the fix:
processTipEventsnapshotss.bestBlock.Heightand the watchedpkScript set, drops duplicates (
event.Height <= lastDelivered),and walks
(lastDelivered, event.Height-1]viafillGapbeforeprocessing the live event.
fillGapre-fetches hash + header for each intermediate height(both LRU-cached in
EsploraClient, so the warm path costs no extranetwork round trips) and delegates per-height emission to
deliverHeight. Work is bounded bydefaultMaxGapFillPerTipEvent(256, documented at the const) so a deep outage doesn't hold the
handleTipEventsgoroutine for longer than one poll interval.deliverHeightextracts theGetRawBlock-> filter -> emit pairand advances
s.bestBlockonly on full success. On any failure(raw-block fetch error, shutdown mid-send)
s.bestBlockstays atthe last successfully delivered height, so the next
TipBlockeventre-walks the unfinished stretch.
Same watched-scripts snapshot is passed to gap-fill and the live event
so every height processed in one invocation sees the same watch set.
Test plan
go test ./lwwallet/ -count=1— passes.go test ./lwwallet/ -race -count=1— passes (~20s).make fmt-changed+make lint-changed-local— clean (0 issues).observed=[102 103](height 101 is missed).observed=[101 102 103].Closes #369.