Merge #6245: backport: bitcoin#18275, bitcoin#20220, bitcoin#20305, b…

…itcoin#20410, bitcoin#20426, bitcoin#20573, bitcoin#21083, bitcoin#21201, bitcoin#21786, bitcoin#21787 (fee backports)

d6946aa fix: offset fee for 1 duff in commission in wallet_basic.py due to missing bitcoin#22949 (Konstantin Akimov)
3ba99b9 Merge bitcoin#21786: wallet: ensure sat/vB feerates are in range (mantissa of 3) (MarcoFalke)
22435f1 Merge bitcoin#21787: test: fix off-by-ones in rpc_fundrawtransaction assertions (W. J. van der Laan)
ccac35c Merge bitcoin#21083: wallet: Avoid requesting fee rates multiple times during coin selection (Samuel Dobson)
9e9975f Merge bitcoin#21201: rpc: Disallow sendtoaddress and sendmany when private keys disabled (Samuel Dobson)
5ad8a48 Merge bitcoin#20573: wallet, bugfix: allow send with string fee_rate amounts (MarcoFalke)
db4a216 Merge bitcoin#20410: wallet: Do not treat default constructed types as None-type (MarcoFalke)
01e41aa Merge bitcoin#20426: wallet: allow zero-fee fundrawtransaction/walletcreatefundedpsbt and other fixes (MarcoFalke)
f436c20 Merge bitcoin#20305: wallet: introduce fee_rate sat/vB param/option (MarcoFalke)
0fa1922 Merge bitcoin#20220: wallet, rpc: explicit fee rate follow-ups/fixes for 0.21 (Samuel Dobson)
d530b73 Merge bitcoin#18275: wallet: error if an explicit fee rate was given but the needed fee rate differed (MarcoFalke)

Pull request description:

  ## Issue being fixed or feature implemented
  Just regular backports from bitcoin v0.21, v22; mostly wallet+fee related

  ## What was done?
   - bitcoin#18275
   - bitcoin#20220
   - bitcoin#20305
   - bitcoin#20426
   - bitcoin#20410
   - bitcoin#20573
   - bitcoin#21201
   - bitcoin#21083
   - bitcoin#21787

  ## How Has This Been Tested?
  Run unit and functional tests

  ## Breaking Changes
  Some wallet rpc (sendtoaddress, sendmany, send) have a new argument `fee_rate` which is inserted before `verbose`.
  Release notes will be provided in a new PR once scope of backports and fixes in this PR is finalized by merging it to develop/

  ## Checklist:
  - [x] I have performed a self-review of my own code
  - [x] I have commented my code, particularly in hard-to-understand areas
  - [ ] I have added or updated relevant unit/integration/functional/e2e tests
  - [ ] I have made corresponding changes to the documentation
  - [x] I have assigned this pull request to a milestone

ACKs for top commit:
  UdjinM6:
    utACK d6946aa
  PastaPastaPasta:
    utACK d6946aa

Tree-SHA512: 8826e1453fe84e3d21f789fab62c23ea13299ce13a7bf1132f70831c3255e823437b6ddd63f69a8e8a0dae95a2638a4454d727e91177b53a5d331872528b92e8

src/bench/coin_selection.cpp

@@ -48,7 +48,10 @@ static void CoinSelection(benchmark::Bench& bench)
         coins.emplace_back(wtx.get(), 0 /* iIn */, 6 * 24 /* nDepthIn */, true /* spendable */, true /* solvable */, true /* safe */);
     }
     const CoinEligibilityFilter filter_standard(1, 6, 0);
