refactor: fees: split fee rate format from fee estimate mode

 - Introduce a `FeeRateFormat` enum and change `CFeeRate::ToString()`
   to use it for `BTC/kvB` vs `sat/vB` output formatting.
 - Handle all enum values, hence remove default case in `CFeeRate::ToString()`
   and `assert(False)` when a `FeeRateFormat` value is not handled.
 - Keep `FeeEstimateMode` focused on fee estimation behavior by removing fee rate format
   values from `FeeEstimateMode`.
 - Update all formatting call sites and tests to pass `FeeRateFormat` explicitly, separating fee rate format
   from fee-estimation mode selection.

Author: ismaelsadeeq

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

@@ -18,6 +18,12 @@
 
 const std::string CURRENCY_UNIT = "BTC"; // One formatted unit
 const std::string CURRENCY_ATOM = "sat"; // One indivisible minimum value unit
+
+enum class FeeRateFormat {
+    BTC_KVB, //!< Use BTC/kvB fee rate unit
+    SAT_VB,  //!< Use sat/vB fee rate unit
+};
+
 /**
  * Fee rate in satoshis per virtualbyte: CAmount / vB
  * the feerate is represented internally as FeeFrac
@@ -66,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

@@ -10,8 +10,6 @@ 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
 };
 
 #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