Avoid signed integer overflow and invalid integer negation when loading malformed mempool.dat files #20383

Why?

Otherwise the following invalid integer negation will take place:

util/moneystr.cpp:16:34: runtime error: negation of -9223372036854775808 cannot be represented in type 'CAmount' (aka 'long'); cast to an unsigned type to negate this value to itself
    [#0](/bitcoin-bitcoin/0/) 0x55b386a14eac in FormatMoney[abi:cxx11](long const&) src/util/moneystr.cpp:16:34
    [#1](/bitcoin-bitcoin/1/) 0x55b3861962f3 in CTxMemPool::PrioritiseTransaction(uint256 const&, long const&) src/txmempool.cpp:861:77

FormatMoney(CAmount n) is not defined for n == std::numeric_limits<CAmount>::min().

The negation of std::numeric_limits<CAmount>::min() (-9223372036854775808) cannot be represented in the type CAmount (int64_t).

To reproduce:

$ git checkout master
$ make -C src/ bitcoind
$ xxd -p -r > mempool-invalid-negation.dat << "EOF"
0100000000000000002e000000005d2d000d020000000000000000000000
200000000000000000000080fc0000002d
EOF
$ cp mempool-invalid-negation.dat ~/.bitcoin/regtest/mempool.dat
$ UBSAN_OPTIONS="print_stacktrace=1:halt_on_error=1:report_error_type=1" src/bitcoind -regtest
…
util/moneystr.cpp:16:34: runtime error: negation of -9223372036854775808 cannot be represented in type 'CAmount' (aka 'long'); cast to an unsigned type to negate this value to itself
…

If the fix to FormayMoney in #20406 is applied we don't have to have the restriction. I think that would be even better :)

This needs to be in the parent commit

Good point. Now fixed.

Maybe a bit more verbosity here would be good.

Now using a slightly more verbose error message which hints that an invalid fee_delta argument is the most likely cause. Let me know if you have any other suggestion on wording.

This should be literally impossible to hit based on invalid data...?

AFAICT, the only possibility is if the user manages to call prioritisetransaction RPC before it's loaded the transaction in question

It is possible, but don't take my word for it. Take my PoC for it! :)

Proof of concept:

$ git checkout master
$ make -C src/ bitcoind
$ xxd -p -r > mempool-signed-integer-overflow.dat << "EOF"
01000000000000003f2d3f3f21f800000000000000000000000000000000
6d697464657363656e64616e00000001000000ec000000003d6a6c000000
000000000000ec9bf601000000000000000000ec9b0001000000000001ff
fffef900000001000000ec0000000000ec9b000001000000000101000100
00000001000000ec000000003d6a6a000000000000000020ec9b000000fa
00
EOF
$ cp mempool-signed-integer-overflow.dat ~/.bitcoin/regtest/mempool.dat
$ UBSAN_OPTIONS="print_stacktrace=1:halt_on_error=1:report_error_type=1" src/bitcoind -regtest
…
txmempool.cpp:839:15: runtime error: signed integer overflow: -7211388903327006720 + -7211353718954917888 cannot be represented in type 'long'
    [#0](/bitcoin-bitcoin/0/) 0x556807f763ec in CTxMemPool::PrioritiseTransaction(uint256 const&, long const&) src/txmempool.cpp:839:15
    [#1](/bitcoin-bitcoin/1/) 0x556807ff3c61 in LoadMempool(CTxMemPool&) src/validation.cpp:5084:22
…

@practicalswift do you have a PoC for this one too? Just curious what the input looks like.

@Crypt-iQ Absolutely, your wish is my command: here you go! :)

Proof of concept:

$ git checkout master
$ make -C src/ bitcoind
$ xxd -p -r > mempool-poc-for-second-case-on-line-5115.dat << EOF
0100000000000000010000000000000000000000000000003f0020202020
202020202020202020ff20ff016e2682a8e0af181ae4ffa4ce2fc42b1e3d
8ce88019b194a45cd7c476b88ef1e220202020202020802020ff2020
EOF
$ cp mempool-poc-for-second-case-on-line-5115.dat ~/.bitcoin/regtest/mempool.dat
$ UBSAN_OPTIONS="print_stacktrace=1:halt_on_error=1:report_error_type=1" src/bitcoind -regtest
…
txmempool.cpp:839:15: runtime error: signed integer overflow: -62769881340174304 + -9214329515250016224 cannot be represented in type 'long'
    [#0](/bitcoin-bitcoin/0/) 0x5652ab2203ec in CTxMemPool::PrioritiseTransaction(uint256 const&, long const&) src/txmempool.cpp:839:15
    [#1](/bitcoin-bitcoin/1/) 0x5652ab29e1ad in LoadMempool(CTxMemPool&) src/validation.cpp:5115:18
…

:)

Add as a test?

@promag A fuzz test is added in #19259 ("tests: Add fuzzing harness for LoadMempool(...) and DumpMempool(...)"). Review welcome! :)

This check isn't complete. You'd have to check all ancestors and descandants as well. Also, I don't understand why this can't be FeeDeltaRange https://github.com/bitcoin/bitcoin/pull/20089/files#r557249710

Implemented an alternative that also checks all ancestors and descendants here: #23418

might be good to update the docs and release notes. Something like

diff --git a/src/amount.h b/src/amount.h
index 47968e80b1..9c770a8fe3 100644
--- a/src/amount.h
+++ b/src/amount.h
@@ -24,5 +24,6 @@ static const CAmount COIN = 100000000;
  * */
 static const CAmount MAX_MONEY = 21000000 * COIN;
 inline bool MoneyRange(const CAmount& nValue) { return (nValue >= 0 && nValue <= MAX_MONEY); }
+inline bool FeeDeltaRange(const CAmount& fee_delta) { return -MAX_MONEY <= fee_delta && fee_delta <= MAX_MONEY; }
 
 #endif //  BITCOIN_AMOUNT_H
diff --git a/src/rpc/mining.cpp b/src/rpc/mining.cpp
index 0e96beb6db..181570807c 100644
--- a/src/rpc/mining.cpp
+++ b/src/rpc/mining.cpp
@@ -444,14 +444,14 @@ static RPCHelpMan prioritisetransaction()
                 {
                     {"txid", RPCArg::Type::STR_HEX, RPCArg::Optional::NO, "The transaction id."},
                     {"dummy", RPCArg::Type::NUM, RPCArg::Optional::OMITTED_NAMED_ARG, "API-Compatibility for previous API. Must be zero or null.\n"
-            "                  DEPRECATED. For forward compatibility use named arguments and omit this parameter."},
-                    {"fee_delta", RPCArg::Type::NUM, RPCArg::Optional::NO, "The fee value (in satoshis) to add (or subtract, if negative).\n"
-            "                  Note, that this value is not a fee rate. It is a value to modify absolute fee of the TX.\n"
-            "                  The fee is not actually paid, only the algorithm for selecting transactions into a block\n"
-            "                  considers the transaction as it would have paid a higher (or lower) fee."},
+                              "DEPRECATED. For forward compatibility use named arguments and omit this parameter."},
+                    {"fee_delta", RPCArg::Type::NUM, RPCArg::Optional::NO, "The fee value (in satoshis) to add (or subtract, if negative). Must be in the range +-" + ToString(MAX_MONEY) + " sat.\n"
+                              "Note, that this value is not a fee rate. It is a value to modify absolute fee of the TX.\n"
+                              "The fee is not actually paid, only the algorithm for selecting transactions into a block\n"
+                              "considers the transaction as it would have paid a higher (or lower) fee."},
                 },
                 RPCResult{
-                    RPCResult::Type::BOOL, "", "Returns true"},
+                    RPCResult::Type::BOOL, "", "Returns true or throws on error"},
                 RPCExamples{
                     HelpExampleCli("prioritisetransaction", "\"txid\" 0.0 10000")
             + HelpExampleRpc("prioritisetransaction", "\"txid\", 0.0, 10000")
@@ -460,14 +460,17 @@ static RPCHelpMan prioritisetransaction()
 {
     LOCK(cs_main);
 
-    uint256 hash(ParseHashV(request.params[0], "txid"));
-    CAmount nAmount = request.params[2].get_int64();
+    const uint256 hash(ParseHashV(request.params[0], "txid"));
+    const CAmount nAmount = request.params[2].get_int64();
 
     if (!(request.params[1].isNull() || request.params[1].get_real() == 0)) {
         throw JSONRPCError(RPC_INVALID_PARAMETER, "Priority is no longer supported, dummy argument to prioritisetransaction must be 0.");
     }
 
-    EnsureMemPool(request.context).PrioritiseTransaction(hash, nAmount);
+    CTxMemPool& mempool = EnsureMemPool(request.context);
+    if (!mempool.PrioritiseTransaction(hash, nAmount)) {
+        throw JSONRPCError(RPC_INVALID_PARAMETER, "Invalid fee_delta passed");
+    }
     return true;
 },
     };

Fee deltas have no such range limit.

Context: #20089 (review) + #20383 (review)