-    const CoinSelectionParams coin_selection_params(true, 34, 148, CFeeRate(0), 0, false);
+    const CoinSelectionParams coin_selection_params(/* use_bnb= */ true, /* change_output_size= */ 34,
+                                                    /* change_spend_size= */ 148, /* effective_feerate= */ CFeeRate(0),
+                                                    /* long_term_feerate= */ CFeeRate(0), /* discard_feerate= */ CFeeRate(0),
+                                                    /* tx_no_inputs_size= */ 0, /* avoid_partial= */ false);
     bench.run([&] {
         std::set<CInputCoin> setCoinsRet;
         CAmount nValueRet;

src/coinjoin/util.cpp

@@ -116,7 +116,7 @@ CTransactionBuilder::CTransactionBuilder(std::shared_ptr<CWallet> pwalletIn, con
     // Generate a feerate which will be used to consider if the remainder is dust and will go into fees or not
     coinControl.m_discard_feerate = ::GetDiscardRate(*pwallet);
     // Generate a feerate which will be used by calculations of this class and also by CWallet::CreateTransaction
-    coinControl.m_feerate = std::max(pwallet->chain().estimateSmartFee(int(pwallet->m_confirm_target), true, nullptr), pwallet->m_pay_tx_fee);
+    coinControl.m_feerate = std::max(GetRequiredFeeRate(*pwallet), pwallet->m_pay_tx_fee);
     // Change always goes back to origin
     coinControl.destChange = tallyItemIn.txdest;
     // Only allow tallyItems inputs for tx creation

src/policy/feerate.h

@@ -19,8 +19,8 @@ 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
-    DASH_KB,      //!< Use explicit DASH/kB fee given in coin control
-    DUFF_B,       //!< Use explicit duff/B fee given in coin control
+    DASH_KB,      //!< Use DASH/kB fee rate unit
+    DUFF_B,       //!< Use duff/B fee rate unit
 };
 
 /**
@@ -39,7 +39,12 @@ class CFeeRate
         // We've previously had bugs creep in from silent double->int conversion...
         static_assert(std::is_integral<I>::value, "CFeeRate should be used without floats");
     }
-    /** Constructor for a fee rate in satoshis per kB. The size in bytes must not exceed (2^63 - 1)*/
+    /** Constructor for a fee rate in satoshis per kB (duff/kB).
+     *
+     *  Passing a num_bytes value of COIN (1e8) returns a fee rate in satoshis per B (sat/B),
+     *  e.g. (nFeePaid * 1e8 / 1e3) == (nFeePaid / 1e5),
+     *  where 1e5 is the ratio to convert from DASH/kB to sat/B.
+     */
     CFeeRate(const CAmount& nFeePaid, uint32_t num_bytes);
     /**
      * Return the fee in satoshis for the given size in bytes.

src/rpc/client.cpp

@@ -44,7 +44,8 @@ static const CRPCConvertParam vRPCConvertParams[] =
     { "sendtoaddress", 6, "use_cj" },
     { "sendtoaddress", 7, "conf_target" },
     { "sendtoaddress", 9, "avoid_reuse" },
-    { "sendtoaddress", 10, "verbose"},
+    { "sendtoaddress", 10, "fee_rate"},
+    { "sendtoaddress", 11, "verbose"},
     { "settxfee", 0, "amount" },
     { "sethdseed", 0, "newkeypool" },
     { "getreceivedbyaddress", 1, "minconf" },
@@ -91,7 +92,8 @@ static const CRPCConvertParam vRPCConvertParams[] =
     { "sendmany", 6, "use_is" },
     { "sendmany", 7, "use_cj" },
     { "sendmany", 8, "conf_target" },
-    { "sendmany", 10, "verbose" },
+    { "sendmany", 10, "fee_rate" },
+    { "sendmany", 11, "verbose" },
     { "deriveaddresses", 1, "range" },
     { "scantxoutset", 1, "scanobjects" },
     { "addmultisigaddress", 0, "nrequired" },
@@ -152,7 +154,8 @@ static const CRPCConvertParam vRPCConvertParams[] =
     { "lockunspent", 1, "transactions" },
     { "send", 0, "outputs" },
     { "send", 1, "conf_target" },
-    { "send", 3, "options" },
+    { "send", 3, "fee_rate"},
+    { "send", 4, "options" },
     { "importprivkey", 2, "rescan" },
     { "importelectrumwallet", 1, "index" },
     { "importaddress", 2, "rescan" },

src/rpc/mining.cpp

@@ -1160,7 +1160,7 @@ static RPCHelpMan estimatesmartfee()
     if (!request.params[1].isNull()) {
         FeeEstimateMode fee_mode;
         if (!FeeModeFromString(request.params[1].get_str(), fee_mode)) {
-            throw JSONRPCError(RPC_INVALID_PARAMETER, "Invalid estimate_mode parameter");
+            throw JSONRPCError(RPC_INVALID_PARAMETER, InvalidEstimateModeErrorMessage());
         }
         if (fee_mode == FeeEstimateMode::ECONOMICAL) conservative = false;
     }

src/rpc/util.cpp

@@ -72,12 +72,12 @@ void RPCTypeCheckObj(const UniValue& o,
     }
 }
 
-CAmount AmountFromValue(const UniValue& value)
+CAmount AmountFromValue(const UniValue& value, int decimals)
 {
     if (!value.isNum() && !value.isStr())
         throw JSONRPCError(RPC_TYPE_ERROR, "Amount is not a number or string");
     CAmount amount;
-    if (!ParseFixedPoint(value.getValStr(), 8, &amount))
+    if (!ParseFixedPoint(value.getValStr(), decimals, &amount))
         throw JSONRPCError(RPC_TYPE_ERROR, "Invalid amount");
     if (!MoneyRange(amount))
         throw JSONRPCError(RPC_TYPE_ERROR, "Amount out of range");
@@ -319,11 +319,12 @@ UniValue DescribeAddress(const CTxDestination& dest)
 
 unsigned int ParseConfirmTarget(const UniValue& value, unsigned int max_target)
 {
-    int target = value.get_int();
-    if (target < 1 || (unsigned int)target > max_target) {
-        throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("Invalid conf_target, must be between %u - %u", 1, max_target));
+    const int target{value.get_int()};
+    const unsigned int unsigned_target{static_cast<unsigned int>(target)};
+    if (target < 1 || unsigned_target > max_target) {
+        throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("Invalid conf_target, must be between %u and %u", 1, max_target));
     }
-    return (unsigned int)target;
+    return unsigned_target;
 }
 
 /**

src/rpc/util.h

@@ -84,7 +84,14 @@ int64_t ParseInt64V(const UniValue& v, const std::string &strName);
 double ParseDoubleV(const UniValue& v, const std::string &strName);
 bool ParseBoolV(const UniValue& v, const std::string &strName);
 
-CAmount AmountFromValue(const UniValue& value);
+/**
+ * Validate and return a CAmount from a UniValue number or string.
+ *
+ * @param[in] value     UniValue number or string to parse.
+ * @param[in] decimals  Number of significant digits (default: 8).
+ * @returns a CAmount if the various checks pass.
+ */
+CAmount AmountFromValue(const UniValue& value, int decimals = 8);
 
 using RPCArgList = std::vector<std::pair<std::string, UniValue>>;
 std::string HelpExampleCli(const std::string& methodname, const std::string& args);

src/test/amount_tests.cpp

@@ -109,6 +109,8 @@ BOOST_AUTO_TEST_CASE(ToStringTest)
     CFeeRate feeRate;
     feeRate = CFeeRate(1);
     BOOST_CHECK_EQUAL(feeRate.ToString(), "0.00000001 DASH/kB");
+    BOOST_CHECK_EQUAL(feeRate.ToString(FeeEstimateMode::DASH_KB), "0.00000001 DASH/kB");
+    BOOST_CHECK_EQUAL(feeRate.ToString(FeeEstimateMode::DUFF_B), "0.001 duff/B");
 }
 
 BOOST_AUTO_TEST_SUITE_END()

src/test/util_tests.cpp

@@ -2044,6 +2044,15 @@ BOOST_AUTO_TEST_CASE(test_ParseFixedPoint)
     BOOST_CHECK(!ParseFixedPoint("1.1e", 8, &amount));
     BOOST_CHECK(!ParseFixedPoint("1.1e-", 8, &amount));
     BOOST_CHECK(!ParseFixedPoint("1.", 8, &amount));
+
+    // Test with 3 decimal places for fee rates in sat/vB.
+    BOOST_CHECK(ParseFixedPoint("0.001", 3, &amount));
+    BOOST_CHECK_EQUAL(amount, CAmount{1});
+    BOOST_CHECK(!ParseFixedPoint("0.0009", 3, &amount));
+    BOOST_CHECK(!ParseFixedPoint("31.00100001", 3, &amount));
+    BOOST_CHECK(!ParseFixedPoint("31.0011", 3, &amount));
+    BOOST_CHECK(!ParseFixedPoint("31.99999999", 3, &amount));
+    BOOST_CHECK(!ParseFixedPoint("31.999999999999999999999", 3, &amount));
 }
 
 static void TestOtherThread(fs::path dirname, std::string lockname, bool *result)

