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

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: member

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: member

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.

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(…);.

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 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:

0            ::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:

0        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:

0            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?

0fee = Parse;
1if (!fee || !*fee) {
2 // same
3}
4// 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:

 0--- a/src/wallet/wallet.cpp
 1+++ b/src/wallet/wallet.cpp
 2@@ -2602,22 +2602,16 @@ std::shared_ptr<CWallet> CWallet::Create(interfaces::Chain* chain, const std::st
 3     }
 4 
 5     if (gArgs.IsArgSet("-mintxfee")) {
 6-        if (std::optional<CAmount> min_tx_fee = ParseMoney(gArgs.GetArg("-mintxfee", ""))) {
 7-            if (min_tx_fee.value() == 0) {
 8-                error = AmountErrMsg("mintxfee", gArgs.GetArg("-mintxfee", ""));
 9-                return nullptr;
10-            }
11-
12-            if (min_tx_fee.value() > HIGH_TX_FEE_PER_KB) {
13-            warnings.push_back(AmountHighWarn("-mintxfee") + Untranslated(" ") +
14-                               _("This is the minimum transaction fee you pay on every transaction."));
15-            }
16-
17-            walletInstance->m_min_fee = CFeeRate(min_tx_fee.value());
18-        } else {
19+        auto min_tx_fee = ParseMoney(gArgs.GetArg("-mintxfee", ""));
20+        if (!min_tx_fee || min_tx_fee.value() == 0) {
21             error = AmountErrMsg("mintxfee", gArgs.GetArg("-mintxfee", ""));
22             return nullptr;
23+        } else if (min_tx_fee.value() > HIGH_TX_FEE_PER_KB) {
24+            warnings.push_back(AmountHighWarn("-mintxfee") + Untranslated(" ") +
25+                               _("This is the minimum transaction fee you pay on every transaction."));
26         }
27+
28+        walletInstance->m_min_fee = CFeeRate(min_tx_fee.value());
29     }
30 
31     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

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

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

review ACK f7752adba5dd35fccd3f2144cfcf03538ebf275b 📄

Signature:

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3review ACK f7752adba5dd35fccd3f2144cfcf03538ebf275b 📄
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
 7pUh9JgwApNfnr4XDam6JFxoS7iOSf7DQeVVVn0nExZiL2GHSj9w0cy83qgzgTGMb
 8pMxS4Qym8eonS05VJ1lhkFiGWmZ1dyNdxzHw9roxsFw6KXKyzV4SUG9RSKc01fzL
 96pNeJ4tC05HuYV6k6MjiI6lRetkWXBf6BYHQFA7QYW1CGi3FVpfavhmd3o5uXr+b
10pgb2sWxokm6Gnqs0yq2ZlR3D+hCDxPrgjpj1C0MJ1VC3R4raSwYxetyBrSBFJj/v
11oFOFOgVNOhFohA7r+xoehN8TWeoMvywGKxzLGjcGaQSc/HQSKVvtUOeke6mTJBze
12jEtSCQlrcxqd+kdVE/+j9NTesaZldMnp84dMEzfbc3dvSz/MThsT3nO/+eG26D9j
13wKeynSG82aLymQpH7OqcFho7+FpkqsGjYEwnnpUD5S8EwauQvGoY0Ab/cW2axYMQ
14aP/V9Q0PlMiA+rcm/Ut9QFZ4WHhl/eC2FZbUwFW8j6An3sYNRhEietOo+jdc2EN2
15cYVt+7du
16=cjvz
17-----END PGP SIGNATURE-----

Timestamp of file with hash 78c354d0f3f17696d32ea10f11f0a9424812baff1f8ecdedfe5fcbc144129d53 -

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

DrahtBot locked this on Aug 24, 2022

util: make ParseMoney return a std::optional #22220

Conflicts