merge bitcoin#22220: make ParseMoney return a std::optional<CAmount>

src/bitcoin-tx.cpp

@@ -187,10 +187,11 @@ static void RegisterLoad(const std::string& strInput)
 
 static CAmount ExtractAndValidateValue(const std::string& strValue)
 {
-    CAmount value;
+    if (std::optional<CAmount> parsed = ParseMoney(strValue)) {
-    if (!ParseMoney(strValue, value))
+        return parsed.value();
+    } else {
         throw std::runtime_error("invalid TX output value");
-    return value;
+    }
 }
 
 static void MutateTxVersion(CMutableTransaction& tx, const std::string& cmdVal)

src/init.cpp

@@ -1431,10 +1431,11 @@ bool AppInitParameterInteraction(const ArgsManager& args)
     // incremental relay fee sets the minimum feerate increase necessary for BIP 125 replacement in the mempool
     // and the amount the mempool min fee increases above the feerate of txs evicted due to mempool limiting.
     if (args.IsArgSet("-incrementalrelayfee")) {
-        CAmount n = 0;
+        if (std::optional<CAmount> inc_relay_fee = ParseMoney(args.GetArg("-incrementalrelayfee", ""))) {
-        if (!ParseMoney(args.GetArg("-incrementalrelayfee", ""), n))
+            ::incrementalRelayFee = CFeeRate{inc_relay_fee.value()};
+        } else {
             return InitError(AmountErrMsg("incrementalrelayfee", args.GetArg("-incrementalrelayfee", "")));
-        incrementalRelayFee = CFeeRate(n);
+        }
     }
 
     // block pruning; get the amount of disk space (in MiB) to allot for block & undo files
