rpc: Fixed signed integer overflow for large feerates

maflcko commented at 4:36 pm on February 14, 2024: member

Passing large BTC/kvB feerates to RPCs is problematic, because:

They are likely a typo. 1BTC/kvB (or larger) seems absurd.
They may cause signed integer overflow.
Anyone really wanting to pick such a large value can set 0 to disable the check.

Fix all issues by rejecting anything more than 1BTC/kvB during parsing.

rpc: Implement RPCHelpMan::ArgValue<> for UniValue fa0ff66109

DrahtBot commented at 4:36 pm on February 14, 2024: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	vasild, tdb3, brunoerg, fjahr, achow101
Stale ACK	BrandonOdiwuor

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#28950 (RPC: Add maxfeerate and maxburnamount args to submitpackage by instagibbs)

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.

DrahtBot added the label RPC/REST/ZMQ on Feb 14, 2024

maflcko commented at 4:38 pm on February 14, 2024: member

The following FUZZ=rpc should pass with this pull and crash on master:

 0c2VuZHJhd3RyYW5zYWN0aW9uXAEAAAAAHgBiu7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7
 1u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7
 2u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7
 3u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7
 4u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7
 5u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7
 6u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7
 7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7
 8u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7
 9u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7
10u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7sBALu7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7
11u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7
12u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7
13u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7
14u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7
15u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7
16u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7EgAA
17AAAAAAC7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7
18u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7
19u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7
20u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7
21u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7
22u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u2dl
23dGNvbm5lY3Rpb25jb3VudLu7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7
24u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7
25u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7
26u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7
27u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7
28u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7
29u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7
30u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7
31u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7
32u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7
33u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7AAAAAAAAABK7u7u7u7u7
34u7u7u0S7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7
35u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7
36u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7
37u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7
38u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7
39u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7
40u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7uzhVbmlWYWx1Zbu7
41u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7v+/////////7u7u7u7u7u7u7u7u7u7
42u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7
43u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7
44u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7
45u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7
46u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7OFVu
47aVZhbHVlu7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7
48u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7sA8P///wAAAAAAAAAA
49AAAAOgABAAAAAAAAAQAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAABAAAHAAAAAAAAAAAAAAAA
50AAAAAAAAAAAAAAAAAAF0ZQAAWwAAALu7u7u7u7u7u7u7u7u7uzhVbmlWYWx1Zbu7u7u7u7u7u7u7
51u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7
52u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7
53u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7
54u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7
55u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7
56u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7OFVuaVZhbHVl
57u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7
58u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7
59u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7uzhVbmlWYWx1Zbu7u7u7u7u7u7u7u7u7u7u7u7u7u7u7
60u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7
61u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7
62APD///8AAAAAAAAAAAAAADoAAQAAAAAAAAEAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAQAA
63BwAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAABdGUAAFsAAAC7u7u7u7u7u7u7u7u7u7s4VW5pVmFs
64dWW7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7
65u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7
66u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7
67u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7
68u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7
69u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7
70u7u7uzhVbmlWYWx1Zbu7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7
71u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7
72u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7uwDw////AAAAAAAAAAAAAAA6AAEAAAAA
73AAABAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAEAAAcAAAAAAAAAAAAAAAAAAAAAAAAAAAAA
74AAAAAXRlAABbAAAAAAAA////HHNkYbu7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7
75u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7
76u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7
77u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7
78u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7uzhVbmlWYWx1Zbu7
79u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7
80u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7
81u7u7u7u7u7u7u7u7u7u7u7u7uwDw////AAAAAAAAAAAAAAA6AAEAAAAAAAABAAAAAAAAAAAAAAAA
82AAAAAAAAAAAAAAAAAQAABwAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAABdGUAAFsAAAAAAABuAAAA
83czD8XAAAdCsBAAAAgPWNkXN0KWFraWRh

in src/rpc/util.h:110 in fa19cf33f9 outdated

102@@ -103,6 +103,11 @@ std::vector<unsigned char> ParseHexO(const UniValue& o, std::string_view strKey)
103  * @returns a CAmount if the various checks pass.
104  */
105 CAmount AmountFromValue(const UniValue& value, int decimals = 8);
106+/**
107+ * Parse a json number or string into a rate of BTC/kvB.
108+ * Reject negative values or rates larger than 1BTC/kvB.
109+ */
110+CFeeRate ParseFeeRate(const UniValue& json);

