rpc: Optionally print feerates in sat/vb #33741

polespinasa · 2025-10-30T00:39:41Z

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 · 2025-10-30T00:39:46Z

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:

#34113 (refactor: [rpc] Remove confusing and brittle integral casts by maflcko)
#34075 (fees: Introduce Mempool Based Fee Estimation to reduce overestimation by ismaelsadeeq)
#32138 (wallet, rpc: remove settxfee and paytxfee by polespinasa)
#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.

w0xlt

Concept ACK

maflcko

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.

maflcko · 2025-10-30T07:10:57Z

src/rpc/fees.cpp

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


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?

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.

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;

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.

maflcko · 2025-10-30T07:29:19Z

src/rpc/fees.cpp

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


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.

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?

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.

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.

polespinasa · 2025-10-30T20:29:21Z

@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.

w0xlt · 2025-11-06T01:34:36Z

src/policy/feerate.h

@@ -8,6 +8,7 @@

 #include <consensus/amount.h>
 #include <serialize.h>
+#include <univalue/include/univalue.h>


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:

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

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

w0xlt · 2025-11-06T01:38:00Z

src/test/amount_tests.cpp

+    BOOST_CHECK_EQUAL(CFeeRate(1, 1).ToUniValue(FeeEstimateMode::SAT_VB).write(), "1.000");
+    BOOST_CHECK_EQUAL(CFeeRate(10, 1).ToUniValue(FeeEstimateMode::SAT_VB).write(), "10.000");
+    BOOST_CHECK_EQUAL(CFeeRate(1000, 1).ToUniValue(FeeEstimateMode::SAT_VB).write(), "1000.000");


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

yup, good catch tanks! :)

w0xlt · 2025-11-06T01:43:06Z

src/rpc/net.cpp

-        // Those fields can be deprecated, to be replaced by the getmempoolinfo fields
-        obj.pushKV("relayfee", ValueFromAmount(node.mempool->m_opts.min_relay_feerate.GetFeePerK()));
-        obj.pushKV("incrementalfee", ValueFromAmount(node.mempool->m_opts.incremental_relay_feerate.GetFeePerK()));
+        UniValue mempool_info = MempoolInfoToJSON(*node.mempool, self.Arg<bool>("satvB"));


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.

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.

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

polespinasa

Rebased following @w0xlt suggestions :)

polespinasa · 2025-11-10T23:22:21Z

src/rpc/net.cpp

-        // Those fields can be deprecated, to be replaced by the getmempoolinfo fields
-        obj.pushKV("relayfee", ValueFromAmount(node.mempool->m_opts.min_relay_feerate.GetFeePerK()));
-        obj.pushKV("incrementalfee", ValueFromAmount(node.mempool->m_opts.incremental_relay_feerate.GetFeePerK()));
+        UniValue mempool_info = MempoolInfoToJSON(*node.mempool, self.Arg<bool>("satvB"));


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.

polespinasa · 2025-11-10T23:27:52Z

src/test/amount_tests.cpp

+    BOOST_CHECK_EQUAL(CFeeRate(1, 1).ToUniValue(FeeEstimateMode::SAT_VB).write(), "1.000");
+    BOOST_CHECK_EQUAL(CFeeRate(10, 1).ToUniValue(FeeEstimateMode::SAT_VB).write(), "10.000");
+    BOOST_CHECK_EQUAL(CFeeRate(1000, 1).ToUniValue(FeeEstimateMode::SAT_VB).write(), "1000.000");


yup, good catch tanks! :)

polespinasa · 2025-11-10T23:28:59Z

src/policy/feerate.h

@@ -8,6 +8,7 @@

 #include <consensus/amount.h>
 #include <serialize.h>
+#include <univalue/include/univalue.h>


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

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

glozow · 2025-11-11T14:27:21Z

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 · 2025-11-11T18:46:45Z

@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 · 2025-11-11T19:14:24Z

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 · 2025-11-12T22:09:56Z

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.

polespinasa · 2025-11-13T01:52:17Z

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.

Yes the idea is to be able to use sat/vB or BTC/kvB in inputting and outputting. This PR is just a PoC on how we could do it for outputting.

