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

1. 
   Sjors commented at 0: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:

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

   After:

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

   QT still checks the max fee rate as expected:

2. fanquake added the label Wallet on Jun 21, 2019
3. 
   promag commented at 0: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:

    0        // reject absurdly high fee. (This can never happen because the
    1        // wallet caps the fee at m_default_max_tx_fee. This merely serves as a
    2        // belt-and-suspenders check)
    3        if (nFeeRequired > m_wallet->getDefaultMaxTxFee())
    4            return AbsurdFee;
    

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

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

    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 0: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?

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
Sjors promag practicalswift gertjaap achow101 jonasschnelli meshcollider kallewoof DrahtBot MarcoFalke Jaker9 laanwj hebasto fanquake gmaxwell

Labels
Wallet

Milestone
0.18.1