Merge bitcoin#34552: fees: refactor: separate feerate format from fee estimate mode

c135549 refactor: fees: split fee rate format from fee estimate mode (ismaelsadeeq)
922ebf9 refactor: move-only: move `FeeEstimateMode` enum to `util/fees.h` (ismaelsadeeq)

Pull request description:

  ### Motivation

  Part of bitcoin#34075

  - The `FeeEstimateMode` enum was responsible for both selecting the fee estimation algorithm and specifying the fee rate' format.

  ####  Changes in this PR:
     * The `FeeEstimateMode` enum (`UNSET`, `ECONOMICAL`, `CONSERVATIVE`) is moved to a new util/fees.h header.
     * A new `FeeRateFormat `enum (`BTC_KVB`, `SAT_VB`) is introduced in `policy/feerate.h` for feerate formatting.
     * The `CFeeRate::ToString()` method is updated to use `FeeRateFormat`.
     * All relevant function calls have been updated to use the new `FeeRateFormat` enum for formatting and `FeeEstimateMode` for fee estimation mode.

   This refactoring separates these unrelated responsibilities to improve code clarity.

ACKs for top commit:
  l0rinc:
    ACK c135549
  furszy:
    utACK c135549
  musaHaruna:
    ACK [c135549](bitcoin@c135549) — reviewed in the context of PR [34075](bitcoin#34075)
  willcl-ark:
    ACK c135549

Tree-SHA512: 7cbe36350744313d3d688d3fd282a58c441af1818b1e8ad9cddbc911c499a5205f8d4a39c36b21fed60542db1ef763eb69752d141bcef3393bf33c0922018645

Author: sedited

src/common/messages.cpp

@@ -4,11 +4,11 @@
 // file COPYING or http://www.opensource.org/licenses/mit-license.php.
 
 #include <common/messages.h>
-
 #include <common/types.h>
-#include <policy/fees/block_policy_estimator.h>
 #include <node/types.h>
+#include <policy/fees/block_policy_estimator.h>
 #include <tinyformat.h>
+#include <util/fees.h>
 #include <util/strencodings.h>
 #include <util/string.h>
 #include <util/translation.h>
@@ -68,7 +68,6 @@ std::string FeeModeInfo(const std::pair<std::string, FeeEstimateMode>& mode, std
                    "less responsive to short-term drops in the prevailing fee market. This mode\n"
                    "potentially returns a higher fee rate estimate.\n", mode.first);
         default:
-            // Other modes apart from the ones handled are fee rate units; they should not be clarified.
             assert(false);
     }
 }

src/policy/feerate.cpp

@@ -26,11 +26,12 @@ CAmount CFeeRate::GetFee(int32_t virtual_bytes) const
     return nFee;
 }
 
-std::string CFeeRate::ToString(const FeeEstimateMode& fee_estimate_mode) const
+std::string CFeeRate::ToString(FeeRateFormat fee_rate_format) const
 {
-    const CAmount feerate_per_kvb = GetFeePerK();
-    switch (fee_estimate_mode) {
-    case FeeEstimateMode::SAT_VB: return strprintf("%d.%03d %s/vB", feerate_per_kvb / 1000, feerate_per_kvb % 1000, CURRENCY_ATOM);
-    default:                      return strprintf("%d.%08d %s/kvB", feerate_per_kvb / COIN, feerate_per_kvb % COIN, CURRENCY_UNIT);
-    }
+    const CAmount feerate_per_kvb{GetFeePerK()};
+    switch (fee_rate_format) {
+    case FeeRateFormat::BTC_KVB: return strprintf("%d.%08d %s/kvB", feerate_per_kvb / COIN, feerate_per_kvb % COIN, CURRENCY_UNIT);
+    case FeeRateFormat::SAT_VB: return strprintf("%d.%03d %s/vB", feerate_per_kvb / 1000, feerate_per_kvb % 1000, CURRENCY_ATOM);
+    } // no default case, so the compiler can warn about missing cases
+    assert(false);
 }

src/policy/feerate.h

@@ -9,6 +9,7 @@
 #include <consensus/amount.h>
 #include <serialize.h>
 #include <util/feefrac.h>
+#include <util/fees.h>
 
 
 #include <cstdint>
@@ -18,13 +19,9 @@
 const std::string CURRENCY_UNIT = "BTC"; // One formatted unit
 const std::string CURRENCY_ATOM = "sat"; // One indivisible minimum value unit
 
