#33741 rpc: Optionally print feerates in sat/vb
| Reviewability | Reviews | Agreement | Size | ||||
|---|---|---|---|---|---|---|---|
| #33741 rpc: Optionally print feerates in sat/vb | CI failing | 0 (+1) | Strong | 88 + 80 tests | |||
|
|
|
|
| |||
Reviewability: ReadyStalePaused Agreement: StrongPositivePositive w/ caveatsNeutralMildDisputedBlockedCrickets Reviews: current code-review ACKs, then (+stale ACKs) and -NACKs; greener = more ACKs. Size: lines added or modified outside tests, then in tests; greener = smaller. | |||||||
Goal
Adds an optional boolean argument `sat_vb` to `getmempoolinfo`, `getnetworkinfo`, `estimatesmartfee`, and `estimaterawfee` RPCs. When enabled, returned fee rates are represented in sat/vB instead of the default BTC/kvB. It implements a `ValueFromFeeRate` helper in `core_io` to format fee rates with 3 decimal digits of precision without relying on integer rounding.
Problem: Bitcoin Core RPCs return fee rates denominated in BTC/kvB, while users and downstream applications standardly work with sat/vB. Callers must convert units manually, which introduces friction and can cause rounding errors, particularly in sub-1 sat/vB conditions where naive integer conversions round to zero or one.
Evidence of importance:
- PR description references issue #32093, stating: 'Returning feerates in BTC/kvB it can be very burdensome and is not good practice, as the most widely adopted units are sat/vB.'
- PR description highlights sub-1 sat/vB rounding problems: 'Because of sub 1sat/vB environment we cannot rely on GetFee() because it internally uses FeeFrac EvaluateFee() function which rounds the values to always return an int. If the feerate < 1 then it will be rounded to 1 or 0.'
Scope: Although the PR description mentions modifying `getwalletinfo`, that RPC is not touched in this diff. Reviewers should note that adding `sat_vb` as a positional parameter to `estimatesmartfee` shifts the `options` argument from index 2 to index 3.
Risk of a mistake: medium (rpc_api). Modifying positional RPC parameter layouts or fee rate formatting outputs could break existing client applications relying on exact argument indexes or values.
Changed since the previous assessment: The underlying approach and affected RPCs remain unchanged from the previous description.
Reviewability
Ready · CI failing
CI is red; check whether the failure is the PR's before a deep review.
Waiting on (from the thread read):
- author: author to see if argument can be set in options object and address remaining suggestions 2026-10-26
- reviewer: reviewers to inspect latest force push 2026-10-26
Agreement
Strong
substantive support, no open objection (musaHaruna)
Thread notes: Author replied to maflcko's suggestion regarding OBJ_NAMED_PARAMS in c:6422612931, agreeing to fix it and checking if using an options object works, leaving the suggestion open as a deferral.
Objections, suggestions, and questions:
| Reviewer | Type | Harm, request, or question | Status | Must be | Author replied | Quote |
|---|---|---|---|---|---|---|
| maflcko (member) | suggestion, interface | add a global configuration option to toggle the default feerate representation | answered · author explained it could be dangerous for external programs like Lightning nodes | answered | yes | 2025-10-30: 'There could be a global option to toggle the default, to improve the UX.' Status from: 2025-10-30: polespinasa: 'That would be great for a `bitcoin-cli` user, but probably fatal for other programs that rely on the Bitcoin Core RPC.' |
| maflcko (member) | objection, maintenance | encoding default values into the code logic prevents altering defaults without rewriting logic | fixed · fix pushed · author changed parameter handling to use Arg<bool> | changed | yes | 2025-10-30: 'Please don't encode default values in how the code is written. This makes it impossible to change the default values without rewriting the code logic.' Status from: 2025-10-31: polespinasa: 'Thanks for the suggestion, yep it is cleaner. Now should be as easy as changing the default in the arg definition.' |
| maflcko (member) | suggestion, style | use self.(Maybe)Arg<bool> instead of manual argument parsing | fixed · fix pushed · author implemented self.Arg<bool> | answered | yes | 2025-10-30: 'Also, could use `self.(Maybe)Arg<bool>` instead?' Status from: 2025-10-31: polespinasa: 'btw: I used `self.Arg<bool>`, I think it makes more sense, `self.MaybeArg<bool>` is defined for optional params with no ' |
| maflcko (member) | objection, correctness | floating-point types cannot represent decimal monetary amounts accurately, causing imprecise or wrong values in RPC output | fixed · fix pushed · author switched to fixed-point integer arithmetic and string formatting | changed | yes | 2025-10-30: 'this is wrong, you can't use `double` to denote monetary amounts. `double` is a floating point type, which can not represent decimal values accurately.' Status from: 2025-10-31: polespinasa: 'Done' |
| maflcko (member) | objection, correctness | the new feerate formatting logic does not handle negative feerates correctly | fixed · fix pushed · author fixed and added tests | changed | yes | 2025-10-31: 'I don't think it handles negative feerates correctly. Generally, it is best to write unit or fuzz tests for newly written code.' Status from: 2025-10-31: polespinasa: 'Done' |
| w0xlt (contributor) | objection, maintenance | drags an RPC-only dependency (UniValue) into a widely included policy header | fixed · fix pushed · author moved the helper to core_write.cpp and core_io.h | changed | yes | 2025-11-06: 'This drags an RPC‑only dependency (UniValue) into a widely included policy header.' Status from: 2025-11-10: polespinasa: 'Moved the function into `core_write.cpp` and `core_io.h` similar to function `ValueFromAmount`.' |
| w0xlt (contributor) | question, correctness | do the last six lines of the test duplicate assertions instead of testing negative sat/vB cases? | answered · fix pushed · author confirmed and fixed in subsequent commit | answered | yes | 2025-11-06: 'Do the last six lines duplicate three assertions ? You probably intended to add negative sat/vB cases.' Status from: 2025-11-10: polespinasa: 'yup, good catch tanks! :)' |
| w0xlt (contributor) | objection, maintenance | introduces an RPC-to-RPC include and constructs an entire object with locking just to read two values | fixed · fix pushed · author removed the dependency and locking in a rebase | changed | yes | 2025-11-06: 'This line constructs a full object (with locking) only to reuse two values. It also adds a new include `<rpc/mempool.h>` into `rpc/net.cpp`, creating an RPC‑RPC dependency.' Status from: 2025-11-10: polespinasa: 'True! ... I think I just misunderstood and the intention was to maybe remove them in the future' |
| glozow (contributor) | objection, interface | changing the default unit format in the future would be a massive API break leaving downstream software readings orders of magnitude off | answered · author confirmed the PR is backwards compatible and that they would not push to change the default | answered | yes | 2025-11-11: 'It seems extremely dangerous to ever change the default... no matter how much notice is given, it's a massive api break that would cause software to suddenly be several orders of magnitude off.' Status from: 2025-11-13: polespinasa: 'I don't think the default will ever be changed. I left the door open to it in some comments, but I would not push for it' |
| glozow (contributor) | objection, interface | changing the default unit format in the future would be a massive API break leaving downstream software readings orders of magnitude off | answered · author replied agreeing not to change the default | answered | yes | 2025-11-11: 'It seems extremely dangerous to ever change the default... no matter how much notice is given, it's a massive api break that would cause software to suddenly be several orders of magnitude off.' Status from: 2025-11-13: polespinasa: 'I don't think the default will ever be changed. I left the door open to it in some comments, but I would not push for it' |
| glozow (contributor) | suggestion, approach | add a separate field to each result like feerate_satvb controlled by a global config option rather than changing existing fields | answered · discussed between author and glozow | answered | yes | 2025-11-11: 'A different approach could be to add another field to each result, like "feerate_satvb". A global config option like `-feeraterepresentation" could determine which results are returned' Status from: 2025-11-13: polespinasa: 'how would you do it for rpc arguments though? Unless you force to use named arguments I think it can still lead to error' |
| w0xlt (contributor) | objection, correctness | estimaterawfee produces mixed-unit responses by converting the top-level feerate but leaving pass/fail bucket ranges in sat/kvB | fixed · fix pushed · author converted bucket ranges in estimaterawfee and w0xlt verified | changed | yes | 2026-05-26: '`estimaterawfee` is converting only the top-level feerate when `satvB=true`, while `pass/fail.startrange` and `endrange` remained in the `sat/kvB` units.' Status from: 2026-06-03: w0xlt: 'ACK 26316c53ef65c1289227b1c2b9dcc2d7ad802aec' |
| musaHaruna (contributor) | suggestion, approach | simplify negative feerate handling by using unsigned cast before division | answered · author declined, preferring current conditional structure for readability | answered | yes | 2026-07-07: 'Would it make sense, or is there a reason not to simplify this by stripping the negative sign right at the start using static_cast<uint64_t>?' Status from: 2026-07-28: polespinasa: 'I rather keep as is for now, unless someone finds a better reason for it. I think is more readable' |
| jonatack (member) | suggestion, interface | use snake_case sat_vb instead of satvb per developer naming guidelines | fixed · fix pushed · author changed parameter naming to sat_vb | answered | yes | 2026-08-21: 'Would it be better to use `sat_vb` or `satvb` for these? (Sorry if this has already been discussed above.) Per the developer notes' Status from: 2026-08-22: polespinasa: 'Fair enough, changed to `sat_vb`' |
| willcl-ark (member) | suggestion, interface | provide a release note if changing positional RPC argument order | open | answered | no | 2026-10-07: 'In particular changing positional RPC arguments porbably warrants a release note (if we do have to break that ordering).' |
| willcl-ark (member) | suggestion, maintenance | update PR description if it is no longer a PoC | open | answered | no | 2026-10-07: 'I think the description needs updating too, if this is no longer a PoC.' |
| maflcko (member) | suggestion, interface | use RPCArg::Type::OBJ_NAMED_PARAMS for arguments | open · author agreed to fix and deferred to investigate options object in c:6422612931 | answered | yes | 2026-10-23: 'Should use `RPCArg::Type::OBJ_NAMED_PARAMS`?' |
Support:
- w0xlt (contributor) (Concept ACK, 2025-10-30): no reason given [not substantive]
- w0xlt (contributor) (ACK, 2026-06-03): no reason given [not substantive]
- musaHaruna (contributor) (Concept ACK, 2026-07-07) [read]: migrating to sat/vb in a backwards compatible manner
- jonatack (member) (Concept ACK, 2026-08-21): no reason given [not substantive]
- w0xlt (contributor) (ACK, 2026-10-07): no reason given [not substantive]
Participants:
- maflcko (member, objection, 7 statements 2025-10-30 to 2026-10-23): objected to floating-point representation for monetary amounts and hardcoded default logic; suggested OBJ_NAMED_PARAMS
- w0xlt (contributor, objection, 8 statements 2025-10-30 to 2026-10-07): Concept ACK, full ACK, and reACK after raising concerns about header dependencies and estimaterawfee bucket ranges
- glozow (contributor, objection, 2 statements 2025-11-11 to 2025-11-11): objected to ever changing defaults due to API break risk and suggested separate output fields
- sedited (member, neutral, 1 statements 2026-05-14 to 2026-05-14): reminded author to rebase to resolve merge conflicts
- musaHaruna (contributor, support, 3 statements 2026-07-07 to 2026-07-07): Concept ACK for backwards-compatible sat/vB migration and suggested simplification for negative feerate logic
- jonatack (member, support, 2 statements 2026-08-21 to 2026-08-21): Concept ACK and advised using snake_case sat_vb naming
- willcl-ark (member, neutral, 1 statements 2026-10-07 to 2026-10-07): suggested adding a release note for positional RPC argument changes and updating PR description
Files
122 lines under test/bench/ci.
- test/functional/feature_fee_estimation.py +34/-32
- src/test/rpc_tests.cpp +36/-0
- src/rpc/fees.cpp +24/-9
- src/core_io.cpp +26/-0
- src/rpc/mempool.cpp +12/-10
- test/functional/rpc_estimatefee.py +10/-10
- src/rpc/net.cpp +8/-5
- src/core_io.h +10/-0
- src/rpc/client.cpp +5/-1
- src/rpc/mempool.h +3/-1
Card
The short text the ranking pass compares across PRs.
This pull request adds an optional sat_vb boolean parameter to getmempoolinfo, getnetworkinfo, estimatesmartfee, and estimaterawfee, returning fee rates in sat/vB rather than BTC/kvB. It addresses user friction and downstream calculation bugs stemming from client-side conversion of legacy BTC/kvB units, especially in sub-1 sat/vB scenarios. Fee rate serialization is handled by a new ValueFromFeeRate helper providing 3 decimal places of precision. The work implements part of issue #32093 and has no prerequisites.
Data
dossier JSON · extract JSON · model ledger, generated 2026-10-28T01:21, confidence high, input hash 80638f0c01e5bc32
thread read through 2026-10-26 (44 statements), head 77aa2f4e69
labels: RPC/REST/ZMQ, CI failed
thread read through 2026-10-26 (44 statements), head 77aa2f4e69
labels: RPC/REST/ZMQ, CI failed
Categories
RPC / REST / ZMQ
P3 · user request (ranking pass: Moved down 10: clear ergonomic win, but an optional convenience field ranks below the interface correctness fixes above.)
Addresses long-standing downstream caller friction and precision loss from manual BTC/kvB conversions (#32093). The enhancement offers clear utility for client applications but is reasonably deferrable interface ergonomics rather than a breaking bug fix.
Membership: Carries RPC/REST/ZMQ label and adds `sat_vb` parameters to multiple node RPCs across src/rpc/.
Factors: security/stability 0, bug 0, performance 0, user value 2, leverage 0