foster/bitcoin-bitcoin-core
0
0
Fork 0
mirror of https://github.com/bitcoin/bitcoin.git synced 2025-02-02 09:46:52 -05:00
Code Issues Activity

Merge bitcoin/bitcoin#29434: rpc: Fixed signed integer overflow for large feerates

Browse source 
dddd7be9bf doc: Clarify maxfeerate help (MarcoFalke)
fa2a4fdef7 rpc: Fixed signed integer overflow for large feerates (MarcoFalke)
fade94d11a rpc: Add ParseFeeRate helper (MarcoFalke)
fa0ff66109 rpc: Implement RPCHelpMan::ArgValue<> for UniValue (MarcoFalke)

Pull request description:

  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.

ACKs for top commit:
  brunoerg:
    crACK dddd7be9bf
  achow101:
    ACK dddd7be9bf
  vasild:
    ACK dddd7be9bf
  tdb3:
    Code review ACK and basic test ACK for dddd7be9bf.
  fjahr:
    utACK dddd7be9bf

Tree-SHA512: 5dcce1f0abe059dc6b2ff56787e11081d73a45b4ddd6dcc2c1ea13709ebc13af5e7265e84fffb97ef32027b56b81955672a67ed7702e8fa30c2e849d67727bac
This commit is contained in:
Ava Chow 2024-02-19 13:07:56 -05:00
commit c265aad5b5
No known key found for this signature in database
GPG key ID: 17565732E08E5E41
4 changed files with 27 additions and 8 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

13
src/rpc/mempool.cpp
View file

@ -45,7 +45,7 @@ static RPCHelpMan sendrawtransaction()
{"hexstring", RPCArg::Type::STR_HEX, RPCArg::Optional::NO, "The hex string of the raw transaction"},
{"maxfeerate", RPCArg::Type::AMOUNT, RPCArg::Default{FormatMoney(DEFAULT_MAX_RAW_TX_FEE_RATE.GetFeePerK())},
"Reject transactions whose fee rate is higher than the specified value, expressed in " + CURRENCY_UNIT +
"/kvB.\nSet to 0 to accept any fee rate."},
"/kvB.\nFee rates larger than 1BTC/kvB are rejected.\nSet to 0 to accept any fee rate."},
{"maxburnamount", RPCArg::Type::AMOUNT, RPCArg::Default{FormatMoney(0)},
"Reject transactions with provably unspendable outputs (e.g. 'datacarrier' outputs that use the OP_RETURN opcode) greater than the specified value, expressed in " + CURRENCY_UNIT + ".\n"
"If burning funds through unspendable outputs is desired, increase this value.\n"
@ -81,9 +81,7 @@ static RPCHelpMan sendrawtransaction()
CTransactionRef tx(MakeTransactionRef(std::move(mtx)));
const CFeeRate max_raw_tx_fee_rate = request.params[1].isNull() ?
DEFAULT_MAX_RAW_TX_FEE_RATE :
CFeeRate(AmountFromValue(request.params[1]));
const CFeeRate max_raw_tx_fee_rate{ParseFeeRate(self.Arg<UniValue>(1))};
int64_t virtual_size = GetVirtualTransactionSize(*tx);
CAmount max_raw_tx_fee = max_raw_tx_fee_rate.GetFee(virtual_size);
@ -117,7 +115,8 @@ static RPCHelpMan testmempoolaccept()
},
},
{"maxfeerate", RPCArg::Type::AMOUNT, RPCArg::Default{FormatMoney(DEFAULT_MAX_RAW_TX_FEE_RATE.GetFeePerK())},
"Reject transactions whose fee rate is higher than the specified value, expressed in " + CURRENCY_UNIT + "/kvB\n"},
"Reject transactions whose fee rate is higher than the specified value, expressed in " + CURRENCY_UNIT +
"/kvB.\nFee rates larger than 1BTC/kvB are rejected.\nSet to 0 to accept any fee rate."},
},
RPCResult{
RPCResult::Type::ARR, "", "The result of the mempool acceptance test for each raw transaction in the input array.\n"
@ -162,9 +161,7 @@ static RPCHelpMan testmempoolaccept()
"Array must contain between 1 and " + ToString(MAX_PACKAGE_COUNT) + " transactions.");
}
const CFeeRate max_raw_tx_fee_rate = request.params[1].isNull() ?
DEFAULT_MAX_RAW_TX_FEE_RATE :
CFeeRate(AmountFromValue(request.params[1]));
const CFeeRate max_raw_tx_fee_rate{ParseFeeRate(self.Arg<UniValue>(1))};
std::vector<CTransactionRef> txns;
txns.reserve(raw_transactions.size());

