rpc: Fixed signed integer overflow for large feerates #29434

pull maflcko wants to merge 4 commits into bitcoin:master from maflcko:2402-rpc-UB- changing 4 files +27 −8
  1. 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.

  2. rpc: Implement RPCHelpMan::ArgValue<> for UniValue fa0ff66109
  3. DrahtBot commented at 4:36 PM on February 14, 2024: contributor

    <!--e57a25ab6845829454e8d69fc972939a-->

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

    <!--006a51241073e994b41acfe9ec718e94-->

    Code Coverage

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

    <!--021abf342d371248e50ceaed478a90ca-->

    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.

    <!--174a7506f384e20aa4161008e828411d-->

    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.

  4. DrahtBot added the label RPC/REST/ZMQ on Feb 14, 2024
  5. maflcko commented at 4:38 PM on February 14, 2024: member

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

    c2VuZHJhd3RyYW5zYWN0aW9uXAEAAAAAHgBiu7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7
u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7
u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7
u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7
u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7
u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7
u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7
u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7
u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7
u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7
u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7sBALu7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7
u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7
u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7
u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7
u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7
u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7
u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7EgAA
AAAAAAC7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7
u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7
u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7
u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7
u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7
u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u2dl
dGNvbm5lY3Rpb25jb3VudLu7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7
u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7
u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7
u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7
u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7
u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7
u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7
u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7
u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7
u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7
u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7AAAAAAAAABK7u7u7u7u7
u7u7u0S7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7
u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7
u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7
u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7
u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7
u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7
u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7uzhVbmlWYWx1Zbu7
u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7v+/////////7u7u7u7u7u7u7u7u7u7
u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7
u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7
u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7
u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7
u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7OFVu
aVZhbHVlu7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7
u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7sA8P///wAAAAAAAAAA
AAAAOgABAAAAAAAAAQAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAABAAAHAAAAAAAAAAAAAAAA
AAAAAAAAAAAAAAAAAAF0ZQAAWwAAALu7u7u7u7u7u7u7u7u7uzhVbmlWYWx1Zbu7u7u7u7u7u7u7
u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7
u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7
u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7
u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7
u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7
u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7OFVuaVZhbHVl
u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7
u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7
u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7uzhVbmlWYWx1Zbu7u7u7u7u7u7u7u7u7u7u7u7u7u7u7
u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7
u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7
APD///8AAAAAAAAAAAAAADoAAQAAAAAAAAEAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAQAA
BwAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAABdGUAAFsAAAC7u7u7u7u7u7u7u7u7u7s4VW5pVmFs
dWW7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7
u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7
u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7
u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7
u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7
u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7
u7u7uzhVbmlWYWx1Zbu7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7
u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7
u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7uwDw////AAAAAAAAAAAAAAA6AAEAAAAA
AAABAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAEAAAcAAAAAAAAAAAAAAAAAAAAAAAAAAAAA
AAAAAXRlAABbAAAAAAAA////HHNkYbu7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7
u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7
u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7
u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7
u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7uzhVbmlWYWx1Zbu7
u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7
u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7
u7u7u7u7u7u7u7u7u7u7u7u7uwDw////AAAAAAAAAAAAAAA6AAEAAAAAAAABAAAAAAAAAAAAAAAA
AAAAAAAAAAAAAAAAAQAABwAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAABdGUAAFsAAAAAAABuAAAA
czD8XAAAdCsBAAAAgPWNkXN0KWFraWRh
  6. 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:

    /**
 * Parse a json number or string into a fee rate. The input is interpreted as a number
 * in BTC/kvB while the output is in sat/kvB.
 * For example ParseFeeRate("1").GetFeePerK() == 100'000'000
 * Reject negative values or rates larger than 1BTC/kvB.
 */
