Skip to content

rpc: Fixed signed integer overflow for large feerates #29434

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
achow101 merged 4 commits into from
Feb 19, 2024
Merged

rpc: Fixed signed integer overflow for large feerates #29434

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
fa0ff66
rpc: Implement RPCHelpMan::ArgValue<> for UniValue
Feb 14, 2024
fade94d
rpc: Add ParseFeeRate helper
Feb 14, 2024
fa2a4fd
rpc: Fixed signed integer overflow for large feerates
Feb 14, 2024
dddd7be
doc: Clarify maxfeerate help
Feb 15, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 5 additions & 8 deletions src/rpc/mempool.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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))};
Copy link
Contributor

@vasild vasild Feb 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Copy link
Member Author

@maflcko maflcko Feb 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


int64_t virtual_size = GetVirtualTransactionSize(*tx);
CAmount max_raw_tx_fee = max_raw_tx_fee_rate.GetFee(virtual_size);
Expand Down Expand Up @@ -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"
Expand Down Expand Up @@ -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());
Expand Down
9 changes: 9 additions & 0 deletions src/rpc/util.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,13 @@ CAmount AmountFromValue(const UniValue& value, int decimals)
return amount;
}

CFeeRate ParseFeeRate(const UniValue& json)
Copy link
Contributor

@fjahr fjahr Feb 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@fjahr fjahr Feb 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

@maflcko maflcko Feb 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, may do if I re-touch.

{
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());
Expand Down Expand Up @@ -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>(););
Expand Down
5 changes: 5 additions & 0 deletions src/rpc/util.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
8 changes: 8 additions & 0 deletions test/functional/mempool_accept.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,9 +90,17 @@ def run_test(self):
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(
Copy link
Contributor

@brunoerg brunoerg Feb 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

@maflcko maflcko Feb 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@kevkevinpal kevkevinpal Feb 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Created a PR to cover this #29459

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')
Expand Down