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

fa43e7c2d9dc5e2df70acd2019bdd24023c1d333 bitcoin-tx: Avoid treating overflow as OP_0 (MarcoFalke)
fa053c0019bc8b2174c485f4885f894f2b5de472 style: Fix whitespace in Parse* functions (MarcoFalke)
fa03dec7e98bdda8aa596ef7943cf0a8d0bcb127 refactor: Use C++11 range based for loop in ParseScript (MarcoFalke)
fad55e79ca18a5894a8da6db6309c323eecbb178 doc: Fixup ToIntegral docs (MarcoFalke)

Pull request description:

  Seems odd to treat integer overflow as `OP_0`, so fix that.

ACKs for top commit:
  theStack:
    re-ACK fa43e7c2d9dc5e2df70acd2019bdd24023c1d333
  shaavan:
    ACK fa43e7c2d9dc5e2df70acd2019bdd24023c1d333

Tree-SHA512: 1bbe2de62d853badc18d57d169c6e78ddcdff037e5a85357995dead11c8e67a4fe35087e08a181c60753f8ce91058b7fcc06f5b7901afedc78fbacea8bc3ef4f

3 files changed, 25 insertions, 34 deletions

diff --git a/src/core_read.cpp b/src/core_read.cpp
index 320811b9e9..2149b428d2 100644
--- a/src/core_read.cpp
+++ b/src/core_read.cpp
@@ -26,20 +26,20 @@ opcodetype ParseOpCode(const std::string& s)
 {
     static std::map<std::string, opcodetype> mapOpNames;
 
-    if (mapOpNames.empty())
-    {
-        for (unsigned int op = 0; op <= MAX_OPCODE; op++)
-        {
+    if (mapOpNames.empty()) {
+        for (unsigned int op = 0; op <= MAX_OPCODE; op++) {
             // Allow OP_RESERVED to get into mapOpNames
-            if (op < OP_NOP && op != OP_RESERVED)
+            if (op < OP_NOP && op != OP_RESERVED) {
                 continue;
+            }
 
             std::string strName = GetOpName(static_cast<opcodetype>(op));
-            if (strName == "OP_UNKNOWN")
+            if (strName == "OP_UNKNOWN") {
                 continue;
+            }
             mapOpNames[strName] = static_cast<opcodetype>(op);
             // Convenience: OP_ADD and just ADD are both recognized:
-            if (strName.compare(0, 3, "OP_") == 0) {  // strName starts with "OP_"
+            if (strName.compare(0, 3, "OP_") == 0) { // strName starts with "OP_"
                 mapOpNames[strName.substr(3)] = static_cast<opcodetype>(op);
             }
         }
@@ -59,44 +59,35 @@ CScript ParseScript(const std::string& s)
     std::vector<std::string> words;
     boost::algorithm::split(words, s, boost::algorithm::is_any_of(" \t\n"), boost::algorithm::token_compress_on);
 
-    for (std::vector<std::string>::const_iterator w = words.begin(); w != words.end(); ++w)
-    {
-        if (w->empty())
-        {
+    for (const std::string& w : words) {
+        if (w.empty()) {
             // Empty string, ignore. (boost::split given '' will return one word)
-        }
-        else if (std::all_of(w->begin(), w->end(), ::IsDigit) ||
-            (w->front() == '-' && w->size() > 1 && std::all_of(w->begin()+1, w->end(), ::IsDigit)))
+        } else if (std::all_of(w.begin(), w.end(), ::IsDigit) ||
+                   (w.front() == '-' && w.size() > 1 && std::all_of(w.begin() + 1, w.end(), ::IsDigit)))
         {
             // Number
-            int64_t n = LocaleIndependentAtoi<int64_t>(*w);
+            const auto num{ToIntegral<int64_t>(w)};
 
-            //limit the range of numbers ParseScript accepts in decimal
-            //since numbers outside -0xFFFFFFFF...0xFFFFFFFF are illegal in scripts
-            if (n > int64_t{0xffffffff} || n < -1 * int64_t{0xffffffff}) {
+            // limit the range of numbers ParseScript accepts in decimal
+            // since numbers outside -0xFFFFFFFF...0xFFFFFFFF are illegal in scripts
+            if (!num.has_value() || num > int64_t{0xffffffff} || num < -1 * int64_t{0xffffffff}) {
                 throw std::runtime_error("script parse error: decimal numeric value only allowed in the "
                                          "range -0xFFFFFFFF...0xFFFFFFFF");
             }
 
-            result << n;
-        }
-        else if (w->substr(0,2) == "0x" && w->size() > 2 && IsHex(std::string(w->begin()+2, w->end())))
-        {
+            result << num.value();
+        } else if (w.substr(0, 2) == "0x" && w.size() > 2 && IsHex(std::string(w.begin() + 2, w.end()))) {
             // Raw hex data, inserted NOT pushed onto stack:
-            std::vector<unsigned char> raw = ParseHex(std::string(w->begin()+2, w->end()));
+            std::vector<unsigned char> raw = ParseHex(std::string(w.begin() + 2, w.end()));
             result.insert(result.end(), raw.begin(), raw.end());
-        }
-        else if (w->size() >= 2 && w->front() == '\'' && w->back() == '\'')
-        {
+        } else if (w.size() >= 2 && w.front() == '\'' && w.back() == '\'') {
             // Single-quoted string, pushed as data. NOTE: this is poor-man's
             // parsing, spaces/tabs/newlines in single-quoted strings won't work.
-            std::vector<unsigned char> value(w->begin()+1, w->end()-1);
+            std::vector<unsigned char> value(w.begin() + 1, w.end() - 1);
             result << value;
-        }
-        else
-        {
+        } else {
             // opcode, e.g. OP_ADD or ADD:
-            result << ParseOpCode(*w);
+            result << ParseOpCode(w);
         }
     }
 
diff --git a/src/test/script_parse_tests.cpp b/src/test/script_parse_tests.cpp
index 5b8b6a725f..004c1a9a84 100644
--- a/src/test/script_parse_tests.cpp
+++ b/src/test/script_parse_tests.cpp
@@ -38,7 +38,6 @@ BOOST_AUTO_TEST_CASE(parse_script)
         {"'17'", "023137"},
         {"ELSE", "67"},
         {"NOP10", "b9"},
-        {"11111111111111111111", "00"},
     };
     std::string all_in;
     std::string all_out;
@@ -49,6 +48,7 @@ BOOST_AUTO_TEST_CASE(parse_script)
     }
     BOOST_CHECK_EQUAL(HexStr(ParseScript(all_in)), all_out);
 
+    BOOST_CHECK_EXCEPTION(ParseScript("11111111111111111111"), std::runtime_error, HasReason("script parse error: decimal numeric value only allowed in the range -0xFFFFFFFF...0xFFFFFFFF"));
     BOOST_CHECK_EXCEPTION(ParseScript("11111111111"), std::runtime_error, HasReason("script parse error: decimal numeric value only allowed in the range -0xFFFFFFFF...0xFFFFFFFF"));
     BOOST_CHECK_EXCEPTION(ParseScript("OP_CHECKSIGADD"), std::runtime_error, HasReason("script parse error: unknown opcode"));
 }
diff --git a/src/util/strencodings.h b/src/util/strencodings.h
index 1f7762aeef..eedb5ec2f8 100644
--- a/src/util/strencodings.h
+++ b/src/util/strencodings.h
@@ -72,7 +72,7 @@ void SplitHostPort(std::string in, uint16_t& portOut, std::string& hostOut);
 
 // LocaleIndependentAtoi is provided for backwards compatibility reasons.
 //
-// New code should use the ParseInt64/ParseUInt64/ParseInt32/ParseUInt32 functions
+// New code should use ToIntegral or the ParseInt* functions
 // which provide parse error feedback.
 //
 // The goal of LocaleIndependentAtoi is to replicate the exact defined behaviour
@@ -125,7 +125,7 @@ constexpr inline bool IsSpace(char c) noexcept {
 /**
  * Convert string to integral type T. Leading whitespace, a leading +, or any
  * trailing character fail the parsing. The required format expressed as regex
- * is `-?[0-9]+`.
+ * is `-?[0-9]+`. The minus sign is only permitted for signed integer types.
  *
  * @returns std::nullopt if the entire string could not be parsed, or if the
  *   parsed value is not in the range representable by the type T.