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

pull practicalswift wants to merge 1 commits into bitcoin:master from practicalswift:parsemoney-followup changing 2 files +27 −9
  1. practicalswift commented at 8:43 PM on March 5, 2020: contributor

    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)) {
  2. practicalswift force-pushed on Mar 5, 2020
  3. in src/util/moneystr.cpp:40 in e83b03af3f outdated 
      36 | @@ -37,7 +37,7 @@ bool ParseMoney(const std::string& str, CAmount& nRet)
  37 |          return false;
  38 |      }
  39 |  
  40 | -    if (str.empty()) {
  41 | +    if (TrimString(str, " ").empty()) {
    luke-jr commented at 8:47 PM on March 5, 2020:

    Why not just check !p[0] below (after line 48)?

    practicalswift commented at 8:54 PM on March 5, 2020:

    TMTOWTDI :) I find the current formulation more idiomatic and clear, but I'll let others chime in and I'll happily change if your opinion is the consensus opinion :)

    Empact commented at 4:24 PM on March 7, 2020:

    Why not the default pattern, which covers other whitespace? (asking again because I think this was lost in the resolved thread above)

    practicalswift commented at 4:28 PM on March 7, 2020:

    Only spaces are handled in the existing code and I want to keep the changes to the minimum required to make the function consistent in its handling of spaces (which is the only whitespace considered).

  4. luke-jr commented at 8:48 PM on March 5, 2020: member

    utACK, though I prefer doing it another way

  5. in src/test/util_tests.cpp:1221 in e83b03af3f outdated 
    1215 |  
1216 |      // Parsing empty string should fail
1217 |      BOOST_CHECK(!ParseMoney("", ret));
1218 | +    BOOST_CHECK(!ParseMoney(" ", ret));
1219 | +    BOOST_CHECK(!ParseMoney("  ", ret));
1220 |
    MarcoFalke commented at 8:58 PM on March 5, 2020: 
    
   // Parsing two numbers should fail
   BOOST_CHECK(!ParseMoney("1 2", ret));
    practicalswift commented at 9:20 PM on March 5, 2020:

    Good idea! Tests added. Please re-review :)

  6. MarcoFalke approved
  7. MarcoFalke commented at 8:59 PM on March 5, 2020: member

    ACK

  8. practicalswift force-pushed on Mar 5, 2020
  9. MarcoFalke commented at 9:44 PM on March 5, 2020: member

    ACK 5a507f508b90e5e12800b9218dc54d52dcd38d05

  10. DrahtBot added the label Tests on Mar 5, 2020
  11. DrahtBot added the label Utils/log/libs on Mar 5, 2020
  12. laanwj commented at 8:48 PM on March 6, 2020: member

    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?

  13. practicalswift commented at 2:41 PM on March 7, 2020: contributor

    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?

  14. in src/util/moneystr.cpp:40 in 5a507f508b outdated 
      36 | @@ -37,7 +37,7 @@ bool ParseMoney(const std::string& str, CAmount& nRet)
  37 |          return false;
  38 |      }
  39 |  
  40 | -    if (str.empty()) {
    laanwj commented at 12:46 PM on March 12, 2020:

    OK, I would prefer to do a str = TrimString(str, " ") here, to at least avoid going over the spaces twice! (and keep the empty-check as it, after that)

    MarcoFalke commented at 1:27 PM on March 12, 2020:

    The signature needs to be changed to bool ParseMoney(std::string str, CAmount& nRet)

  15. practicalswift force-pushed on Mar 12, 2020
  16. MarcoFalke commented at 1:25 PM on March 12, 2020: member

    ACK 43e4f1249ae2a9a1a150637c2d1d5aa8f544f46e

  17. practicalswift force-pushed on Mar 12, 2020
  18. practicalswift commented at 1:41 PM on March 12, 2020: contributor

    @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? :)

  19. practicalswift force-pushed on Mar 12, 2020
  20. practicalswift force-pushed on Mar 12, 2020
  21. util: Fail to parse space-only strings in ParseMoney(...) (instead of parsing as zero) 100213c5c2
  22. practicalswift force-pushed on Mar 12, 2020
  23. practicalswift renamed this:
    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)
    on Mar 12, 2020
  24. practicalswift force-pushed on Mar 12, 2020
  26. theStack approved
  28. practicalswift commented at 9:25 PM on March 26, 2020: contributor

    Ready for merge? :)

  29. sipa commented at 10:34 PM on March 26, 2020: member

    ACK 100213c5c29ebd7bd50aa885e54594ae10bf87a4

  30. in src/util/moneystr.cpp:61 in 100213c5c2 
      57 | @@ -60,14 +58,14 @@ bool ParseMoney(const std::string& str, CAmount& nRet)
  58 |              break;
  59 |          }
  60 |          if (IsSpace(*p))
  61 | -            break;
  62 | +            return false;
    promag commented at 11:12 PM on March 26, 2020:

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

  31. in src/util/moneystr.cpp:66 in 100213c5c2 
      65 |          strWhole.insert(strWhole.end(), *p);
  66 |      }
  67 | -    for (; *p; p++)
  68 | -        if (!IsSpace(*p))
  69 | -            return false;
  70 | +    if (*p) {
    promag commented at 11:13 PM on March 26, 2020:

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

  32. promag commented at 11:17 PM on March 26, 2020: member

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

  33. MarcoFalke merged this on Mar 27, 2020
  34. MarcoFalke closed this on Mar 27, 2020
  35. practicalswift commented at 4:01 PM on March 27, 2020: contributor

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

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

  36. sidhujag referenced this in commit ffd945a4e4 on Mar 28, 2020
  37. MarkLTZ referenced this in commit 3ec6a5c547 on Apr 9, 2020
  38. Fabcien referenced this in commit 6a46b54112 on Jan 12, 2021
  39. practicalswift deleted the branch on Apr 10, 2021
  40. DrahtBot locked this on Aug 18, 2022
Contributors
practicalswiftluke-jrEmpactMarcoFalkelaanwjtheStacksipapromag
Labels
TestsUtils/log/libs
Linked (view graph)
#18225 util: Fail to parse empty string in ParseMoney
github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-04-13 15:14 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me