Skip to content

lnwallet: reply to funding requests with a promise instead of channels - #11152

Open
Roasbeef wants to merge 3 commits into
lightningnetwork:reject-absurd-feeratesfrom
Roasbeef:lnwallet-fn-future
Open

lnwallet: reply to funding requests with a promise instead of channels#11152
Roasbeef wants to merge 3 commits into
lightningnetwork:reject-absurd-feeratesfrom
Roasbeef:lnwallet-fn-future

Conversation

@Roasbeef

@Roasbeef Roasbeef commented Aug 31, 2026

Copy link
Copy Markdown
Member

Builds on #11151, and is based on that branch so the diff here stays to just
the three commits that are new. Land #11151 first.

In this PR, we get the reply channels out of the internal requests the
LightningWallet message handler serves. Each of those requests used to carry
its own err chan error, and two of them carried a second channel for the value
they answer with, which left every caller draining a pair of channels in the
right order. We hand back an actor.Future instead, following the pattern the
gossiper already uses for ProcessRemoteAnnouncement (see
discovery/gossip_result.go).

The two requests that answer with a value alongside their error use
actor.Promise[fn.Result[T]], so there's no bespoke result struct anywhere:
fn.Result already is the value-or-error type, and callers just Unpack() it
back into the (T, error) pair they had before. The requests whose only answer
is an error keep an actor.Promise[error] behind a small embedded errRequest,
same as the gossiper.

On error types

One thing worth calling out for review, since it's the part that would break
silently. funding.Manager.failFundingFlow decides whether an error is safe to
forward to the remote peer with a plain type switch on
lnwallet.ReservationError, not errors.As. So anything that wraps an error on
its way out of the wallet would quietly turn every reservation error into a
generic one on the wire. fn.Result.Unpack returns the stored error verbatim,
so the concrete type survives, and TestReservationErrorTypePreserved in
lnwallet/wallet_test.go pins that down by running failFundingFlow's exact type
switch over both promise shapes.

Behavior

This is a pure refactor. Every check, error string, and early-return ordering in
the request handler is unchanged, and the awaits use context.Background() so a
caller blocks for exactly as long as the bare channel receive did.

@Roasbeef
Roasbeef force-pushed the lnwallet-fn-future branch from fd6f193 to a63804a Compare August 31, 2026 23:37
@github-actions github-actions Bot added the severity-critical Requires expert review - security/consensus critical label Aug 31, 2026
@github-actions

Copy link
Copy Markdown

🔴 PR Severity: CRITICAL

Static analysis | 9 files | 522 lines changed (excluding tests/docs where noted)

🔴 Critical (4 files)
  • funding/manager.go - channel funding workflow coordination
  • lnwallet/errors.go - wallet operation error definitions
  • lnwallet/reservation.go - channel funding reservation / commitment construction
  • lnwallet/wallet.go - core wallet operations, channel funding and signing
  • lnwallet/wallet_result.go - wallet operation result handling
🟢 Low (1 file, excluded from bump calculation)
  • docs/release-notes/release-notes-0.22.0.md - release notes
Tests (excluded from bump calculation)
  • funding/manager_test.go
  • lnwallet/reservation_test.go
  • lnwallet/wallet_test.go

Analysis

This PR modifies core wallet and channel-funding logic (lnwallet/*, funding/*), which directly touches channel funding, commitment/reservation handling, and wallet error/result plumbing — all CRITICAL-tier packages per policy. Non-test/non-generated lines changed total ~522 (over the 500-line bump threshold) and the change spans two distinct critical packages (funding and lnwallet), though this doesn't raise the tier further since CRITICAL is already the ceiling. Recommend expert review from someone familiar with the wallet/funding reservation flow.


To override, add a severity-override-{critical,high,medium,low} label.

In this commit, we lay the groundwork for getting the reply channels out of
the wallet's internal request messages. We follow the pattern the gossiper
already uses for ProcessRemoteAnnouncement, where a caller is handed an
actor.Future instead of being asked to drain a channel the request itself
carried in.

Two of the wallet's requests answer with a value alongside their error, so
those reply with an fn.Result of that value rather than a hand rolled pair
of fields. The requests whose only answer is an error get the same
treatment through errRequest. Each shape gets an embeddable request type
that owns the promise, so a handler answers a request without knowing
anything about how the caller waits on it. Resolving the promise on its
success side and letting the result carry the error, as the gossiper does,
means the concrete error type a caller type switches on reaches it
untouched.

We await with a background context so the wait can never be cut short. That
preserves the semantics of the plain channel receive these helpers are
about to replace, where a caller blocks until the request handler answers
it.
@Roasbeef
Roasbeef force-pushed the lnwallet-fn-future branch from a63804a to 60f184e Compare August 31, 2026 23:48
@Roasbeef
Roasbeef changed the base branch from master to reject-absurd-feerates September 1, 2026 00:34
In this commit, we drop the err and result channels the wallet's request
messages used to carry, and reply through the promise each request now
holds instead. The message handler goroutine and the order in which it
runs its checks are untouched; only the way an answer travels back to the
caller changes.

The old shape was easy to get wrong. A caller had to allocate a buffered
channel per request, remember that the two-value requests answer over two
separate channels, and read them back in the same order the handler wrote
them. Every early return in the handler had to remember to write to both,
and a missed write wedged the caller for good. A promise is completed once,
with everything the caller needs, and completing it twice is harmless.

We also add a test that runs the same type switch funding's failFundingFlow
does over an error carried by each of the two promise shapes. That switch
decides whether an error is safe to forward to the remote peer, so it only
works as long as nothing along the reply path wraps a ReservationError.
In this commit, we note the wallet's move from reply channels to promises
under code health.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

severity-critical Requires expert review - security/consensus critical

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant