util: Fail to parse whitespace-only strings in ParseMoney(...) (instead of parsing as zero) #18270

practicalswift · 2020-03-05T20:43:10Z

Fail to parse whitespace-only strings in ParseMoney(...) (instead of parsing as 0).

This is a follow-up to #18225 ("util: Fail to parse empty string in ParseMoney") which made ParseMoney("") fail instead of parsing as 0.

Context: #18225 (comment)

Current non-test call sites:

$ git grep ParseMoney ":(exclude)src/test/"
src/bitcoin-tx.cpp:    if (!ParseMoney(strValue, value))
src/init.cpp:        if (!ParseMoney(gArgs.GetArg("-incrementalrelayfee", ""), n))
src/init.cpp:        if (!ParseMoney(gArgs.GetArg("-minrelaytxfee", ""), n)) {
src/init.cpp:        if (!ParseMoney(gArgs.GetArg("-blockmintxfee", ""), n))
src/init.cpp:        if (!ParseMoney(gArgs.GetArg("-dustrelayfee", ""), n))
src/miner.cpp:    if (gArgs.IsArgSet("-blockmintxfee") && ParseMoney(gArgs.GetArg("-blockmintxfee", ""), n)) {
src/util/moneystr.cpp:bool ParseMoney(const std::string& str, CAmount& nRet)
src/util/moneystr.h:NODISCARD bool ParseMoney(const std::string& str, CAmount& nRet);
src/wallet/wallet.cpp:        if (!ParseMoney(gArgs.GetArg("-mintxfee", ""), n) || 0 == n) {
src/wallet/wallet.cpp:        if (!ParseMoney(gArgs.GetArg("-fallbackfee", ""), nFeePerK)) {
src/wallet/wallet.cpp:        if (!ParseMoney(gArgs.GetArg("-discardfee", ""), nFeePerK)) {
src/wallet/wallet.cpp:        if (!ParseMoney(gArgs.GetArg("-paytxfee", ""), nFeePerK)) {
src/wallet/wallet.cpp:        if (!ParseMoney(gArgs.GetArg("-maxtxfee", ""), nMaxFee)) {

luke-jr

utACK, though I prefer doing it another way

src/util/moneystr.cpp

maflcko

ACK

src/test/util_tests.cpp

maflcko · 2020-03-05T21:44:59Z

ACK 5a507f508b90e5e12800b9218dc54d52dcd38d05

laanwj · 2020-03-06T20:48:00Z

utACK, though I prefer doing it another way

Same, this seems something that's better handled in the parsing code itself than as a pre-check calling out to an external function. Still, it's a good catch.

Do we need to accept trailing and leading whitespace at all here?

practicalswift · 2020-03-07T14:41:47Z

Do we need to accept trailing and leading whitespace at all here?

I don't have any strong opinions as long as it is done consistently (in order to avoid any gotchas for the callers of the function).

Perhaps we can move forward with this PR as-is to get the current behaviour consistent, and then take the discussion of removal of existing logic in a follow-up issue/PR?

src/util/moneystr.cpp

maflcko · 2020-03-12T13:25:34Z

ACK 43e4f1249ae2a9a1a150637c2d1d5aa8f544f46e

practicalswift · 2020-03-12T13:41:47Z

@laanwj Updated. Does my implementation seem to be in line with your your suggestion? :)

@MarcoFalke Could you please re-review in light of the latest update? :)

… parsing as zero)

Empact · 2020-03-12T21:38:52Z

ACK 100213c

theStack

ACK 100213c

practicalswift · 2020-03-26T21:25:14Z

Ready for merge? :)

sipa · 2020-03-26T22:34:14Z

ACK 100213c

promag

TrimString drops \f\n\r\t\v, is this intended?

promag · 2020-03-26T23:12:42Z

src/util/moneystr.cpp

        }
        if (IsSpace(*p))
-            break;
+            return false;


nit, if (IsSpace(*p)) return false;

promag · 2020-03-26T23:13:55Z

src/util/moneystr.cpp

-    for (; *p; p++)
-        if (!IsSpace(*p))
-            return false;
+    if (*p) {


Move to L58, replace break; with if (*p) return false;.

practicalswift · 2020-03-27T16:01:01Z

TrimString drops \f\n\r\t\v, is this intended?

That is consistent with how the previously used IsSpace works, no?

…arseMoney(...) (instead of parsing as zero) 100213c util: Fail to parse space-only strings in ParseMoney(...) (instead of parsing as zero) (practicalswift) Pull request description: Fail to parse whitespace-only strings in `ParseMoney(...)` (instead of parsing as `0`). This is a follow-up to bitcoin#18225 ("util: Fail to parse empty string in `ParseMoney`") which made `ParseMoney("")` fail instead of parsing as `0`. Context: bitcoin#18225 (comment) Current non-test call sites: ``` $ git grep ParseMoney ":(exclude)src/test/" src/bitcoin-tx.cpp: if (!ParseMoney(strValue, value)) src/init.cpp: if (!ParseMoney(gArgs.GetArg("-incrementalrelayfee", ""), n)) src/init.cpp: if (!ParseMoney(gArgs.GetArg("-minrelaytxfee", ""), n)) { src/init.cpp: if (!ParseMoney(gArgs.GetArg("-blockmintxfee", ""), n)) src/init.cpp: if (!ParseMoney(gArgs.GetArg("-dustrelayfee", ""), n)) src/miner.cpp: if (gArgs.IsArgSet("-blockmintxfee") && ParseMoney(gArgs.GetArg("-blockmintxfee", ""), n)) { src/util/moneystr.cpp:bool ParseMoney(const std::string& str, CAmount& nRet) src/util/moneystr.h:NODISCARD bool ParseMoney(const std::string& str, CAmount& nRet); src/wallet/wallet.cpp: if (!ParseMoney(gArgs.GetArg("-mintxfee", ""), n) || 0 == n) { src/wallet/wallet.cpp: if (!ParseMoney(gArgs.GetArg("-fallbackfee", ""), nFeePerK)) { src/wallet/wallet.cpp: if (!ParseMoney(gArgs.GetArg("-discardfee", ""), nFeePerK)) { src/wallet/wallet.cpp: if (!ParseMoney(gArgs.GetArg("-paytxfee", ""), nFeePerK)) { src/wallet/wallet.cpp: if (!ParseMoney(gArgs.GetArg("-maxtxfee", ""), nMaxFee)) { ``` ACKs for top commit: Empact: ACK bitcoin@100213c sipa: ACK 100213c theStack: ACK bitcoin@100213c Tree-SHA512: cadfb1ac8276cf54736c3444705f2650e7a08023673aedc729fabe751ae80f6c490fc0945ee38dbfd02c95e4d9853d1e4c84f5d3c310f44eaf3585afec8a4c22

- rpc/wallet: initialize nFeeRequired to avoid using garbage value on failure bitcoin#18274 - httpserver: use own HTTP status codes bitcoin#18168 - tests: Add fuzzing harnesses for various Base{32,58,64} and hex related functions bitcoin#17229 - util: Don't allow Base32/64-decoding or ParseMoney(…) on strings with embedded NUL characters. Add tests. bitcoin#17753 - util: Fail to parse empty string in ParseMoney bitcoin#18225 - util: Fail to parse whitespace-only strings in ParseMoney(...) (instead of parsing as zero) bitcoin#18270 - Replace the LogPrint function with a macro bitcoin#17218 - Fix wallet unload race condition bitcoin#18338 - qt: Fix Window -> Minimize menu item bitcoin#18549 - windows: Enable heap terminate-on-corruption bitcoin#17916

… parsing as zero) Summary: This is a backport of Core [[bitcoin/bitcoin#18270 | PR18270]] Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, deadalnix Reviewed By: #bitcoin_abc, deadalnix Differential Revision: https://reviews.bitcoinabc.org/D8882

practicalswift force-pushed the parsemoney-followup branch from 9b7e757 to e83b03a Compare March 5, 2020 20:43

luke-jr reviewed Mar 5, 2020

View reviewed changes

src/util/moneystr.cpp Outdated Show resolved Hide resolved

maflcko approved these changes Mar 5, 2020

View reviewed changes

src/test/util_tests.cpp Outdated Show resolved Hide resolved

practicalswift force-pushed the parsemoney-followup branch from e83b03a to 5a507f5 Compare March 5, 2020 21:19

DrahtBot added Tests Utils/log/libs labels Mar 5, 2020

Empact reviewed Mar 7, 2020

View reviewed changes

src/util/moneystr.cpp Outdated Show resolved Hide resolved

laanwj reviewed Mar 12, 2020

View reviewed changes

src/util/moneystr.cpp Outdated Show resolved Hide resolved

practicalswift force-pushed the parsemoney-followup branch from 5a507f5 to 43e4f12 Compare March 12, 2020 13:14

practicalswift force-pushed the parsemoney-followup branch from 43e4f12 to 5a93e67 Compare March 12, 2020 13:33

practicalswift force-pushed the parsemoney-followup branch 2 times, most recently from e500e61 to 29f2e95 Compare March 12, 2020 14:11

util: Fail to parse space-only strings in ParseMoney(...) (instead of…

100213c

… parsing as zero)

practicalswift force-pushed the parsemoney-followup branch from 29f2e95 to 100213c Compare March 12, 2020 14:15

practicalswift changed the title ~~util: Fail to parse space-only strings in ParseMoney(...) (instead of parsing as zero)~~ util: Fail to parse whitespace-only strings in ParseMoney(...) (instead of parsing as zero) Mar 12, 2020

practicalswift force-pushed the parsemoney-followup branch from 5fac6b8 to 100213c Compare March 12, 2020 15:20

theStack approved these changes Mar 20, 2020

View reviewed changes

promag reviewed Mar 26, 2020

View reviewed changes

maflcko merged commit 0dc6218 into bitcoin:master Mar 27, 2020

MarkLTZ mentioned this pull request Apr 4, 2020

Bitcoin PR tracking litecoinz-core/litecoinz#14

Closed

practicalswift deleted the parsemoney-followup branch April 10, 2021 19:40

bitcoin locked as resolved and limited conversation to collaborators Aug 18, 2022

util: Fail to parse whitespace-only strings in ParseMoney(...) (instead of parsing as zero) #18270

util: Fail to parse whitespace-only strings in ParseMoney(...) (instead of parsing as zero) #18270

Uh oh!

Conversation

practicalswift commented Mar 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

luke-jr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

maflcko commented Mar 5, 2020

Uh oh!

laanwj commented Mar 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

practicalswift commented Mar 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

maflcko commented Mar 12, 2020

Uh oh!

practicalswift commented Mar 12, 2020

Uh oh!

Empact commented Mar 12, 2020

Uh oh!

theStack left a comment

Choose a reason for hiding this comment

Uh oh!

practicalswift commented Mar 26, 2020

Uh oh!

sipa commented Mar 26, 2020

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

promag Mar 26, 2020

Choose a reason for hiding this comment

Uh oh!

promag Mar 26, 2020

Choose a reason for hiding this comment

Uh oh!

practicalswift commented Mar 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

practicalswift commented Mar 5, 2020 •

edited

Loading

laanwj commented Mar 6, 2020 •

edited

Loading

practicalswift commented Mar 7, 2020 •

edited

Loading

practicalswift commented Mar 27, 2020 •

edited

Loading