bitcoin-tx: Avoid treating integer overflow as OP_0 #23227

1. 
   MarcoFalke commented at 1:56 pm on October 8, 2021: member

   Seems odd to treat integer overflow as OP_0, so fix that.
2. doc: Fixup ToIntegral docs fad55e79ca
3. refactor: Use C++11 range based for loop in ParseScript fa03dec7e9
4. style: Fix whitespace in Parse* functions fa053c0019
5. MarcoFalke renamed this:
   bitcoin-tx: Avoid treating overflow as OP_0
   bitcoin-tx: Avoid treating integer overflow as OP_0
   on Oct 8, 2021
6. DrahtBot added the label Utils/log/libs on Oct 8, 2021
7. 
   practicalswift commented at 2:45 pm on October 8, 2021: contributor

   Concept ACK

   Nice to see LocaleIndependentAtoi be replaced by ToIntegral too.

8. 
   DrahtBot commented at 6:59 pm on October 8, 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:

   • #22875 (Fix Racy ParseOpCode function initialization by JeremyRubin)

   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.

9. 
   meshcollider commented at 0:25 am on October 9, 2021: contributor

   utACK fa0c2f92aa226c51b36220c2b31b029b6188f8aa
10. in src/core_read.cpp:73 in fa0c2f92aa outdated

    70+            const auto num{ToIntegral<int64_t>(w)};
    71 
    72             // limit the range of numbers ParseScript accepts in decimal
    73             // since numbers outside -0xFFFFFFFF...0xFFFFFFFF are illegal in scripts
    74-            if (n > int64_t{0xffffffff} || n < -1 * int64_t{0xffffffff}) {
    75+            if (!num || num > int64_t{0xffffffff} || num < -1 * int64_t{0xffffffff}) {
    

    ---

    ---

    

    theStack commented at 9:28 am on October 10, 2021:

    nit: personally I think the code is slightly more readable if at least the first condition doesn’t use implicit conversion from the std::optional (since we use auto above the type of num is not immediately visible)

    0            if (!num.has_value() || num > int64_t{0xffffffff} || num < -1 * int64_t{0xffffffff}) {
    

    ---

    

    MarcoFalke commented at 7:35 am on October 11, 2021:

    Thanks and done
11. theStack approved
12. 
    theStack commented at 9:35 am on October 10, 2021: member

    Concept and code-review ACK fa0c2f92aa226c51b36220c2b31b029b6188f8aa
13. bitcoin-tx: Avoid treating overflow as OP_0 fa43e7c2d9
14. MarcoFalke force-pushed on Oct 11, 2021
15. theStack approved
16. 
    theStack commented at 7:50 am on October 11, 2021: member

    re-ACK fa43e7c2d9dc5e2df70acd2019bdd24023c1d333
17. shaavan approved
18. 
    shaavan commented at 1:55 pm on October 11, 2021: contributor

    ACK fa43e7c2d9dc5e2df70acd2019bdd24023c1d333

    This PR’s primary focus is to avoid treating an overflow integer value as output 0.

    This is done by using the ToIntegral function that returns the integer value of the number if it is in the int_64 range; otherwise, it produces a nullopt. Followed by this, a check is applied to a returned value which checks if it has a valid value. And return an error in case if its not.

    This PR also deals with further minors changes, which are:

    • Appropriate additions to the comments in strencodings.h and lint-locale-dependence.sh files.
    • Using C++11 range based indicator in ParseScript function.
    • Fixing whitespaces in ParseScript function.

    I agree with the above changes, and I think they are necessary and valuable.

    I would like to suggest one addition, though. In case you have to update the PR for some major reason, how about also adding the test for negative -999….999 (i.e., huge number)

    0  { "exec": "./bitcoin-tx",
    1    "args": ["-create", "outscript=0:-999999999999999999999999999999"],
    2    "return_code": 1,
    3    "error_txt": "error: script parse error: decimal numeric value only allowed in the range -0xFFFFFFFF...0xFFFFFFFF",
    4    “description”: “Try to parse an output script with a decimal number below the allowed range.”
    5  },
    

19. 
    meshcollider commented at 2:31 am on October 12, 2021: contributor

    re-utACK fa43e7c2d9dc5e2df70acd2019bdd24023c1d333
20. meshcollider merged this on Oct 12, 2021
21. meshcollider closed this on Oct 12, 2021

    ---

22. MarcoFalke deleted the branch on Oct 12, 2021
23. sidhujag referenced this in commit abb82730e7 on Oct 12, 2021
24. DrahtBot locked this on Oct 30, 2022

Contributors
MarcoFalke practicalswift DrahtBot meshcollider theStack shaavan

Labels
Utils/log/libs