Cleanup CFeeRate constructor (sat/vB vs BTC/kvB) #23129

issue maflcko opened this issue on September 29, 2021

maflcko commented at 8:17 AM on September 29, 2021: member

Tracking issue for discussion in #20391 (review)
jonatack commented at 8:27 AM on September 29, 2021: member

Since the merge of #21786 is the discussion still relevant?
maflcko commented at 8:55 AM on September 29, 2021: member

Yes, I think so. That pull only fixed the AmountFromValue bug.

Cleaning up the constructor would be more of a refactor.
maflcko added the label Refactoring on Sep 29, 2021
jonatack commented at 9:01 AM on September 29, 2021: member

Reminds me, @Xekyo and I prepared a refactoring a few months ago. @Xekyo, want to open it?
willcl-ark commented at 7:41 AM on June 26, 2025: member

@jonatack @murchandamus is this issue effectively solved via previous changes + #24305
murchandamus commented at 10:02 PM on June 30, 2025: member

@willcl-ark: I don’t think this is resolved. The state in src/policy/feerate.cpp still matches @maflcko’s comment from the linked PR that predates this issue by a day. I think #32750 is making some progress towards improvement, though.
polespinasa commented at 1:39 AM on September 22, 2025: member

Is this still relevant after #32750 is merged?
maflcko commented at 5:58 PM on September 24, 2025: member

@polespinasa I don't think this is resolved. To explain it a bit better, looking at the code:

https://github.com/bitcoin/bitcoin/blob/d41b503ae128ac36ef27e652d2935c6fe7981a79/src/wallet/rpc/spend.cpp#L223-L224

AmountFromValue is a function that returns a CAmount (i64 of satoshis). However, the CFeeRate constructor that is called takes a feerate per kvB:

https://github.com/bitcoin/bitcoin/blob/d41b503ae128ac36ef27e652d2935c6fe7981a79/src/policy/feerate.h#L44

So calling CFeeRate{AmountFromValue("0.0000'0001")} makes sense to call the BTC/kvB constructor.

However, calling CFeeRate{AmountFromValue("0.001", 3)} feels like a layer and type violation to parse sat/vB.

I guess if I am the only one bothered by it, the issue can be closed. The code behaves correctly and this is more in the style/refactor territory.

Maybe this can also be closed and transferred into #32093?
maflcko commented at 6:39 AM on September 25, 2025: member

Closing for now. Seems fine to just organically change this, the next time the code is touched.
maflcko closed this on Sep 25, 2025
bitcoin locked this on Sep 25, 2026

Contributors

Labels

Linked (view graph)

#20391 wallet: introduce setfeerate (an improved settxfee, in sat/vB)#21786 wallet: ensure sat/vB feerates are in range (mantissa of 3)