Skip to content

lnrpc/chainrpc: surface re-org depth and Done over the chain notifier#10943

Open
ellemouton wants to merge 2 commits into
lightningnetwork:masterfrom
ellemouton:ellemouton/chainntfn-reorg-depth-done
Open

lnrpc/chainrpc: surface re-org depth and Done over the chain notifier#10943
ellemouton wants to merge 2 commits into
lightningnetwork:masterfrom
ellemouton:ellemouton/chainntfn-reorg-depth-done

Conversation

@ellemouton

@ellemouton ellemouton commented Jun 30, 2026

Copy link
Copy Markdown
Collaborator

Summary

The ChainNotifier confirmation/spend gRPC streams drop two pieces of
information that lnd already tracks internally:

  1. Re-org depthConfirmationEvent.NegativeConf carries the depth as an
    int32, but the server discarded it and sent an empty Reorg{} (the proto
    message was a // TODO(wilmer) placeholder).
  2. Finality / DoneConfirmationEvent.Done / SpendEvent.Done fire once
    a watch is past re-org safety depth, but the server conveyed this only by
    returning (closing the stream), which a client cannot distinguish from an
    unrelated teardown. There was no Done event in the ConfEvent/SpendEvent
    oneof at all.

This makes the chain-event surface reversible and terminable end to end, which
reorg-safety-aware consumers (e.g. anything that gates terminal state eviction
on reaching re-org safety depth) need.

Changes

  • proto: add a depth field to Reorg, add a new Done message, and add
    done to both the ConfEvent and SpendEvent oneofs. Additive and
    backwards compatible.
  • chain_server.go: forward the real re-org depth on the confirmation path's
    Reorg, and send an explicit Done event before exiting on both the
    confirmation and spend paths. Spend-side re-org depth is left at 0 because the
    notifier does not track one internally.

Notes

  • Spend-side re-org depth is intentionally left at 0: the notifier's
    SpendEvent.Reorg is a bare signal with no depth, so surfacing it would
    require a deeper notifier change.

@ellemouton ellemouton force-pushed the ellemouton/chainntfn-reorg-depth-done branch from 03d9901 to a8baef0 Compare June 30, 2026 20:39
@github-actions github-actions Bot added the severity-high Requires knowledgeable engineer review label Jun 30, 2026
@github-actions

Copy link
Copy Markdown

🟠 PR Severity: HIGH

gh pr files | 5 files | 288 lines changed (92 excluding generated/docs)

🟠 High (3 files)
  • lnrpc/chainrpc/chain_server.go - lnrpc/* RPC/API definition logic (chain notifier server implementation)
  • lnrpc/chainrpc/chainnotifier.proto - lnrpc/* wire/API definition change (new proto fields)
  • lnrpc/chainrpc/chainnotifier.swagger.json - lnrpc/* generated API surface (REST gateway spec)
🟢 Low (2 files)
  • docs/release-notes/release-notes-0.21.0.md - release notes only
  • lnrpc/chainrpc/chainnotifier.pb.go - auto-generated protobuf code (excluded from line/file counting)

Analysis

The highest-severity files touched are under lnrpc/chainrpc/*, which falls under the lnrpc/* category (RPC/API definitions) — classified as HIGH severity. This includes a .proto change (new/changed RPC fields) and the corresponding server-side handler in chain_server.go, plus the generated swagger spec.

No bump conditions apply: only 4 non-generated files changed (well under the 20-file threshold), ~92 non-generated/non-doc lines changed (well under the 500-line threshold), and no critical packages (lnwallet, htlcswitch, contractcourt, sweep, etc.) are touched.

Since this PR modifies a public RPC API surface (chainnotifier), it warrants review from someone familiar with the lnrpc API conventions and backward-compatibility concerns, consistent with HIGH severity.


To override, add a severity-override-{critical,high,medium,low} label.
<!-- pr-severity-bot -->

@ellemouton ellemouton force-pushed the ellemouton/chainntfn-reorg-depth-done branch from a8baef0 to 188d227 Compare June 30, 2026 21:10
@ellemouton ellemouton changed the base branch from v0.21.x-branch to master June 30, 2026 21:10
@ellemouton ellemouton marked this pull request as ready for review June 30, 2026 21:10
ellemouton added a commit to ellemouton/lndclient that referenced this pull request Jun 30, 2026
Temporary replace onto the lnd branch that adds the reorg depth and Done
events to the ChainNotifier proto (lightningnetwork/lnd#10943). Lands
before the code change so each commit builds. Drop this once that lnd
change is merged and tagged.
The ChainNotifier confirmation/spend streams previously exposed re-org
only as a payload-less event and had no terminal event for reaching
re-org safety depth. Add a depth field to the Reorg message and a new
Done event to both the ConfEvent and SpendEvent oneofs so the gRPC
surface can carry the same information lnd already tracks internally
(ConfirmationEvent.NegativeConf depth and the Done channel).

This is an additive, backwards-compatible proto change; the server side
is wired in a follow-up commit.
The RegisterConfirmationsNtfn/RegisterSpendNtfn handlers read the
internal NegativeConf depth and Done channels but dropped both at the
gRPC boundary: the re-org depth was discarded and Done was conveyed only
by returning (closing the stream), which a caller cannot distinguish
from an unrelated teardown.

Forward the re-org depth on the confirmation path's Reorg message and
send an explicit Done event before exiting on both the confirmation and
spend paths. Spend-side re-org depth is left as 0 because the notifier
does not currently track it internally.
@ellemouton ellemouton force-pushed the ellemouton/chainntfn-reorg-depth-done branch from 188d227 to 8b4da07 Compare July 1, 2026 21:30
ellemouton added a commit to ellemouton/lndclient that referenced this pull request Jul 1, 2026
Temporary replace onto the lnd branch that adds the reorg depth and Done
events to the ChainNotifier proto (lightningnetwork/lnd#10943). Lands
before the code change so each commit builds. Drop this once that lnd
change is merged and tagged.
@ellemouton ellemouton requested a review from Roasbeef July 6, 2026 14:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

severity-high Requires knowledgeable engineer review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant