lnrpc+rpcserver: include short channel id in pending close channels - #10989
lnrpc+rpcserver: include short channel id in pending close channels#109890xfandom wants to merge 2 commits into
Conversation
The PendingChannels response identifies channels only by their channel point, which makes it awkward to correlate an entry with output from ListChannels or the forwarding history, both of which key off the short channel id. Both the close summary of a force closed channel and the OpenChannel of a waiting close channel already carry the confirmed short channel id, so surface it as chan_id on ForceClosedChannel and WaitingCloseChannel. Pending open channels are deliberately left out: their funding transaction has not confirmed yet, so there is no short channel id to report.
Extend the existing memo assertions in the anchor reserve test to also check that the short channel id captured while the channel was open is echoed back in both the waiting close and pending force close entries. Fixes lightningnetwork#5575
Lrifton92
left a comment
There was a problem hiding this comment.
Checked the claim this PR rests on — that the surfaced value is the same id a client already saw from ListChannels — rather than assuming it, because if the two sides read different fields the feature is worse than useless (it would silently pair up the wrong channel).
It holds. ListChannels builds ChanId from dbScid := dbChannel.ShortChannelID (rpcserver.go L5044, used at L5065), which is exactly the field fetchWaitingCloseChannels now reads. On the force-close side, pendingClose.ShortChanID is the same field ClosedChannels already reports (L5317), so the new value is consistent with both neighbouring RPCs.
I went looking specifically for a zero-conf / option_scid_alias mismatch — the obvious way this could break, where ListChannels hands back an alias while the pending-close path reports a confirmed SCID — and there isn't one: both sides read ShortChannelID off the same DB channel, and aliases are surfaced separately through alias_scids. So the correlation promise survives the alias case rather than quietly failing on it.
One thing that will fail CI as-is: rpcserver.go is not gofmt-clean on the head commit (bb43319). I ran gofmt -d on both revisions — master is clean, the head is not, and the only hunk it reports is the line this PR adds:
BlocksTilCloseConfirmed: blocksTilCloseConfirmed,
- ChanId: waitingClose.ShortChannelID.ToUint64(),
+ ChanId: waitingClose.ShortChannelID.ToUint64(),
CloseHeight: waitingClose.CloseConfirmationHeight.
The CloseHeight entry below it looks like it escapes the alignment group only because its value wraps, which makes the short padding look right by eye — it isn't.
Worth noting the gofmt-correct form is then 88 columns, over lnd's 80-char limit, so it can't just be reformatted in place. The cleanest fix is the one fetchPendingForceCloseChannels already uses two hunks earlier — hoist it to a local before the literal, mirroring closeChanID:
waitingCloseChanID := waitingClose.ShortChannelID.ToUint64()
...
ChanId: waitingCloseChanID,Smaller points, all fine as they stand:
- Field numbers 8 (
WaitingCloseChannel) and 10 (ForceClosedChannel) don't collide with the reserved ranges in those messages, and both carry[jstype = JS_STRING]— I confirmed it survives into the generated descriptor (B\x020\x01on both). That matters more than it looks for auint64channel id: without it, REST/JSON clients on a 53-bit float silently mangle the value, and this field would be the one place inPendingChannelswhere that bites. - The itest asserts equality against the id captured while the channel was still open, rather than merely asserting non-zero. That is the assertion that actually tests the property being added, so it will catch a regression that swaps the field for a different-but-plausible id.
|
@0xfandom, remember to re-request review from reviewers when ready |
1 similar comment
|
@0xfandom, remember to re-request review from reviewers when ready |
Change Description
PendingChannelsidentifies channels only by their channel point, so correlating an entry withListChannelsoutput or forwarding history (both keyed on the short channel id) means matching on the outpoint by hand.This adds
chan_idtoWaitingCloseChannelandForceClosedChannel. Both already have the confirmed short channel id available server side —ChannelCloseSummary.ShortChanIDfor force closed channels andOpenChannel.ShortChannelIDfor waiting close ones — so it is just a matter of surfacing it.Pending open channels are deliberately left out: the funding transaction has not confirmed yet, so there is no short channel id to report.
This picks up #8000, which had already been reviewed and green-lit but was closed because its staging branch got deleted (see #8000 (comment)). The proto field numbers and server-side wiring match that PR; I additionally added itest coverage.
Fixes #5575
Steps to Test
The existing
testAnchorReservedValueitest now captures the short channel id while the channel is open and asserts it is echoed back in both the waiting close and pending force close entries:Pull Request Checklist
release-notesfile