util: Fail to parse empty string in ParseMoney #18225

1. 
   MarcoFalke commented at 5:29 pm on February 28, 2020: member

   Supplying a fee rate or an amount on the command line as an empty string, which currently parses as 0 seems fragile and confusing. See for example the confusion in #18214.

   Fixes #18214

2. util: Remove unused ParseMoney that takes a c_str fab30b61eb
3. util: Fail to parse empty string in ParseMoney 8888461f68
4. 
   instagibbs commented at 5:42 pm on February 28, 2020: member

   was just about to suggest that… looking
5. 
   instagibbs commented at 5:43 pm on February 28, 2020: member

   utACK https://github.com/bitcoin/bitcoin/pull/18225/commits/8888461f6814ae8b6221b02049fb9e1f69a5ff70
6. DrahtBot added the label Tests on Feb 28, 2020
7. DrahtBot added the label Utils/log/libs on Feb 28, 2020
8. 
   Empact commented at 6:50 pm on February 28, 2020: member

   Code Review ACK https://github.com/bitcoin/bitcoin/pull/18225/commits/8888461f6814ae8b6221b02049fb9e1f69a5ff70
9. MarcoFalke removed the label Tests on Feb 28, 2020
10. 
    achow101 commented at 8:36 pm on February 28, 2020: member

    ACK 8888461f6814ae8b6221b02049fb9e1f69a5ff70
11. fanquake merged this on Feb 29, 2020
12. fanquake closed this on Feb 29, 2020

    ---

13. sidhujag referenced this in commit 2885532fc0 on Feb 29, 2020
14. MarcoFalke deleted the branch on Feb 29, 2020
15. 
    practicalswift commented at 9:08 pm on February 29, 2020: contributor

    Since leading spaces are stripped by ParseMoney(…) shouldn’t " " (one space), two spaces, three spaces, etc. fail in the same way as "" (empty string)?

    In other words if (TrimString(s, " ").empty()) { return false; } instead of if (s.empty()) { return false; }?

16. 
    luke-jr commented at 5:09 pm on March 4, 2020: member

    Agree with @practicalswift - this wasn’t complete enough.
17. 
    MarcoFalke commented at 5:17 pm on March 4, 2020: member

    Is it possible to pass in a string that consists only of spaces on the command line?
18. 
    instagibbs commented at 5:21 pm on March 4, 2020: member

    bitcoind -regtest "-fallbackfee= "

    harder to hit obviously but possible

19. 
    practicalswift commented at 5:23 pm on March 4, 2020: contributor

    @MarcoFalke It sure is but I’m not convinced that is even relevant: ParseMoney(…) doesn’t come with any documented precondition that input must come from the command line :)

    We should strive to write robust functions that gracefully and consistently handle all kinds of crazy and potentially malicious input, not functions that happen to work with the “happy path input” we currently feed them with. Robust is strictly better than fragile and future-proof is strictly better than at-the-moment-proof :)

    If the intention here was to only fix the command-line handling of -fallbackfee= then the emptiness check should not have been added to ParseMoney(…) but to the code at the call site, no? :)

20. luke-jr referenced this in commit 7a5228fdf1 on Mar 5, 2020
21. MarcoFalke referenced this in commit 0dc6218c79 on Mar 27, 2020
22. sidhujag referenced this in commit ffd945a4e4 on Mar 28, 2020
23. MarkLTZ referenced this in commit 3ec6a5c547 on Apr 9, 2020
24. sidhujag referenced this in commit a2cbd0de22 on Nov 10, 2020
25. Fabcien referenced this in commit 97abb2a88f on Jan 4, 2021
26. DrahtBot locked this on Feb 15, 2022

Contributors
MarcoFalke instagibbs Empact achow101 practicalswift luke-jr

Labels
Utils/log/libs