rpc: Optionally print feerates in sat/vb#33741
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code Coverage & BenchmarksFor details see: https://corecheck.dev/bitcoin/bitcoin/pulls/33741. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please copy-paste ConflictsReviewers, this pull request conflicts with the following ones:
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. LLM Linter (✨ experimental)Possible places where named args for integral literals may be used (e.g.
2026-06-20 06:31:33 |
b581138 to
1389b97
Compare
maflcko
left a comment
There was a problem hiding this comment.
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.
| @@ -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()) { | |||
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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;
There was a problem hiding this comment.
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.
| @@ -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); | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Generally, it is best to write unit or fuzz tests for newly written code
Done
Also, I am not sure if
CFeeRateshould 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.
1389b97 to
ed354df
Compare
|
@maflcko thanks for reviewing :)
I don't think there is either, at least not in a backwards compatible way.
That would be great for a |
f4f932d to
c34f6a3
Compare
| @@ -8,6 +8,7 @@ | |||
|
|
|||
| #include <consensus/amount.h> | |||
| #include <serialize.h> | |||
| #include <univalue/include/univalue.h> | |||
There was a problem hiding this comment.
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);There was a problem hiding this comment.
Yes maybe it's a better approach.
Moved the function into core_write.cpp and core_io.h similar to function ValueFromAmount.
| 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"); |
There was a problem hiding this comment.
Do the last six lines duplicate three assertions ?
You probably intended to add negative sat/vB cases.
There was a problem hiding this comment.
yup, good catch tanks! :)
| // 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")); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
c34f6a3 to
6caa122
Compare
polespinasa
left a comment
There was a problem hiding this comment.
Rebased following @w0xlt suggestions :)
| // 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")); |
There was a problem hiding this comment.
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.
| 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"); |
There was a problem hiding this comment.
yup, good catch tanks! :)
| @@ -8,6 +8,7 @@ | |||
|
|
|||
| #include <consensus/amount.h> | |||
| #include <serialize.h> | |||
| #include <univalue/include/univalue.h> | |||
There was a problem hiding this comment.
Yes maybe it's a better approach.
Moved the function into core_write.cpp and core_io.h similar to function ValueFromAmount.
6caa122 to
004d22b
Compare
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.
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. |
|
@glozow thanks for your comments :)
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.
I agree, this PR doesn't change any default behavior. It MUST be done in a backwards compatible way.
Yes! This is another approach already taken by
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. |
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.
That's why the results must have different names. If they forget the config option, they should get a key error. |
|
Adding a new field in the result called |
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.
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.
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. |
w0xlt
left a comment
There was a problem hiding this comment.
estimaterawfee is converting only the top-level feerate when satvB=true, while pass/fail.startrange and endrange remained in the sat/kvB units.
The below patch applies the same unit selection to those bucket range feerates, avoiding mixed-unit RPC responses.
diff
diff --git a/src/rpc/fees.cpp b/src/rpc/fees.cpp
index 140ba9d64f..854f4f203f 100644
--- a/src/rpc/fees.cpp
+++ b/src/rpc/fees.cpp
@@ -4,6 +4,7 @@
// file COPYING or http://www.opensource.org/licenses/mit-license.php.
#include <common/messages.h>
+#include <consensus/amount.h>
#include <core_io.h>
#include <node/context.h>
#include <policy/feerate.h>
@@ -21,6 +22,7 @@
#include <algorithm>
#include <array>
#include <cmath>
+#include <limits>
#include <string>
#include <string_view>
@@ -29,6 +31,19 @@ using common::FeeModesDetail;
using common::InvalidEstimateModeErrorMessage;
using node::NodeContext;
+static UniValue ValueFromEstimateBucketFeeRate(double fee_rate_per_kvb, bool sat_vb)
+{
+ const double rounded_fee_rate_per_kvb{round(fee_rate_per_kvb)};
+
+ // Preserve estimaterawfee's legacy bucket range unit unless sat/vB was requested.
+ if (!sat_vb || rounded_fee_rate_per_kvb < 0) return rounded_fee_rate_per_kvb;
+
+ if (rounded_fee_rate_per_kvb > static_cast<double>(std::numeric_limits<CAmount>::max())) {
+ return rounded_fee_rate_per_kvb / 1000.0;
+ }
+ return ValueFromFeeRate(CFeeRate{static_cast<CAmount>(rounded_fee_rate_per_kvb)}, FeeRateUnit::SAT_VB);
+}
+
static RPCMethod estimatesmartfee()
{
return RPCMethod{
@@ -168,6 +183,8 @@ static RPCMethod estimaterawfee()
}
UniValue result(UniValue::VOBJ);
+ const bool sat_vb{self.Arg<bool>("satvB")};
+ const FeeRateUnit feerate_units{sat_vb ? FeeRateUnit::SAT_VB : FeeRateUnit::BTC_KVB};
for (const FeeEstimateHorizon horizon : ALL_FEE_ESTIMATE_HORIZONS) {
CFeeRate feeRate;
@@ -180,15 +197,15 @@ static RPCMethod estimaterawfee()
UniValue horizon_result(UniValue::VOBJ);
UniValue errors(UniValue::VARR);
UniValue passbucket(UniValue::VOBJ);
- passbucket.pushKV("startrange", round(buckets.pass.start));
- passbucket.pushKV("endrange", round(buckets.pass.end));
+ passbucket.pushKV("startrange", ValueFromEstimateBucketFeeRate(buckets.pass.start, sat_vb));
+ passbucket.pushKV("endrange", ValueFromEstimateBucketFeeRate(buckets.pass.end, sat_vb));
passbucket.pushKV("withintarget", round(buckets.pass.withinTarget * 100.0) / 100.0);
passbucket.pushKV("totalconfirmed", round(buckets.pass.totalConfirmed * 100.0) / 100.0);
passbucket.pushKV("inmempool", round(buckets.pass.inMempool * 100.0) / 100.0);
passbucket.pushKV("leftmempool", round(buckets.pass.leftMempool * 100.0) / 100.0);
UniValue failbucket(UniValue::VOBJ);
- failbucket.pushKV("startrange", round(buckets.fail.start));
- failbucket.pushKV("endrange", round(buckets.fail.end));
+ failbucket.pushKV("startrange", ValueFromEstimateBucketFeeRate(buckets.fail.start, sat_vb));
+ failbucket.pushKV("endrange", ValueFromEstimateBucketFeeRate(buckets.fail.end, sat_vb));
failbucket.pushKV("withintarget", round(buckets.fail.withinTarget * 100.0) / 100.0);
failbucket.pushKV("totalconfirmed", round(buckets.fail.totalConfirmed * 100.0) / 100.0);
failbucket.pushKV("inmempool", round(buckets.fail.inMempool * 100.0) / 100.0);
@@ -196,7 +213,7 @@ static RPCMethod estimaterawfee()
// CFeeRate(0) is used to indicate error as a return value from estimateRawFee
if (feeRate != CFeeRate(0)) {
- horizon_result.pushKV("feerate", ValueFromFeeRate(feeRate, self.Arg<bool>("satvB") ? FeeRateUnit::SAT_VB : FeeRateUnit::BTC_KVB ));
+ horizon_result.pushKV("feerate", ValueFromFeeRate(feeRate, feerate_units));
horizon_result.pushKV("decay", buckets.decay);
horizon_result.pushKV("scale", buckets.scale);
horizon_result.pushKV("pass", std::move(passbucket));99b041c to
0810ba2
Compare
|
@w0xlt good catch. I picked it with a slightly different approach. |
0810ba2 to
26316c5
Compare
w0xlt
left a comment
There was a problem hiding this comment.
ACK 26316c5
nit: RPC help texts can be update
diff
diff --git a/src/rpc/fees.cpp b/src/rpc/fees.cpp
index a1bef9615f..e429c4e473 100644
--- a/src/rpc/fees.cpp
+++ b/src/rpc/fees.cpp
@@ -47,7 +47,7 @@ static RPCMethod estimatesmartfee()
RPCResult{
RPCResult::Type::OBJ, "", "",
{
- {RPCResult::Type::NUM, "feerate", /*optional=*/true, "estimate fee rate in " + CURRENCY_UNIT + "/kvB (only present if no errors were encountered)"},
+ {RPCResult::Type::NUM, "feerate", /*optional=*/true, "estimate fee rate in " + CURRENCY_UNIT + "/kvB, or " + CURRENCY_ATOM + "/vB if satvB is true (only present if no errors were encountered)"},
{RPCResult::Type::ARR, "errors", /*optional=*/true, "Errors encountered during processing (if there are any)",
{
{RPCResult::Type::STR, "", "error"},
diff --git a/src/rpc/mempool.cpp b/src/rpc/mempool.cpp
index ed5d8fb662..e6428518eb 100644
--- a/src/rpc/mempool.cpp
+++ b/src/rpc/mempool.cpp
@@ -1089,9 +1089,9 @@ static RPCMethod getmempoolinfo()
{RPCResult::Type::NUM, "usage", "Total memory usage for the mempool"},
{RPCResult::Type::STR_AMOUNT, "total_fee", "Total fees for the mempool in " + CURRENCY_UNIT + ", ignoring modified fees through prioritisetransaction"},
{RPCResult::Type::NUM, "maxmempool", "Maximum memory usage for the mempool"},
- {RPCResult::Type::STR_AMOUNT, "mempoolminfee", "Minimum fee rate in " + CURRENCY_UNIT + "/kvB for tx to be accepted. Is the maximum of minrelaytxfee and minimum mempool fee"},
- {RPCResult::Type::STR_AMOUNT, "minrelaytxfee", "Current minimum relay fee for transactions"},
- {RPCResult::Type::NUM, "incrementalrelayfee", "minimum fee rate increment for mempool limiting or replacement in " + CURRENCY_UNIT + "/kvB"},
+ {RPCResult::Type::STR_AMOUNT, "mempoolminfee", "Minimum fee rate in " + CURRENCY_UNIT + "/kvB, or " + CURRENCY_ATOM + "/vB if satvB is true, for tx to be accepted. Is the maximum of minrelaytxfee and minimum mempool fee"},
+ {RPCResult::Type::STR_AMOUNT, "minrelaytxfee", "Current minimum relay fee for transactions in " + CURRENCY_UNIT + "/kvB, or " + CURRENCY_ATOM + "/vB if satvB is true"},
+ {RPCResult::Type::NUM, "incrementalrelayfee", "minimum fee rate increment for mempool limiting or replacement in " + CURRENCY_UNIT + "/kvB, or " + CURRENCY_ATOM + "/vB if satvB is true"},
{RPCResult::Type::NUM, "unbroadcastcount", "Current number of transactions that haven't passed initial broadcast yet"},
{RPCResult::Type::BOOL, "permitbaremultisig", "True if the mempool accepts transactions with bare multisig outputs"},
{RPCResult::Type::NUM, "maxdatacarriersize", "Maximum number of bytes that can be used by OP_RETURN outputs in the mempool"},
diff --git a/src/rpc/net.cpp b/src/rpc/net.cpp
index 742287a12b..ab49a386e7 100644
--- a/src/rpc/net.cpp
+++ b/src/rpc/net.cpp
@@ -670,8 +670,8 @@ static RPCMethod getnetworkinfo()
{RPCResult::Type::BOOL, "proxy_randomize_credentials", "Whether randomized credentials are used"},
}},
}},
- {RPCResult::Type::NUM, "relayfee", "minimum relay fee rate for transactions in " + CURRENCY_UNIT + "/kvB"},
- {RPCResult::Type::NUM, "incrementalfee", "minimum fee rate increment for mempool limiting or replacement in " + CURRENCY_UNIT + "/kvB"},
+ {RPCResult::Type::NUM, "relayfee", "minimum relay fee rate for transactions in " + CURRENCY_UNIT + "/kvB, or " + CURRENCY_ATOM + "/vB if satvB is true"},
+ {RPCResult::Type::NUM, "incrementalfee", "minimum fee rate increment for mempool limiting or replacement in " + CURRENCY_UNIT + "/kvB, or " + CURRENCY_ATOM + "/vB if satvB is true"},
{RPCResult::Type::ARR, "localaddresses", "list of local addresses",
{
{RPCResult::Type::OBJ, "", "",26316c5 to
b218138
Compare
Similar to ValueFromAmount this function returns a UniValue object with the feerate given a CFeeRate object.
This commit also includes mempool.h, which was missing and was causing function definition issues.
Co-authored-by: w0xlt <woltx@protonmail.com>
b218138 to
5135555
Compare
|
Rebased on top of master |
musaHaruna
left a comment
There was a problem hiding this comment.
Concept ACK on migrating to sat/vb in a backwards compatible manner.
Code Review 740b6e8
I noticed that this section handles negative values by mutating the signed variables after division:
int64_t quotient = feerate_per_kvb / units;
int64_t remainder = feerate_per_kvb % units;
if (feerate_per_kvb < 0) {
quotient = -quotient;
remainder = -remainder;
}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>? i.e:
const uint64_t abs_feerate = (feerate_per_kvb < 0) ? -static_cast<uint64_t (feerate_per_kvb) : feerate_per_kvb;
const uint64_t quotient = abs_feerate / units;
const uint64_t remainder = abs_feerate % units;Handling the magnitude on pure unsigned types feels like it would eliminate the need for the conditional if block, making the function completely linear.
I might be likely missing some subtleties that I'm not aware of though.
| { | ||
| const CAmount feerate_per_kvb = fee_rate.GetFeePerK(); | ||
| const bool is_sat_vb = (fee_rate_unit == FeeRateUnit::SAT_VB); | ||
| const int64_t units = is_sat_vb ? 1000 : COIN; |
There was a problem hiding this comment.
Nit: Extra whitespaces can be removed
| const int64_t units = is_sat_vb ? 1000 : COIN; | |
| const int64_t units = is_sat_vb ? 1000 : COIN; | |
| const int8_t decimals = is_sat_vb ? 3 : 8; |
| return UniValue( | ||
| UniValue::VNUM, | ||
| strprintf("%s%d.%0*d", | ||
| feerate_per_kvb < 0 ? "-" : "", | ||
| quotient, | ||
| decimals, | ||
| remainder) | ||
| ); | ||
| } |
There was a problem hiding this comment.
I think this can be returned on a single line?
| return UniValue( | |
| UniValue::VNUM, | |
| strprintf("%s%d.%0*d", | |
| feerate_per_kvb < 0 ? "-" : "", | |
| quotient, | |
| decimals, | |
| remainder) | |
| ); | |
| } | |
| return UniValue(UniValue::VNUM, strprintf("%s%d.%0*d", feerate_per_kvb < 0 ? "-" : "", quotient, decimals, remainder)); |
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,estimatesmartfeeandestimaterawfee.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 < 1then 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.