rpc: Optionally print feerates in sat/vb

polespinasa commented at 0:39 am on October 30, 2025: member

Part of #32093

Returning feerates in BTC/kvB it can be very burdensome and is not good practice, as the most widely adopted units are sat/vB. This PR aims to show a PoC of how we could migrate to sat/vb in a backwards compatible manner.

The RPC affectd by this PR are getmempoolinfo, getnetworkinfo, getwalletinfo, estimatesmartfee and estimaterawfee.

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.

For more context or discuss how to migrate also arguments please refer to the open issue #32093.

DrahtBot added the label RPC/REST/ZMQ on Oct 30, 2025

DrahtBot commented at 0:39 am on October 30, 2025: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/33741.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept ACK	w0xlt

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#32138 (wallet, rpc: remove settxfee and paytxfee by polespinasa)
#30157 (Fee Estimation via Fee rate Forecasters by ismaelsadeeq)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

polespinasa renamed this:
~~rpc: [PoC] Optionally print feerates in sat/vb~~
rpc: Optionally print feerates in sat/vb
on Oct 30, 2025

polespinasa force-pushed on Oct 30, 2025

w0xlt commented at 0:59 am on October 30, 2025: contributor

Concept ACK

in src/rpc/fees.cpp:88 in 1389b97904

84@@ -84,7 +85,11 @@ static RPCHelpMan estimatesmartfee()
85                 CFeeRate min_mempool_feerate{mempool.GetMinFee()};
86                 CFeeRate min_relay_feerate{mempool.m_opts.min_relay_feerate};
87                 feeRate = std::max({feeRate, min_mempool_feerate, min_relay_feerate});
88-                result.pushKV("feerate", ValueFromAmount(feeRate.GetFeePerK()));
89+                if (!request.params[2].isNull() && request.params[2].get_bool()) {

maflcko commented at 7:10 am on October 30, 2025:

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.

Also, could use self.(Maybe)Arg<bool> instead?

polespinasa commented at 8:21 pm on October 30, 2025:

I think with the rebase it’s easier to change defaults without touching code logic.

What would be the benefits of using self.MaybeArg<bool>? I’m not really familiar with it tbh.

maflcko commented at 7:46 am on October 31, 2025:

What would be the benefits of using self.MaybeArg<bool>? I’m not really familiar with it tbh.

The function is explained in the docstring above the function. It will read the default value by itself, so it doesn’t need to be hardcoded several times.

I think with the rebase it’s easier to change defaults without touching code logic.

I don’t think so. I can still see FeeEstimateMode feerate_units = (!request.params[2].isNull() && request.params[2].get_bool()) ? FeeEstimateMode::SAT_VB : FeeEstimateMode::BTC_KVB;

polespinasa commented at 6:51 pm on October 31, 2025:

Thanks for the suggestion, yep it is cleaner. Now should be as easy as changing the default in the arg definition.

btw: I used self.Arg<bool>, I think it makes more sense, self.MaybeArg<bool> is defined for optional params with no defaults.

in src/rpc/fees.cpp:89 in 1389b97904

84@@ -84,7 +85,11 @@ static RPCHelpMan estimatesmartfee()
85                 CFeeRate min_mempool_feerate{mempool.GetMinFee()};
86                 CFeeRate min_relay_feerate{mempool.m_opts.min_relay_feerate};
87                 feeRate = std::max({feeRate, min_mempool_feerate, min_relay_feerate});
88-                result.pushKV("feerate", ValueFromAmount(feeRate.GetFeePerK()));
89+                if (!request.params[2].isNull() && request.params[2].get_bool()) {
90+                    result.pushKV("feerate", static_cast<double>(feeRate.GetFeePerK()) / 1000.0);

maflcko commented at 7:29 am on October 30, 2025:

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.

For example, if GetFeePerK() returns CAmount{665714}, the Univalue will be 665.7140000000001, which is confusing at best, but also wrong.

polespinasa commented at 8:19 pm on October 30, 2025:

You are right, although I think it would not be a problem because we don’t print that many decimals.

Anyway, I think the approach in afeeac1596da47b0e431d468a665d344b39e1a87 is cleaner and follows what we already do for btc/kvB. What do you think?

maflcko commented at 7:47 am on October 31, 2025:

You are right, although I think it would not be a problem because we don’t print that many decimals.

They are printed. You can try it locally by printing the constructed univalue or by checking it in a test.

Anyway, I think the approach in afeeac1 is cleaner and follows what we already do for btc/kvB. What do you think?

I don’t think it handles negative feerates correctly. Generally, it is best to write unit or fuzz tests for newly written code. Also, I am not sure if CFeeRate should be bundled with univalue. The conversion could be a standalone helper, but this is just a nit.

polespinasa commented at 10:00 pm on October 31, 2025:

Generally, it is best to write unit or fuzz tests for newly written code

Done

Also, I am not sure if CFeeRate should be bundled with univalue. The conversion could be a standalone helper, but this is just a nit.

I decided to make it a function of CFeeRate because is the only class that should use it. But I’m ok moving it to core_writte.

maflcko commented at 7:32 am on October 30, 2025: member

I guess it is a bit verbose to have each RPC annotated with a type optional arg, but there probably isn’t an easier solution.

There could be a global option to toggle the default, to improve the UX.

Though, please don’t use floating point types. For monetary values, a fixed point type should be used.

polespinasa force-pushed on Oct 30, 2025

polespinasa commented at 8:29 pm on October 30, 2025: member

@maflcko thanks for reviewing :)

I guess it is a bit verbose to have each RPC annotated with a type optional arg, but there probably isn’t an easier solution.

I don’t think there is either, at least not in a backwards compatible way.

There could be a global option to toggle the default, to improve the UX.

That would be great for a bitcoin-cli user, but probably fatal for other programs that rely on the Bitcoin Core RPC. E.g. if a user with a Lightning Node enables sat/vB, thus making estimatesmartfee return sat/vb by default, probably the LN node will force close the channels due to lack of fees to pay.

polespinasa force-pushed on Oct 31, 2025

rpc: Add ToUniValue function to CFeeRate class 638826cd31

test: add unit tests for ToUniValue 7ac0d1615c

rpc: let MempoolInfoToJSON to return feerates in sat/vb

This commit also includes mempool.h, which was missing and was causing function definition issues.

00acb70c46

rpc: allow getmempoolinfo to return feerates in sat/vb 18c02cea13

rpc: allow getnetworkinfo to return feerates in sat/vb f25c24696f

rpc: allow getwalletinfo to return feerates in sat/vb 86bc69cb90

rpc: allow estimatesmartfee to return feerates in sat/vb 7168f5197c

rpc: allow estimaterawfee to return feerates in sat/vB c34f6a3514

rpc: Optionally print feerates in sat/vb #33741

Code Coverage & Benchmarks

Reviews

Conflicts