Repository navigation
Conversation
Each block subscriber opens its own lnd stream and the height-only lndclient wrapper requires a separate hash RPC for each event. Share one lazy backend stream and read the hash already present in authenticated chainrpc notifications, keeping block identity intact across reorgs. Bound each subscriber queue and isolate cancellation and mutable event values. Reconnect once from the last hash/height cursor, and join upstream cleanup on the last cancellation or backend shutdown. Keep live tip queries independent of the notification cache. Exercise concurrent fanout, reconnect, slow consumers, cancellation races, and malformed events in unit tests, plus hash-preserving fanout through a real bitcoind/lnd reorg in the system harness.
| runCtx, cancel := context.WithCancel( | ||
| context.Background(), | ||
| ) | ||
| event, err := b.notifier.RegisterBlockEpochNtfn(nil) |
There was a problem hiding this comment.
Nit: this opens the upstream stream while holding blockMu, and establishment can take up to lndRegistrationTimeout (15s). In that window cancelBlockSubscriber, stopBlocks and any other RegisterBlocks call all wait on the lock. It only happens on first subscribe or after a full teardown, so low impact, but it could be avoided by marking the run as starting, dropping the lock for the registration, and re-taking it to install the run (having concurrent callers wait on run.done-style like the teardown case already does).
A bounded block queue can close while retaining a height that reached finality, even if a later reorg could not be enqueued. Do not synthesize Done from that stale closed backlog. Re-arm the watch and refresh the live tip before evaluating finality again. Regression tests reproduce premature confirmation and spend completion before the fix. Exercise the real block actor over the shared backend to verify that a blocked target recovers after overflow or stops promptly on cancellation while another subscriber stays live. Document queue closure as a reconciliation boundary for callers.
djkazic
left a comment
There was a problem hiding this comment.
Nice approach. Reading the hash off the raw chainrpc event and resuming from a cursor are both clear wins over a per-height GetBlockHash. Two notes from working on the same problem in #1481, which takes a different approach to both of these.
| runCtx, cancel := context.WithCancel( | ||
| context.Background(), | ||
| ) | ||
| event, err := b.notifier.RegisterBlockEpochNtfn(nil) |
There was a problem hiding this comment.
This opens the upstream stream while blockMu is held. Against a remote lnd, RegisterBlockEpochNtfn can take up to lndRegistrationTimeout (15s). For that whole time, every other RegisterBlocks and cancelBlockSubscriber call is stuck on the mutex. So is stopBlocks, which means backend shutdown waits behind the open, and the caller's ctx is only checked before the lock is taken.
One option is to mark the open as in progress (an opening attempt with its own cancel and done channel), release the lock while the open runs, then re-take it to install the run. Concurrent registrations wait on the attempt's done channel. stopBlocks can cancel the open, then join it. #1481 does this in blockEpochHub.register/open.
| copyBlock := block | ||
| select { | ||
| case sub.epochs <- ©Block: | ||
| default: |
There was a problem hiding this comment.
When a subscriber falls more than 10 epochs behind, it gets closed here, and that's why the conf/spend actors need the re-arm guard in the second commit. Any future consumer that needs every block also has to reconcile on its own.
One case worth checking: after an outage, the cursor reconnect makes lnd replay all the missed blocks in a quick burst. If more than 10 were missed, every subscriber that doesn't drain fast enough overflows during the replay. They then all reconnect at once and receive only the latest tip, which defeats the catch-up.
An alternative is an unbounded queue per subscriber. Fan-out pushes onto the subscriber's pending list and wakes a worker that delivers to epochs. Nothing is dropped, so the finality guard isn't needed. The cost is memory growth if a consumer wedges, which is the same backlog lnd held per client before this change. #1481 takes that route (see epochSub.enqueue), but either way the catch-up burst may warrant a larger buffer.
|
Superseded by #1482, which combines this PR with #1481. It keeps the raw chainrpc hash-in-event stream, the shared reconnect with cursor resume, and the finality re-arm commit (cherry-picked unchanged, authorship kept). It also addresses the review here: the stream is opened without holding the lock, and queues are sized for the reconnect catch-up burst. |
Each block subscriber currently opens a separate lnd notification stream. The remote adapter uses lndclient's height-only convenience wrapper, then calls
ChainKit.GetBlockHashfor every event on every stream. N subscribers therefore create N upstream streams and N hash RPCs per block.This change shares one lazy block stream in
LNDBackend, including subscriptions used for confirmation/spend finality. It reads lnd's authenticated raw chainrpc stream, whose events already contain both height and hash, eliminating the hash lookup entirely and keeping block identity tied to the notification across reorgs. Subscriber actors retain their existing local registration and cancellation APIs.BestBlockremains a live ChainKit query. Notification caching does not replace it. Explicit disconnect signals remain on confirmation/spend streams.Validation:
chainbackends,chainsource,chainfees,vtxo,txconfirm,unroll, andround.chainbackendsandchainsource; backend unit target withstdlog trace.make fmt-changed,make lint-changed-local(zero issues, including actorblock), and module tidiness checks.TestLNDBlockFanoutReorg,TestChainSourceConfReorgRoundTrip, andTestTxConfirmReorgRoundTrip.Limitations: reconnect retries indefinitely, matching current recovery policy. Related outage-escalation work in #762 would need to observe shared-reader health now that the backend owns reconnect. The chainntnfs registration interface has no caller context; remote stream establishment is bounded to 15 seconds, so shutdown racing establishment may wait up to that bound. Per-actor event processing remains; this change removes duplicated upstream work and does not claim a measured CPU improvement.