cmd/loop: fix route hint parsing - #1209
Conversation
|
/gateway review |
![lightninglabs-gateway[bot]](https://avatars.githubusercontent.com/in/3635431?s=60&v=4){kind=small}
There was a problem hiding this comment.
Gateway review — 5 findings
🔴 0 Blocker · 🟠 2 Major · 🟡 3 Minor · 🔵 0 Nit
Summary
This PR fixes two real bugs in the --route_hints path: the shadowed hints := in validateRouteHints that silently discarded every decoded hint, and the comma-splitting of JSON hint values by the CLI's slice-flag parser. Both fixes are correct in substance, and the release-note entry is present.
The concern is scope. Rather than disabling the separator on the flag or the commands that own it, the change walks the whole command graph and flips DisableSliceFlagSeparator on every command, which redefines the parsing rule for every string-slice flag in the CLI — a user-visible change this PR never intended to make and does not document. Separately, now that decoded hints actually reach the swap request for the first time since the shadowing landed, the absence of any post-decode validation becomes reachable: a hint with mistyped JSON keys unmarshals cleanly into a zero-valued RouteHint and is forwarded without complaint.
The new test locks in the three DisableSliceFlagSeparator bits but then replaces the real command graph with a synthetic subcommand before running, so it does not exercise the shipped commands end to end.
Bot commands
/gateway re-review— re-run after pushing changes (maintainers)/gateway dismiss <id>— silence a finding (maintainers)/gateway explain <id>— elaborate on a finding (anyone)
|
🤖 gateway audit metadata for this PR — auto-generated, please don't edit. |
85035cb to
d1699f3
Compare
|
/gateway re-review |
1 similar comment
|
/gateway re-review |
There was a problem hiding this comment.
Gateway review — 2 findings
🔴 0 Blocker · 🟠 0 Major · 🟡 2 Minor · 🔵 0 Nit
Summary
The fix was rewritten rather than patched, and the new approach resolves the substance of the prior review. DisableSliceFlagSeparator is now declared on the three command literals that carry routeHintsFlag (cmd/loop/loopin.go:56, cmd/loop/quote.go:30, cmd/loop/staticaddr.go:422) instead of being recursively stamped onto the shared command graph from main.go, which removes both the graph-wide blast radius and the construct-time mutation of package-level singletons. The shadowed slice is fixed and json.Unmarshal is replaced with protojson.Unmarshal, which accepts both hop_hints and hopHints and rejects unknown fields outright, and unmarshallRouteHints now rejects nil/empty route hints, nil hop hints, zero ChanId, zero CltvExpiryDelta, and deltas above math.MaxUint16 — closing the silent uint16() truncation in unmarshallHopHint. The new tests exercise the real command graph across all three paths rather than a synthetic stand-in.
What remains is narrow. quote in and static in each define their own string-slice flag (--deposit_outpoint, --utxo), and both now lose comma-splitting along with --route_hints; the release note still describes only the route-hint fix. Both are minor: the affected invocations fail loudly at the daemon rather than proceeding with wrong values.
One thing I could not check: swapclient_server.go is truncated in the provided context before the StaticAddressLoopIn handler, so I could not confirm that the route hints forwarded from loop static in also pass through the newly-hardened unmarshallRouteHints. Worth a glance before merge.
Status of prior findings
- F2 addressed: Fixed in
cmd/loop/utils.go:40andloopd/swapclient_server.go:1248.protojson.Unmarshalaccepts both JSON name forms and errors on unknown fields —TestRouteHintsRejectUnknownFieldspins the"unknown field"behavior on the misspelledcltv_expiry_delatcase — and the daemon now rejects nil/empty route hints, nil hop hints, zeroChanId, zeroCltvExpiryDelta, andCltvExpiryDelta > 65535beforeunmarshallHopHintcan truncate it touint16, with the hop-hint parse error wrapped and indexed. - F3 addressed: Fixed by construction —
main.gois no longer in the diff andDisableSliceFlagSeparator: trueis declared in the command literals themselves (cmd/loop/loopin.go:56,cmd/loop/quote.go:30,cmd/loop/staticaddr.go:422), so the property lives where it is defined instead of being applied as a side effect of building the root command. - F4 addressed: Fixed in
cmd/loop/utils_test.go:35. The test now builds the real root command and walks to each target viacommandAtPath, overriding only theAction, and table-drives all three real paths (in,quote in,static in) — so the comma-preservation assertions fail if the per-command wiring is dropped, which the synthetic stand-in could not detect.
Bot commands
/gateway re-review— re-run after pushing changes (maintainers)/gateway dismiss <id>— silence a finding (maintainers)/gateway explain <id>— elaborate on a finding (anyone)
|
/gateway approve |
|
❌ Cannot approve: 2 finding(s) still open.
Address them and run |
Prevent urfave/cli from splitting route hint arguments on commas so that their JSON remains intact. Limit this behavior to route-hint commands while preserving comma parsing for their other slice flags. Return and strictly decode the hints instead of discarding them through a shadowed local variable. Reject malformed hints at the loopd RPC boundary before invoice creation.
d1699f3 to
dc96c49
Compare
|
/gateway re-review |
There was a problem hiding this comment.
Gateway review — 8 findings
🔴 0 Blocker · 🟠 1 Major · 🟡 7 Minor · 🔵 0 Nit
Summary
The rewrite between reviews is a clear improvement. DisableSliceFlagSeparator is now declared on the three command literals that actually carry routeHintsFlag rather than being walked over the whole command graph from main.go, and the two non-route-hint slice flags on those commands keep comma parsing through the new commaSeparatedStringSlice helper — so F1 and F3 are closed, and the test now drives the real graph (F4). Switching the CLI to protojson plus the new structural checks in unmarshallRouteHints closes F2 properly: a misspelled field now errors instead of decoding into a zero-valued hint.
What is left is the cost of that server-side strictness. unmarshallRouteHints is reached from LoopIn, Probe and GetLoopInQuote, so requests that previously succeeded with a zero ChanId, a zero CltvExpiryDelta or an empty HopHints list now fail outright for every gRPC client, including loopd's own autoloop path — which is not in the provided context and therefore unverified. That is the one new blocking concern, and it is also what keeps F5 open: the release note still describes this as a CLI-side bug fix.
The remaining new findings are smaller: the range guard sits in the caller rather than at the truncating uint16 conversion it protects, nothing bounds how many hints a request may carry, the helper forwards empty and untrimmed outpoint tokens into deposit selection, and --route_hints still ships with usage text that does not describe the input it now requires.
Status of prior findings
- F1 addressed: Fixed by narrowing the toggle —
DisableSliceFlagSeparator: trueis now set on the three command literals that carryrouteHintsFlag(cmd/loop/loopin.go:56,cmd/loop/quote.go:30,cmd/loop/staticaddr.go:422) rather than on the whole graph, and the two other slice flags on those commands keep comma parsing viacommaSeparatedStringSlice(cmd/loop/quote.go:90,cmd/loop/staticaddr.go:572).cmd/loop/main.gois untouched, soloop out,static withdrawand every other command are unaffected. - F2 addressed: Fixed in
cmd/loop/utils.go:53-56andloopd/swapclient_server.go:1249-1285—hints = make(...)no longer shadows,protojson.Unmarshalaccepts bothhop_hintsandhopHintsand errors on unknown fields (covered byTestRouteHintsRejectUnknownFields), and the server rejects nil, empty and zero-valued hints before invoice creation. See F6 for the compatibility cost of that strictness. - F3 addressed: Fixed — the
disableSliceFlagSeparatorwalk over the constructed root command is gone, and the property is declared in each command literal (cmd/loop/loopin.go:56,cmd/loop/quote.go:30,cmd/loop/staticaddr.go:422), which is where the original finding asked for it. - F4 addressed: Fixed in
cmd/loop/utils_test.go:49-52— the test walks the real graph withcommandAtPathover["in"],["quote","in"]and["static","in"]and exercises actual flag parsing instead of a synthetic command. F9 notes a side effect of how it does so.
Bot commands
/gateway re-review— re-run after pushing changes (maintainers)/gateway dismiss <id>— silence a finding (maintainers)/gateway explain <id>— elaborate on a finding (anyone)
| "%d is nil", hopIndex, routeIndex) | ||
| } | ||
|
|
||
| if rpcHint.ChanId == 0 { |
There was a problem hiding this comment.
🟠 F6 (Major) — Route hints that were accepted before now fail the whole request · loopd/swapclient_server.go:1268
Any gRPC caller that sends a route hint with a zero ChanId, a zero CltvExpiryDelta, or an empty HopHints list now has its entire LoopIn, Probe or GetLoopInQuote call rejected, where previously the useless hint was forwarded and ignored. That population includes third-party clients and loopd's own autoloop/liquidity manager.
Why this matters
Two of the new checks are unambiguously right: nil entries were a latent panic risk, and CltvExpiryDelta > math.MaxUint16 was being silently truncated to 0 by uint16(rpcHint.CltvExpiryDelta) in the unchanged unmarshallHopHint, which a caller had no way to detect. The zero-value rejections are the ones that narrow accept semantics without a matching safety gain — an advisory hint naming channel 0 cannot help routing, but it could not hurt it either. I cannot tell from the provided context whether any in-tree caller emits such a hint: the autoloop/liquidity RouteHint construction path is not in file_contents[], and loopd/swapclient_server.go is truncated. Confirm that before merging; a regression there breaks automated swaps with no user-visible cause. If tolerance matters more than strictness for the advisory fields, an alternative is to hard-error only on structurally invalid hints (nil entries, unparseable node id, out-of-range CLTV) and drop-and-log the merely-useless ones.
| Name: "in", | ||
| Usage: "perform an on-chain to off-chain swap (loop in)", | ||
| ArgsUsage: "amt", | ||
| DisableSliceFlagSeparator: true, |
There was a problem hiding this comment.
🟡 F7 (Minor) — --route_hints usage text does not describe the input it now requires · cmd/loop/loopin.go:56
This is the first release in which --route_hints does anything, and disabling the slice separator makes repeating the flag the only way to pass more than one hint, yet routeHintsFlag's Usage string is unchanged and documents neither that nor the accepted protojson object. Add a one-line example of the JSON shape and the repeat-the-flag requirement to the flag definition.
| values := cmd.StringSlice(name) | ||
| result := make([]string, 0, len(values)) | ||
| for _, value := range values { | ||
| result = append(result, strings.Split(value, ",")...) |
There was a problem hiding this comment.
🟡 F8 (Minor) — Empty and untrimmed tokens reach deposit-outpoint selection · cmd/loop/utils.go:19
strings.Split keeps empty fields and does not trim, so --utxo "a:0," yields an "" entry and --deposit_outpoint "a:0, b:1" yields " b:1"; both flow through containsDuplicates() into QuoteRequest.DepositOutpoints / StaticAddressLoopInRequest.Outpoints, where an outpoint the server cannot resolve quietly shrinks the quoted deposit total instead of erroring, because depositAmount only sums FilteredDeposits. I cannot confirm whether urfave's native splitting behaved identically, but this helper is now the single point every slice flag on these three commands must go through, so it is the right place to trim and drop empties.
| routeHints []*swapserverrpc.RouteHint | ||
| sliceValues []string | ||
| ) | ||
| routeHintCommand.Action = func(_ context.Context, |
There was a problem hiding this comment.
🟡 F9 (Minor) — Test leaves a stub Action on shared command singletons · cmd/loop/utils_test.go:58
loopInCommand, quoteInCommand and staticAddressLoopInCommand are package-level vars, so unless newRootCommandForReplay returns copies — its definition is not in the provided context — this assignment leaves the stub Action in package state for any later test in package main that runs those commands. Restoring the original with t.Cleanup removes the coupling.
| @@ -1244,14 +1245,49 @@ func unmarshallRouteHints(rpcRouteHints []*swapserverrpc.RouteHint) ( | |||
| [][]zpay32.HopHint, error) { | |||
|
|
|||
| routeHints := make([][]zpay32.HopHint, 0, len(rpcRouteHints)) | |||
| for _, rpcRouteHint := range rpcRouteHints { | |||
| for routeIndex, rpcRouteHint := range rpcRouteHints { | |||
There was a problem hiding this comment.
🟡 F10 (Minor) — Field values are bounded but hint counts are not · loopd/swapclient_server.go:1248
Nothing caps len(rpcRouteHints) or len(rpcRouteHint.HopHints), so an authenticated caller can fill a request up to the gRPC receive limit with minimally-valid hints and drive one secp256k1 decompression per hop in unmarshallHopHint. Bounding both in the loop this PR just added is cheap and also functionally correct, since an invoice can only carry so many hints.
| return nil, fmt.Errorf("route hint %d is nil", routeIndex) | ||
| } | ||
|
|
||
| if len(rpcRouteHint.HopHints) == 0 { |
There was a problem hiding this comment.
🟡 F11 (Minor) — Cannot confirm StaticAddressLoopIn validates the hints the CLI now sends · loopd/swapclient_server.go:1253
staticAddressLoopIn forwards the newly-populated hints to StaticAddressLoopInRequest.RouteHints, but that handler falls beyond the truncation boundary of file_contents[] for this file, so I cannot tell whether it routes through unmarshallRouteHints; if it does not, those hints skip every check added here and this is a major. Via the CLI the preceding GetLoopInQuote call happens to screen the same hints first, so only direct gRPC callers would be exposed.
| routeIndex) | ||
| } | ||
|
|
||
| if rpcHint.CltvExpiryDelta > math.MaxUint16 { |
There was a problem hiding this comment.
🟡 F12 (Minor) — Range guard is not at the truncating conversion · loopd/swapclient_server.go:1280
The > math.MaxUint16 check lives in unmarshallRouteHints while the narrowing it protects, CLTVExpiryDelta: uint16(rpcHint.CltvExpiryDelta), stays in the unchanged unmarshallHopHint, which still truncates 65536 to 0 for any caller that reaches it directly. Moving the bound — and the nil and zero checks — into unmarshallHopHint makes the invariant unbypassable instead of dependent on each caller pre-checking.
| @@ -16,6 +16,10 @@ | |||
|
|
|||
| #### Bug Fixes | |||
|
|
|||
| * Loop In commands and quotes now correctly parse, validate, and forward | |||
There was a problem hiding this comment.
🟡 F5 (Minor) · docs/release-notes/release-notes-next.md:19 · partially_addressed
The CLI half is resolved: with the toggle narrowed to three commands, no operator's existing comma-form invocation changes, so there is nothing to warn about there. What the note still omits is the server side — unmarshallRouteHints now rejects route hints that LoopIn, Probe and GetLoopInQuote previously accepted (F6), which affects every gRPC client and not just loop. "Malformed or empty hints are rejected before invoice creation" under Bug Fixes reads as client-side hygiene; the file already has a Breaking Changes section carrying exactly this kind of entry.
1 participant
Summary
Testing