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

fanquake commented at 4:35 AM on June 11, 2021: member

Related discussion in #22193.

fanquake added the label Utils/log/libs on Jun 11, 2021

fanquake force-pushed on Jun 11, 2021

MarcoFalke commented at 7:57 AM on June 11, 2021: member

I was hoping to have both fixes in the same pull in different commits, since they require the reviewers to look up all call sites anyway:

std::optional
Check money range inside ParseMoney (#22193, #22044)

theStack commented at 8:52 AM on June 11, 2021: contributor

Concept ACK

kiminuo commented at 12:09 PM on June 11, 2021: contributor

Concept ACK

Just out of curiosity, I was interested how this changes perf numbers for ParseMoney. It's not important though because the function is not used in a repeated way anywhere. Anyway, a benchmark says:

| ns/op | op/s | err% | total | benchmark |--------------------:|--------------------:|--------:|----------:|:---------- | 1,200.00 | 833,333.33 | 1.7% | 0.00 | MoneyStrParseMoney | 1,388.89 | 720,000.00 | 1.8% | 0.00 | MoneyStrParseMoneyNew

(No guarrantees, I have tried this for the first time.)

DrahtBot commented at 3:15 PM on June 11, 2021: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#22766 (refactor: Clarify and disable unused ArgsManager flags by ryanofsky)
#22183 (Remove gArgs from wallet.h and wallet.cpp by kiminuo)
#22044 (Sanitize fee options by amadeuszpawlik)
#20452 (util: Replace use of locale dependent atoi(…) with locale-independent std::from_chars(…) (C++17) by practicalswift)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

laanwj commented at 3:38 PM on June 11, 2021: member

Concept ACK, I like this kind of API much better than passing around separate booleans or error codes.

DrahtBot cross-referenced this on Jun 11, 2021 from issue bitcoin-tx: validate range of parsed output amount by theStack

DrahtBot cross-referenced this on Jun 11, 2021 from issue Remove `gArgs` from `wallet.h` and `wallet.cpp` by kiminuo

DrahtBot cross-referenced this on Jun 11, 2021 from issue Sanitize fee options by amadeuszpawlik

practicalswift commented at 6:46 PM on June 11, 2021: contributor

Concept ACK

Non-blocking nit: Personally I think auto foo = [RHS]; is appropriate in contexts where the type of foo is immediately obvious from reading [RHS] (such as when [RHS] is std::make_unique<SomeType>(…), v.begin() or similar), or when the type is irrelevant. In the context of this PR I think std::optional<CAmount> parsed = ParseMoney(…); would be slightly more clear compared to the suggested auto parsed = ParseMoney(…);.

DrahtBot cross-referenced this on Jun 13, 2021 from issue util: Replace use of locale dependent atoi(…) with locale-independent std::from_chars(…) (C++17) by practicalswift

in src/init.cpp:918 in 81ceb56157 outdated

 913 | @@ -914,10 +914,12 @@ bool AppInitParameterInteraction(const ArgsManager& args)
 914 |      // incremental relay fee sets the minimum feerate increase necessary for BIP 125 replacement in the mempool
 915 |      // and the amount the mempool min fee increases above the feerate of txs evicted due to mempool limiting.
 916 |      if (args.IsArgSet("-incrementalrelayfee")) {
 917 | -        CAmount n = 0;
 918 | -        if (!ParseMoney(args.GetArg("-incrementalrelayfee", ""), n))
 919 | -            return InitError(AmountErrMsg("incrementalrelayfee", args.GetArg("-incrementalrelayfee", "")));
 920 | -        incrementalRelayFee = CFeeRate(n);
 921 | +        const auto arg = args.GetArg("-incrementalrelayfee", "");
 922 | +        if (auto inc_relay_fee = ParseMoney(arg)) {

laanwj commented at 11:01 AM on August 2, 2021:

TFW no rust-like unwrapping pattern matching if let Some(inc_relay_fee) = Parsemoney(arg) {

laanwj commented at 11:03 AM on August 2, 2021: member

In the context of this PR I think std::optional<CAmount> parsed = ParseMoney(…); would be slightly more clear compared to the suggested auto parsed = ParseMoney(…);.

I hate the verbosity but I do agree in this case, the return type is not obvious.

fanquake force-pushed on Aug 3, 2021

fanquake commented at 5:08 AM on August 3, 2021: member

Check money range inside ParseMoney

I've added that now.

I hate the verbosity but I do agree in this case, the return type is not obvious.

Changed to using std::optional<CAmount> over auto.

I've also rebased and removed some unrelated arg refactoring. Will PR that separately.

in src/init.cpp:919 in b3c0e67fa1 outdated

 914 | @@ -915,10 +915,11 @@ bool AppInitParameterInteraction(const ArgsManager& args)
 915 |      // incremental relay fee sets the minimum feerate increase necessary for BIP 125 replacement in the mempool
 916 |      // and the amount the mempool min fee increases above the feerate of txs evicted due to mempool limiting.
 917 |      if (args.IsArgSet("-incrementalrelayfee")) {
 918 | -        CAmount n = 0;
 919 | -        if (!ParseMoney(args.GetArg("-incrementalrelayfee", ""), n))
 920 | +        if (std::optional<CAmount> inc_relay_fee = ParseMoney(args.GetArg("-incrementalrelayfee", ""))) {
 921 | +            incrementalRelayFee = CFeeRate(inc_relay_fee.value());

MarcoFalke commented at 6:59 AM on August 3, 2021:

            ::incrementalRelayFee = CFeeRate{inc_relay_fee.value()};

in src/init.cpp:969 in b3c0e67fa1 outdated

 965 | @@ -965,18 +966,18 @@ bool AppInitParameterInteraction(const ArgsManager& args)
 966 |      // Sanity check argument for min fee for including tx in block
 967 |      // TODO: Harmonize which arguments need sanity checking and where that happens
 968 |      if (args.IsArgSet("-blockmintxfee")) {
 969 | -        CAmount n = 0;
 970 | -        if (!ParseMoney(args.GetArg("-blockmintxfee", ""), n))
 971 | +        if (!ParseMoney(args.GetArg("-blockmintxfee", "")))

MarcoFalke commented at 7:00 AM on August 3, 2021:

        if (!ParseMoney(args.GetArg("-blockmintxfee", ""))) {

in src/wallet/wallet.cpp:2606 in b3c0e67fa1 outdated

2607 | -            error = AmountErrMsg("mintxfee", gArgs.GetArg("-mintxfee", ""));
2608 | -            return nullptr;
2609 | -        }
2610 | -        if (n > HIGH_TX_FEE_PER_KB) {
2611 | +        if (std::optional<CAmount> min_tx_fee = ParseMoney(gArgs.GetArg("-mintxfee", ""))) {
2612 | +            if(min_tx_fee.value() == 0) {

MarcoFalke commented at 7:03 AM on August 3, 2021:

            if (min_tx_fee.value() == 0) {

MarcoFalke commented at 7:05 AM on August 3, 2021:

Also wouldn't it be easier to review to keep the min_tx_fee symbol outside the if and then keep the if conditions the same?

fee = Parse;
if (!fee || !*fee) {
 // same
}
// same

fanquake commented at 8:11 AM on August 3, 2021:

Possibly, going to leave for the minute, as I could change this again in a followup.

MarcoFalke commented at 7:08 AM on August 3, 2021: member

approach ACK, didn't review the src/wallet/wallet.cpp part yet

fanquake force-pushed on Aug 3, 2021

fanquake commented at 8:12 AM on August 3, 2021: member

approach ACK, didn't review the src/wallet/wallet.cpp part yet

Addressed suggestions so far.

in src/wallet/wallet.cpp:2650 in 6aeed8dd9c outdated

2666 | +            }
2667 | +
2668 | +            walletInstance->m_fallback_fee = CFeeRate(fallback_fee.value());
2669 | +
2670 | +            // Disable fallback fee in case value was set to 0, enable if non-null value
2671 | +            walletInstance->m_allow_fallback_fee = walletInstance->m_fallback_fee.GetFeePerK() != 0;

jnewbery commented at 12:34 PM on August 3, 2021:

Why is this allowed to move inside the if (gArgs.IsArgSet("-fallbackfee")) { code block? Prior to this change, walletInstance->m_fallback_fee would be set to the default (0), and walletInstance->m_allow_fallback_fee would therefore be flipped from its initialized value (true) to false.

fanquake commented at 3:23 AM on August 4, 2021:

Think this was just a mistake.

in src/wallet/wallet.cpp:2605 in 6aeed8dd9c outdated

2606 | -        if (!ParseMoney(gArgs.GetArg("-mintxfee", ""), n) || 0 == n) {
2607 | -            error = AmountErrMsg("mintxfee", gArgs.GetArg("-mintxfee", ""));
2608 | -            return nullptr;
2609 | -        }
2610 | -        if (n > HIGH_TX_FEE_PER_KB) {
2611 | +        if (std::optional<CAmount> min_tx_fee = ParseMoney(gArgs.GetArg("-mintxfee", ""))) {

jnewbery commented at 12:35 PM on August 3, 2021:

I think you could achieve the same, with a smaller diff and less code duplication as follows:

--- a/src/wallet/wallet.cpp
+++ b/src/wallet/wallet.cpp
@@ -2602,22 +2602,16 @@ std::shared_ptr<CWallet> CWallet::Create(interfaces::Chain* chain, const std::st
     }
 
     if (gArgs.IsArgSet("-mintxfee")) {
-        if (std::optional<CAmount> min_tx_fee = ParseMoney(gArgs.GetArg("-mintxfee", ""))) {
-            if (min_tx_fee.value() == 0) {
-                error = AmountErrMsg("mintxfee", gArgs.GetArg("-mintxfee", ""));
-                return nullptr;
-            }
-
-            if (min_tx_fee.value() > 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(min_tx_fee.value());
-        } else {
+        auto min_tx_fee = ParseMoney(gArgs.GetArg("-mintxfee", ""));
+        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) {
+            warnings.push_back(AmountHighWarn("-mintxfee") + Untranslated(" ") +
+                               _("This is the minimum transaction fee you pay on every transaction."));
         }
+
+        walletInstance->m_min_fee = CFeeRate(min_tx_fee.value());
     }
 
     if (gArgs.IsArgSet("-maxapsfee")) {

MarcoFalke commented at 12:46 PM on August 3, 2021:

Suggested the same in #22220 (review) ;)

fanquake commented at 3:53 AM on August 4, 2021:

Updated to take your suggestions.

fanquake force-pushed on Aug 4, 2021

fanquake cross-referenced this on Aug 4, 2021 from issue make ParseOutputType return a std::optional<OutputType> by fanquake

util: make ParseMoney return a std::optional<CAmount> 5ef2738089

util: check MoneyRange() inside ParseMoney() f7752adba5

in src/miner.cpp:76 in 974abfbe6d outdated

  77 | -    if (gArgs.IsArgSet("-blockmintxfee") && ParseMoney(gArgs.GetArg("-blockmintxfee", ""), n)) {
  78 | -        options.blockMinFeeRate = CFeeRate(n);
  79 | -    } else {
  80 | -        options.blockMinFeeRate = CFeeRate(DEFAULT_BLOCK_MIN_TX_FEE);
  81 | -    }
  82 | +    const CAmount min_tx_fee = ParseMoney(gArgs.GetArg("-blockmintxfee", "")).value_or(DEFAULT_BLOCK_MIN_TX_FEE);

jnewbery commented at 11:02 AM on August 4, 2021:

I don't think this improves clarity. Previously, ParseMoney() would only be called if -blockmintxfee is set. Now, we call ParseMoney() even if -blockmintxfee has not been set, and rely on the fact that it'll return a nullopt if we pass it an empty string.

fanquake commented at 11:48 AM on August 4, 2021:

Reverted to something more like the previous code.

fanquake force-pushed on Aug 4, 2021

Fabcien referenced this in commit 4f1a75b1aa on Aug 4, 2021

sidhujag referenced this in commit 7f55ebaabc on Aug 4, 2021

DrahtBot cross-referenced this on Aug 21, 2021 from issue refactor: Clarify and disable unused ArgsManager flags by ryanofsky

MarcoFalke commented at 11:47 AM on August 23, 2021: member

review ACK f7752adba5dd35fccd3f2144cfcf03538ebf275b 📄

<details><summary>Show signature and timestamp</summary>

Signature:

Timestamp of file with hash 78c354d0f3f17696d32ea10f11f0a9424812baff1f8ecdedfe5fcbc144129d53 -

</details>

fanquake merged this on Aug 24, 2021

fanquake closed this on Aug 24, 2021

fanquake deleted the branch on Aug 24, 2021

sidhujag referenced this in commit ba83c0e35f on Aug 24, 2021

glozow cross-referenced this on Aug 27, 2021 from issue wallet: Decide which coin selection solution to use based on waste metric by achow101

bitcoin locked this on Aug 24, 2022