[wallet] abort when attempting to fund a transaction above -maxtxfee #16257

pull Sjors wants to merge 1 commits into bitcoin:master from Sjors:2019/06/max_fee changing 10 files +27 −13
  1. Sjors commented at 12:01 AM on June 21, 2019: member

    FundTransaction calls GetMinimumFee which, when the fee rate is absurdly high, quietly reduces the fee to -maxtxfee.

    Becaue an absurdly high fee rate is usually the result of a fat finger, aborting seems safer behavior.

    Before:

    bitcoin-cli walletcreatefundedpsbt '[]' '[{"tb1q...": 0.01}]' 0 '{"feeRate": 10}' true
{
  "psbt": "cHNidP8...gAA=",
  "fee": 0.10000000,
  "changepos": 1
}

    After:

    bitcoin-cli walletcreatefundedpsbt '[]' '[{"tb1q...": 0.01}]' 0 '{"feeRate": 10}' true
error code: -25
error message:
Fee exceeds maximum configured by -maxtxfee

    QT still checks the max fee rate as expected: <img width="566" alt="Schermafbeelding 2019-06-20 om 19 52 00" src="https://user-images.githubusercontent.com/10217/59888424-a2aa7100-9395-11e9-8ae6-8a3c1f7de585.png">

  2. fanquake added the label Wallet on Jun 21, 2019
  3. promag commented at 12:42 AM on June 21, 2019: member

    Also affects fundrawtransaction. Despite the fact that these are low level calls and the client can also discard high fees, I guess it's ok to be on the safe side.

    Deserves a small release note like

    walletcreatefundedpsbt and fundrawtransaction now fail to create transactions with a resulting fee greater then -maxtxfee.

    Concept ACK.

  4. Sjors commented at 1:45 AM on June 21, 2019: member

    In additional to small transactions with a fat fingered fee rate, there's also an issue with extremely large transactions, which silently get their fee rate reduced. I didn't check if this PR fixes that as well, but I would assume so.

    See also #16192 and #16192.

  5. Sjors commented at 1:49 AM on June 21, 2019: member

    Full list of transactions that pay exactly 0.1 BTC in fees: https://blockchair.com/bitcoin/transactions?q=fee(10000000)# (there could be other wallets doing that of course)

  6. practicalswift commented at 6:28 AM on June 21, 2019: contributor

    Concept ACK: user safety first!

  7. gertjaap commented at 7:23 AM on June 24, 2019: contributor

    If aborting above maxtxfee is the desired behavior, then this PR should be merged in stead of #16192 because it catches that situation too.

  8. laanwj added this to the milestone 0.18.1 on Jun 27, 2019
  9. laanwj added the label Needs backport on Jun 27, 2019
  10. laanwj added this to the "Bugfixes" column in a project
  11. achow101 commented at 7:26 PM on June 27, 2019: member

    Are we certain that this does not affect the other methods of transaction creation (i.e. CreateTransaction)? Are there tests for absurdly high fees in regular wallet transactions, and if not, could you add some?

  12. Sjors commented at 7:52 PM on June 27, 2019: member

    One specific case this prevents is when a user sets feeRate: 1 (thinking it's satoshi per byte). A 1 SegWit input to 1 output transaction is 110 vbytes or 0.11 kilobyte. At 1 BTC per kilobyte the fee for that transaction would be 0.11 BTC which is just above -maxtxfee. @achow101 CreateTransaction is called by:

    • CreateRateBumpTransaction
    • sendtoaddress (via SendMoney)
    • sendmany
    • fundrawtransaction (via FundTransaction)

    I'll try to add some tests for those. Note that sendtoaddress and sendmany don't have a feeRate argument.

  13. jonasschnelli commented at 8:01 PM on June 27, 2019: contributor

    1. in coin control, absurd fees would no longer be capt to maxtxfee (which probably is okay, just that we are aware). You can't send from the GUI with fees > maxfee, but you currently can play with absurde fees in coin-control (I don't think this makses sense).

    2. There is a comment in walletmodel.cpp that needs overhaul:

            // reject absurdly high fee. (This can never happen because the
        // wallet caps the fee at m_default_max_tx_fee. This merely serves as a
        // belt-and-suspenders check)
        if (nFeeRequired > m_wallet->getDefaultMaxTxFee())
            return AbsurdFee;
    1. verified that the impact of removing the FeeReason::MAXTXFEE (and remove of the silent auto-cap to maxfee) in other places then FundTransaction are non-significant.

    Tested ACK 5735eb4749cdd7cddf0303e150493a6381b656f5

  14. in src/wallet/rpcwallet.cpp:3213 in 5735eb4749 outdated 
    3199 | @@ -3200,6 +3200,10 @@ void FundTransaction(CWallet* const pwallet, CMutableTransaction& tx, CAmount& f
3200 |      if (!pwallet->FundTransaction(tx, fee_out, change_position, strFailReason, lockUnspents, setSubtractFeeFromOutputs, coinControl)) {
3201 |          throw JSONRPCError(RPC_WALLET_ERROR, strFailReason);
3202 |      }
3203 | +
3204 | +    if (fee_out > pwallet->m_default_max_tx_fee) {
3205 | +      throw JSONRPCTransactionError(TransactionError::MAX_FEE_EXCEEDED);
    meshcollider commented at 10:12 PM on June 27, 2019:

    nit: indentation

  15. meshcollider approved
  16. meshcollider commented at 10:19 PM on June 27, 2019: contributor

    LGTM 5735eb4749cdd7cddf0303e150493a6381b656f5

    Much safer behaviour IMO

  17. Sjors commented at 11:45 PM on June 27, 2019: member

    Added release note and test for fundrawtransaction, sorry for breaking ACKs. Regarding @jonasschnelli's points:

    1. I think allowing this "playing" is OK. If someone encounters the error, they can google it and find out about the -maxtxfee setting. If we simply make it impossible, they won't know what to google for. Anyway, in practice it's unlikely to matter: it's way more difficult to make a fee fat finger mistake in the GUI than in the RPC.
    2. Fixed
  18. Sjors force-pushed on Jun 27, 2019
  19. Sjors force-pushed on Jun 27, 2019
  20. Sjors force-pushed on Jun 27, 2019
  21. kallewoof commented at 2:16 AM on June 28, 2019: member

    Concept ACK

    utACK 734c00d

  22. DrahtBot commented at 4:40 AM on June 28, 2019: member

    <!--e57a25ab6845829454e8d69fc972939a-->

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

    <!--174a7506f384e20aa4161008e828411d-->

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #16293 (test: Make test cases separate functions by MarcoFalke)

    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.

  23. in src/util/fees.cpp:20 in 734c00dd3d outdated 
      16 | @@ -17,8 +17,7 @@ std::string StringForFeeReason(FeeReason reason) {
  17 |          {FeeReason::MEMPOOL_MIN, "Mempool Min Fee"},
  18 |          {FeeReason::PAYTXFEE, "PayTxFee set"},
  19 |          {FeeReason::FALLBACK, "Fallback fee"},
  20 | -        {FeeReason::REQUIRED, "Minimum Required Fee"},
  21 | -        {FeeReason::MAXTXFEE, "MaxTxFee limit"}
  22 | +        {FeeReason::REQUIRED, "Minimum Required Fee"}
    MarcoFalke commented at 1:35 PM on June 28, 2019:

    style-nit: Can keep the trailing (optional) comma in the initializer list to reduce the diff

  24. in doc/release-notes-16257.md:1 in 734c00dd3d outdated 
       0 | @@ -0,0 +1,6 @@
   1 | +Wallet changes
    MarcoFalke commented at 1:54 PM on June 28, 2019:

    Could rename this file to release-notes-0.18.1-16257.md to indicate that it is not meant for the master branch release notes?

    Sjors commented at 6:05 PM on June 28, 2019:

    I think this is equally useful as a master release note.

    MarcoFalke commented at 6:40 PM on June 28, 2019:

    I think we put it in the release notes only of the earliest release that contains the new feature/or bugfix. But not blocking of course.

  25. in src/qt/walletmodel.cpp:226 in 734c00dd3d outdated 
     220 | @@ -221,9 +221,7 @@ WalletModel::SendCoinsReturn WalletModel::prepareTransaction(WalletModelTransact
 221 |              return TransactionCreationFailed;
 222 |          }
 223 |  
 224 | -        // reject absurdly high fee. (This can never happen because the
 225 | -        // wallet caps the fee at m_default_max_tx_fee. This merely serves as a
 226 | -        // belt-and-suspenders check)
    MarcoFalke commented at 2:47 PM on June 28, 2019:

    I don't like removing this belt-and-suspenders check and making it the only check.

  26. in src/wallet/rpcwallet.cpp:3214 in 734c00dd3d outdated 
    3207 | @@ -3208,6 +3208,10 @@ void FundTransaction(CWallet* const pwallet, CMutableTransaction& tx, CAmount& f
3208 |      if (!pwallet->FundTransaction(tx, fee_out, change_position, strFailReason, lockUnspents, setSubtractFeeFromOutputs, coinControl)) {
3209 |          throw JSONRPCError(RPC_WALLET_ERROR, strFailReason);
3210 |      }
3211 | +
3212 | +    if (fee_out > pwallet->m_default_max_tx_fee) {
3213 | +        throw JSONRPCTransactionError(TransactionError::MAX_FEE_EXCEEDED);
3214 | +    }
    MarcoFalke commented at 2:50 PM on June 28, 2019:

    I don't mind having a belt-and-suspenders check in the rpc code (similar to the GUI code), but ultimately the check should happen in the wallet.

    In fact the wallet already does this check in a hacky way (Transaction too large...). So I'd prefer if you removed the hacky check and replaced it with a check against m_default_max_tx_fee

    Sjors commented at 6:09 PM on June 28, 2019:

    It indeed makes sense for the wallet to do this check. Will look into that.

    Sjors commented at 2:46 AM on June 29, 2019:

    Done

  27. MarcoFalke commented at 2:51 PM on June 28, 2019: member

    Concept ACK, some style questions

  28. promag commented at 12:36 AM on June 29, 2019: member

    @jonasschnelli not sure if I understand your review, it's still possible to set a high fee rate that causes the fee capping effectively reducing the fee rate. I think this PR should also change the GUI behaviour, or is there a reason to have them different?

    Edit: my bad, bitcoin-qt wasn't compiled because of other changes I had.

    It would be nice to have the same error in the GUI?

    Uploading Screenshot 2019-06-29 at 01.48.10.png…

  29. [wallet] abort when attempting to fund a transaction above maxtxfee
    FundTransaction calls GetMinimumFee which, when the fee rate is absurdly high, quietly reduced the fee to -maxtxfee. Becaue an absurdly high fee rate is usually the result of a fat finger, aborting seems safer behavior.
    806b0052c3
  30. Sjors force-pushed on Jun 29, 2019
  31. Sjors commented at 2:53 AM on June 29, 2019: member

    I moved the check from RPC to the wallet.

    On a related note, I have in a work in progress branch which replaces all _failReason occurrences with TransactionError. @promag I'd rather not change the GUI messaging here.

  32. promag commented at 10:27 PM on June 29, 2019: member

    @promag I'd rather not change the GUI messaging here. @Sjors fair enough.

    Is it me or RPC bumpfee (thru feebumper::CreateRateBumpTransaction) doesn't check for -maxtxfee?

  33. Jaker9 approved
  34. laanwj approved
  35. laanwj commented at 2:02 PM on July 1, 2019: member

    Code review ACK 806b0052c3b45415862f74f20ba5f389e5b673de

  36. laanwj merged this on Jul 1, 2019
  37. laanwj closed this on Jul 1, 2019
  38. laanwj referenced this in commit 1212808762 on Jul 1, 2019
  39. MarcoFalke commented at 2:40 PM on July 1, 2019: member

    @promag Looks like it. At the very least we should have a test for this.

  40. Sjors commented at 2:52 PM on July 1, 2019: member

    I made a note to look into bumpfee and either add a test or fix the issue (happy to review if someone else does it first).

  41. Sjors deleted the branch on Jul 1, 2019
  42. laanwj deleted a comment on Jul 1, 2019
  43. in test/functional/rpc_psbt.py:139 in 806b0052c3 
     134 | +        res = self.nodes[1].walletcreatefundedpsbt([{"txid":txid,"vout":p2wpkh_pos},{"txid":txid,"vout":p2sh_p2wpkh_pos},{"txid":txid,"vout":p2pkh_pos}], {self.nodes[1].getnewaddress():29.99}, 0, {"feeRate": 0.1})
 135 | +        assert_greater_than(res["fee"], 0.05)
 136 | +        assert_greater_than(0.06, res["fee"])
 137 | +
 138 | +        # feeRate of 10 BTC / KB produces a total fee well above -maxtxfee
 139 | +        # previously this was silenty capped at -maxtxfee
    practicalswift commented at 9:29 AM on July 2, 2019:

    Post-merge typo: "silenty" :-)

    hebasto commented at 1:20 PM on July 2, 2019:

    Why didn't Travis catch it while running test/lint/lint-spelling.sh?

    practicalswift commented at 1:29 PM on July 2, 2019:

    I think it does, but lint-spelling.sh intentionally does not break the build in case of new typos. It just prints them to the Travis log for periodic speling correction :-)

  44. laanwj referenced this in commit 6c1e45c4c4 on Jul 10, 2019
  45. sidhujag referenced this in commit 6a23f381b5 on Jul 10, 2019
  46. fanquake commented at 3:21 AM on July 25, 2019: member

    Being backported in #16414.

  47. fanquake removed the label Needs backport on Jul 25, 2019
  48. Sjors commented at 12:14 PM on August 17, 2019: member

    0.17 backport in #16639

  49. Sjors referenced this in commit e9adb96f88 on Aug 19, 2019
  50. promag referenced this in commit 8f354ced6e on Aug 25, 2019
  51. laanwj referenced this in commit 1659b9b099 on Sep 12, 2019
  52. MarcoFalke referenced this in commit 3b04221183 on Sep 23, 2019
  53. gmaxwell commented at 11:18 AM on January 26, 2020: contributor

    oy. this interface takes feerates in btc but most ecosystem stuff takes feerates in satoshis. :( So not as likely a "fat finger" as just genuine misunderstanding about the interface.

  54. MarcoFalke referenced this in commit c2e53ff064 on Apr 17, 2020
  55. sidhujag referenced this in commit a041dfaaf0 on Apr 17, 2020
  56. deadalnix referenced this in commit ec2417bd70 on May 29, 2020
  57. meshcollider referenced this in commit 6bb5f6d8e3 on Jun 21, 2020
  58. sidhujag referenced this in commit 8b7f46f92c on Jul 7, 2020
  59. jnewbery removed this from the "Bugfixes" column in a project
  60. Munkybooty referenced this in commit 2a6ddb4baf on Nov 4, 2021
  61. Munkybooty referenced this in commit 8f421a49fc on Nov 4, 2021
  62. kittywhiskers referenced this in commit 4e62d2bbf0 on Dec 4, 2021
  63. kittywhiskers referenced this in commit aeb75e5e15 on Dec 8, 2021
  64. kittywhiskers referenced this in commit d050af7b4f on Dec 8, 2021
  65. kittywhiskers referenced this in commit 0915207866 on Dec 12, 2021
  66. kittywhiskers referenced this in commit b2ca69100c on Dec 12, 2021
  67. kittywhiskers referenced this in commit 9208854ca0 on Dec 12, 2021
  68. kittywhiskers referenced this in commit 32fa3d38f9 on Dec 13, 2021
  69. kittywhiskers referenced this in commit 367434c486 on Dec 13, 2021
  70. kittywhiskers referenced this in commit 43293d1063 on Dec 13, 2021
  71. kittywhiskers referenced this in commit cfa737c9c7 on Dec 17, 2021
  72. kittywhiskers referenced this in commit 642b776050 on Dec 17, 2021
  73. kittywhiskers referenced this in commit 3524044388 on Dec 19, 2021
  74. kittywhiskers referenced this in commit 80bf53ed9d on Dec 21, 2021
  75. kittywhiskers referenced this in commit dece025270 on Dec 22, 2021
  76. UdjinM6 referenced this in commit 6f4e11a34b on Jan 30, 2022
  77. Munkybooty referenced this in commit c516869bcf on Jan 30, 2022
  78. DrahtBot locked this on Feb 15, 2022
Contributors
Sjorspromagpracticalswiftgertjaapachow101jonasschnellimeshcolliderkallewoofDrahtBotMarcoFalkeJaker9laanwjhebastofanquakegmaxwell
Labels
Wallet

Milestone
0.18.1

Linked (view graph)
#16322 wallet: Fix -maxtxfee check by moving it to CWallet::CreateTransaction#16377 [rpc] don't automatically append inputs in walletcreatefundedpsbt#16414 0.18: wallet: Fix -maxtxfee check by moving it to CWallet::CreateTransaction#16539 wallet: lower -txmaxfee default from 0.1 to 0.01 BTC#16639 [0.17] abort when attempting to fund a transaction above -maxtxfee
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-04-26 12:14 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me