@@ -1473,12 +1474,12 @@ bool AppInitParameterInteraction(const ArgsManager& args)
     }
 
     if (args.IsArgSet("-minrelaytxfee")) {
-        CAmount n = 0;
+        if (std::optional<CAmount> min_relay_fee = ParseMoney(args.GetArg("-minrelaytxfee", ""))) {
-        if (!ParseMoney(args.GetArg("-minrelaytxfee", ""), n)) {
+            // High fee check is done afterward in CWallet::Create()
+            ::minRelayTxFee = CFeeRate{min_relay_fee.value()};
+        } else {
             return InitError(AmountErrMsg("minrelaytxfee", args.GetArg("-minrelaytxfee", "")));
         }
-        // High fee check is done afterward in CWallet::CreateWalletFromFile()
-        ::minRelayTxFee = CFeeRate(n);
     } else if (incrementalRelayFee > ::minRelayTxFee) {
         // Allow only setting incrementalRelayFee to control both
         ::minRelayTxFee = incrementalRelayFee;
@@ -1488,18 +1489,19 @@ bool AppInitParameterInteraction(const ArgsManager& args)
     // Sanity check argument for min fee for including tx in block
     // TODO: Harmonize which arguments need sanity checking and where that happens
     if (args.IsArgSet("-blockmintxfee")) {
-        CAmount n = 0;
+        if (!ParseMoney(args.GetArg("-blockmintxfee", ""))) {
-        if (!ParseMoney(args.GetArg("-blockmintxfee", ""), n))
             return InitError(AmountErrMsg("blockmintxfee", args.GetArg("-blockmintxfee", "")));
+        }
     }
 
     // Feerate used to define dust.  Shouldn't be changed lightly as old
     // implementations may inadvertently create non-standard transactions
     if (args.IsArgSet("-dustrelayfee")) {
-        CAmount n = 0;
+        if (std::optional<CAmount> parsed = ParseMoney(args.GetArg("-dustrelayfee", ""))) {
-        if (!ParseMoney(args.GetArg("-dustrelayfee", ""), n))
+            dustRelayFee = CFeeRate{parsed.value()};
+        } else {
             return InitError(AmountErrMsg("dustrelayfee", args.GetArg("-dustrelayfee", "")));
-        dustRelayFee = CFeeRate(n);
+        }
     }
 
     fRequireStandard = !args.GetBoolArg("-acceptnonstdtxn", !chainparams.RequireStandard());

src/miner.cpp

@@ -81,11 +81,11 @@ static BlockAssembler::Options DefaultOptions()
     if (gArgs.IsArgSet("-blockmaxsize")) {
         options.nBlockMaxSize = gArgs.GetArg("-blockmaxsize", DEFAULT_BLOCK_MAX_SIZE);
     }
-    CAmount n = 0;
+    if (gArgs.IsArgSet("-blockmintxfee")) {
-    if (gArgs.IsArgSet("-blockmintxfee") && ParseMoney(gArgs.GetArg("-blockmintxfee", ""), n)) {
+        std::optional<CAmount> parsed = ParseMoney(gArgs.GetArg("-blockmintxfee", ""));
-        options.blockMinFeeRate = CFeeRate(n);
+        options.blockMinFeeRate = CFeeRate{parsed.value_or(DEFAULT_BLOCK_MIN_TX_FEE)};
     } else {
-        options.blockMinFeeRate = CFeeRate(DEFAULT_BLOCK_MIN_TX_FEE);
+        options.blockMinFeeRate = CFeeRate{DEFAULT_BLOCK_MIN_TX_FEE};
     }
     return options;
 }

src/test/fuzz/integer.cpp

@@ -78,9 +78,8 @@ FUZZ_TARGET_INIT(integer, initialize_integer)
     (void)FormatISO8601DateTime(i64);
     // FormatMoney(i) not defined when i == std::numeric_limits<int64_t>::min()
     if (i64 != std::numeric_limits<int64_t>::min()) {
-        int64_t parsed_money;
+        if (std::optional<CAmount> parsed = ParseMoney(FormatMoney(i64))) {
-        if (ParseMoney(FormatMoney(i64), parsed_money)) {
+            assert(parsed.value() == i64);
-            assert(parsed_money == i64);
         }
     }
     (void)GetSizeOfCompactSize(u64);
@@ -127,9 +126,8 @@ FUZZ_TARGET_INIT(integer, initialize_integer)
     (void)ToUpper(ch);
     // ValueFromAmount(i) not defined when i == std::numeric_limits<int64_t>::min()
     if (i64 != std::numeric_limits<int64_t>::min()) {
-        int64_t parsed_money;
+        if (std::optional<CAmount> parsed = ParseMoney(ValueFromAmount(i64).getValStr())) {
-        if (ParseMoney(ValueFromAmount(i64).getValStr(), parsed_money)) {
+            assert(parsed.value() == i64);
-            assert(parsed_money == i64);
         }
     }
     const std::chrono::seconds seconds{i64};

src/test/fuzz/parse_numbers.cpp

@@ -12,8 +12,7 @@ FUZZ_TARGET(parse_numbers)
 {
     const std::string random_string(buffer.begin(), buffer.end());
 
-    CAmount amount;
+    (void)ParseMoney(random_string);
-    (void)ParseMoney(random_string, amount);
 
     double d;
     (void)ParseDouble(random_string, &d);

src/test/util_tests.cpp

@@ -1056,86 +1056,59 @@ BOOST_AUTO_TEST_CASE(util_FormatMoney)
 
 BOOST_AUTO_TEST_CASE(util_ParseMoney)
 {
-    CAmount ret = 0;
+    BOOST_CHECK_EQUAL(ParseMoney("0.0").value(), 0);
-    BOOST_CHECK(ParseMoney("0.0", ret));
-    BOOST_CHECK_EQUAL(ret, 0);
 
-    BOOST_CHECK(ParseMoney("12345.6789", ret));
+    BOOST_CHECK_EQUAL(ParseMoney("12345.6789").value(), (COIN/10000)*123456789);
-    BOOST_CHECK_EQUAL(ret, (COIN/10000)*123456789);
 
-    BOOST_CHECK(ParseMoney("100000000.00", ret));
+    BOOST_CHECK_EQUAL(ParseMoney("10000000.00").value(), COIN*10000000);
-    BOOST_CHECK_EQUAL(ret, COIN*100000000);
+    BOOST_CHECK_EQUAL(ParseMoney("1000000.00").value(), COIN*1000000);
-    BOOST_CHECK(ParseMoney("10000000.00", ret));
+    BOOST_CHECK_EQUAL(ParseMoney("100000.00").value(), COIN*100000);
-    BOOST_CHECK_EQUAL(ret, COIN*10000000);
+    BOOST_CHECK_EQUAL(ParseMoney("10000.00").value(), COIN*10000);
-    BOOST_CHECK(ParseMoney("1000000.00", ret));
+    BOOST_CHECK_EQUAL(ParseMoney("1000.00").value(), COIN*1000);
-    BOOST_CHECK_EQUAL(ret, COIN*1000000);
+    BOOST_CHECK_EQUAL(ParseMoney("100.00").value(), COIN*100);
-    BOOST_CHECK(ParseMoney("100000.00", ret));
+    BOOST_CHECK_EQUAL(ParseMoney("10.00").value(), COIN*10);
-    BOOST_CHECK_EQUAL(ret, COIN*100000);
+    BOOST_CHECK_EQUAL(ParseMoney("1.00").value(), COIN);
-    BOOST_CHECK(ParseMoney("10000.00", ret));
+    BOOST_CHECK_EQUAL(ParseMoney("1").value(), COIN);
-    BOOST_CHECK_EQUAL(ret, COIN*10000);
+    BOOST_CHECK_EQUAL(ParseMoney("   1").value(), COIN);
-    BOOST_CHECK(ParseMoney("1000.00", ret));
+    BOOST_CHECK_EQUAL(ParseMoney("1   ").value(), COIN);
-    BOOST_CHECK_EQUAL(ret, COIN*1000);
+    BOOST_CHECK_EQUAL(ParseMoney("  1 ").value(), COIN);
-    BOOST_CHECK(ParseMoney("100.00", ret));
+    BOOST_CHECK_EQUAL(ParseMoney("0.1").value(), COIN/10);
-    BOOST_CHECK_EQUAL(ret, COIN*100);
+    BOOST_CHECK_EQUAL(ParseMoney("0.01").value(), COIN/100);
-    BOOST_CHECK(ParseMoney("10.00", ret));
+    BOOST_CHECK_EQUAL(ParseMoney("0.001").value(), COIN/1000);
-    BOOST_CHECK_EQUAL(ret, COIN*10);
+    BOOST_CHECK_EQUAL(ParseMoney("0.0001").value(), COIN/10000);
-    BOOST_CHECK(ParseMoney("1.00", ret));
+    BOOST_CHECK_EQUAL(ParseMoney("0.00001").value(), COIN/100000);
-    BOOST_CHECK_EQUAL(ret, COIN);
+    BOOST_CHECK_EQUAL(ParseMoney("0.000001").value(), COIN/1000000);
-    BOOST_CHECK(ParseMoney("1", ret));
+    BOOST_CHECK_EQUAL(ParseMoney("0.0000001").value(), COIN/10000000);
-    BOOST_CHECK_EQUAL(ret, COIN);
+    BOOST_CHECK_EQUAL(ParseMoney("0.00000001").value(), COIN/100000000);
-    BOOST_CHECK(ParseMoney("   1", ret));
+    BOOST_CHECK_EQUAL(ParseMoney(" 0.00000001 ").value(), COIN/100000000);
-    BOOST_CHECK_EQUAL(ret, COIN);
+    BOOST_CHECK_EQUAL(ParseMoney("0.00000001 ").value(), COIN/100000000);
-    BOOST_CHECK(ParseMoney("1   ", ret));
+    BOOST_CHECK_EQUAL(ParseMoney(" 0.00000001").value(), COIN/100000000);
-    BOOST_CHECK_EQUAL(ret, COIN);
-    BOOST_CHECK(ParseMoney("  1 ", ret));
-    BOOST_CHECK_EQUAL(ret, COIN);
-    BOOST_CHECK(ParseMoney("0.1", ret));
-    BOOST_CHECK_EQUAL(ret, COIN/10);
-    BOOST_CHECK(ParseMoney("0.01", ret));
-    BOOST_CHECK_EQUAL(ret, COIN/100);
-    BOOST_CHECK(ParseMoney("0.001", ret));
-    BOOST_CHECK_EQUAL(ret, COIN/1000);
-    BOOST_CHECK(ParseMoney("0.0001", ret));
-    BOOST_CHECK_EQUAL(ret, COIN/10000);
-    BOOST_CHECK(ParseMoney("0.00001", ret));
-    BOOST_CHECK_EQUAL(ret, COIN/100000);
-    BOOST_CHECK(ParseMoney("0.000001", ret));
-    BOOST_CHECK_EQUAL(ret, COIN/1000000);
-    BOOST_CHECK(ParseMoney("0.0000001", ret));
-    BOOST_CHECK_EQUAL(ret, COIN/10000000);
-    BOOST_CHECK(ParseMoney("0.00000001", ret));
-    BOOST_CHECK_EQUAL(ret, COIN/100000000);
-    BOOST_CHECK(ParseMoney(" 0.00000001 ", ret));
-    BOOST_CHECK_EQUAL(ret, COIN/100000000);
-    BOOST_CHECK(ParseMoney("0.00000001 ", ret));
-    BOOST_CHECK_EQUAL(ret, COIN/100000000);
-    BOOST_CHECK(ParseMoney(" 0.00000001", ret));
-    BOOST_CHECK_EQUAL(ret, COIN/100000000);
 
-    // Parsing amount that can not be represented in ret should fail
+    // Parsing amount that can not be represented should fail
-    BOOST_CHECK(!ParseMoney("0.000000001", ret));
+    BOOST_CHECK(!ParseMoney("100000000.00"));
+    BOOST_CHECK(!ParseMoney("0.000000001"));
 
     // Parsing empty string should fail
-    BOOST_CHECK(!ParseMoney("", ret));
+    BOOST_CHECK(!ParseMoney(""));
-    BOOST_CHECK(!ParseMoney(" ", ret));
+    BOOST_CHECK(!ParseMoney(" "));
-    BOOST_CHECK(!ParseMoney("  ", ret));
+    BOOST_CHECK(!ParseMoney("  "));
 
     // Parsing two numbers should fail
-    BOOST_CHECK(!ParseMoney("1 2", ret));
+    BOOST_CHECK(!ParseMoney("1 2"));
-    BOOST_CHECK(!ParseMoney(" 1 2 ", ret));
+    BOOST_CHECK(!ParseMoney(" 1 2 "));
-    BOOST_CHECK(!ParseMoney(" 1.2 3 ", ret));
+    BOOST_CHECK(!ParseMoney(" 1.2 3 "));
-    BOOST_CHECK(!ParseMoney(" 1 2.3 ", ret));
+    BOOST_CHECK(!ParseMoney(" 1 2.3 "));
 
     // Attempted 63 bit overflow should fail
-    BOOST_CHECK(!ParseMoney("92233720368.54775808", ret));
+    BOOST_CHECK(!ParseMoney("92233720368.54775808"));
 
     // Parsing negative amounts must fail
-    BOOST_CHECK(!ParseMoney("-1", ret));
+    BOOST_CHECK(!ParseMoney("-1"));
 
     // Parsing strings with embedded NUL characters should fail
-    BOOST_CHECK(!ParseMoney(std::string("\0-1", 3), ret));
+    BOOST_CHECK(!ParseMoney(std::string("\0-1", 3)));
-    BOOST_CHECK(!ParseMoney(STRING_WITH_EMBEDDED_NULL_CHAR, ret));
+    BOOST_CHECK(!ParseMoney(STRING_WITH_EMBEDDED_NULL_CHAR));
-    BOOST_CHECK(!ParseMoney(std::string("1\0", 2), ret));
+    BOOST_CHECK(!ParseMoney(std::string("1\0", 2)));
 }
 
 BOOST_AUTO_TEST_CASE(util_IsHex)

src/util/moneystr.cpp

@@ -5,10 +5,13 @@
 
 #include <util/moneystr.h>
 
+#include <amount.h>
 #include <tinyformat.h>
 #include <util/strencodings.h>
 #include <util/string.h>
 
+#include <optional>
+
 std::string FormatMoney(const CAmount& n)
 {
     // Note: not using straight sprintf here because we do NOT want
@@ -31,14 +34,14 @@ std::string FormatMoney(const CAmount& n)
 }
 
 
-bool ParseMoney(const std::string& money_string, CAmount& nRet)
+std::optional<CAmount> ParseMoney(const std::string& money_string)
 {
     if (!ValidAsCString(money_string)) {
-        return false;
+        return std::nullopt;
     }
     const std::string str = TrimString(money_string);
     if (str.empty()) {
-        return false;
+        return std::nullopt;
     }
 
     std::string strWhole;
@@ -58,21 +61,25 @@ bool ParseMoney(const std::string& money_string, CAmount& nRet)
             break;
         }
         if (IsSpace(*p))
-            return false;
+            return std::nullopt;
         if (!IsDigit(*p))
-            return false;
+            return std::nullopt;
         strWhole.insert(strWhole.end(), *p);
     }
     if (*p) {
-        return false;
+        return std::nullopt;
     }
     if (strWhole.size() > 10) // guard against 63 bit overflow
-        return false;
+        return std::nullopt;
     if (nUnits < 0 || nUnits > COIN)
-        return false;
+        return std::nullopt;
     int64_t nWhole = atoi64(strWhole);
-    CAmount nValue = nWhole*COIN + nUnits;
 
-    nRet = nValue;
+    CAmount value = nWhole * COIN + nUnits;
-    return true;
+
+    if (!MoneyRange(value)) {
+        return std::nullopt;
+    }
+
+    return value;
 }

src/util/moneystr.h

@@ -12,6 +12,7 @@
 #include <amount.h>
 #include <attributes.h>
 
+#include <optional>
 #include <string>
 
 /* Do not use these functions to represent or parse monetary amounts to or from
@@ -19,6 +20,6 @@
  */
 std::string FormatMoney(const CAmount& n);
 /** Parse an amount denoted in full coins. E.g. "0.0034" supplied on the command line. **/
-[[nodiscard]] bool ParseMoney(const std::string& str, CAmount& nRet);
+std::optional<CAmount> ParseMoney(const std::string& str);
 
 #endif // BITCOIN_UTIL_MONEYSTR_H

src/wallet/wallet.cpp

@@ -4402,55 +4402,53 @@ std::shared_ptr<CWallet> CWallet::Create(interfaces::Chain& chain, const std::st
     }
 
     if (gArgs.IsArgSet("-mintxfee")) {
-        CAmount n = 0;
+        std::optional<CAmount> min_tx_fee = ParseMoney(gArgs.GetArg("-mintxfee", ""));
-        if (!ParseMoney(gArgs.GetArg("-mintxfee", ""), n) || 0 == n) {
+        if (!min_tx_fee || min_tx_fee.value() == 0) {
             error = AmountErrMsg("mintxfee", gArgs.GetArg("-mintxfee", ""));
             return nullptr;
-        }
+        } else if (min_tx_fee.value() > HIGH_TX_FEE_PER_KB) {
-        if (n > HIGH_TX_FEE_PER_KB) {
             warnings.push_back(AmountHighWarn("-mintxfee") + Untranslated(" ") +
                               _("This is the minimum transaction fee you pay on every transaction."));
         }
-        walletInstance->m_min_fee = CFeeRate(n);
+
+        walletInstance->m_min_fee = CFeeRate{min_tx_fee.value()};
     }
 
     walletInstance->m_allow_fallback_fee = Params().IsTestChain();
     if (gArgs.IsArgSet("-fallbackfee")) {
-        CAmount nFeePerK = 0;
+        std::optional<CAmount> fallback_fee = ParseMoney(gArgs.GetArg("-fallbackfee", ""));
-        if (!ParseMoney(gArgs.GetArg("-fallbackfee", ""), nFeePerK)) {
+        if (!fallback_fee) {
             error = strprintf(_("Invalid amount for -fallbackfee=<amount>: '%s'"), gArgs.GetArg("-fallbackfee", ""));
             return nullptr;
-        }
+        } else if (fallback_fee.value() > HIGH_TX_FEE_PER_KB) {
-        if (nFeePerK > HIGH_TX_FEE_PER_KB) {
             warnings.push_back(AmountHighWarn("-fallbackfee") + Untranslated(" ") +
                               _("This is the transaction fee you may pay when fee estimates are not available."));
         }
-        walletInstance->m_fallback_fee = CFeeRate(nFeePerK);
+        walletInstance->m_fallback_fee = CFeeRate{fallback_fee.value()};
-        walletInstance->m_allow_fallback_fee = nFeePerK != 0; //disable fallback fee in case value was set to 0, enable if non-null value
+        walletInstance->m_allow_fallback_fee = walletInstance->m_fallback_fee.GetFeePerK() != 0; //disable fallback fee in case value was set to 0, enable if non-null value
     }
     if (gArgs.IsArgSet("-discardfee")) {
-        CAmount nFeePerK = 0;
+        std::optional<CAmount> discard_fee = ParseMoney(gArgs.GetArg("-discardfee", ""));
-        if (!ParseMoney(gArgs.GetArg("-discardfee", ""), nFeePerK)) {
+        if (!discard_fee) {
             error = strprintf(_("Invalid amount for -discardfee=<amount>: '%s'"), gArgs.GetArg("-discardfee", ""));
             return nullptr;
-        }
+        } else if (discard_fee.value() > HIGH_TX_FEE_PER_KB) {
-        if (nFeePerK > HIGH_TX_FEE_PER_KB) {
             warnings.push_back(AmountHighWarn("-discardfee") + Untranslated(" ") +
                               _("This is the transaction fee you may discard if change is smaller than dust at this level"));
         }
-        walletInstance->m_discard_rate = CFeeRate(nFeePerK);
+        walletInstance->m_discard_rate = CFeeRate{discard_fee.value()};
     }
+
     if (gArgs.IsArgSet("-paytxfee")) {
-        CAmount nFeePerK = 0;
+        std::optional<CAmount> pay_tx_fee = ParseMoney(gArgs.GetArg("-paytxfee", ""));
-        if (!ParseMoney(gArgs.GetArg("-paytxfee", ""), nFeePerK)) {
+        if (!pay_tx_fee) {
             error = AmountErrMsg("paytxfee", gArgs.GetArg("-paytxfee", ""));
             return nullptr;
-        }
+        } else if (pay_tx_fee.value() > HIGH_TX_FEE_PER_KB) {
-        if (nFeePerK > HIGH_TX_FEE_PER_KB) {
             warnings.push_back(AmountHighWarn("-paytxfee") + Untranslated(" ") +
                               _("This is the transaction fee you will pay if you send a transaction."));
         }
-        walletInstance->m_pay_tx_fee = CFeeRate(nFeePerK, 1000);
+        walletInstance->m_pay_tx_fee = CFeeRate{pay_tx_fee.value(), 1000};
         if (walletInstance->m_pay_tx_fee < chain.relayMinFee()) {
             error = strprintf(_("Invalid amount for -paytxfee=<amount>: '%s' (must be at least %s)"),
                 gArgs.GetArg("-paytxfee", ""), chain.relayMinFee().ToString());
@@ -4459,20 +4457,20 @@ std::shared_ptr<CWallet> CWallet::Create(interfaces::Chain& chain, const std::st
     }
 
     if (gArgs.IsArgSet("-maxtxfee")) {
-        CAmount nMaxFee = 0;
+        std::optional<CAmount> max_fee = ParseMoney(gArgs.GetArg("-maxtxfee", ""));
-        if (!ParseMoney(gArgs.GetArg("-maxtxfee", ""), nMaxFee)) {
+        if (!max_fee) {
             error = AmountErrMsg("maxtxfee", gArgs.GetArg("-maxtxfee", ""));
             return nullptr;
-        }
+        } else if (max_fee.value() > HIGH_MAX_TX_FEE) {
-        if (nMaxFee > HIGH_MAX_TX_FEE) {
             warnings.push_back(_("-maxtxfee is set very high! Fees this large could be paid on a single transaction."));
         }
-        if (CFeeRate(nMaxFee, 1000) < chain.relayMinFee()) {
+        if (CFeeRate{max_fee.value(), 1000} < chain.relayMinFee()) {
             error = strprintf(_("Invalid amount for -maxtxfee=<amount>: '%s' (must be at least the minrelay fee of %s to prevent stuck transactions)"),
                 gArgs.GetArg("-maxtxfee", ""), chain.relayMinFee().ToString());
             return nullptr;
         }
-        walletInstance->m_default_max_tx_fee = nMaxFee;
+
+        walletInstance->m_default_max_tx_fee = max_fee.value();
     }
 
     if (chain.relayMinFee().GetFeePerK() > HIGH_TX_FEE_PER_KB)