vasild commented at 8:44 am on February 15, 2024:

That is a bit confusing - the input is in BTC/kvB and the output is in sat/kvB. Maybe elaborate more:

0/**
1 * Parse a json number or string into a fee rate. The input is interpreted as a number
2 * in BTC/kvB while the output is in sat/kvB.
3 * For example ParseFeeRate("1").GetFeePerK() == 100'000'000
4 * Reject negative values or rates larger than 1BTC/kvB.
5 */
6CFeeRate ParseFeeRate(const UniValue& json);

maflcko commented at 9:56 am on February 15, 2024:

Thanks, I pushed something.

in src/rpc/mempool.cpp:84 in fa19cf33f9 outdated

80@@ -81,9 +81,7 @@ static RPCHelpMan sendrawtransaction()
81 
82             CTransactionRef tx(MakeTransactionRef(std::move(mtx)));
83 
84-            const CFeeRate max_raw_tx_fee_rate = request.params[1].isNull() ?
85-                                                     DEFAULT_MAX_RAW_TX_FEE_RATE :
86-                                                     CFeeRate(AmountFromValue(request.params[1]));
87+            const CFeeRate max_raw_tx_fee_rate{ParseFeeRate(self.Arg<UniValue>(1))};

vasild commented at 9:09 am on February 15, 2024:

It is nice that DEFAULT_MAX_RAW_TX_FEE_RATE is now deduplicated and is only mentioned once at the parameter definition:

0{"maxfeerate", RPCArg::Type::AMOUNT, RPCArg::Default{FormatMoney(DEFAULT_MAX_RAW_TX_FEE_RATE.GetFeePerK())}

maflcko commented at 9:43 am on February 15, 2024:

Yes, this is a nice side-effect of the changes here, due to using self.Arg().

in src/rpc/mempool.cpp:163 in fa19cf33f9 outdated

159@@ -162,9 +160,7 @@ static RPCHelpMan testmempoolaccept()
160                                    "Array must contain between 1 and " + ToString(MAX_PACKAGE_COUNT) + " transactions.");
161             }
162 
163-            const CFeeRate max_raw_tx_fee_rate = request.params[1].isNull() ?
164-                                                     DEFAULT_MAX_RAW_TX_FEE_RATE :
165-                                                     CFeeRate(AmountFromValue(request.params[1]));
166+            const CFeeRate max_raw_tx_fee_rate{ParseFeeRate(self.Arg<UniValue>(1))};

vasild commented at 9:27 am on February 15, 2024:

What is the integer overflow?

This passes on master @ 7143d4388407ab3d12005e55a02d5e8f334e4dc9:

 0$ echo "c2VuZHJhd3RyYW5zYWN0aW9uXAEAAAAAHgBiu7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7sBALu7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7EgAAAAAAAAC7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u2dldGNvbm5lY3Rpb25jb3VudLu7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7AAAAAAAAABK7u7u7u7u7u7u7u0S7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7uzhVbmlWYWx1Zbu7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7v+/////////7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7OFVuaVZhbHVlu7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7sA8P///wAAAAAAAAAAAAAAOgABAAAAAAAAAQAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAABAAAHAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAF0ZQAAWwAAALu7u7u7u7u7u7u7u7u7uzhVbmlWYWx1Zbu7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7uu7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7uu7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7OFVuaVZhbHVlu7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7uzhVbmlWYWx1Zbu7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7APD///8AAAAAAAAAAAAAADoAAQAAAAAAAAEAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAQAABwAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAABdGUAAFsAAAC7u7u7u7u7u7u7u7u7u7s4VW5pVmFsdWW7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7uzhVbmlWYWx1Zbu7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7uwDw////AAAAAAAAAAAAAAA6AAEAAAAAAAABAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAEAAAcAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAXRlAABbAAAAAAAA////HHNkYbu7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7uzhVbmlWYWx1Zbu7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7uwDw////AAAAAAAAAAAAAAA6AAEAAAAAAAABAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAQAABwAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAABdGUAAFsAAAAAAABuAAAAczD8XAAAdCsBAAAAgPWNkXN0KWFraWRh" |base64 -d > /tmp/fuzzraw
 1
 2$ sha256 /tmp/fuzzraw 
 3SHA256 (/tmp/fuzzraw) = c96713d51f04497c88ef60029bbe41024cda526dd39801c5994c114f66dfa9bd
 4
 5$ FUZZ=rpc ./src/test/fuzz/fuzz /tmp/fuzzraw 
 6INFO: Running with entropic power schedule (0xFF, 100).
 7INFO: Seed: 1616231335
 8INFO: Loaded 1 modules   (1269543 inline 8-bit counters): 1269543 [0x258a892cff18, 0x258a89405e3f), 
 9INFO: Loaded 1 PC tables (1269543 PCs): 1269543 [0x258a89405e40,0x258a8a7650b0), 
