Merge bitcoin/bitcoin#32520: Remove legacy Parse(U)Int*

faf55fc doc: Remove ParseInt mentions in documentation (MarcoFalke)
3333282 refactor: Remove unused Parse(U)Int* (MarcoFalke)
fa84e6c bitcoin-tx: Reject + sign in MutateTxDel* (MarcoFalke)
face251 bitcoin-tx: Reject + sign in vout parsing (MarcoFalke)
fa8acaf bitcoin-tx: Reject + sign in replaceable parsing (MarcoFalke)
faff25a bitcoin-tx: Reject + sign in locktime (MarcoFalke)
dddd9e5 bitcoin-tx: Reject + sign in nversion parsing (MarcoFalke)
fab06ac rest: Use SAFE_CHARS_URI in SanitizeString error msg (MarcoFalke)
8888bb4 rest: Reject + sign in /blockhashbyheight/ (MarcoFalke)
fafd43c test: Reject + sign when parsing regtest deployment params (MarcoFalke)
fa123af Reject + sign when checking -ipcfd (MarcoFalke)
fa47985 Reject + sign in SplitHostPort (MarcoFalke)
fab4c29 net: Reject + sign when parsing subnet mask (MarcoFalke)
fa89652 init: Reject + sign in -*port parsing (MarcoFalke)
fa9c455 cli: Reject + sign in -netinfo level parsing (MarcoFalke)
fa98041 refactor: Use ToIntegral in CreateFromDump (MarcoFalke)
fa23ed7 refactor: Use ToIntegral in ParseHDKeypath (MarcoFalke)

