Repository navigation
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 — 2 findings
🔴 0 Blocker · 🟠 2 Major · 🟡 0 Minor · 🔵 0 Nit
Summary
The PR lets UpdateChannelPolicy and lncli updatechanpolicy change one field at a time. It adds an opt-in partial_update flag plus *_specified booleans. Requests that don't set the flag behave exactly as before. The server-side conversion is tested, and so is how updateEdge handles the new Keep* flags. In a partial update, a non-zero value without its flag is rejected, which closes the most obvious ambiguity. The create_missing_edge path still works: a recreated edge keeps the default values for unset fields, which matches what the existing field documentation says.
Both concerns come from the opt-in design. In each case a client gets the old "unset means zero" behaviour while expecting partial semantics, and nothing tells it. One case is the new lncli talking to an older lnd. The other is a gRPC client that sets a *_specified flag but forgets partial_update. Either way, the fee fields on every channel in scope are reset to 0 and announced to the network.
Reviewed at medium effort.
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 (maintainers or PR author)
| MaxHtlcMsat: ctx.Uint64("max_htlc_msat"), | ||
| InboundFee: inboundFee, | ||
| CreateMissingEdge: createMissingEdge, | ||
| PartialUpdate: true, |
There was a problem hiding this comment.
🟠 F1 (Major) — New lncli silently zeroes fees on older lnd · cmd/commands/commands.go:2839
lncli updatechanpolicy --time_lock_delta=80 against an lnd older than this change resets base fee and fee rate to 0 on every channel (global scope by default), and the zero-fee policy is broadcast. The older lnd discards the unknown partial_update field and applies the omitted fees as 0.
Why this matters
The CLI now always sends PartialUpdate: true with omitted fields left at zero. Proto3 drops unknown fields on older servers, so the request is processed as a legacy one. Calls without a time lock delta are still rejected there, but any call that passes time_lock_delta with a fee field missing goes through. The only safeguard is a line of help text and a release note, which an operator running a newer lncli against an older node won't see at the moment it matters. The old CLI required all three values, so before this PR that call couldn't drop fees at all.
Fix: before sending, call GetInfo and refuse when the daemon version predates partial-update support. Alternatively, have the CLI still require base fee and fee rate whenever the server's support can't be confirmed.
There was a problem hiding this comment.
Fixed. When time_lock_delta is passed and a fee field is left out, lncli now checks GetInfo and refuses if lnd predates partial updates (< 0.21.99). That's the only case an older lnd accepts, since it rejects a missing time_lock_delta. Gate is supportsPartialPolicyUpdate, with a unit test. One gap: master builds from before this PR also report 0.21.99.
There was a problem hiding this comment.
If && becomes || in checkPartialPolicyUpdate, --time_lock_delta=80 --base_fee_msat=1000 sets the fee rate to 0 on an old lnd, and no test fails. Could you add that case?
There was a problem hiding this comment.
Added as base fee without fee rate in TestCheckPartialPolicyUpdate.
| // In a partial update, a field the caller didn't mark as specified is | ||
| // left unchanged. Reject non-zero values for such fields, as they | ||
| // would otherwise be silently ignored. | ||
| keepBaseFee := req.PartialUpdate && !req.BaseFeeMsatSpecified |
There was a problem hiding this comment.
🟠 F2 (Major) — Specified flags without partial_update silently ignored · rpcserver.go:7474
A request that sets base_fee_msat_specified or fee_rate_specified but not partial_update is processed as a legacy full update. Every fee field the caller left at zero, meaning to keep it, is applied as 0. This happens silently and can affect all channels.
Why this matters
keepBaseFee and keepFeeRate are only ever true when req.PartialUpdate is set, so without it the *_specified flags have no effect. For example, a client sends {BaseFeeMsatSpecified: true, BaseFeeMsat: 1000, TimeLockDelta: 80} and forgets partial_update: the fee rate drops to 0 on every targeted channel. The PR already rejects the mirror-image mistake (a non-zero value without its flag in a partial update), so this direction should be rejected too.
Fix: return codes.InvalidArgument when !req.PartialUpdate && (req.BaseFeeMsatSpecified || req.FeeRateSpecified).
There was a problem hiding this comment.
Fixed. base_fee_msat_specified or fee_rate_specified without partial_update now returns InvalidArgument. Covered in TestChannelPolicyFromRequest.
|
🤖 gateway audit metadata for this PR — auto-generated, please don't edit. |
2beda0e to
9e5a03a
Compare
|
/gateway re-review |
There was a problem hiding this comment.
Gateway review — 1 finding
🔴 0 Blocker · 🟠 0 Major · 🟡 1 Minor · 🔵 0 Nit
Summary
Both earlier findings are fixed. The server now rejects *_specified flags sent without partial_update. Before lncli sends a partial update that includes time_lock_delta but leaves out a fee field, it checks the node's version. That is the only partial update an older lnd would accept, so the check is in the right place.
The version check has one gap, which the author already pointed out: builds from master before this PR also report 0.21.99. Those are unreleased development builds, so I'm not raising it again as an open finding. The one new issue is that the check adds a GetInfo call. A macaroon that has only offchain:write can't make that call, so a partial update with such a macaroon now fails.
Lower-severity findings (1)
🟡 F3 (Minor) — Partial update fails for macaroons without info:read · cmd/commands/commands.go:2905
The version check calls GetInfo, which needs info:read, while UpdateChannelPolicy only needs offchain:write. So lncli --macaroonpath=fees.macaroon updatechanpolicy --time_lock_delta=80 with a fee-management macaroon now fails with "unable to check lnd version: permission denied". The same macaroon could have made the update itself. Either mention this requirement in the error message, or fall back to requiring both fee fields when the version can't be read.
Status of prior findings
- F1 addressed: Fixed in cmd/commands/commands.go:2902. When
TimeLockDelta != 0and either fee field is unspecified,updateChannelPolicycallsGetInfoand refuses ifsupportsPartialPolicyUpdate(info.Version)is false. Calls withouttime_lock_deltaare still rejected by older lnd, so this covers the only path where an older lnd would reset fees to 0. Remaining gap, already acknowledged in the thread: master builds from before this PR also report0.21.99. - F2 addressed: Fixed in rpcserver.go:7470.
channelPolicyFromRequestreturnscodes.InvalidArgumentwhen!req.PartialUpdate && (req.BaseFeeMsatSpecified || req.FeeRateSpecified). The "specified flag without partial update" case inTestChannelPolicyFromRequest(rpcserver_test.go:238) covers it.
Reviewed at medium effort.
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 (maintainers or PR author)
|
F3 fixed: if |
9e5a03a to
894f96a
Compare
|
/gateway re-review |
github-actions
Bot
added
the
severity-critical
🔴 PR Severity: CRITICAL
🔴 Critical (1 file)
🟠 High (4 files)
🟡 Medium (1 file)
🟢 Low (7 files)
AnalysisThe highest-severity file touched is To override, add a |
There was a problem hiding this comment.
Gateway review
No findings.
Status of prior findings
- F1 addressed: Fixed in
cmd/commands/commands.go:2680-2705.checkPartialPolicyUpdatecallsGetInfoand refuses below 0.21.99 whentime_lock_deltais set and a fee field is left out. That is the only case an older lnd accepts and misapplies. - F2 addressed: Fixed in
rpcserver.gochannelPolicyFromRequest.base_fee_msat_specifiedorfee_rate_specifiedwithoutpartial_updatenow returnsInvalidArgument("…require partial_update"). The "specified flag without partial update" case inTestChannelPolicyFromRequestcovers it. - F3 addressed: Fixed in
cmd/commands/commands.go:2690-2695. TheGetInfofailure now reads "unable to check that lnd supports partial policy updates (needs info:read permission), pass base_fee_msat and fee_rate or fee_rate_ppm as well". It names the macaroon requirement and the workaround. The "no info permission" case incmd/commands/commands_test.go:692-698covers it.
Reviewed at medium effort.
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 (maintainers or PR author)
|
/gateway approve |
There was a problem hiding this comment.
✅ Approved — all prior findings addressed.
Findings recap
Findings:
F1(major) — addressed:lncli updatechanpolicy --time_lock_delta=80against an lnd older than this change resets base fee and fee rate to 0 on every channel (glob…F2(major) — addressed: A request that setsbase_fee_msat_specifiedorfee_rate_specifiedbut notpartial_updateis processed as a legacy full update. Every f…F3(minor) — addressed: The version check callsGetInfo, which needsinfo:read, whileUpdateChannelPolicyonly needsoffchain:write. So `lncli --macaroonpat…
Approved by @saubyk via /gateway approve. Last reviewed at 894f96a. Skill v0.6.0, model claude-opus-5-5.
| // TestUpdatePolicyKeepFields checks that the Keep* flags of a policy update | ||
| // leave the matching fields of the existing edge unchanged, both on the edge | ||
| // sent to the gossiper and on the policy sent to the links. | ||
| func TestUpdatePolicyKeepFields(t *testing.T) { |
There was a problem hiding this comment.
Great coverage of the Keep* flags on the updateEdge path.
One gap: the createMissingEdge + Keep* combination is untested
this test passes createMissingEdge=false, the upstream missing-edge
case uses a legacy policy, and CreateMissingEdge appears in no other
test. Since createEdge seeds from DefaultRoutingPolicy, kept fields
fall back to defaults there. Worth one case asserting that?
There was a problem hiding this comment.
Added two cases to TestUpdatePolicyKeepFields with createMissingEdge=true: kept fields on a recreated edge fall back to the default policy.
| baseFeeSet = true | ||
|
|
||
| case args.Present(): | ||
| baseFee, err = strconv.ParseInt(args.First(), 10, 64) |
There was a problem hiding this comment.
lncli updatechanpolicy --fee_rate_ppm=100 <chan_point> fails here, because the chan point is read as the base fee. And --fee_rate_ppm=5 40 sets the base fee to 40 with no error.
There was a problem hiding this comment.
Reproduced both cases.
Case 1 looks like a real one: in --fee_rate_ppm=100 <chan_point> the chan point is the first bare arg, so it lands in the base-fee slot. A
base fee can never contain ':', so a bare arg that fails ParseInt but
parses as a chan point could be routed to the chan-point slot instead
of erroring. That shape ("only change the fee rate of this channel")
is exactly what partial updates invite.
Case 2 seems fine by design, though worth an explicit call-out: 40
is the first bare arg, so it fills the base-fee slot per the documented
order. Master would have failed later with "time_lock_delta argument
missing", which made the misalignment visible; now it succeeds
silently. Not sure there's a safe fix (intent is unknowable), but a
line in the help text on how bare args fill untagged fields would
help.
There was a problem hiding this comment.
Thanks, both fixed. A trailing argument containing : is now taken as the chan point first, so --fee_rate_ppm=100 <chan_point> works. Positional values are only accepted when base fee, fee rate and time lock delta are all set; --fee_rate_ppm=5 40 now errors and points to the flags. Help text and release note updated, tests added in TestParsePolicyUpdateRequest.
894f96a to
1e455cf
Compare
TechLateef
left a comment
TechLateef
left a comment
There was a problem hiding this comment.
LGTM
re-checked the follow-up push: the trailing chan-point
extraction and the positional gate cover both cases from
@Vandit1604's thread, and tests pass locally
(cmd/commands, routing/localchans).
bitromortac
left a comment
bitromortac
left a comment
There was a problem hiding this comment.
Thanks for taking this old issue on. My main request is to replace the three partial update fields with one message. The other comments are small.
| // If true, only the fields the caller marks as specified are updated and | ||
| // all others keep their current value: base_fee_msat is applied only if | ||
| // base_fee_msat_specified is set, fee_rate/fee_rate_ppm only if | ||
| // fee_rate_specified is set, and time_lock_delta only if non-zero. If | ||
| // false, base_fee_msat, fee_rate/fee_rate_ppm and time_lock_delta are | ||
| // always applied, with unset values treated as zero. | ||
| bool partial_update = 12; | ||
|
|
||
| // If true and partial_update is set, base_fee_msat is applied. | ||
| bool base_fee_msat_specified = 13; | ||
|
|
||
| // If true and partial_update is set, fee_rate or fee_rate_ppm is applied. | ||
| // If both are zero, the fee rate is set to zero. | ||
| bool fee_rate_specified = 14; |
There was a problem hiding this comment.
These three loose fields allow combinations that mean nothing, so the server needs the extra rule against a specified mark without partial_update. Could you use one partial_update field of a new message PartialPolicyUpdate with the bools base_fee_msat, fee_rate and time_lock_delta? A nil message is a legacy update, and an unmarked field is kept. Then the extra rule goes away, zero time_lock_delta no longer means "keep", and the server only rejects a non-zero value in an unmarked field.
There was a problem hiding this comment.
Done. partial_update is now a PartialPolicyUpdate message with the three bools. Nil is a legacy update, an unmarked field is kept, and the server only rejects a non-zero value in an unmarked field. A marked zero time_lock_delta fails the usual minimum check.
| // If true, only the fields the caller marks as specified are updated and | ||
| // all others keep their current value: base_fee_msat is applied only if |
There was a problem hiding this comment.
nit: "All others keep their current value" is true only for the base fee, the fee rate and the time lock delta. inbound_fee and the HTLC limits keep their own rules.
There was a problem hiding this comment.
Fixed. The comment now names the three fields and says inbound_fee and the HTLC limits aren't affected.
| // base_fee_msat_specified is set, fee_rate/fee_rate_ppm only if | ||
| // fee_rate_specified is set, and time_lock_delta only if non-zero. If | ||
| // false, base_fee_msat, fee_rate/fee_rate_ppm and time_lock_delta are | ||
| // always applied, with unset values treated as zero. |
There was a problem hiding this comment.
nit: With create_missing_edge, a recreated edge takes the default policy for each kept field, as the create_missing_edge comment says. This comment promises the current value. Could you add a sentence about the recreated edge here?
There was a problem hiding this comment.
Added a sentence: a recreated edge has no current value, so an unmarked field takes the default policy.
| // KeepBaseFee, KeepFeeRate and KeepTimeLockDelta leave the current | ||
| // value of the respective field unchanged instead of applying the one | ||
| // in this policy. | ||
| KeepBaseFee bool | ||
| KeepFeeRate bool | ||
| KeepTimeLockDelta bool |
There was a problem hiding this comment.
nit: The Keep* flags duplicate the fields, so KeepBaseFee: true with a non-zero BaseFee is valid but unclear. Could BaseFee, FeeRate and TimeLockDelta become fn.Option values, like FeeSchema.InboundFee, with None meaning "keep"?
There was a problem hiding this comment.
Done. BaseFee, FeeRate and TimeLockDelta are fn.Option now, None keeps the current value, and the Keep* flags are gone.
| var major, minor, patch uint32 | ||
| _, err := fmt.Sscanf(version, "%d.%d.%d", &major, &minor, &patch) | ||
| if err != nil { | ||
| return false |
There was a problem hiding this comment.
Sscanf reads 0.22.0garbage as 0.22.0, but this guard must fail closed. Could you use golang.org/x/mod/semver (already in go.sum) and compare with v0.21.99-0, so that 0.21.99-beta still passes?
There was a problem hiding this comment.
Done, using semver against v0.21.99-0. 0.22.0garbage now fails and 0.21.99-beta passes, both covered in the test. This makes golang.org/x/mod a direct dependency in go.mod.
| timeLockDeltaStr = args.First() | ||
| positional = true | ||
| args = args.Tail() | ||
| } |
There was a problem hiding this comment.
nit: An empty --time_lock_delta="" is silently ignored, but an empty --base_fee_msat="" fails. Could you parse the value whenever the flag is set, and add a test case?
There was a problem hiding this comment.
Fixed. The value is parsed whenever the flag is set, so --time_lock_delta="" fails. Test case added.
| if n := len(args); n > 0 && strings.Contains(args[n-1], ":") { | ||
| chanPointStr = args[n-1] | ||
| args = args[:n-1] |
There was a problem hiding this comment.
What about returning an error when positional arguments are left over? With the trailing chan point taken first, the remaining args are never checked, so a malformed call is now silently accepted:
lncli updatechanpolicy 1000 0.001 40 garbage <chan_point>succeeds and ignoresgarbage. Before this PR,garbagelanded in the chan point slot and failed to parse.lncli updatechanpolicy 1000 0.001 40 <chan_A> <chan_B>now updates B. Before this PR it updated A (and also ignored B).
Since this command changes live policy, a typo should fail instead of updating a different channel than the one the user may have meant. Checking that no args remain after the policy values and one chan point would cover both, plus a TestParsePolicyUpdateRequest case for each.
There was a problem hiding this comment.
Done. Any argument left after the policy values and one chan point is now an error, so both examples fail. --chan_point together with a positional chan point is rejected too. Cases added to TestParsePolicyUpdateRequest.
| // If true, only the fields the caller marks as specified are updated and | ||
| // all others keep their current value: base_fee_msat is applied only if | ||
| // base_fee_msat_specified is set, fee_rate/fee_rate_ppm only if | ||
| // fee_rate_specified is set, and time_lock_delta only if non-zero. If | ||
| // false, base_fee_msat, fee_rate/fee_rate_ppm and time_lock_delta are | ||
| // always applied, with unset values treated as zero. | ||
| bool partial_update = 12; | ||
|
|
||
| // If true and partial_update is set, base_fee_msat is applied. | ||
| bool base_fee_msat_specified = 13; | ||
|
|
||
| // If true and partial_update is set, fee_rate or fee_rate_ppm is applied. | ||
| // If both are zero, the fee rate is set to zero. | ||
| bool fee_rate_specified = 14; |
1e455cf to
642e6b6
Compare
bitromortac
left a comment
bitromortac
left a comment
There was a problem hiding this comment.
Thanks for the updates! All my earlier comments are addressed. I did some cli tests and only the help text needs one more change, otherwise LGTM 🎉
642e6b6 to
407eb16
Compare
GustavoStingelin
left a comment
GustavoStingelin
left a comment
There was a problem hiding this comment.
LGTM! pending rebase
PolicyUpdateRequest can't tell an unset base fee, fee rate or time lock delta from zero, so every update has to send all three. Add an opt-in partial_update message that marks which of the three fields to apply, so callers can update individual fields. Without partial_update the existing semantics are unchanged, as current clients rely on omitted (zero) values being applied. Fixes lightningnetwork#1523.
Turn BaseFee, FeeRate and TimeLockDelta of ChannelPolicy into options, so the local channel manager leaves the matching fields of the current edge untouched when they're empty. UpdateChannelPolicy keeps setting all three.
Move the request validation and conversion of UpdateChannelPolicy into channelPolicyFromRequest and honor partial_update there. Non-zero values for fields not marked in partial_update are rejected rather than silently ignored.
updatechanpolicy no longer requires base_fee_msat, fee_rate and time_lock_delta on every call; fields that aren't passed keep their current value. The flags are now string flags parsed explicitly, which removes the misleading "(default: 0)" from the help text and rejects invalid input instead of treating it as zero. Individual fields are set with the flags. Positional arguments are only accepted when all three fields are set, since it's otherwise ambiguous which field a value fills. A trailing channel point is recognised by its colon, so it's never read as a policy field, and any argument left over after the policy values and one channel point is rejected. An lnd that predates partial updates would reset an omitted fee to zero, so lncli checks the version first and refuses such an update. Fixes lightningnetwork#1523.
407eb16 to
acf4f3f
Compare
UpdateChannelPolicycan't tell an unsetbase_fee_msat,fee_rate/fee_rate_ppmortime_lock_deltafrom zero, solncli updatechanpolicyrequires all three on every call and its help shows a misleading(default: 0).Design
Making these fields "unset means keep" by default would break existing clients. Proto3 doesn't put zero values on the wire, so today's request for a zero base fee looks the same as an omitted one. The change is therefore opt-in:
partial_update(newPartialPolicyUpdatemessage) has one bool each forbase_fee_msat,fee_rateandtime_lock_delta. When it is set, only the marked fields are applied and an unmarked field keeps its current value.fee_ratecoversfee_rate/fee_rate_ppm(both zero sets the rate to 0).time_lock_deltagoes through the usual validation, so zero is rejected.partial_update, behaviour is unchanged.create_missing_edge, a recreated edge has no current value, so an unmarked field takes the default policy.A separate message is used instead of proto3
optional, as suggested in #10499 (comment), becauseoptionalwould turn the generated Go fields into pointers and break lnrpc consumers such as lndclient at compile time.lncli
updatechanpolicyalways sends a partial update, so each field can be set on its own. The policy flags are now parsed explicitly, which drops(default: 0)from the help and rejects invalid input instead of treating it as zero.Individual fields are set with the flags. Positional values are only accepted when all three fields are set, and an argument left over after the policy values and one channel point is an error.
An older lnd ignores
partial_update. It still rejects a call withouttime_lock_delta, but a call with it and a missing fee field would reset that fee to 0. For that case lncli checks the lnd version viaGetInfofirst and refuses below 0.21.99.Tests
update_channel_policy_partial, which also checks over gRPC that a legacy request still applies a zero base fee.Supersedes #10500, whose
*_specifieddefaults would have kept existing clients' fee values unchanged.