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)
#24539 (Add a “tx output spender” index by sstone)

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

in src/policy/feerate.h:11 in c34f6a3514

 7@@ -8,6 +8,7 @@
 8 
 9 #include <consensus/amount.h>
10 #include <serialize.h>
11+#include <univalue/include/univalue.h>

w0xlt commented at 1:34 am on November 6, 2025:

This drags an RPC‑only dependency (UniValue) into a widely included policy header. Instead, a helper in rpc/util.{h,cpp} (or rpc/fees.h) could be added such as:

0enum class FeeRateUnit { BTC_KVB, SAT_VB };
1UniValue ValueFromFeeRate(const CFeeRate& fr, FeeRateUnit unit);

polespinasa commented at 11:28 pm on November 10, 2025:

Yes maybe it’s a better approach. Moved the function into core_write.cpp and core_io.h similar to function ValueFromAmount.

in src/test/amount_tests.cpp:183 in c34f6a3514

178+    BOOST_CHECK_EQUAL(CFeeRate(1, 1).ToUniValue(FeeEstimateMode::SAT_VB).write(), "1.000");
179+    BOOST_CHECK_EQUAL(CFeeRate(10, 1).ToUniValue(FeeEstimateMode::SAT_VB).write(), "10.000");
180+    BOOST_CHECK_EQUAL(CFeeRate(1000, 1).ToUniValue(FeeEstimateMode::SAT_VB).write(), "1000.000");
181+    BOOST_CHECK_EQUAL(CFeeRate(1, 1).ToUniValue(FeeEstimateMode::SAT_VB).write(), "1.000");
182+    BOOST_CHECK_EQUAL(CFeeRate(10, 1).ToUniValue(FeeEstimateMode::SAT_VB).write(), "10.000");
183+    BOOST_CHECK_EQUAL(CFeeRate(1000, 1).ToUniValue(FeeEstimateMode::SAT_VB).write(), "1000.000");

w0xlt commented at 1:38 am on November 6, 2025:

Do the last six lines duplicate three assertions ? You probably intended to add negative sat/vB cases.

polespinasa commented at 11:27 pm on November 10, 2025:

yup, good catch tanks! :)

in src/rpc/net.cpp:718 in c34f6a3514

714@@ -712,9 +715,9 @@ static RPCHelpMan getnetworkinfo()
715     }
716     obj.pushKV("networks",      GetNetworksInfo());
717     if (node.mempool) {
718-        // Those fields can be deprecated, to be replaced by the getmempoolinfo fields
719-        obj.pushKV("relayfee", ValueFromAmount(node.mempool->m_opts.min_relay_feerate.GetFeePerK()));
720-        obj.pushKV("incrementalfee", ValueFromAmount(node.mempool->m_opts.incremental_relay_feerate.GetFeePerK()));
721+        UniValue mempool_info = MempoolInfoToJSON(*node.mempool, self.Arg<bool>("satvB"));

w0xlt commented at 1:43 am on November 6, 2025:

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. It may be better to compute these two fields directly.

polespinasa commented at 11:22 pm on November 10, 2025:

True! I thought that there was the intention to add all values and not only relayfee and incrementalfee here as the comment // Those fields can be deprecated, to be replaced by the getmempoolinfo fields seems suggest.

But seeing https://github.com/bitcoin/bitcoin/pull/25648/files#r935563077 I think I just misunderstood and the intention was to maybe remove them in the future and just use getmempool rpc to know that info.

rpc: Add ValueFromFeeRate function to core_write

Similar to ValueFromAmount this function returns a UniValue object with the feerate given a CFeeRate object.

dfc9e270a0

test: add unit tests for ValueFromFeeRate 1fc7a5c8cb

polespinasa force-pushed on Nov 11, 2025

polespinasa commented at 0:25 am on November 11, 2025: member

Rebased following @w0xlt suggestions :)

polespinasa force-pushed on Nov 11, 2025

rpc: let MempoolInfoToJSON to return feerates in sat/vb

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

7cbf96a974

rpc: allow getmempoolinfo to return feerates in sat/vb 7ee792d957

rpc: allow getnetworkinfo to return feerates in sat/vb 335a1e84e3

rpc: allow getwalletinfo to return feerates in sat/vb 0eb291ad47

rpc: allow estimatesmartfee to return feerates in sat/vb dd245f24d1

rpc: allow estimaterawfee to return feerates in sat/vB 004d22b9a3

glozow commented at 2:27 pm on November 11, 2025: member

Returning feerates in BTC/kvB it can be very burdensome and is not good practice, as the most widely adopted units are sat/vB.

Can you explain why it’s not good practice? I agree it’s not as human-readable but I don’t software minds either way. Not to be snarky, but I think the most widely adopted usage of this RPC interface must be to handle what it returns, including converting it before printing to users.

This PR aims to show a PoC of how we could migrate to sat/vb in a backwards compatible manner.

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.

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 so that RPC results aren’t bloated. I’m not familiar with how we might pass a global config option to the RPCs though.

polespinasa commented at 6:46 pm on November 11, 2025: member

@glozow thanks for your comments :)

Can you explain why it’s not good practice?

We open the door to other software/users to make easy mistakes by forcing them to do a unit conversion that Core shouldn’t have done in the first place. I understand that this has been like this since ¿always?, but giving users the option to have sat/vB (or even more units thanks to FeeFrac) can be useful and reduce the burden on other software.

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.

I agree, this PR doesn’t change any default behavior. It MUST be done in a backwards compatible way.

A different approach could be to add another field to each result, like “feerate_satvb”.

Yes! This is another approach already taken by fundrawtransaction for example (context).

A global config option like `-feeraterepresentation" could determine which results are returned so that RPC results aren’t bloated. I’m not familiar with how we might pass a global config option to the RPCs though.

I don’t dislike at all this approach and I would probably be open to switching to it if there’s consensus that it is better. But I am afraid that with that approach users can easily make huge mistakes. They could start their node with -feeraterepresentation=satvB and then use some software on top that expects BTC/kvB. That could be fatal. Because of that, I decided to follow the bool option in each RPC call. See #33741 (comment)

glozow commented at 7:14 pm on November 11, 2025: member

forcing them to do a unit conversion that Core shouldn’t have done in the first place.

I still don’t see the argument for why a unit per kvB is so problematic. BTC and satoshis are easily convertible.

I do think it’s worthwhile to unify the format for inputting and and outputting feerates. And I agree it should be configurable, given there are multiple preferred formats.

My concern is the idea that the default format might be changed in the future, causing somebody’s reading of a feerate to be off by a factor of 100,000. I think the acceptable way to do this is to create new fields and maybe phase out the old ones.

They could start their node with -feeraterepresentation=satvB and then use some software on top that expects BTC/kvB. That could be fatal.

That’s why the results must have different names. If they forget the config option, they should get a key error.

w0xlt commented at 10:09 pm on November 12, 2025: contributor

Adding a new field in the result called feerate_unit (string) to specify the unit used (sat/vB or BTC/kvB) may address this issue. The current approach looks good to me, though.

rpc: Optionally print feerates in sat/vb #33741

Code Coverage & Benchmarks

Reviews

Conflicts