9
src/rpc/util.cpp
View file

@ -75,6 +75,13 @@ CAmount AmountFromValue(const UniValue& value, int decimals)
return amount;
}
CFeeRate ParseFeeRate(const UniValue& json)
{
CAmount val{AmountFromValue(json)};
if (val >= COIN) throw JSONRPCError(RPC_INVALID_PARAMETER, "Fee rates larger than or equal to 1BTC/kvB are not accepted");
return CFeeRate{val};
}
uint256 ParseHashV(const UniValue& v, std::string_view name)
{
const std::string& strHex(v.get_str());
@ -678,11 +685,13 @@ static void CheckRequiredOrDefault(const RPCArg& param)
void force_semicolon(ret_type)
// Optional arg (without default). Can also be called on required args, if needed.
TMPL_INST(nullptr, const UniValue*, maybe_arg;);
TMPL_INST(nullptr, std::optional<double>, maybe_arg ? std::optional{maybe_arg->get_real()} : std::nullopt;);
TMPL_INST(nullptr, std::optional<bool>, maybe_arg ? std::optional{maybe_arg->get_bool()} : std::nullopt;);
TMPL_INST(nullptr, const std::string*, maybe_arg ? &maybe_arg->get_str() : nullptr;);
// Required arg or optional arg with default value.
TMPL_INST(CheckRequiredOrDefault, const UniValue&, *CHECK_NONFATAL(maybe_arg););
TMPL_INST(CheckRequiredOrDefault, bool, CHECK_NONFATAL(maybe_arg)->get_bool(););
TMPL_INST(CheckRequiredOrDefault, int, CHECK_NONFATAL(maybe_arg)->getInt<int>(););
TMPL_INST(CheckRequiredOrDefault, uint64_t, CHECK_NONFATAL(maybe_arg)->getInt<uint64_t>(););

5
src/rpc/util.h
View file

@ -103,6 +103,11 @@ std::vector<unsigned char> ParseHexO(const UniValue& o, std::string_view strKey)
* @returns a CAmount if the various checks pass.
*/
CAmount AmountFromValue(const UniValue& value, int decimals = 8);
/**
* Parse a json number or string, denoting BTC/kvB, into a CFeeRate (sat/kvB).
* Reject negative values or rates larger than 1BTC/kvB.
*/
CFeeRate ParseFeeRate(const UniValue& json);
using RPCArgList = std::vector<std::pair<std::string, UniValue>>;
std::string HelpExampleCli(const std::string& methodname, const std::string& args);

8
test/functional/mempool_accept.py
View file

@ -90,9 +90,17 @@ class MempoolAcceptanceTest(BitcoinTestFramework):
txid_in_block = self.wallet.sendrawtransaction(from_node=node, tx_hex=raw_tx_in_block)
self.generate(node, 1)
self.mempool_size = 0
# Also check feerate. 1BTC/kvB fails
assert_raises_rpc_error(-8, "Fee rates larger than or equal to 1BTC/kvB are not accepted", lambda: self.check_mempool_result(
result_expected=None,
rawtxs=[raw_tx_in_block],
maxfeerate=1,
))
# ... 0.99 passes
self.check_mempool_result(
result_expected=[{'txid': txid_in_block, 'allowed': False, 'reject-reason': 'txn-already-known'}],
rawtxs=[raw_tx_in_block],
maxfeerate=0.99,
)
self.log.info('A transaction not in the mempool')