-/* Used to determine type of fee estimation requested */
-enum class FeeEstimateMode {
-    UNSET,        //!< Use default settings based on other criteria
-    ECONOMICAL,   //!< Force estimateSmartFee to use non-conservative estimates
-    CONSERVATIVE, //!< Force estimateSmartFee to use conservative estimates
-    BTC_KVB,      //!< Use BTC/kvB fee rate unit
-    SAT_VB,       //!< Use sat/vB fee rate unit
+enum class FeeRateFormat {
+    BTC_KVB, //!< Use BTC/kvB fee rate unit
+    SAT_VB,  //!< Use sat/vB fee rate unit
 };
 
 /**
@@ -75,7 +72,7 @@ class CFeeRate
         m_feerate = FeePerVSize(GetFeePerK() + a.GetFeePerK(), 1000);
         return *this;
     }
-    std::string ToString(const FeeEstimateMode& fee_estimate_mode = FeeEstimateMode::BTC_KVB) const;
+    std::string ToString(FeeRateFormat fee_rate_format = FeeRateFormat::BTC_KVB) const;
     friend CFeeRate operator*(const CFeeRate& f, int a) { return CFeeRate(a * f.m_feerate.fee, f.m_feerate.size); }
     friend CFeeRate operator*(int a, const CFeeRate& f) { return CFeeRate(a * f.m_feerate.fee, f.m_feerate.size); }
 

src/rpc/fees.cpp

@@ -15,6 +15,7 @@
 #include <rpc/util.h>
 #include <txmempool.h>
 #include <univalue.h>
+#include <util/fees.h>
 #include <validationinterface.h>
 
 #include <algorithm>

src/test/amount_tests.cpp

@@ -138,8 +138,8 @@ BOOST_AUTO_TEST_CASE(ToStringTest)
     CFeeRate feeRate;
     feeRate = CFeeRate(1);
     BOOST_CHECK_EQUAL(feeRate.ToString(), "0.00000001 BTC/kvB");
-    BOOST_CHECK_EQUAL(feeRate.ToString(FeeEstimateMode::BTC_KVB), "0.00000001 BTC/kvB");
-    BOOST_CHECK_EQUAL(feeRate.ToString(FeeEstimateMode::SAT_VB), "0.001 sat/vB");
+    BOOST_CHECK_EQUAL(feeRate.ToString(FeeRateFormat::BTC_KVB), "0.00000001 BTC/kvB");
+    BOOST_CHECK_EQUAL(feeRate.ToString(FeeRateFormat::SAT_VB), "0.001 sat/vB");
 }
 
 BOOST_AUTO_TEST_SUITE_END()

src/test/fuzz/string.cpp

@@ -22,6 +22,7 @@
 #include <test/fuzz/FuzzedDataProvider.h>
 #include <test/fuzz/fuzz.h>
 #include <test/fuzz/util.h>
+#include <util/fees.h>
 #include <util/strencodings.h>
 #include <util/string.h>
 #include <util/translation.h>
@@ -34,8 +35,6 @@
 #include <string>
 #include <vector>
 
-enum class FeeEstimateMode;
-
 using common::AmountErrMsg;
 using common::AmountHighWarn;
 using common::FeeModeFromString;

src/util/fees.h

@@ -0,0 +1,15 @@
+// Copyright (c) The Bitcoin Core developers
+// Distributed under the MIT software license, see the accompanying
+// file COPYING or http://www.opensource.org/licenses/mit-license.php.
+
+#ifndef BITCOIN_UTIL_FEES_H
+#define BITCOIN_UTIL_FEES_H
+
+/* Used to determine type of fee estimation requested */
+enum class FeeEstimateMode {
+    UNSET,        //!< Use default settings based on other criteria
+    ECONOMICAL,   //!< Force estimateSmartFee to use non-conservative estimates
+    CONSERVATIVE, //!< Force estimateSmartFee to use conservative estimates
+};
+
+#endif // BITCOIN_UTIL_FEES_H

src/wallet/coincontrol.h

@@ -11,6 +11,7 @@
 #include <primitives/transaction.h>
 #include <script/keyorigin.h>
 #include <script/signingprovider.h>
+#include <util/fees.h>
 
 #include <algorithm>
 #include <map>

src/wallet/rpc/spend.cpp

@@ -1008,7 +1008,7 @@ static std::vector<RPCArg> OutputsDoc()
 static RPCHelpMan bumpfee_helper(std::string method_name)
 {
     const bool want_psbt = method_name == "psbtbumpfee";
-    const std::string incremental_fee{CFeeRate(DEFAULT_INCREMENTAL_RELAY_FEE).ToString(FeeEstimateMode::SAT_VB)};
+    const std::string incremental_fee{CFeeRate(DEFAULT_INCREMENTAL_RELAY_FEE).ToString(FeeRateFormat::SAT_VB)};
 
     return RPCHelpMan{method_name,
         "Bumps the fee of a transaction T, replacing it with a new transaction B.\n"
@@ -1483,7 +1483,7 @@ RPCHelpMan sendall()
             // Do not, ever, assume that it's fine to change the fee rate if the user has explicitly
             // provided one
             if (coin_control.m_feerate && fee_rate > *coin_control.m_feerate) {
-               throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("Fee rate (%s) is lower than the minimum fee rate setting (%s)", coin_control.m_feerate->ToString(FeeEstimateMode::SAT_VB), fee_rate.ToString(FeeEstimateMode::SAT_VB)));
+                throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("Fee rate (%s) is lower than the minimum fee rate setting (%s)", coin_control.m_feerate->ToString(FeeRateFormat::SAT_VB), fee_rate.ToString(FeeRateFormat::SAT_VB)));
             }
             if (fee_calc_out.reason == FeeReason::FALLBACK && !pwallet->m_allow_fallback_fee) {
                 // eventually allow a fallback fee

src/wallet/spend.cpp

@@ -1153,7 +1153,7 @@ static util::Result<CreatedTransactionResult> CreateTransactionInternal(
     // Do not, ever, assume that it's fine to change the fee rate if the user has explicitly
     // provided one
     if (coin_control.m_feerate && coin_selection_params.m_effective_feerate > *coin_control.m_feerate) {
-        return util::Error{strprintf(_("Fee rate (%s) is lower than the minimum fee rate setting (%s)"), coin_control.m_feerate->ToString(FeeEstimateMode::SAT_VB), coin_selection_params.m_effective_feerate.ToString(FeeEstimateMode::SAT_VB))};
+        return util::Error{strprintf(_("Fee rate (%s) is lower than the minimum fee rate setting (%s)"), coin_control.m_feerate->ToString(FeeRateFormat::SAT_VB), coin_selection_params.m_effective_feerate.ToString(FeeRateFormat::SAT_VB))};
     }
     if (feeCalc.reason == FeeReason::FALLBACK && !wallet.m_allow_fallback_fee) {
         // eventually allow a fallback fee