Remove deprecated fee rate option --sat_per_byte#10889
Conversation
github-actions
Bot
added
the
severity-critical
🔴 PR Severity: CRITICAL
🔴 Critical (1 file)
🟠 High (2 files)
🟡 Medium (3 files)
🟢 Low / Excluded (4 files)
AnalysisThis PR touches Severity bump rules were checked:
The CRITICAL classification stands based on the presence of To override, add a |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request finalizes the removal of the deprecated sat_per_byte field, which has been superseded by sat_per_vbyte. By enforcing this change at the RPC and CLI layers, the codebase is simplified and technical debt is reduced. The implementation ensures that any legacy usage is now explicitly rejected, maintaining consistency across the API. Highlights
New Features🧠 You can now enable Memory (public preview) to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request removes the deprecated --sat_per_byte fee rate option across multiple CLI commands and RPCs (including CloseChannel, OpenChannel, SendCoins, SendMany, and BumpFee) in favor of sat_per_vbyte. It also updates the corresponding tests and release notes. The review feedback identifies a broken markdown link in the release notes that needs to be corrected.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
MPins
changed the title
MPins
changed the title
ff14956 to
0207a05
Compare
|
/gateway review |
|
✅ Review posted: #10889 (review) 1 finding(s); 1 inline, 0 in body. 🔁 Need a re-review after pushing changes? Reply with |
![lightninglabs-gateway[bot]](https://avatars.githubusercontent.com/in/3635431?s=60&v=4){kind=small}
There was a problem hiding this comment.
This PR completes the removal of the deprecated sat_per_byte fee-rate field: the CLI flags are dropped, CalculateFeeRate/maybeUseDefaultConf are simplified to sat_per_vbyte only, and every RPC handler that previously honored the field (SendCoins, SendMany, parseOpenChannelReq, CloseChannel, validateBumpFeeRequest) now rejects a non-zero SatPerByte with ErrSatPerByteRemoved. The intent and behavior change are clearly documented in the release notes.
The change is correct and consistent. Each rejection is placed before any server state is dereferenced, which the new rpcserver_test.go and walletkit_server_test.go tests exercise with zero-value/nil fixtures. The removal of SatPerByte from the internal guards in checkPsbtFlags and newPsbtAssembler is safe because those paths sit behind the early rejection in parseOpenChannelReq. Test and itest updates correctly switch to SatPerVbyte. I could not independently confirm from the diff that OpenChannelSync and BatchOpenChannel route through the guarded handlers, but nothing in the visible context contradicts the release-notes enumeration.
Findings: 🔴 0 Blocker · 🟠 0 Major · 🟡 1 Minor · 🔵 0 Nit
|
🤖 gateway audit metadata for this PR — auto-generated, please don't edit. |
The --sat_per_byte flag was hidden and deprecated in v0.13.0 in favor of --sat_per_vbyte. Remove it from the sendcoins, sendmany, openchannel, closechannel, closeallchannels, bumpfee, bumpclosefee and bumpforceclosefee commands, and simplify the fee-rate flag validation accordingly.
0207a05 to
395372b
Compare
|
/gateway re-review |
There was a problem hiding this comment.
Gateway review — 1 finding
🔴 0 Blocker · 🟠 1 Major · 🟡 0 Minor · 🔵 0 Nit
Summary
This is a clean, well-tested completion of the long-planned sat_per_byte removal: the deprecated field is rejected at the RPC boundary, the fee helpers (CalculateFeeRate, maybeUseDefaultConf) are correctly narrowed, stale //nolint:staticcheck markers are dropped, and new unit tests cover the rejection on every affected handler. The fee-path semantics were verified to have full parity with the prior three-argument behavior, and no fund-loss, panic, or DoS path was found.
The one prior finding is partially resolved. The rejection was upgraded to codes.InvalidArgument in the four rpcserver.go handlers exactly as suggested, but the walletrpc.BumpFee path was missed and still surfaces the removal as codes.Unknown — now inconsistent with its five sibling RPCs in this very PR. No new defects were introduced.
Bot commands
/gateway re-review— re-run after pushing changes/gateway dismiss <id>— silence a finding (maintainers)/gateway explain <id>— elaborate on a finding (anyone)
The sat_per_byte field was deprecated in v0.13.0 in favor of sat_per_vbyte. Until now the server silently treated sat_per_byte as sat/vbyte, which was misleading. Rather than honoring it, the affected RPCs (SendCoins, SendMany, OpenChannel, OpenChannelSync, CloseChannel and walletrpc.BumpFee) now return an explicit error when the field is set, so that an old client talking to a new daemon fails loudly instead of having its requested fee rate silently dropped. CalculateFeeRate and maybeUseDefaultConf are simplified to only take the sat_per_vbyte value now that the deprecated field is no longer honored.
The wallet import test set the deprecated SatPerByte field on SendCoinsRequest, which the server now rejects. Switch it to SatPerVbyte, which carries the same sat/vbyte value.
395372b to
1009fa8
Compare
|
/gateway re-review |
|
Sorry, |
|
/gateway re-review |
There was a problem hiding this comment.
Gateway review — 1 finding
🔴 0 Blocker · 🟠 1 Major · 🟡 0 Minor · 🔵 0 Nit
Summary
The prior concern (F1) is resolved: every client-facing rejection of the removed sat_per_byte field now returns status.Error(codes.InvalidArgument, …) — the four rpcserver.go handlers wrap it directly, and walletkit_server.go's BumpFee catches ErrSatPerByteRemoved via errors.Is and re-wraps it, so gRPC clients get a stable InvalidArgument code instead of codes.Unknown.
One genuinely-new issue surfaced in the newly-added rejection checks. The diff systematically removes // nolint:staticcheck from every place rpcserver.go (package lnd) previously read the deprecated lnrpc.*Request.SatPerByte field, but the four new if in.SatPerByte != 0 guards it adds omit that same directive — while the same-package walletrpc code, which never needed the directive, correctly goes without. This is a cross-package deprecated-field read that the repo's own convention (visible throughout this very diff) requires be annotated, so it will fail make lint.
Otherwise the mechanical removal is clean: CalculateFeeRate/maybeUseDefaultConf signatures are simplified with all call sites updated, the CLI flag/checkNotBothSet plumbing is consistent, and test coverage exercises all four rejecting RPCs plus validateBumpFeeRequest.
Status of prior findings
- F1 addressed: Resolved.
rpcserver.gonow returnsstatus.Error(codes.InvalidArgument, lnrpc.ErrSatPerByteRemoved.Error())inSendCoins(line ~1361),SendMany(~1609),parseOpenChannelReq(~2118), andCloseChannel(~2759);lnrpc/walletrpc/walletkit_server.goBumpFeemaps the sentinel tocodes.InvalidArgumentviaerrors.Is. Clients can now detect the removed-field rejection by gRPC status code rather than by string-matching, which was the substance of the prior concern.
Bot commands
/gateway re-review— re-run after pushing changes/gateway dismiss <id>— silence a finding (maintainers)/gateway explain <id>— elaborate on a finding (anyone)
| func (r *rpcServer) SendCoins(ctx context.Context, | ||
| in *lnrpc.SendCoinsRequest) (*lnrpc.SendCoinsResponse, error) { | ||
|
|
||
| // The deprecated sat_per_byte field is no longer honored. |
There was a problem hiding this comment.
The CI reports no lint errors.
There was a problem hiding this comment.
/gateway dismiss per #10889 (comment)
Reads of the deprecated SatPerByte field can't produce a lint error, with or without nolint annotations — which is why the lint CI job passes on this PR. The // nolint:staticcheck comments this diff removes are vestigial from before the check was disabled (and the space-separated // nolint: form isn't a machine-readable directive to golangci-lint anyway). Adding the suggested annotations would just introduce dead comments.
| func (r *rpcServer) SendCoins(ctx context.Context, | ||
| in *lnrpc.SendCoinsRequest) (*lnrpc.SendCoinsResponse, error) { | ||
|
|
||
| // The deprecated sat_per_byte field is no longer honored. |
There was a problem hiding this comment.
The CI reports no lint errors.
|
/gateway dismiss F2 per #10889 (comment) |
|
🚫 Dismissed F2 (major) by @saubyk — per #10889 (comment) All findings on this PR are addressed or dismissed. |
|
/gateway re-review |
|
No new commits since the last gateway review (HEAD is still |
| params, existing, err := w.prepareSweepParams(in, *op, currentHeight) | ||
| if err != nil { | ||
| // Surface the removal of the deprecated sat_per_byte field as | ||
| // InvalidArgument. |
There was a problem hiding this comment.
nit: since this is the only path where a user-supplied request can produce ErrSatPerByteRemoved (BumpForceCloseFee builds its BumpFeeRequest internally), validateBumpFeeRequest could return the status.Error(codes.InvalidArgument, ...) directly, as rpcserver.go does, avoiding the errors.Is translation here.
| } | ||
|
|
||
| // Only fee rate flag or conf_target should be set, not both. | ||
| // Check that only one is set sat_per_vbyte or conf_target, not both. |
There was a problem hiding this comment.
nit: grammar — "Check that only one of sat_per_vbyte or conf_target is set, not both." (same comment at L852)
| | [`lnrpc.OpenChannel`](https://lightning.engineering/api-docs/api/lnd/lightning/open-channel/) | [`lnrpc.OpenChannelRequest`](https://lightning.engineering/api-docs/api/lnd/lightning/open-channel/#lnrpcopenchannelrequest) | sat_per_byte | ||
| | [`lnrpc.SendCoins`](https://lightning.engineering/api-docs/api/lnd/lightning/send-coins/) | [`lnrpc.SendCoinsRequest`](https://lightning.engineering/api-docs/api/lnd/lightning/send-coins/#lnrpcsendcoinsrequest) | sat_per_byte | ||
| | [`lnrpc.SendMany`](https://lightning.engineering/api-docs/api/lnd/lightning/send-many/) | [`lnrpc.SendManyRequest`](https://lightning.engineering/api-docs/api/lnd/lightning/send-many/#lnrpcsendmanyrequest) | sat_per_byte | ||
| | [`walletrpc.BumpFee`](https://lightning.engineering/api-docs/api/lnd/wallet-kit/bump-fee/) | [`walletrpc.BumpFeeRequest`](https://lightning.engineering/api-docs/api/lnd/wallet-kit/bump-fee/#walletrpcbumpfeerequest) | sat_per_byte |
There was a problem hiding this comment.
Could you also mention the removed lncli flags here (openchannel, sendcoins, sendmany, closechannel, closeallchannels, bumpfee, bumpclosefee, bumpforceclosefee)?
nit: the table rows are missing their trailing |.
| } | ||
|
|
||
| // `sat_per_byte` was deprecated we only use sats/vbyte now. | ||
| if ctx.IsSet("sat_per_byte") { |
There was a problem hiding this comment.
With the hidden flags fully removed, lncli ... --sat_per_byte=N now fails with urfave/cli's generic "flag provided but not defined" instead of a targeted message like the one removed here. Probably fine for a removal PR, but an alternative is keeping the hidden flags for one more release with a handler error pointing at sat_per_vbyte, mirroring the RPC's InvalidArgument message. Judgment call — flagging for a conscious decision.
|
One item not visible in this diff: the proto field comments for |
3 participants
As defined at release doc v0.21.0 we now remove the option --sat_per_byte.
Change Description
The sat_per_byte field was deprecated back in v0.13.0 in favor of sat_per_vbyte. This PR completes its removal: the deprecated field is no longer honored by the RPC server, the CLI, or the wallet RPC, and any request that still sets it is now explicitly rejected.
Behavior change
Previously, sat_per_byte was still accepted and silently converted, with a guard preventing both fields from being set at once.
Now, if a request sets sat_per_byte the
ErrSatPerByteRemovedis returned.