Skip to content

waved: answer inbound RPCs to the envelope sender #1110

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
ellemouton wants to merge 4 commits into
base: main
Choose a base branch
from
Open

waved: answer inbound RPCs to the envelope sender #1110

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
2ac35c0
waved: Answer inbound RPCs to the envelope sender
ellemouton Aug 6, 2026
fa52ed8
waved: Address review notes on the response-sender change
ellemouton Aug 6, 2026
a1e504f
multi: Surface mailbox send rejections when answering RPCs
ellemouton Aug 11, 2026
6abaafe
multi: Document the send-rejection and sender guards
ellemouton Aug 11, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions serverconn/AGENTS.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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`.
Expand Down Expand Up @@ -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).
Expand Down
14 changes: 14 additions & 0 deletions serverconn/CLAUDE.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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`.
Expand Down Expand Up @@ -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).
Expand Down
11 changes: 11 additions & 0 deletions serverconn/compatibility.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,17 @@ func edgeResponseError[T edgeStatusCarrier](op string, resp T,
return nil
}

// SendResponseError exposes edgeResponseError for the one Edge.Send call site
// that lives outside this package (waved answers inbound mailbox RPCs on its
// own edge client). Without it that path would have to re-implement the
// status check, and a send whose rejection arrives as Status.Ok=false rather
// than as a transport error would read as a success.
func SendResponseError(op string, resp *mailboxpb.SendResponse,
err error) error {

return edgeResponseError(op, resp, err)
}

// compatibilityError returns the cached permanent version error if the
// connector has transitioned to the terminal INCOMPATIBLE state, or nil while
// it is still COMPATIBLE. Send paths consult this before contacting the edge.
Expand Down
11 changes: 8 additions & 3 deletions serverconn/e2e_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -205,9 +205,14 @@ func (s *testServer) handleRequest(ctx context.Context,
ProtocolVersion: 1,
ArkProtocolVersion: 1,
Sender: s.serverMailboxID,
Recipient: env.Rpc.ReplyTo,
Headers: headers,
Body: body,

// Mirror the production responder (waved's handleInboundRPC),
// which answers the envelope sender rather than the request's
// ReplyTo. The two are equal here, so this changes nothing
// today -- it keeps the double honest if they ever diverge.
Recipient: env.Sender,
Headers: headers,
Body: body,
Rpc: &mailboxpb.RpcMeta{
Kind: mailboxpb.RpcMeta_KIND_RESPONSE,
CorrelationId: env.Rpc.CorrelationId,
Expand Down
18 changes: 18 additions & 0 deletions waved/AGENTS.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
18 changes: 18 additions & 0 deletions waved/CLAUDE.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
46 changes: 41 additions & 5 deletions waved/server.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3567,6 +3567,16 @@ func (s *Server) handleInboundRPC(ctx context.Context,
return fmt.Errorf("missing envelope body")
}

// The sender is where the answer goes, so an absent one is the same
// lost-response failure an absent ReplyTo used to cause: the recipient
// would be empty and the mailbox store rejects that outright. Nothing
// upstream asserts it — the ingress version check only compares the
// two version fields — so refuse here, where the value is used, and
// let the ingress loop log the dispatch error.
if env.Sender == "" {
return fmt.Errorf("missing envelope sender")
}

// Dispatch through the mux to the registered handler.
respMsg, err := s.mailboxMux.ServeRPC(
ctx, env.Rpc.Service, env.Rpc.Method, env.Body.Value,
Expand Down Expand Up @@ -3605,8 +3615,27 @@ func (s *Server) handleInboundRPC(ctx context.Context,
}

responseEnv := &mailboxpb.Envelope{
Sender: s.localMailboxID,
Recipient: env.Rpc.ReplyTo,
Sender: s.localMailboxID,

// A response belongs to whoever sent the request, so address
// it to the sender rather than to the request's ReplyTo.
//
// The two agree for every producer in this repo: each sets
// Sender and Rpc.ReplyTo to the same LocalMailboxID (see
// serverconn/unary_facade.go, serverconn/actor.go,
// serverconn/heartbeat.go and the operator's clientconn
// equivalents). So this changes nothing for a well-formed
// peer, and it stops a request choosing a destination that
// disagrees with where it came from.
//
// 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.

@litbot-9000 litbot-9000 Aug 10, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟠 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,

@litbot-9000 litbot-9000 Aug 10, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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.

Headers: headers,
Body: body,
Rpc: &mailboxpb.RpcMeta{
Expand All @@ -3622,11 +3651,18 @@ func (s *Server) handleInboundRPC(ctx context.Context,
// client's negotiated mailbox transport and Ark protocol versions.
s.runtime.StampEnvelope(responseEnv)

_, err = edge.Send(ctx, &mailboxpb.SendRequest{
// Fold the response status in alongside the transport error: a mailbox
// rejection (an unknown recipient, a version mismatch) travels as
// Status.Ok=false, not as an error, so discarding it would report a
// dropped answer as a successful dispatch and leave the caller waiting
// on its correlation ID until it times out.
resp, err := edge.Send(ctx, &mailboxpb.SendRequest{
Envelope: responseEnv,
})
if err != nil {
return fmt.Errorf("send RPC response: %w", err)
if sErr := serverconn.SendResponseError(
"send rpc response", resp, err,
); sErr != nil {
return sErr
}

return nil
Expand Down
Loading
Loading