10./src/test/fuzz/fuzz: Running 1 inputs 1 time(s) each.
11Running: /tmp/fuzzraw
12Executed /tmp/fuzzraw in 0 ms
13***
14*** NOTE: fuzzing was not performed, you have only
15***       executed the target code on a fixed set of inputs.
16***
17$

maflcko commented at 9:49 am on February 15, 2024:

What is the integer overflow?

You will have to correctly copy-paste the full base64 input. Mutating the base64 inputs may result in a different fuzz input, which will trigger a different code path. Or it may be invalid base64. On your base64, I get:

0base64: invalid input

To double check, if you correctly copy-paste the full input, you should get:

 0$ sha1sum /tmp/b
 16ef1eadaa3a1ad54365358b1ceb677bd1632396c  /tmp/b
 2$ sha256sum /tmp/b
 3fa82e44660796a5196cfb05483cae34356e9a2c55aab79d59161c887d5f23b37  /tmp/b
 4$ FUZZ=rpc UBSAN_OPTIONS="suppressions=$(pwd)/test/sanitizer_suppressions/ubsan:print_stacktrace=1:halt_on_error=1:report_error_type=1" ./src/test/fuzz/fuzz /tmp/b 
 5
 6Executed /tmp/b in 3 ms
 7
 8or
 9
10policy/feerate.cpp:29:63: runtime error: signed integer overflow: 1962496000000000 * 4714 cannot be represented in type 'int64_t' (aka 'long')

vasild commented at 3:15 pm on February 16, 2024:

I can reproduce now, my bad probably removing the newlines from the base64 input (even though my base64 tool succeeds in decoding that).

vasild approved

vasild commented at 9:40 am on February 15, 2024: contributor

ACK fa19cf33f9dcfd3ccaa8012d09875fa939c02365

rpc: Add ParseFeeRate helper fade94d11a

rpc: Fixed signed integer overflow for large feerates fa2a4fdef7

maflcko force-pushed on Feb 15, 2024

in test/functional/mempool_accept.py:94 in fa2a4fdef7 outdated

