waved: answer inbound RPCs to the envelope sender - #1110
Conversation
|
@claude please review this PR. |
|
Claude finished @ellemouton's task in 2m 36s —— View job PR Review: answer inbound RPCs to the envelope sender
Verdict: LGTM. The change is small, correct, and well-justified. The reasoning in the commit and the inline comment holds up under inspection. Correctness — verified
So for a well-formed peer the destination is unchanged, and the change removes a request's ability to name a reply destination that disagrees with its own origin. Good. The dropped-response fix is real: an absent Security angle is a net positive
Test — solid
Minor / non-blocking observations
None of these block merge. |
|
Thanks — all three addressed in 1. 2. 3. Response Also useful: the
|
handleInboundRPC addressed its response to the request's Rpc.ReplyTo verbatim. A response belongs to whoever sent the request, so the sender is the authoritative destination and is what we address now. The two agree for every producer in this repo: serverconn's unary_facade, actor and heartbeat paths all set Sender and Rpc.ReplyTo to the same LocalMailboxID, as do the operator's clientconn equivalents. So this changes nothing for a well-formed peer. What it removes is a request's ability to name a reply destination that disagrees with where the request came from. It also fixes an absent ReplyTo, which produced an empty Recipient. The mailbox store rejects that outright, so a caller that left the field unset lost its answer rather than receiving it. TestHandleInboundRPCAnswersTheSender covers all three cases through the handler's error path, which still sends a response and so still has to address one.
Three follow-ups from review, none behavioural. Say in the comment that Rpc.ReplyTo is advisory from the responder's point of view. Producers still set it and nothing on this path reads it, which is worth stating so a future reader does not assume it is still honoured. Mirror the change in serverconn's testServer double, which still modelled the old behaviour. Sender and ReplyTo are equal there, so it kept working either way; matching the production handler keeps the double honest if they ever diverge. Assert the response Sender as well. newCompatTestServer left localMailboxID unset, so the test only ever pinned one half of the envelope.
c84f966 to
fa52ed8
Compare
litbot-9000
left a comment
litbot-9000
left a comment
There was a problem hiding this comment.
FINAL — The change is correct and I could not break it. Recipient: env.Rpc.ReplyTo -> Recipient: env.Sender is behaviour-preserving for every well-formed peer, and it is a small hardening win. Two findings, neither a blocker, both about gaps around the change rather than the change itself.
Is env.Sender the right reply target? Yes. I traced every producer of a KIND_REQUEST in this repo, and each sets Sender and Rpc.ReplyTo to the same LocalMailboxID: serverconn/heartbeat.go:102/:110, serverconn/actor.go:860/:869, serverconn/actor.go:1029/:1039, serverconn/unary_facade.go:129/:139. There is no proxying, relaying or fan-out consumer of ReplyTo anywhere — nothing in the repo reads Rpc.ReplyTo at all outside these producers, so the author's "advisory from the responder's point of view" framing is accurate. This is not a regression risk.
It is also mildly better on trust grounds: ReplyTo lets a request nominate a destination that disagrees with where the request came from, and answering env.Sender removes that. Test case 3 ("reply-to naming another mailbox") pins exactly this — under the old code that response went to somebody-else.
Does the new test actually fail against the old code? Yes — I proved it rather than trusting it. I patched waved/server.go back to Recipient: env.Rpc.ReplyTo on top of the PR head and re-ran:
--- FAIL: .../absent_reply-to expected "operator-1", actual ""
--- FAIL: .../reply-to_naming_another_mailbox expected "operator-1", actual "somebody-else"
--- PASS: .../matching_reply-to
2 of 3 subcases discriminate; the third is a deliberate control. That is real coverage. Worktree restored afterwards.
Checks: go vet ./waved/ clean. go test ./waved/ ./serverconn/ -count=1 both pass (18.8s / 5.6s).
One claim I could not verify. The new comment says an empty recipient is "rejected outright" by the mailbox store. The store is the operator's (lumos), which is not vendored here, so I could not confirm it in this tree — and the new test's recordingReplyEdge accepts every envelope unconditionally, so the test does not establish the drop either. I believe the claim is true, but nothing in this PR demonstrates it. Finding 1 is what makes that gap matter: because SendResponse.Status is discarded, whether the store rejects an empty recipient is precisely the thing waved cannot observe.
Inherited from my own earlier interim run and NOT re-verified here (the lumos checkout that run used is gone from this container, so treat as a lead, not evidence): on the operator side, clientconn/unary_facade.go:113/:118 and clientconn/actor.go:709/:716 likewise set Sender == ReplyTo, and lumos authenticates Envelope.Sender on Send against the TLS leaf fingerprint bound at Schnorr-verified registration (server_mtls.go:588, :740-753, server_autoregister.go:242) while nothing validates Rpc.ReplyTo. If that holds, it makes Sender the strictly better-attested field and strengthens the case for this change. It does not affect either finding below.
Verdict rationale: COMMENT, not REQUEST_CHANGES. The PR does what it says. Finding 1 is a genuine bug but pre-existing and only adjacent — it is flagged because the PR's own comment asserts the lost-answer problem is fixed, and it is only narrowed. Finding 2 is a three-line guard.
| // | ||
| // It also fixes an absent ReplyTo. That used to produce an | ||
| // empty recipient, which the mailbox store rejects outright, | ||
| // so the caller lost its answer entirely. |
There was a problem hiding this comment.
🟠 major — This comment claims more than the change delivers. Removing the empty-ReplyTo cause is right, but the silence is what made that bug expensive, and the silence is still here.
SendResponse.Status is this codebase's canonical application-level failure channel — mailboxconn.StatusError, StatusMailboxVersionUnsupported, StatusArkVersionMismatch, StatusUpgradeRequired all travel that way, never as a transport error. Three of the four Edge.Send call sites in the repo fold it correctly through edgeResponseError (serverconn/unary_facade.go:143, serverconn/actor.go:1109, serverconn/heartbeat.go:117). This one, at line 3644, is the only one that discards it:
_, err = edge.Send(ctx, &mailboxpb.SendRequest{
Envelope: responseEnv,
})
if err != nil {
return fmt.Errorf("send RPC response: %w", err)
}So any rejection that arrives as Status.Ok=false — a version mismatch, an unknown recipient, whatever the operator adds next — returns nil here. handleInboundRPC reports success, the ingress loop acks the request as dispatched, and the operator's waiter blocks until it times out. Nothing is logged on either side. That is the same failure mode the empty-ReplyTo bug had; this PR removed one cause of it and left the detector missing, so the next cause will be just as invisible.
This also makes the claim on these lines unfalsifiable from inside waved: whether the store really "rejects outright" is exactly what this send path cannot see. (The new test's recordingReplyEdge accepts everything, so it does not establish the rejection either.)
Suggest folding this send into the same helper as its three siblings:
resp, err := edge.Send(ctx, &mailboxpb.SendRequest{
Envelope: responseEnv,
})
if sErr := edgeResponseError("send rpc response", resp, err); sErr != nil {
return sErr
}(edgeResponseError is unexported in serverconn, so this needs a small exported wrapper — worth it to stop waved being the one send path that cannot see a rejection.)
To be clear: this is pre-existing and not introduced here, and I am not blocking on it. But the comment at these lines asserts the lost-answer problem is fixed, and with the status still dropped it is only narrowed.
| // Rpc.ReplyTo is therefore advisory from the responder's | ||
| // point of view: producers still set it, and nothing here | ||
| // reads it. | ||
| Recipient: env.Sender, |
There was a problem hiding this comment.
🟡 minor — env.Sender is now the sole input that decides where the answer goes, and it is the one field this function does not guard.
The preamble checks its two siblings:
if env.Rpc == nil { ... }
if env.Body == nil { ... }and serverconn's ingress validation (validateInboundEnvelope, serverconn/inbound_version.go:18) only compares the two version fields — it never looks at Sender. I grepped the inbound path: nothing anywhere asserts env.Sender != "".
So an inbound KIND_REQUEST with an empty Sender produces exactly the empty-Recipient response this PR set out to eliminate. The hazard moved from Rpc.ReplyTo to Sender rather than going away — and combined with the dropped SendResponse.Status above, the loss is silent. The new test covers absent ReplyTo but never absent Sender, so the one case that is still broken is the one case not exercised.
A third guard alongside the existing two closes it, and turns a silent drop into a dispatch error the ingress loop logs:
if env.Sender == "" {
return fmt.Errorf("missing envelope sender")
}Worth a fourth table entry in TestHandleInboundRPCAnswersTheSender asserting the handler errors and sends nothing.
The daemon's inbound-RPC responder was the one Edge.Send call site in the repo that discarded SendResponse.Status. That status is the mailbox's application-level failure channel: a rejected recipient or a version mismatch arrives as Status.Ok=false, never as a transport error. Discarding it meant handleInboundRPC returned nil for a response that was never delivered, the ingress loop acked the request as dispatched, and the caller blocked on its correlation ID until it timed out, with nothing logged on either side. Fold the status through the same check its three siblings already use. edgeResponseError is unexported, so add a small exported wrapper for the send case rather than re-implementing the check in waved. Also guard an empty envelope Sender. Sender is now the sole input deciding where an answer goes, and nothing upstream asserts it: the ingress validation only compares the two version fields. An empty one therefore produces the same empty recipient the absent-ReplyTo case used to, so refuse it alongside the existing nil-Rpc and nil-Body checks and let the ingress loop log the dispatch error.
📚 Doc gardening advisoryThis PR's Go changes in diff --git a/serverconn/AGENTS.md b/serverconn/AGENTS.md
index a88da6d0..433d8be7 100644
--- a/serverconn/AGENTS.md
+++ b/serverconn/AGENTS.md
@@ -31,6 +31,14 @@ background ingress polling with event routing.
- `NewAuthenticatedMailboxClient` — `mailboxpb.MailboxServiceClient` decorator
that signs and attaches the `x-mailbox-auth-sig` header to every `Send`
before forwarding to the wrapped edge transport.
+- `SendResponseError` — Exported wrapper over the package-internal
+ `edgeResponseError` (`compatibility.go`) for the one `Edge.Send` call site
+ outside this package: `waved` answering an inbound mailbox RPC on its own
+ edge client. Maps a `(*mailboxpb.SendResponse, error)` pair to the single
+ error the caller should return — transport error wrapped with `op`, nil
+ response reported as a transport error, non-OK `Status` converted to a
+ `*mailboxconn.StatusError` — or nil when the send was accepted. It does not
+ drive the incompatibility transition; callers still pass the error on.
- `AckState` — Four-cursor watermark state machine (PullCursor, DispatchCommittedTo, AckTarget, AckCommittedTo).
- `SendUnaryRequest` — Durable typed unary request that becomes a real unary RPC after commit. The response arrives via KIND_RESPONSE and, if no in-memory waiter exists, falls back to durable route dispatch via the EventRouter.
- `DurableUnaryRequestBuilder` — Interface for proof-gated request-body construction. Implementations build the actual proto request (e.g., with signed proofs) at send time, not at persist time. The interface is provided via `ConnectorConfig.DurableUnaryBuilder`.
@@ -84,6 +92,12 @@ background ingress polling with event routing.
the logical request (see `mailboxrpc.Retry`). `SendRPC` mints a fresh key
only when the caller leaves `RPCOptions.IdempotencyKey` empty, which is
correct for a single-shot call and defeats deduplication for a retry.
+- A mailbox rejection (unknown recipient, empty recipient, version mismatch)
+ travels back as `SendResponse.Status.Ok = false`, **not** as a gRPC error,
+ so no `Edge.Send` caller may discard the response and check only `err` —
+ that reports a dropped envelope as a successful dispatch. Inside the
+ package every path routes through `edgeResponseError`; outside it, use the
+ exported `SendResponseError`.
- Ingress loop checkpoints pull cursor and ack state; on restart, resumes from checkpoint.
- `DurableUnaryQuery` values are handled generically in `ServerConnectionActor.Receive` via `buildDurableUnary`: the query is converted to a `SendUnaryRequest` using the configured `DurableUnaryRequestBuilder`. Adding a new durable indexer query type requires only implementing `DurableUnaryQuery` — no new `Receive` case is needed.
- `DurableUnaryQuery` implementations must produce stable identity bytes in `BuildBody` so that `MsgID` and `IdempotencyKey` are deterministic across restarts (auto-derived via `mailboxconn.StableEventMsgID` / `StableEventIdempotencyKey` when the caller leaves them empty).
diff --git a/serverconn/CLAUDE.md b/serverconn/CLAUDE.md
index a88da6d0..433d8be7 100644
--- a/serverconn/CLAUDE.md
+++ b/serverconn/CLAUDE.md
@@ -31,6 +31,14 @@ background ingress polling with event routing.
- `NewAuthenticatedMailboxClient` — `mailboxpb.MailboxServiceClient` decorator
that signs and attaches the `x-mailbox-auth-sig` header to every `Send`
before forwarding to the wrapped edge transport.
+- `SendResponseError` — Exported wrapper over the package-internal
+ `edgeResponseError` (`compatibility.go`) for the one `Edge.Send` call site
+ outside this package: `waved` answering an inbound mailbox RPC on its own
+ edge client. Maps a `(*mailboxpb.SendResponse, error)` pair to the single
+ error the caller should return — transport error wrapped with `op`, nil
+ response reported as a transport error, non-OK `Status` converted to a
+ `*mailboxconn.StatusError` — or nil when the send was accepted. It does not
+ drive the incompatibility transition; callers still pass the error on.
- `AckState` — Four-cursor watermark state machine (PullCursor, DispatchCommittedTo, AckTarget, AckCommittedTo).
- `SendUnaryRequest` — Durable typed unary request that becomes a real unary RPC after commit. The response arrives via KIND_RESPONSE and, if no in-memory waiter exists, falls back to durable route dispatch via the EventRouter.
- `DurableUnaryRequestBuilder` — Interface for proof-gated request-body construction. Implementations build the actual proto request (e.g., with signed proofs) at send time, not at persist time. The interface is provided via `ConnectorConfig.DurableUnaryBuilder`.
@@ -84,6 +92,12 @@ background ingress polling with event routing.
the logical request (see `mailboxrpc.Retry`). `SendRPC` mints a fresh key
only when the caller leaves `RPCOptions.IdempotencyKey` empty, which is
correct for a single-shot call and defeats deduplication for a retry.
+- A mailbox rejection (unknown recipient, empty recipient, version mismatch)
+ travels back as `SendResponse.Status.Ok = false`, **not** as a gRPC error,
+ so no `Edge.Send` caller may discard the response and check only `err` —
+ that reports a dropped envelope as a successful dispatch. Inside the
+ package every path routes through `edgeResponseError`; outside it, use the
+ exported `SendResponseError`.
- Ingress loop checkpoints pull cursor and ack state; on restart, resumes from checkpoint.
- `DurableUnaryQuery` values are handled generically in `ServerConnectionActor.Receive` via `buildDurableUnary`: the query is converted to a `SendUnaryRequest` using the configured `DurableUnaryRequestBuilder`. Adding a new durable indexer query type requires only implementing `DurableUnaryQuery` — no new `Receive` case is needed.
- `DurableUnaryQuery` implementations must produce stable identity bytes in `BuildBody` so that `MsgID` and `IdempotencyKey` are deterministic across restarts (auto-derived via `mailboxconn.StableEventMsgID` / `StableEventIdempotencyKey` when the caller leaves them empty).
diff --git a/waved/AGENTS.md b/waved/AGENTS.md
index b778f21c..f5aa6530 100644
--- a/waved/AGENTS.md
+++ b/waved/AGENTS.md
@@ -136,6 +136,24 @@ For field-level detail, use `go doc github.com/lightninglabs/wavelength/waved.<S
exceeds `OORConfig.MaxTransientSubmitRetry` (default 1h), persisting the
window start (`FirstRejectUnixNanos`) in the outgoing snapshot (version 5)
so the bound survives restarts.
+- `handleInboundRPC` (the mux bridge registered by `buildRPCDispatchers` for
+ every `NonTxRoutes` dispatcher) addresses its response envelope to
+ `env.Sender`, **not** to `env.Rpc.ReplyTo`. Every producer in this repo sets
+ both to the same `LocalMailboxID` (`serverconn/unary_facade.go`,
+ `serverconn/actor.go`, `serverconn/heartbeat.go`, and the operator's
+ `clientconn` equivalents), so the two agree for a well-formed peer; routing
+ by sender stops a request naming a destination that disagrees with its
+ origin, and stops an absent `ReplyTo` producing an empty recipient that the
+ mailbox store rejects outright. `Rpc.ReplyTo` is therefore advisory from the
+ responder's side: producers still set it, nothing here reads it. An empty
+ `env.Sender` is refused up front (the ingress version check only compares the
+ version fields, so nothing upstream asserts it) and the ingress loop logs the
+ dispatch error.
+- The response `Edge.Send` in `handleInboundRPC` folds the send status in
+ alongside the transport error via `serverconn.SendResponseError`. A mailbox
+ rejection arrives as `Status.Ok = false` rather than as an error, so checking
+ only `err` would report a dropped answer as a successful dispatch and leave
+ the caller blocked on its correlation ID until it times out.
- `operatorTermsFromResponse` and daemon `GetInfo` must preserve
`FreeRefreshWindowBlocks` end to end.
- The VTXO manager reads `FreeRefreshWindowBlocks` from the latest cached
diff --git a/waved/CLAUDE.md b/waved/CLAUDE.md
index b778f21c..f5aa6530 100644
--- a/waved/CLAUDE.md
+++ b/waved/CLAUDE.md
@@ -136,6 +136,24 @@ For field-level detail, use `go doc github.com/lightninglabs/wavelength/waved.<S
exceeds `OORConfig.MaxTransientSubmitRetry` (default 1h), persisting the
window start (`FirstRejectUnixNanos`) in the outgoing snapshot (version 5)
so the bound survives restarts.
+- `handleInboundRPC` (the mux bridge registered by `buildRPCDispatchers` for
+ every `NonTxRoutes` dispatcher) addresses its response envelope to
+ `env.Sender`, **not** to `env.Rpc.ReplyTo`. Every producer in this repo sets
+ both to the same `LocalMailboxID` (`serverconn/unary_facade.go`,
+ `serverconn/actor.go`, `serverconn/heartbeat.go`, and the operator's
+ `clientconn` equivalents), so the two agree for a well-formed peer; routing
+ by sender stops a request naming a destination that disagrees with its
+ origin, and stops an absent `ReplyTo` producing an empty recipient that the
+ mailbox store rejects outright. `Rpc.ReplyTo` is therefore advisory from the
+ responder's side: producers still set it, nothing here reads it. An empty
+ `env.Sender` is refused up front (the ingress version check only compares the
+ version fields, so nothing upstream asserts it) and the ingress loop logs the
+ dispatch error.
+- The response `Edge.Send` in `handleInboundRPC` folds the send status in
+ alongside the transport error via `serverconn.SendResponseError`. A mailbox
+ rejection arrives as `Status.Ok = false` rather than as an error, so checking
+ only `err` would report a dropped answer as a successful dispatch and leave
+ the caller blocked on its correlation ID until it times out.
- `operatorTermsFromResponse` and daemon `GetInfo` must preserve
`FreeRefreshWindowBlocks` end to end.
- The VTXO manager reads `FreeRefreshWindowBlocks` from the latest cachedHow to apply: save the diff above and Note on This check is advisory only and does not block merge. Run log: https://github.com/lightninglabs/wavelength/pull/1110/checks |
The doc-gardening advisory on this PR is right: the Go change left both packages' docs stale. serverconn gains SendResponseError, the exported seam that lets a caller outside the package fold an application-level rejection through the same check the package uses internally -- a Status.Ok of false is this codebase's failure channel and never arrives as a transport error, so a caller that only checks err reports success on a rejected send. waved records that an inbound RPC is answered to env.Sender and that the field is now guarded, since it is the sole input deciding where the answer goes and nothing on the ingress path asserts it is set.
|
@ellemouton, remember to re-request review from reviewers when ready |
1 similar comment
|
@ellemouton, remember to re-request review from reviewers when ready |
2 participants
A response belongs to whoever sent the request.
handleInboundRPCwas addressing its response to the request'sRpc.ReplyToverbatim, so a request could name a reply destination that disagrees with where it came from.What changes
Recipient: env.Rpc.ReplyTobecomesRecipient: env.Sender.Nothing changes for a well-formed peer. Every producer in this repo sets
SenderandRpc.ReplyToto the sameLocalMailboxID:serverconn/unary_facade.go:129/:139serverconn/actor.go:860/:869and:1029/:1039serverconn/heartbeat.go:102/:110The operator's
clientconnequivalents do the same. So the two fields already agree everywhere they are produced, and addressing the sender is the same destination by a more direct route.It also fixes a dropped response
An absent
ReplyToproduced an emptyRecipient. The mailbox store rejects an empty recipient outright, so a caller that left the field unset lost its answer entirely rather than receiving it. That case now answers the sender like any other.Testing
TestHandleInboundRPCAnswersTheSendercovers matching, absent, and divergentReplyTo. It dispatches a deliberately unregistered method so the handler takes its error path — that path still sends a response envelope, so it still has to address one, and it is the easier of the two to get wrong.Verified the two changed cases (absent and divergent) fail against the previous behaviour and the matching case passes, so the test pins the change rather than restating it.
make unit pkg=waved,make lint-changed-local(0 issues) andmake commitmsg-lintall pass.