Skip to content

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

Open
ellemouton wants to merge 3 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 3 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 currently 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{}.
  2. Policy finality / DoneConfirmationEvent.Done and
    SpendEvent.Done fire once a watch reaches lnd's configured re-org-safety
    depth, but the RPC server conveyed that only by closing the stream. A client
    cannot distinguish normal completion from unrelated teardown that way.

This PR makes the notification lifecycle reversible and explicitly terminal:

positive → reorg → positive → Done

Changes

  • notifier: historical confirmation and spend registrations which are
    already beyond the re-org-safety limit now receive Done immediately after
    their positive event. Without this, they can never be reached by the
    per-block finality sweep and wait forever.
  • proto: add Reorg.depth, add a payload-less Done message, and add
    done to the ConfEvent and SpendEvent oneofs. These are additive wire
    changes.
  • RPC server: forward confirmation re-org depth and emit explicit Done
    for confirmation and spend streams. The server enforces positive-before-
    Done ordering even if Go selects the terminal channel first when both are
    ready.
  • tests: cover already-mature historical registrations, terminal ordering,
    and the complete positive/reorg/positive/Done lifecycle for both stream
    types.

Semantics and compatibility

  • Done means the backend's configured re-org-safety boundary, not a claim of
    consensus-level finality.
  • Spend-side re-org depth remains 0: SpendEvent.Reorg is currently a bare
    internal signal and does not track depth.
  • Older clients ignore the additive fields/oneof variants. Lifecycle-aware
    clients can distinguish normal completion from stream failure.

Downstream adapter: lightninglabs/lndclient#283.

Validation

  • go test -tags=chainrpc ./lnrpc/chainrpc ./chainntnfs
  • go test -race -tags=chainrpc ./lnrpc/chainrpc ./chainntnfs
  • GitHub CI runs the repository-wide lint, unit, race, cross-compile, and itest
    matrix on the pushed head.

@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.
@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
@lightninglabs-deploy

Copy link
Copy Markdown
Collaborator

@Roasbeef: review reminder

@ellemouton
ellemouton force-pushed the ellemouton/chainntfn-reorg-depth-done branch 3 times, most recently from 42555ce to d451363 Compare July 15, 2026 20:37
Historical rescan matches that are already deeper than the reorg-safety
limit never pass through the per-block finality sweep in ConnectTip.
That sweep only signals Done for requests that mature at the connecting
height, so callers waiting for finality can block forever.

Signal Done while dispatching both confirmation and spend details when
the event is already past the safety limit. Deliver Confirmed or Spend
first to preserve event ordering.
The ChainNotifier confirmation and spend streams expose reorgs only as
payload-less events and provide no terminal event when a request reaches
the reorg-safety depth.

Add a depth field to Reorg and a Done variant to both event oneofs. This
is an additive protobuf change; the server begins emitting the new
values in the following commit.
@ellemouton
ellemouton force-pushed the ellemouton/chainntfn-reorg-depth-done branch from d451363 to fd02a89 Compare July 15, 2026 20:51
Forward confirmation reorg depth and explicit Done events through the
ChainNotifier RPC streams. Spend reorg depth remains zero because the
notifier does not track it internally.

Historical registrations can make the primary confirmation or spend
event ready at the same time as Done. Drain a ready primary event first
so Done cannot overtake it across the separate buffered channels. Cover
both RPC paths with ordering tests.
@ellemouton
ellemouton force-pushed the ellemouton/chainntfn-reorg-depth-done branch from f4ef0e9 to ed0a3dd Compare July 16, 2026 17:27
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.

2 participants