routerrpc: refuse local dispatch on a switchrpc build - #11013
Open
calvinrzachman wants to merge 3 commits into
Open
Conversation
A gateway that hands its payment lifecycle to an external router over the switchrpc interface still exposes a working embedded router. Both draw HTLC attempt IDs from independent sequencers into one shared, unowned attempt store, so overlapping IDs can deliver one payment's result to the wrong caller. Nothing prevents that today except operator discipline, meaning no manual payment RPCs and no tooling that reaches for routerrpc against a gateway. Refuse SendPaymentV2 and SendToRouteV2 with codes.FailedPrecondition on a switchrpc build, and add an enable-local-payment-dispatch flag for a node that wants the embedded router anyway. The check lives in the two routerrpc handlers, not an interceptor, since the blocked set is two methods and a named check keeps rpcperms free of payment semantics. The flag governs payment origination only. Skipping attempt store cleanup and marking the HTLC attempt database follow the build tag, because both are destructive to an external router's state and should not be reachable from a config file. Startup warns when the two differ. The itest binary carries the switchrpc tag, so the harness opts every node in and the guard test opts one back out through a node option.
Assert the guard refuses SendPaymentV2 and SendToRouteV2 on a node that keeps the switchrpc build default, while QueryRoutes, EstimateRouteFee and the HTLC interceptor stream keep answering, since an external router depends on all three.
Record the local payment dispatch guard, and note that it turns the existing single-dispatcher warning into something the daemon enforces rather than something an operator has to remember.
calvinrzachman
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Change Description
The v0.22 release notes say that only one entity, the local router or one external router, may dispatch HTLC attempts through the switch, and that running two "will lead to undefined behavior and potential loss of funds". Nothing enforced it. Both routers draw attempt IDs from independent sequencers into one shared attempt store, so a locally sent payment can collide with an in-flight external attempt and deliver one payment's result to the other caller.
This makes the rule enforced rather than a matter of operational discipline.
routerrpc.SendPaymentV2andSendToRouteV2now returncodes.FailedPreconditionon aswitchrpcbuild; they are the only two RPCs that can originate an attempt.--enable-local-payment-dispatchopts back in for a node that wants the embedded router anyway, with a warning at startup.Attempt store cleanup and the external-management DB marker stay tied to the build tag rather than to this flag. Both are destructive to an external router's state, so a wrong config value should not be able to reach them.
Alternatives considered
ControlTower.InitPaymentinstead of the handlers. That works, sinceSendOnionnever touches the control tower. I chose the handlers to keep the switchrpc concept in the RPC layer.Switch.SendHTLCis not an option, since nothing there says which router is calling.switchrpcbuild. The itest binary is built with that tag, so every itest that sends a payment would fail. Making this work means splitting the itest suite so only some of it builds with the tag.NOTE: This would not be needed if lnd supports a new attempt ID format which ensures that multiple routers do not collide within the switch's attempt result store.
Testing
lnrpc/routerrpcandconfig_test.gocover the guard decision, both handlers, and the build-tag validation. There is also a new itest: