Add multiplication operator to CFeeRate #29037

pull murchandamus wants to merge 2 commits into bitcoin:master from murchandamus:2023-12-multiply-operator-CFeeRate changing 2 files +28 −0

murchandamus commented at 6:57 PM on December 8, 2023: contributor

Allows us to use coin_selection_params.m_long_term_feerate * 3 or 3 * coin_selection_params.m_long_term_feerate instead of
CFeeRate{coin_selection_params.m_long_term_feerate.GetFee(3000)}

inspired by #27877 (review)
DrahtBot commented at 6:57 PM on December 8, 2023: contributor



The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.



Code Coverage

For detailed information about the code coverage, see the test coverage report.



Reviews

See the guideline for information on the review process.

Type Reviewers

ACK ajtowns, kevkevinpal, ismaelsadeeq, achow101

Stale ACK kashifs

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.



Conflicts

No conflicts as of last run.
murchandamus marked this as ready for review on Dec 8, 2023
Add multiplication operator to CFeeRate 1553c80786
in src/policy/feerate.h:75 in 7dacc17e9c outdated
```
  70 | @@ -71,6 +71,8 @@ class CFeeRate
  71 |      friend bool operator!=(const CFeeRate& a, const CFeeRate& b) { return a.nSatoshisPerK != b.nSatoshisPerK; }
  72 |      CFeeRate& operator+=(const CFeeRate& a) { nSatoshisPerK += a.nSatoshisPerK; return *this; }
  73 |      std::string ToString(const FeeEstimateMode& fee_estimate_mode = FeeEstimateMode::BTC_KVB) const;
  74 | +    friend CFeeRate operator*(const CFeeRate& f, int a) { return CFeeRate(a * f.nSatoshisPerK); }
  75 | +    friend CFeeRate operator*(int a, const CFeeRate& f) { return f * a; }
```
kevkevinpal commented at 2:19 AM on December 9, 2023:

nit: To me it confused me for a moment cause I didn't realize that f was using the operator defined on the line above, I think it would make more sense for a reader to just see the same thing above defined backwards like so

friend CFeeRate operator*(int a, const CFeeRate& f) { return CFeeRate(f.nSatoshisPerK * a); }

feel free to ignore this suggestion though

murchandamus commented at 2:34 PM on December 9, 2023:

Sure, I updated it to use the same formula in both lines, so that only the parameters appear in either order
murchandamus force-pushed on Dec 9, 2023
kevkevinpal commented at 1:57 AM on December 10, 2023: contributor

ACK 1553c80
kashifs commented at 2:52 PM on December 10, 2023: contributor

tACK 1553c80

I compiled from source, ran the test suite, and added some additional BOOST_CHECK tests on my local machine that all passed
murchandamus commented at 2:24 PM on December 11, 2023: contributor

Thanks both of you! @kashifs: If you open a PR against my PR or send me a patch by email, I’d be happy to add your commit with the additional tests to the PR.
test: Add tests for CFeeRate multiplication operator 1757452cc5
kashifs commented at 10:05 PM on December 11, 2023: contributor

Hi @murchandamus, I believe that I correctly opened a PR against this PR. Please let me know if this is the correct etiquette or if there is anything more that I should do.
murchandamus force-pushed on Dec 14, 2023
murchandamus force-pushed on Dec 14, 2023
murchandamus commented at 9:15 PM on December 14, 2023: contributor

Thanks @kashifs, I’ve merged your pull request into my pull request and pushed it.
kashifs commented at 11:01 PM on December 14, 2023: contributor

Okay, great! I'm slowly working my way through #27877
murchandamus requested review from kevkevinpal on Dec 15, 2023

Type	Reviewers
ACK	ajtowns, kevkevinpal, ismaelsadeeq, achow101
Stale ACK	kashifs

in src/policy/feerate.h:75 in 1757452cc5

  70 | @@ -71,6 +71,8 @@ class CFeeRate
  71 |      friend bool operator!=(const CFeeRate& a, const CFeeRate& b) { return a.nSatoshisPerK != b.nSatoshisPerK; }
  72 |      CFeeRate& operator+=(const CFeeRate& a) { nSatoshisPerK += a.nSatoshisPerK; return *this; }
  73 |      std::string ToString(const FeeEstimateMode& fee_estimate_mode = FeeEstimateMode::BTC_KVB) const;
  74 | +    friend CFeeRate operator*(const CFeeRate& f, int a) { return CFeeRate(a * f.nSatoshisPerK); }
  75 | +    friend CFeeRate operator*(int a, const CFeeRate& f) { return CFeeRate(a * f.nSatoshisPerK); }

ajtowns commented at 4:35 PM on December 18, 2023:

Seems strange to have operator+= and operator* but no operator*= or operator+.

ajtowns commented at 4:35 PM on December 18, 2023: contributor

ACK 1757452cc55a6dacc62e4258043ee4d711fd281a ; lgtm
DrahtBot removed review request from kevkevinpal on Dec 18, 2023
DrahtBot requested review from kevkevinpal on Dec 18, 2023
DrahtBot removed review request from kevkevinpal on Dec 18, 2023
DrahtBot requested review from kevkevinpal on Dec 18, 2023
kevkevinpal commented at 5:34 PM on December 18, 2023: contributor

reACK 1757452
DrahtBot removed review request from kevkevinpal on Dec 18, 2023
ismaelsadeeq approved
ismaelsadeeq commented at 4:48 PM on December 19, 2023: member

ACK 1757452cc55a6dacc62e4258043ee4d711fd281a
in src/test/amount_tests.cpp:87 in 1757452cc5
```
  86 | @@ -87,10 +87,30 @@ BOOST_AUTO_TEST_CASE(GetFeeTest)
  87 |      CFeeRate(MAX_MONEY, std::numeric_limits<uint32_t>::max()).GetFeePerK();
```
ismaelsadeeq commented at 4:49 PM on December 19, 2023:

nit: will be nice to squashed this 1757452cc55a6dacc62e4258043ee4d711fd281a to the first commit
achow101 commented at 12:24 AM on December 20, 2023: member

ACK 1757452cc55a6dacc62e4258043ee4d711fd281a
achow101 merged this on Dec 20, 2023
achow101 closed this on Dec 20, 2023
bitcoin locked this on Dec 19, 2024

Contributors

Linked (view graph)

#27877 wallet: Add CoinGrinder coin selection algorithm

Add multiplication operator to CFeeRate #29037

Code Coverage

Reviews

Conflicts