This simple patch is the start of a series of CFeeRate refactoring changes and improvements. It contains changes from both #20391 and #20546 as well as a future pull to create estimatefeerate in sat/vB. It also resolves feedback in #20305 and #20546 to have a simpler fee rate constructor interface for most needs (BTC/kvB, sat/vB) without exposing CFeeRate internals.
policy, refactor: CFeeRate::FromSatB/FromBtcKb named constructors #20790
pull jonatack wants to merge 3 commits into bitcoin:master from jonatack:CFeeRate-named-constructors changing 3 files +42 −5-
jonatack commented at 9:38 PM on December 28, 2020: contributor
-
policy: create CFeeRate::FromSatB and FromBtcKb named ctors 3abb8b7ba6
-
test: add CFeeRate::FromSatB/FromBtcKb unit test coverage 6db85fbf45
-
refactor: update rpcwallet to CFeeRate named constructors 510cc9c477
- DrahtBot added the label RPC/REST/ZMQ on Dec 28, 2020
- DrahtBot added the label TX fees and policy on Dec 28, 2020
- DrahtBot added the label Wallet on Dec 28, 2020
-
DrahtBot commented at 12:23 AM on December 29, 2020: contributor
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--174a7506f384e20aa4161008e828411d-->
Conflicts
Reviewers, this pull request conflicts with the following ones:
- #20546 (policy, wallet, refactor: check for non-representable CFeeRates by jonatack)
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.
- DrahtBot cross-referenced this on Dec 29, 2020 from issue wallet: check for non-representable CFeeRates by jonatack
- jonatack renamed this:
policy: create CFeeRate::FromSatB and FromBtcKb named ctors
policy, refactor: CFeeRate::FromSatB/FromBtcKb named constructors
on Jan 1, 2021 -
jonatack commented at 1:41 PM on January 1, 2021: contributor
If anyone is wondering, this "Named Constructor Idiom" is from https://isocpp.org/wiki/faq/ctors#named-ctor-idiom by Stroustrup, Cline, Sutter and Alexandrescu.
-
jonatack commented at 12:16 PM on January 3, 2021: contributor
Seeing no concept ACKs or other interest despite this being requested several times in review feedback, closing for now.
- jonatack closed this on Jan 3, 2021
-
michaelfolkson commented at 12:28 PM on January 3, 2021: contributor
Seems like a clear Concept ACK to me especially if requested several times. I was unclear what was covered by the series of the PRs and what was covered by this individual PR from your initial PR description. Is "It" this PR or the series of PRs?
-
jonatack commented at 3:54 PM on January 3, 2021: contributor
Thanks. I'd like to work on things that people are excited about, e.g. "Strong Concept ACK" or at least a spontaneous "Concept ACK" or two. This proposal didn't make that minimal bar. The
CFeeRateclass could really use some love and I have a series of good changes written for it. They just don't seem to be compelling for reviewers at this time. - adamjonas cross-referenced this on Jan 14, 2021 from issue Signed integer overflow in CFeeRate::GetFee(…) reachable via RPC call analyzepsbt by practicalswift
- jonatack cross-referenced this on Mar 24, 2021 from issue Add feerate histogram to getmempoolinfo by kiminuo
- bitcoin locked this on Aug 16, 2022
Contributors
Linked (view graph)
#20305 wallet: introduce fee_rate sat/vB param/option#20391 wallet: introduce setfeerate (an improved settxfee, in sat/vB)#20546 wallet: check for non-representable CFeeRates#20607 Signed integer overflow in CFeeRate::GetFee(…) reachable via RPC call analyzepsbt#21422 Add feerate histogram to getmempoolinfo