89@@ -90,9 +90,17 @@ def run_test(self):
90         txid_in_block = self.wallet.sendrawtransaction(from_node=node, tx_hex=raw_tx_in_block)
91         self.generate(node, 1)
92         self.mempool_size = 0
93+        # Also check feerate. 1BTC/kvB fails
94+        assert_raises_rpc_error(-8, "Fee rates larger than or equal to 1BTC/kvB are not accepted", lambda: self.check_mempool_result(

brunoerg commented at 2:35 pm on February 15, 2024:

nit (fa2a4fdef779b01e847def5985deafedc6dd3da8): We could test negative values as well.

maflcko commented at 2:37 pm on February 15, 2024:

This is unrelated to this pull request, so I am happy to review another pull that adds this check.

kevkevinpal commented at 3:18 am on February 21, 2024:

Created a PR to cover this https://github.com/bitcoin/bitcoin/pull/29459

brunoerg commented at 2:35 pm on February 15, 2024: contributor

light crACK fa2a4fdef779b01e847def5985deafedc6dd3da8

DrahtBot requested review from vasild on Feb 15, 2024

BrandonOdiwuor approved

BrandonOdiwuor commented at 2:53 pm on February 15, 2024: contributor

Code Review ACK fa2a4fdef779b01e847def5985deafedc6dd3da8

maflcko commented at 4:32 pm on February 15, 2024: member

Pushed a commit to clarify the docs while touching the code here.

doc: Clarify maxfeerate help dddd7be9bf

maflcko force-pushed on Feb 15, 2024

vasild approved

vasild commented at 3:26 pm on February 16, 2024: contributor

ACK dddd7be9bf038c25f3e53c5bd708fb9cf73d4493

DrahtBot requested review from BrandonOdiwuor on Feb 16, 2024

DrahtBot requested review from brunoerg on Feb 16, 2024

tdb3 commented at 3:55 pm on February 16, 2024: contributor

Good work on the find and fix.
Seems to work well. Code review ACK and basic test ACK for dddd7be9bf038c25f3e53c5bd708fb9cf73d4493.

Test actions taken: Checked out PR branch, built, ran all unit and functional tests (all passed). As a basic sanity check, created a regtest transaction with high fee rate (> 1 BTC/kvB) and executed testmempoolaccept/sendrawtransaction on both the PR branch and v26.0. v26.0 allows and PR code rejects with expected error -8.

v26.0:

 0$ bitcoin-cli -regtest testmempoolaccept '["020000000001013fbb09530d97bda4b685b49eb6203acf448a1e453d46731564bf7462282efcb80000000000fdffffff018052302901000000160014c5dd9411b1556a3e13de5e8ba902b3eb3430f19f02473044022054dde0cb3af2efca3847198fc4efc2dcfcef8e4418acefadc0b9ad13287202ad022020798884fbd4e83eac220e4b9ebac339ce6caa15eb86b8e5c5e47f9a9de2f1440121023fb60e94d35f553058d4a5c6b466a5e97910f46d3a8a9f758a813becf15e0ee100000000"]' 2
 1[
 2  {
 3    "txid": "49d886fb41a4e7a079d33bc9b0bf7018e9cb2217e90e924e3dd7d1817cf63b64",
 4    "wtxid": "ab97ee97f595c13f89e6f03f3ed2c3b0ccaf5e75269f87b6a22b19436e43f654",
 5    "allowed": true,
 6    "vsize": 110,
 7    "fees": {
 8      "base": 0.14000000,
 9      "effective-feerate": 1.27272727,
10      "effective-includes": [
11        "ab97ee97f595c13f89e6f03f3ed2c3b0ccaf5e75269f87b6a22b19436e43f654"
12      ]
13    }
14  }
15]
16
17$ bitcoin-cli -regtest sendrawtransaction 020000000001013fbb09530d97bda4b685b49eb6203acf448a1e453d46731564bf7462282efcb80000000000fdffffff018052302901000000160014c5dd9411b1556a3e13de5e8ba902b3eb3430f19f02473044022054dde0cb3af2efca3847198fc4efc2dcfcef8e4418acefadc0b9ad13287202ad022020798884fbd4e83eac220e4b9ebac339ce6caa15eb86b8e5c5e47f9a9de2f1440121023fb60e94d35f553058d4a5c6b466a5e97910f46d3a8a9f758a813becf15e0ee100000000 2
1849d886fb41a4e7a079d33bc9b0bf7018e9cb2217e90e924e3dd7d1817cf63b64

PR 29434 commit dddd7be9bf038c25f3e53c5bd708fb9cf73d4493:

0$ bitcoin-cli -regtest testmempoolaccept '["020000000001013fbb09530d97bda4b685b49eb6203acf448a1e453d46731564bf7462282efcb80000000000fdffffff018052302901000000160014c5dd9411b1556a3e13de5e8ba902b3eb3430f19f02473044022054dde0cb3af2efca3847198fc4efc2dcfcef8e4418acefadc0b9ad13287202ad022020798884fbd4e83eac220e4b9ebac339ce6caa15eb86b8e5c5e47f9a9de2f1440121023fb60e94d35f553058d4a5c6b466a5e97910f46d3a8a9f758a813becf15e0ee100000000"]' 2
1error code: -8
2error message:
3Fee rates larger than or equal to 1BTC/kvB are not accepted
4
5$ bitcoin-cli -regtest sendrawtransaction 020000000001013fbb09530d97bda4b685b49eb6203acf448a1e453d46731564bf7462282efcb80000000000fdffffff018052302901000000160014c5dd9411b1556a3e13de5e8ba902b3eb3430f19f02473044022054dde0cb3af2efca3847198fc4efc2dcfcef8e4418acefadc0b9ad13287202ad022020798884fbd4e83eac220e4b9ebac339ce6caa15eb86b8e5c5e47f9a9de2f1440121023fb60e94d35f553058d4a5c6b466a5e97910f46d3a8a9f758a813becf15e0ee100000000 2
6error code: -8
7error message:
8Fee rates larger than or equal to 1BTC/kvB are not accepted

DrahtBot removed review request from BrandonOdiwuor on Feb 16, 2024

DrahtBot requested review from BrandonOdiwuor on Feb 16, 2024

brunoerg commented at 9:45 pm on February 16, 2024: contributor

crACK dddd7be9bf038c25f3e53c5bd708fb9cf73d4493

DrahtBot removed review request from brunoerg on Feb 16, 2024

DrahtBot removed review request from BrandonOdiwuor on Feb 16, 2024

DrahtBot requested review from BrandonOdiwuor on Feb 16, 2024

in src/rpc/util.cpp:78 in dddd7be9bf

74@@ -75,6 +75,13 @@ CAmount AmountFromValue(const UniValue& value, int decimals)
75     return amount;
76 }
77 
78+CFeeRate ParseFeeRate(const UniValue& json)

fjahr commented at 4:41 pm on February 18, 2024:

nit: Would have named it ParseReasonableFeeRate or ParseLimitedFeeRate to make clear it’s doing more than just parsing.

fjahr commented at 4:43 pm on February 18, 2024:

Alternatively could have made the limit a parameter, because that also makes it clearer.

maflcko commented at 9:42 am on February 19, 2024:

Sure, may do if I re-touch.

fjahr commented at 4:50 pm on February 18, 2024: contributor

utACK dddd7be9bf038c25f3e53c5bd708fb9cf73d4493

DrahtBot removed review request from BrandonOdiwuor on Feb 18, 2024

DrahtBot requested review from BrandonOdiwuor on Feb 18, 2024

DrahtBot removed review request from BrandonOdiwuor on Feb 18, 2024

DrahtBot requested review from BrandonOdiwuor on Feb 18, 2024

DrahtBot removed review request from BrandonOdiwuor on Feb 19, 2024

DrahtBot requested review from BrandonOdiwuor on Feb 19, 2024

fanquake assigned achow101 on Feb 19, 2024

achow101 commented at 6:07 pm on February 19, 2024: member

ACK dddd7be9bf038c25f3e53c5bd708fb9cf73d4493

DrahtBot removed review request from BrandonOdiwuor on Feb 19, 2024

DrahtBot requested review from BrandonOdiwuor on Feb 19, 2024

achow101 merged this on Feb 19, 2024

achow101 closed this on Feb 19, 2024

maflcko deleted the branch on Feb 20, 2024

luke-jr referenced this in commit c46bd008ae on Mar 10, 2024

luke-jr referenced this in commit ea064f73bb on Mar 10, 2024

luke-jr referenced this in commit 2d35cc3995 on Mar 10, 2024

luke-jr referenced this in commit 1d9d47c497 on Mar 10, 2024

mzumsande commented at 2:58 am on April 19, 2024: contributor

Looks like this has uncovered a bug in btcd affecting lnd, see https://twitter.com/roasbeef/status/1781086945986404559.

maflcko commented at 8:39 am on April 19, 2024: member

a bug in btcd

I presume the bug was fixed in https://github.com/btcsuite/btcd/pull/2142

I wonder why this wasn’t found earlier in btcd, because if signed integer overflow happened, it could have resulted in rejected transactions as well (or anything else, since it is undefined behavior).

A possible explanation could be that no one sent a large enough transaction, or set a large enough maxfeerate to trigger the UB, or maybe someone did run into it and then re-tried with different values until it passed, and didn’t think it would be a bug.

It could be interesting to explore ways how to detect bugs in downstream projects faster. One example could be for downstream projects to have one CI task dedicated to testing the Bitcoin Core master development branch, instead of waiting for the rc1 of the next release.

rpc: Fixed signed integer overflow for large feerates #29434

Code Coverage

Reviews

Conflicts