src/util/fees.cpp

@@ -40,8 +40,6 @@ const std::vector<std::pair<std::string, FeeEstimateMode>>& FeeModeMap()
         {"unset", FeeEstimateMode::UNSET},
         {"economical", FeeEstimateMode::ECONOMICAL},
         {"conservative", FeeEstimateMode::CONSERVATIVE},
-        {(CURRENCY_UNIT + "/kB"), FeeEstimateMode::DASH_KB},
-        {(CURRENCY_ATOM + "/B"), FeeEstimateMode::DUFF_B},
     };
     return FEE_MODES;
 }
@@ -51,6 +49,11 @@ std::string FeeModes(const std::string& delimiter)
     return Join(FeeModeMap(), delimiter, [&](const std::pair<std::string, FeeEstimateMode>& i) { return i.first; });
 }
 
+const std::string InvalidEstimateModeErrorMessage()
+{
+    return "Invalid estimate_mode parameter, must be one of: \"" + FeeModes("\", \"") + "\"";
+}
+
 bool FeeModeFromString(const std::string& mode_string, FeeEstimateMode& fee_estimate_mode)
 {
     auto searchkey = ToUpper(mode_string);

src/util/fees.h

@@ -13,5 +13,6 @@ enum class FeeReason;
 bool FeeModeFromString(const std::string& mode_string, FeeEstimateMode& fee_estimate_mode);
 std::string StringForFeeReason(FeeReason reason);
 std::string FeeModes(const std::string& delimiter);
+const std::string InvalidEstimateModeErrorMessage();
 
 #endif // BITCOIN_UTIL_FEES_H