Pull request description:

  The legacy int parsing is problematic, because it accepts the `+` sign for unsigned integers. In all cases this is either:

  * Useless, because the `+` sign was already rejected.
  * Erroneous and inconsistent, when third party parsers reject it. (C.f. bitcoin/bitcoin#32365)
  * Confusing, because the `+` sign is  neither documented, nor can it be assumed to be present.

  Fix all issues by removing the legacy int parsing.

ACKs for top commit:
  stickies-v:
    re-ACK faf55fc
  brunoerg:
    code review ACK faf55fc

Tree-SHA512: a311ab6a58fe02a37741c1800feb3dcfad92377b4bfb61b433b2393f52ba89ef45d00940972b2767b213a3dd7b59e5e35d5b659c586eacdfe4e565a77b12b19f

Author: fanquake

doc/developer-notes.md

@@ -1009,10 +1009,6 @@ Strings and formatting
     buffer overflows, and surprises with `\0` characters. Also, some C string manipulations
     tend to act differently depending on platform, or even the user locale.
 
-- Use `ToIntegral` from [`strencodings.h`](/src/util/strencodings.h) for number parsing. In legacy code you might also find `ParseInt*` family of functions, `ParseDouble` or `LocaleIndependentAtoi`.
-
-  - *Rationale*: These functions do overflow checking and avoid pesky locale issues.
-
 - For `strprintf`, `LogInfo`, `LogDebug`, etc formatting characters don't need size specifiers.
 
   - *Rationale*: Bitcoin Core uses tinyformat, which is type safe. Leave them out to avoid confusion.

src/bitcoin-cli.cpp

@@ -1,5 +1,5 @@
 // Copyright (c) 2009-2010 Satoshi Nakamoto
-// Copyright (c) 2009-2022 The Bitcoin Core developers
+// Copyright (c) 2009-present The Bitcoin Core developers
 // Distributed under the MIT software license, see the accompanying
 // file COPYING or http://www.opensource.org/licenses/mit-license.php.
 
@@ -472,7 +472,8 @@ class NetinfoRequestHandler : public BaseRequestHandler
     {
         if (!args.empty()) {
             uint8_t n{0};
-            if (ParseUInt8(args.at(0), &n)) {
+            if (const auto res{ToIntegral<uint8_t>(args.at(0))}) {
+                n = *res;
                 m_details_level = std::min(n, NETINFO_MAX_LEVEL);
             } else {
                 throw std::runtime_error(strprintf("invalid -netinfo level argument: %s\nFor more information, run: bitcoin-cli -netinfo help", args.at(0)));

src/bitcoin-tx.cpp

@@ -211,35 +211,33 @@ static CAmount ExtractAndValidateValue(const std::string& strValue)
 
 static void MutateTxVersion(CMutableTransaction& tx, const std::string& cmdVal)
 {
-    uint32_t newVersion;
-    if (!ParseUInt32(cmdVal, &newVersion) || newVersion < 1 || newVersion > TX_MAX_STANDARD_VERSION) {
+    const auto ver{ToIntegral<uint32_t>(cmdVal)};
+    if (!ver || *ver < 1 || *ver > TX_MAX_STANDARD_VERSION) {
         throw std::runtime_error("Invalid TX version requested: '" + cmdVal + "'");
     }
-
-    tx.version = newVersion;
+    tx.version = *ver;
 }
 
 static void MutateTxLocktime(CMutableTransaction& tx, const std::string& cmdVal)
 {
-    int64_t newLocktime;
-    if (!ParseInt64(cmdVal, &newLocktime) || newLocktime < 0LL || newLocktime > 0xffffffffLL)
+    const auto locktime{ToIntegral<uint32_t>(cmdVal)};
+    if (!locktime) {
         throw std::runtime_error("Invalid TX locktime requested: '" + cmdVal + "'");
-
-    tx.nLockTime = (unsigned int) newLocktime;
+    }
+    tx.nLockTime = *locktime;
 }
 
 static void MutateTxRBFOptIn(CMutableTransaction& tx, const std::string& strInIdx)
 {
-    // parse requested index
-    int64_t inIdx = -1;
-    if (strInIdx != "" && (!ParseInt64(strInIdx, &inIdx) || inIdx < 0 || inIdx >= static_cast<int64_t>(tx.vin.size()))) {
+    const auto idx{ToIntegral<uint32_t>(strInIdx)};
+    if (strInIdx != "" && (!idx || *idx >= tx.vin.size())) {
         throw std::runtime_error("Invalid TX input index '" + strInIdx + "'");
     }
 
     // set the nSequence to MAX_INT - 2 (= RBF opt in flag)
-    int cnt = 0;
+    uint32_t cnt{0};
     for (CTxIn& txin : tx.vin) {
-        if (strInIdx == "" || cnt == inIdx) {
+        if (strInIdx == "" || cnt == *idx) {
             if (txin.nSequence > MAX_BIP125_RBF_SEQUENCE) {
                 txin.nSequence = MAX_BIP125_RBF_SEQUENCE;
             }
@@ -277,9 +275,10 @@ static void MutateTxAddInput(CMutableTransaction& tx, const std::string& strInpu
 
     // extract and validate vout
     const std::string& strVout = vStrInputParts[1];
-    int64_t vout;
-    if (!ParseInt64(strVout, &vout) || vout < 0 || vout > static_cast<int64_t>(maxVout))
+    const auto vout{ToIntegral<uint32_t>(strVout)};
+    if (!vout || *vout > maxVout) {
         throw std::runtime_error("invalid TX input vout '" + strVout + "'");
+    }
 
     // extract the optional sequence number
     uint32_t nSequenceIn = CTxIn::SEQUENCE_FINAL;
@@ -288,7 +287,7 @@ static void MutateTxAddInput(CMutableTransaction& tx, const std::string& strInpu
     }
 
     // append to transaction input list
-    CTxIn txin(*txid, vout, CScript(), nSequenceIn);
+    CTxIn txin{*txid, *vout, CScript{}, nSequenceIn};
     tx.vin.push_back(txin);
 }
 
@@ -508,26 +507,20 @@ static void MutateTxAddOutScript(CMutableTransaction& tx, const std::string& str
 
 static void MutateTxDelInput(CMutableTransaction& tx, const std::string& strInIdx)
 {
-    // parse requested deletion index
-    int64_t inIdx;
-    if (!ParseInt64(strInIdx, &inIdx) || inIdx < 0 || inIdx >= static_cast<int64_t>(tx.vin.size())) {
+    const auto idx{ToIntegral<uint32_t>(strInIdx)};
+    if (!idx || idx >= tx.vin.size()) {
         throw std::runtime_error("Invalid TX input index '" + strInIdx + "'");
     }
-
-    // delete input from transaction
-    tx.vin.erase(tx.vin.begin() + inIdx);
+    tx.vin.erase(tx.vin.begin() + *idx);
 }
 
 static void MutateTxDelOutput(CMutableTransaction& tx, const std::string& strOutIdx)
 {
-    // parse requested deletion index
-    int64_t outIdx;
-    if (!ParseInt64(strOutIdx, &outIdx) || outIdx < 0 || outIdx >= static_cast<int64_t>(tx.vout.size())) {
+    const auto idx{ToIntegral<uint32_t>(strOutIdx)};
+    if (!idx || idx >= tx.vout.size()) {
         throw std::runtime_error("Invalid TX output index '" + strOutIdx + "'");
     }
-
-    // delete output from transaction
-    tx.vout.erase(tx.vout.begin() + outIdx);
+    tx.vout.erase(tx.vout.begin() + *idx);
 }
 
 static const unsigned int N_SIGHASH_OPTS = 7;

src/chainparams.cpp

@@ -53,14 +53,14 @@ void ReadRegTestArgs(const ArgsManager& args, CChainParams::RegTestOptions& opti
         }
 
         const auto value{arg.substr(found + 1)};
-        int32_t height;
-        if (!ParseInt32(value, &height) || height < 0 || height >= std::numeric_limits<int>::max()) {
+        const auto height{ToIntegral<int32_t>(value)};
+        if (!height || *height < 0 || *height >= std::numeric_limits<int>::max()) {
             throw std::runtime_error(strprintf("Invalid height value (%s) for -testactivationheight=name@height.", arg));
         }
 
         const auto deployment_name{arg.substr(0, found)};
         if (const auto buried_deployment = GetBuriedDeployment(deployment_name)) {
-            options.activation_heights[*buried_deployment] = height;
+            options.activation_heights[*buried_deployment] = *height;
         } else {
             throw std::runtime_error(strprintf("Invalid name (%s) for -testactivationheight=name@height.", arg));
         }
@@ -72,16 +72,22 @@ void ReadRegTestArgs(const ArgsManager& args, CChainParams::RegTestOptions& opti
             throw std::runtime_error("Version bits parameters malformed, expecting deployment:start:end[:min_activation_height]");
         }
         CChainParams::VersionBitsParameters vbparams{};
-        if (!ParseInt64(vDeploymentParams[1], &vbparams.start_time)) {
+        const auto start_time{ToIntegral<int64_t>(vDeploymentParams[1])};
+        if (!start_time) {
             throw std::runtime_error(strprintf("Invalid nStartTime (%s)", vDeploymentParams[1]));
         }
-        if (!ParseInt64(vDeploymentParams[2], &vbparams.timeout)) {
+        vbparams.start_time = *start_time;
+        const auto timeout{ToIntegral<int64_t>(vDeploymentParams[2])};
+        if (!timeout) {
             throw std::runtime_error(strprintf("Invalid nTimeout (%s)", vDeploymentParams[2]));
         }
+        vbparams.timeout = *timeout;
         if (vDeploymentParams.size() >= 4) {
-            if (!ParseInt32(vDeploymentParams[3], &vbparams.min_activation_height)) {
+            const auto min_activation_height{ToIntegral<int64_t>(vDeploymentParams[3])};
+            if (!min_activation_height) {
                 throw std::runtime_error(strprintf("Invalid min_activation_height (%s)", vDeploymentParams[3]));
             }
+            vbparams.min_activation_height = *min_activation_height;
         } else {
             vbparams.min_activation_height = 0;
         }

src/init.cpp

@@ -1151,8 +1151,8 @@ bool CheckHostPortOptions(const ArgsManager& args) {
         "-rpcport",
     }) {
         if (const auto port{args.GetArg(port_option)}) {
-            uint16_t n;
-            if (!ParseUInt16(*port, &n) || n == 0) {
+            const auto n{ToIntegral<uint16_t>(*port)};
+            if (!n || *n == 0) {
                 return InitError(InvalidPortErrMsg(port_option, *port));
             }
         }

src/ipc/process.cpp

@@ -55,9 +55,11 @@ class ProcessImpl : public Process
         // in combination with other arguments because the parent process
         // should be able to control the child process through the IPC protocol
         // without passing information out of band.
-        if (!ParseInt32(argv[2], &fd)) {
+        const auto maybe_fd{ToIntegral<int32_t>(argv[2])};
+        if (!maybe_fd) {
             throw std::runtime_error(strprintf("Invalid -ipcfd number '%s'", argv[2]));
         }
+        fd = *maybe_fd;
         return true;
     }
     int connect(const fs::path& data_dir,

src/netbase.cpp

@@ -829,10 +829,9 @@ CSubNet LookupSubNet(const std::string& subnet_str)
         addr = static_cast<CNetAddr>(MaybeFlipIPv6toCJDNS(CService{addr.value(), /*port=*/0}));
         if (slash_pos != subnet_str.npos) {
             const std::string netmask_str{subnet_str.substr(slash_pos + 1)};
-            uint8_t netmask;
-            if (ParseUInt8(netmask_str, &netmask)) {
+            if (const auto netmask{ToIntegral<uint8_t>(netmask_str)}) {
                 // Valid number; assume CIDR variable-length subnet masking.
-                subnet = CSubNet{addr.value(), netmask};
+                subnet = CSubNet{addr.value(), *netmask};
             } else {
                 // Invalid number; try full netmask syntax. Never allow lookup for netmask.
                 const std::optional<CNetAddr> full_netmask{LookupHost(netmask_str, /*fAllowLookup=*/false)};

src/rest.cpp

@@ -962,9 +962,9 @@ static bool rest_blockhash_by_height(const std::any& context, HTTPRequest* req,
     std::string height_str;
     const RESTResponseFormat rf = ParseDataFormat(height_str, str_uri_part);
 
-    int32_t blockheight = -1; // Initialization done only to prevent valgrind false positive, see https://github.com/bitcoin/bitcoin/pull/18785
-    if (!ParseInt32(height_str, &blockheight) || blockheight < 0) {
-        return RESTERR(req, HTTP_BAD_REQUEST, "Invalid height: " + SanitizeString(height_str));
+    const auto blockheight{ToIntegral<int32_t>(height_str)};
+    if (!blockheight || *blockheight < 0) {
+        return RESTERR(req, HTTP_BAD_REQUEST, "Invalid height: " + SanitizeString(height_str, SAFE_CHARS_URI));
     }
 
     CBlockIndex* pblockindex = nullptr;
@@ -974,10 +974,10 @@ static bool rest_blockhash_by_height(const std::any& context, HTTPRequest* req,
         ChainstateManager& chainman = *maybe_chainman;
         LOCK(cs_main);
         const CChain& active_chain = chainman.ActiveChain();
-        if (blockheight > active_chain.Height()) {
+        if (*blockheight > active_chain.Height()) {
             return RESTERR(req, HTTP_NOT_FOUND, "Block height out of range");
         }
-        pblockindex = active_chain[blockheight];
+        pblockindex = active_chain[*blockheight];
     }
     switch (rf) {
     case RESTResponseFormat::BINARY: {

src/test/fuzz/locale.cpp

@@ -46,35 +46,15 @@ FUZZ_TARGET(locale)
     assert(c_locale != nullptr);
 
     const std::string random_string = fuzzed_data_provider.ConsumeRandomLengthString(5);
-    int32_t parseint32_out_without_locale;
-    const bool parseint32_without_locale = ParseInt32(random_string, &parseint32_out_without_locale);
-    int64_t parseint64_out_without_locale;
-    const bool parseint64_without_locale = ParseInt64(random_string, &parseint64_out_without_locale);
     const int64_t random_int64 = fuzzed_data_provider.ConsumeIntegral<int64_t>();
     const std::string tostring_without_locale = util::ToString(random_int64);
-    // The variable `random_int32` is no longer used, but the harness still needs to
-    // consume the same data that it did previously to not invalidate existing seeds.
-    const int32_t random_int32 = fuzzed_data_provider.ConsumeIntegral<int32_t>();
-    (void)random_int32;
     const std::string strprintf_int_without_locale = strprintf("%d", random_int64);
     const double random_double = fuzzed_data_provider.ConsumeFloatingPoint<double>();
     const std::string strprintf_double_without_locale = strprintf("%f", random_double);
 
     const char* new_locale = std::setlocale(LC_ALL, locale_identifier.c_str());
     assert(new_locale != nullptr);
 
-    int32_t parseint32_out_with_locale;
-    const bool parseint32_with_locale = ParseInt32(random_string, &parseint32_out_with_locale);
-    assert(parseint32_without_locale == parseint32_with_locale);
-    if (parseint32_without_locale) {
-        assert(parseint32_out_without_locale == parseint32_out_with_locale);
-    }
-    int64_t parseint64_out_with_locale;
-    const bool parseint64_with_locale = ParseInt64(random_string, &parseint64_out_with_locale);
-    assert(parseint64_without_locale == parseint64_with_locale);
-    if (parseint64_without_locale) {
-        assert(parseint64_out_without_locale == parseint64_out_with_locale);
-    }
     const std::string tostring_with_locale = util::ToString(random_int64);
     assert(tostring_without_locale == tostring_with_locale);
     const std::string strprintf_int_with_locale = strprintf("%d", random_int64);

src/test/fuzz/parse_numbers.cpp

@@ -5,33 +5,59 @@
 #include <test/fuzz/fuzz.h>
 #include <util/moneystr.h>
 #include <util/strencodings.h>
+#include <util/string.h>
 
+#include <cassert>
+#include <cstdint>
+#include <optional>
 #include <string>
 
 FUZZ_TARGET(parse_numbers)
 {
     const std::string random_string(buffer.begin(), buffer.end());
+    {
+        const auto i8{ToIntegral<int8_t>(random_string)};
+        const auto u8{ToIntegral<uint8_t>(random_string)};
+        const auto i16{ToIntegral<int16_t>(random_string)};
+        const auto u16{ToIntegral<uint16_t>(random_string)};
+        const auto i32{ToIntegral<int32_t>(random_string)};
+        const auto u32{ToIntegral<uint32_t>(random_string)};
+        const auto i64{ToIntegral<int64_t>(random_string)};
+        const auto u64{ToIntegral<uint64_t>(random_string)};
+        // Dont check any values, just that each success result must fit into
+        // the one with the largest bit-width.
+        if (i8) {
+            assert(i8 == i64);
+        }
+        if (u8) {
+            assert(u8 == u64);
+        }
+        if (i16) {
+            assert(i16 == i64);
+        }
+        if (u16) {
+            assert(u16 == u64);
+        }
+        if (i32) {
+            assert(i32 == i64);
+        }
+        if (u32) {
+            assert(u32 == u64);
+        }
+        constexpr auto digits{"0123456789"};
+        if (i64) {
+            assert(util::RemovePrefixView(random_string, "-").find_first_not_of(digits) == std::string::npos);
+        }
+        if (u64) {
+            assert(random_string.find_first_not_of(digits) == std::string::npos);
+        }
+    }
 
     (void)ParseMoney(random_string);
 
-    uint8_t u8;
-    (void)ParseUInt8(random_string, &u8);
-
-    uint16_t u16;
-    (void)ParseUInt16(random_string, &u16);
-
-    int32_t i32;
-    (void)ParseInt32(random_string, &i32);
     (void)LocaleIndependentAtoi<int>(random_string);
 
-    uint32_t u32;
-    (void)ParseUInt32(random_string, &u32);
-
     int64_t i64;
     (void)LocaleIndependentAtoi<int64_t>(random_string);
     (void)ParseFixedPoint(random_string, 3, &i64);
-    (void)ParseInt64(random_string, &i64);
-
-    uint64_t u64;
-    (void)ParseUInt64(random_string, &u64);
 }