CFeeRate ParseFeeRate(const UniValue& json);
    maflcko commented at 9:56 AM on February 15, 2024:

    Thanks, I pushed something.

  7. 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:

    {"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().

  8. 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:

    $ echo "c2VuZHJhd3RyYW5zYWN0aW9uXAEAAAAAHgBiu7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7sBALu7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7EgAAAAAAAAC7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u2dldGNvbm5lY3Rpb25jb3VudLu7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7AAAAAAAAABK7u7u7u7u7u7u7u0S7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7uzhVbmlWYWx1Zbu7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7v+/////////7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7OFVuaVZhbHVlu7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7sA8P///wAAAAAAAAAAAAAAOgABAAAAAAAAAQAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAABAAAHAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAF0ZQAAWwAAALu7u7u7u7u7u7u7u7u7uzhVbmlWYWx1Zbu7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7uu7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7uu7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7OFVuaVZhbHVlu7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7uzhVbmlWYWx1Zbu7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7APD///8AAAAAAAAAAAAAADoAAQAAAAAAAAEAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAQAABwAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAABdGUAAFsAAAC7u7u7u7u7u7u7u7u7u7s4VW5pVmFsdWW7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7uzhVbmlWYWx1Zbu7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7uwDw////AAAAAAAAAAAAAAA6AAEAAAAAAAABAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAEAAAcAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAXRlAABbAAAAAAAA////HHNkYbu7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7uzhVbmlWYWx1Zbu7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7uwDw////AAAAAAAAAAAAAAA6AAEAAAAAAAABAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAQAABwAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAABdGUAAFsAAAAAAABuAAAAczD8XAAAdCsBAAAAgPWNkXN0KWFraWRh" |base64 -d > /tmp/fuzzraw

$ sha256 /tmp/fuzzraw 
SHA256 (/tmp/fuzzraw) = c96713d51f04497c88ef60029bbe41024cda526dd39801c5994c114f66dfa9bd

$ FUZZ=rpc ./src/test/fuzz/fuzz /tmp/fuzzraw 
INFO: Running with entropic power schedule (0xFF, 100).
INFO: Seed: 1616231335
INFO: Loaded 1 modules   (1269543 inline 8-bit counters): 1269543 [0x258a892cff18, 0x258a89405e3f), 
INFO: Loaded 1 PC tables (1269543 PCs): 1269543 [0x258a89405e40,0x258a8a7650b0), 
./src/test/fuzz/fuzz: Running 1 inputs 1 time(s) each.
Running: /tmp/fuzzraw
Executed /tmp/fuzzraw in 0 ms
***
*** NOTE: fuzzing was not performed, you have only
***       executed the target code on a fixed set of inputs.
***
$
    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:

    base64: invalid input

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

    $ sha1sum /tmp/b
6ef1eadaa3a1ad54365358b1ceb677bd1632396c  /tmp/b
$ sha256sum /tmp/b
fa82e44660796a5196cfb05483cae34356e9a2c55aab79d59161c887d5f23b37  /tmp/b
$ 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 

Executed /tmp/b in 3 ms

or

policy/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).

  9. vasild approved
  10. vasild commented at 9:40 AM on February 15, 2024: contributor

    ACK fa19cf33f9dcfd3ccaa8012d09875fa939c02365

  11. rpc: Add ParseFeeRate helper fade94d11a
  12. rpc: Fixed signed integer overflow for large feerates fa2a4fdef7
  13. maflcko force-pushed on Feb 15, 2024
  14. 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

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

    light crACK fa2a4fdef779b01e847def5985deafedc6dd3da8

  16. DrahtBot requested review from vasild on Feb 15, 2024
  17. BrandonOdiwuor approved
  18. BrandonOdiwuor commented at 2:53 PM on February 15, 2024: contributor

    Code Review ACK fa2a4fdef779b01e847def5985deafedc6dd3da8

  19. maflcko commented at 4:32 PM on February 15, 2024: member

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

  20. doc: Clarify maxfeerate help dddd7be9bf
  21. maflcko force-pushed on Feb 15, 2024
  22. vasild approved
  23. vasild commented at 3:26 PM on February 16, 2024: contributor

    ACK dddd7be9bf038c25f3e53c5bd708fb9cf73d4493

  24. DrahtBot requested review from BrandonOdiwuor on Feb 16, 2024
  25. DrahtBot requested review from brunoerg on Feb 16, 2024
  26. 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:

    $ 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 dddd7be9bf038c25f3e53c5bd708fb9cf73d4493:

    $ 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
  27. DrahtBot removed review request from BrandonOdiwuor on Feb 16, 2024
  28. DrahtBot requested review from BrandonOdiwuor on Feb 16, 2024
  29. brunoerg commented at 9:45 PM on February 16, 2024: contributor

    crACK dddd7be9bf038c25f3e53c5bd708fb9cf73d4493

  30. DrahtBot removed review request from brunoerg on Feb 16, 2024
  31. DrahtBot removed review request from BrandonOdiwuor on Feb 16, 2024
  32. DrahtBot requested review from BrandonOdiwuor on Feb 16, 2024
  33. 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.

  34. fjahr commented at 4:50 PM on February 18, 2024: contributor

    utACK dddd7be9bf038c25f3e53c5bd708fb9cf73d4493

  35. DrahtBot removed review request from BrandonOdiwuor on Feb 18, 2024
  36. DrahtBot requested review from BrandonOdiwuor on Feb 18, 2024
  37. DrahtBot removed review request from BrandonOdiwuor on Feb 18, 2024
  38. DrahtBot requested review from BrandonOdiwuor on Feb 18, 2024
  39. DrahtBot removed review request from BrandonOdiwuor on Feb 19, 2024
  40. DrahtBot requested review from BrandonOdiwuor on Feb 19, 2024
  41. fanquake assigned achow101 on Feb 19, 2024
  42. achow101 commented at 6:07 PM on February 19, 2024: member

    ACK dddd7be9bf038c25f3e53c5bd708fb9cf73d4493

  43. DrahtBot removed review request from BrandonOdiwuor on Feb 19, 2024
  44. DrahtBot requested review from BrandonOdiwuor on Feb 19, 2024
  45. achow101 merged this on Feb 19, 2024
  46. achow101 closed this on Feb 19, 2024
  47. maflcko deleted the branch on Feb 20, 2024
  48. luke-jr referenced this in commit c46bd008ae on Mar 10, 2024
  49. luke-jr referenced this in commit ea064f73bb on Mar 10, 2024
  50. luke-jr referenced this in commit 2d35cc3995 on Mar 10, 2024
  51. luke-jr referenced this in commit 1d9d47c497 on Mar 10, 2024
  52. 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.

  53. 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.

  54. bitcoin locked this on Apr 19, 2025
Contributors
maflckoDrahtBotvasildbrunoergkevkevinpalBrandonOdiwuortdb3fjahrachow101mzumsande


BrandonOdiwuor

Labels
RPC/REST/ZMQ
Linked (view graph)
#29459 test: check_mempool_result negative feerate
github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-05-02 15:13 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me