My concern is the idea that the default format might be changed in the future

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 tbh.

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

Oh sorry I didn't understand your idea. So you propose to add a startup option to choose the feerate units and then on the outputs change the key so it will return a key error if someone is expecting a different unit? I think that's a good approach, how would you do it for rpc arguments though? Unless you force to use named arguments I think it can still lead to errors.

DrahtBot · 2025-12-27T17:45:01Z

🐙 This pull request conflicts with the target branch and needs rebase.

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

polespinasa changed the title ~~rpc: [PoC] Optionally print feerates in sat/vb~~ rpc: Optionally print feerates in sat/vb Oct 30, 2025

polespinasa mentioned this pull request Oct 30, 2025

Migrate from BTC/kvB to sat/vB on RPC and startup options #32093

Open

26 tasks

polespinasa force-pushed the 2025-10-29-PoC-rpc-satvb branch from b581138 to 1389b97 Compare October 30, 2025 00:53

w0xlt reviewed Oct 30, 2025

View reviewed changes

maflcko reviewed Oct 30, 2025

View reviewed changes

This was referenced Oct 30, 2025

wallet, rpc: remove settxfee and paytxfee #32138

Open

Fee Estimation via Fee rate Forecasters #30157

Closed

polespinasa force-pushed the 2025-10-29-PoC-rpc-satvb branch from 1389b97 to ed354df Compare October 30, 2025 20:14

polespinasa force-pushed the 2025-10-29-PoC-rpc-satvb branch 3 times, most recently from f4f932d to c34f6a3 Compare October 31, 2025 22:03

w0xlt reviewed Nov 6, 2025

View reviewed changes

polespinasa added 2 commits November 11, 2025 01:10

rpc: Add ValueFromFeeRate function to core_write

dfc9e27

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

test: add unit tests for ValueFromFeeRate

1fc7a5c

polespinasa force-pushed the 2025-10-29-PoC-rpc-satvb branch from c34f6a3 to 6caa122 Compare November 11, 2025 00:24

polespinasa commented Nov 11, 2025

View reviewed changes

polespinasa force-pushed the 2025-10-29-PoC-rpc-satvb branch from 6caa122 to 004d22b Compare November 11, 2025 00:30

polespinasa added 6 commits November 11, 2025 01:31

rpc: let MempoolInfoToJSON to return feerates in sat/vb

7cbf96a

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

rpc: allow getmempoolinfo to return feerates in sat/vb

7ee792d

rpc: allow getnetworkinfo to return feerates in sat/vb

335a1e8

rpc: allow getwalletinfo to return feerates in sat/vb

0eb291a

rpc: allow estimatesmartfee to return feerates in sat/vb

dd245f2

rpc: allow estimaterawfee to return feerates in sat/vB

004d22b

DrahtBot mentioned this pull request Nov 12, 2025

Add a "tx output spender" index #24539

Open

This was referenced Dec 16, 2025

fees: Introduce Mempool Based Fee Estimation to reduce overestimation #34075

Open

rpc: [mempool] Remove erroneous Univalue integral casts #34112

Merged

refactor: [rpc] Remove confusing and brittle integral casts #34113

Merged

DrahtBot added the Needs rebase label Dec 27, 2025

rpc: Optionally print feerates in sat/vb #33741

Are you sure you want to change the base?

rpc: Optionally print feerates in sat/vb #33741

Uh oh!

Conversation

polespinasa commented Oct 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Oct 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage & Benchmarks

Reviews

Conflicts

Uh oh!

w0xlt left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

polespinasa commented Oct 30, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

polespinasa left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

glozow commented Nov 11, 2025

Uh oh!

polespinasa commented Nov 11, 2025

Uh oh!

glozow commented Nov 11, 2025

Uh oh!

w0xlt commented Nov 12, 2025

Uh oh!

polespinasa commented Nov 13, 2025

Uh oh!

DrahtBot commented Dec 27, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

polespinasa commented Oct 30, 2025 •

edited

Loading

DrahtBot commented Oct 30, 2025 •

edited

Loading