rpc: Fixed signed integer overflow for large feerates #29434
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code CoverageFor detailed information about the code coverage, see the test coverage report. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
|
The following |
vasild
left a comment
vasild
left a comment
There was a problem hiding this comment.
ACK fa19cf33f9dcfd3ccaa8012d09875fa939c02365
src/rpc/mempool.cpp
Outdated
There was a problem hiding this comment.
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())}
There was a problem hiding this comment.
Yes, this is a nice side-effect of the changes here, due to using self.Arg().
maflcko
force-pushed
the
2402-rpc-UB-
branch
from
fa19cf3 to
fa2a4fd
Compare
| 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( |
There was a problem hiding this comment.
nit (fa2a4fd): We could test negative values as well.
There was a problem hiding this comment.
This is unrelated to this pull request, so I am happy to review another pull that adds this check.
BrandonOdiwuor
left a comment
BrandonOdiwuor
left a comment
There was a problem hiding this comment.
Code Review ACK fa2a4fd
|
Pushed a commit to clarify the docs while touching the code here. |
maflcko
force-pushed
the
2402-rpc-UB-
branch
from
faa86f0 to
dddd7be
Compare
tdb3
left a comment
tdb3
left a comment
There was a problem hiding this comment.
Good work on the find and fix.
Seems to work well.
Code review ACK and basic test ACK for dddd7be.
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:
$ bitcoin-cli -regtest testmempoolaccept '["020000000001013fbb09530d97bda4b685b49eb6203acf448a1e453d46731564bf7462282efcb80000000000fdffffff018052302901000000160014c5dd9411b1556a3e13de5e8ba902b3eb3430f19f02473044022054dde0cb3af2efca3847198fc4efc2dcfcef8e4418acefadc0b9ad13287202ad022020798884fbd4e83eac220e4b9ebac339ce6caa15eb86b8e5c5e47f9a9de2f1440121023fb60e94d35f553058d4a5c6b466a5e97910f46d3a8a9f758a813becf15e0ee100000000"]' 2
[
{
"txid": "49d886fb41a4e7a079d33bc9b0bf7018e9cb2217e90e924e3dd7d1817cf63b64",
"wtxid": "ab97ee97f595c13f89e6f03f3ed2c3b0ccaf5e75269f87b6a22b19436e43f654",
"allowed": true,
"vsize": 110,
"fees": {
"base": 0.14000000,
"effective-feerate": 1.27272727,
"effective-includes": [
"ab97ee97f595c13f89e6f03f3ed2c3b0ccaf5e75269f87b6a22b19436e43f654"
]
}
}
]
$ bitcoin-cli -regtest sendrawtransaction 020000000001013fbb09530d97bda4b685b49eb6203acf448a1e453d46731564bf7462282efcb80000000000fdffffff018052302901000000160014c5dd9411b1556a3e13de5e8ba902b3eb3430f19f02473044022054dde0cb3af2efca3847198fc4efc2dcfcef8e4418acefadc0b9ad13287202ad022020798884fbd4e83eac220e4b9ebac339ce6caa15eb86b8e5c5e47f9a9de2f1440121023fb60e94d35f553058d4a5c6b466a5e97910f46d3a8a9f758a813becf15e0ee100000000 2
49d886fb41a4e7a079d33bc9b0bf7018e9cb2217e90e924e3dd7d1817cf63b64
PR 29434 commit dddd7be:
$ bitcoin-cli -regtest testmempoolaccept '["020000000001013fbb09530d97bda4b685b49eb6203acf448a1e453d46731564bf7462282efcb80000000000fdffffff018052302901000000160014c5dd9411b1556a3e13de5e8ba902b3eb3430f19f02473044022054dde0cb3af2efca3847198fc4efc2dcfcef8e4418acefadc0b9ad13287202ad022020798884fbd4e83eac220e4b9ebac339ce6caa15eb86b8e5c5e47f9a9de2f1440121023fb60e94d35f553058d4a5c6b466a5e97910f46d3a8a9f758a813becf15e0ee100000000"]' 2
error code: -8
error message:
Fee rates larger than or equal to 1BTC/kvB are not accepted
$ bitcoin-cli -regtest sendrawtransaction 020000000001013fbb09530d97bda4b685b49eb6203acf448a1e453d46731564bf7462282efcb80000000000fdffffff018052302901000000160014c5dd9411b1556a3e13de5e8ba902b3eb3430f19f02473044022054dde0cb3af2efca3847198fc4efc2dcfcef8e4418acefadc0b9ad13287202ad022020798884fbd4e83eac220e4b9ebac339ce6caa15eb86b8e5c5e47f9a9de2f1440121023fb60e94d35f553058d4a5c6b466a5e97910f46d3a8a9f758a813becf15e0ee100000000 2
error code: -8
error message:
Fee rates larger than or equal to 1BTC/kvB are not accepted
DrahtBot
requested review from
BrandonOdiwuor
and removed request for
BrandonOdiwuor
|
crACK dddd7be |
DrahtBot
requested review from
BrandonOdiwuor
and removed request for
BrandonOdiwuor and
brunoerg
| return amount; | ||
| } | ||
|
|
||
| CFeeRate ParseFeeRate(const UniValue& json) |
There was a problem hiding this comment.
nit: Would have named it ParseReasonableFeeRate or ParseLimitedFeeRate to make clear it's doing more than just parsing.
There was a problem hiding this comment.
Alternatively could have made the limit a parameter, because that also makes it clearer.
There was a problem hiding this comment.
Sure, may do if I re-touch.
DrahtBot
requested review from
BrandonOdiwuor
and removed request for
BrandonOdiwuor
DrahtBot
requested review from
BrandonOdiwuor
and removed request for
BrandonOdiwuor
|
ACK dddd7be |
DrahtBot
requested review from
BrandonOdiwuor
and removed request for
BrandonOdiwuor
Github-Pull: bitcoin#29434 Rebased-From: fa0ff66
Github-Pull: bitcoin#29434 Rebased-From: fade94d
Github-Pull: bitcoin#29434 Rebased-From: fa2a4fd
Github-Pull: bitcoin#29434 Rebased-From: dddd7be
|
Looks like this has uncovered a bug in btcd affecting lnd, see https://twitter.com/roasbeef/status/1781086945986404559. |
I presume the bug was fixed in btcsuite/btcd#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 |
10 participants
Passing large BTC/kvB feerates to RPCs is problematic, because:
0to disable the check.Fix all issues by rejecting anything more than 1BTC/kvB during parsing.