wallet: Replace fee magic numbers with named constants

151henry151 commented at 8:30 pm on August 25, 2025: none

Summary

This PR addresses a TODO item in the wallet fee handling code by replacing magic numbers with descriptive named constants. It also adds constexpr constructors to CFeeRate to support compile-time constants.

Changes

Add constexpr CFeeRate constructors to support compile-time constants
Replace magic CFeeRate(0) values with descriptive named constants:
- DEFAULT_FEERATE for default/unset fee rates
- NO_FEE_DATA for fee estimation failures
- FEERATE_DISABLED for disabled fee features
Remove resolved TODO comment in src/wallet/fees.cpp:44 (magic value of 0)
Update comments to be more descriptive

Benefits

This improves code readability and maintainability by:

Eliminating magic numbers and centralizing fee rate constants
Making the code’s intent clearer
Reducing the risk of mistakes when comparing fee rates in different contexts
Enabling compile-time constant initialization with constexpr constructors
Providing compile-time safety for fee rate constants

Testing

Local build successful (RelWithDebInfo, GCC 12.2.0, Linux)
No behavioral changes introduced
No new compiler warnings

Note: Local development build only - not a full Guix reproducible build.

Fixes TODO item in src/wallet/fees.cpp:44

wallet: Replace fee magic numbers with named constants

- Add constexpr CFeeRate constructors to support compile-time constants
- Replace magic CFeeRate(0) values with descriptive named constants:
  * DEFAULT_FEERATE for default/unset fee rates
  * NO_FEE_DATA for fee estimation failures
  * FEERATE_DISABLED for disabled fee features
- Remove resolved TODO comment in src/wallet/fees.cpp:44 (magic value of 0)
- Update comments to be more descriptive

This improves code readability and maintainability by eliminating magic
numbers and centralizing fee rate constants. The use of named constants
makes the code's intent clearer and reduces the risk of mistakes when
comparing fee rates in different contexts. Additionally, constexpr
constructors enable compile-time constant initialization, allowing these
constants to be used in constexpr contexts and providing compile-time
safety.

c7fc07b8e8

DrahtBot added the label Wallet on Aug 25, 2025

DrahtBot commented at 8:30 pm on August 25, 2025: contributor

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/33254.

Reviews

See the guideline for information on the review process. A summary of reviews will appear here.

in src/wallet/fees.cpp:45 in c7fc07b8e8

41@@ -42,7 +42,7 @@ CFeeRate GetMinimumFeeRate(const CWallet& wallet, const CCoinControl& coin_contr
42         // Allow to override automatic min/max check over coin control instance
43         if (coin_control.fOverrideFeeRate) return feerate_needed;
44     }
45-    else if (!coin_control.m_confirm_target && wallet.m_pay_tx_fee != CFeeRate(0)) { // 3. TODO: remove magic value of 0 for wallet member m_pay_tx_fee
46+    else if (!coin_control.m_confirm_target && wallet.m_pay_tx_fee != DEFAULT_FEERATE) { // 3. Check if user has set a custom fee rate (different from default)

maflcko commented at 10:12 am on August 26, 2025:

the todo refers to the value of m_pay_tx_fee, not to the hardcoded 0 here. Also, see https://github.com/bitcoin/bitcoin/pull/32138/files

in src/wallet/wallet.h:111 in c7fc07b8e8

102@@ -103,6 +103,12 @@ std::unique_ptr<WalletDatabase> MakeWalletDatabase(const std::string& name, cons
103 
104 //! -paytxfee default
105 constexpr CAmount DEFAULT_PAY_TX_FEE = 0;
106+//! Default/unset fee rate (user hasn't set a custom fee)
107+constexpr CFeeRate DEFAULT_FEERATE = CFeeRate(0);
108+//! Fee rate indicating "no data available" for estimation
109+constexpr CFeeRate NO_FEE_DATA = CFeeRate(0);
110+//! Fee rate indicating "disabled" feature
111+constexpr CFeeRate FEERATE_DISABLED = CFeeRate(0);

maflcko commented at 10:12 am on August 26, 2025:

this is just removing magic values in one place and adding them in another. what is the point?

151henry151 commented at 12:00 pm on August 26, 2025:

Thanks for the feedback, I must have misunderstood the TODO. I thought this would be an improvement for the reasons outlined in the pull request, and thought it would address the TODO item.

maflcko commented at 10:13 am on August 26, 2025: member

Thanks, but the value of purely LLM generated “vibe” pull requests is limited, because:

The “author” does not understand the changes and can not reply to code review feedback.
There are more than 300 pull requests (most written by real humans) waiting for review.
Incentive to review purely LLM generated changes is limited, because the feedback likely does not help to train a real human planning to contribute in the future, but is likely only passed on to an LLM api.

My recommendation for the future would be to create your first pull request fully by yourself. This will hopefully ensure that you understand the changes yourself and can reply to feedback.

( Generally, I think it is fine to use LLMs, if the author would have written the same code almost exactly the same way without the LLM, and also fully understands it, and also can claim copyright on it. But this is probably a meta discussion. )

maflcko closed this on Aug 26, 2025

151henry151 commented at 12:14 pm on August 26, 2025: none

Thanks, but the value of purely LLM generated “vibe” pull requests is limited, because:

The “author” does not understand the changes and can not reply to code review feedback.

There are more than 300 pull requests (most written by real humans) waiting for review.

Incentive to review purely LLM generated changes is limited, because the feedback likely does not help to train a real human planning to contribute in the future, but is likely only passed on to an LLM api.

My recommendation for the future would be to create your first pull request fully by yourself. This will hopefully ensure that you understand the changes yourself and can reply to feedback.

( Generally, I think it is fine to use LLMs, if the author would have written the same code almost exactly the same way without the LLM, and also fully understands it, and also can claim copyright on it. But this is probably a meta discussion. )

Thanks for your feedback. In this case I did use an AI assistant to help me identify an approach to this TODO and thought that it had helped me understand the issue well enough to address it correctly, but going forward I will do more research to make sure I’m not wasting valuable reviewer time.

The code changes were made by me manually and the description of those changes was carefully written and adjusted numerous times before being submitted, in the hopes of being an actually valuable contribution.

I’ll try harder next time! Thanks for your candid feedback.