Skip to content
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.

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

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

Merged
merged 1 commit into from Jul 1, 2019

Conversation

@Sjors
Copy link
Member

commented Jun 21, 2019

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:
Schermafbeelding 2019-06-20 om 19 52 00

@fanquake fanquake added the Wallet label Jun 21, 2019

@promag

This comment has been minimized.

Copy link
Member

commented Jun 21, 2019

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.

@Sjors

This comment has been minimized.

Copy link
Member Author

commented Jun 21, 2019

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.

@Sjors

This comment has been minimized.

Copy link
Member Author

commented Jun 21, 2019

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)

@practicalswift

This comment has been minimized.

Copy link
Member

commented Jun 21, 2019

Concept ACK: user safety first!

@gertjaap

This comment has been minimized.

Copy link

commented Jun 24, 2019

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

@laanwj laanwj added this to the 0.18.1 milestone Jun 27, 2019

@laanwj laanwj added this to Bugfixes in High-priority for review Jun 27, 2019

@achow101

This comment has been minimized.

Copy link
Member

commented Jun 27, 2019

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?

@Sjors

This comment has been minimized.

Copy link
Member Author

commented Jun 27, 2019

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.

@jonasschnelli

This comment has been minimized.

Copy link
Member

commented Jun 27, 2019

  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 5735eb4

@meshcollider
Copy link
Member

left a comment

LGTM 5735eb4

Much safer behaviour IMO

Show resolved Hide resolved src/wallet/rpcwallet.cpp Outdated
@Sjors

This comment has been minimized.

Copy link
Member Author

commented Jun 27, 2019

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

@Sjors Sjors force-pushed the Sjors:2019/06/max_fee branch 3 times, most recently from a1dfa74 to 734c00d Jun 27, 2019

@kallewoof

This comment has been minimized.

Copy link
Member

commented Jun 28, 2019

Concept ACK

utACK 734c00d

@DrahtBot

This comment has been minimized.

Copy link
Contributor

commented Jun 28, 2019

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.

@MarcoFalke
Copy link
Member

left a comment

Concept ACK, some style questions

Show resolved Hide resolved src/util/fees.cpp Outdated
Show resolved Hide resolved doc/release-notes-16257.md Outdated
Show resolved Hide resolved src/qt/walletmodel.cpp
Show resolved Hide resolved src/wallet/rpcwallet.cpp Outdated
@promag

This comment has been minimized.

Copy link
Member

commented Jun 29, 2019

@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…

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

@Sjors Sjors force-pushed the Sjors:2019/06/max_fee branch from 734c00d to 806b005 Jun 29, 2019

@Sjors

This comment has been minimized.

Copy link
Member Author

commented Jun 29, 2019

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.

@promag

This comment has been minimized.

Copy link
Member

commented Jun 29, 2019

@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?

@Jaker9

Jaker9 approved these changes Jul 1, 2019

@laanwj

laanwj approved these changes Jul 1, 2019

Copy link
Member

left a comment

Code review ACK 806b005

@laanwj laanwj merged commit 806b005 into bitcoin:master Jul 1, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

laanwj added a commit that referenced this pull request Jul 1, 2019

Merge #16257: [wallet] abort when attempting to fund a transaction ab…
…ove -maxtxfee

806b005 [wallet] abort when attempting to fund a transaction above maxtxfee (Sjors Provoost)

Pull request description:

  `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">

ACKs for top commit:
  laanwj:
    Code review ACK 806b005

Tree-SHA512: bee95811711cdab100b614d2347921407af3b400aea613ca156953ed3f60b924ad29a1d335bd0e240c0b7c0fbb360226bab03294d226a5560cdf2a3f21e6d406
@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Jul 1, 2019

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

@Sjors

This comment has been minimized.

Copy link
Member Author

commented Jul 1, 2019

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

@Sjors Sjors deleted the Sjors:2019/06/max_fee branch Jul 1, 2019

@bitcoin bitcoin deleted a comment from Jaker9 Jul 1, 2019

assert_greater_than(0.06, res["fee"])

# feeRate of 10 BTC / KB produces a total fee well above -maxtxfee
# previously this was silenty capped at -maxtxfee

This comment has been minimized.

Copy link
@practicalswift

practicalswift Jul 2, 2019

Member

Post-merge typo: "silenty" :-)

This comment has been minimized.

Copy link
@hebasto

hebasto Jul 2, 2019

Member

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

This comment has been minimized.

Copy link
@practicalswift

practicalswift Jul 2, 2019

Member

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

laanwj added a commit that referenced this pull request Jul 10, 2019

Merge #16322: wallet: Fix -maxtxfee check by moving it to CWallet::Cr…
…eateTransaction

0d101a3 test: Add test for maxtxfee option (MarcoFalke)
1775501 wallet: Remove unreachable code in CreateTransaction (MarcoFalke)
5c1b971 wallet: Fix -maxtxfee check by moving it to CWallet::CreateTransaction (João Barbosa)

Pull request description:

  Follow up to #16257, this PR makes `bumpfee` aware of `-maxtxfee`.

  It also prevents dangling locked unspents when calling `fundrawtransaction` - because the previous check was after `LockCoin`.

ACKs for top commit:
  MarcoFalke:
    re-ACK 0d101a3, only change is small test fixup

Tree-SHA512: 3464b24ae7cd4e72ed41438c6661828ba1304af020f05da62720b23668ae734e16cf47c6d97e150cc84ef631ee099b16fc786c858f3d089905845437338fd512

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Jul 10, 2019

Merge bitcoin#16322: wallet: Fix -maxtxfee check by moving it to CWal…
…let::CreateTransaction

0d101a3 test: Add test for maxtxfee option (MarcoFalke)
1775501 wallet: Remove unreachable code in CreateTransaction (MarcoFalke)
5c1b971 wallet: Fix -maxtxfee check by moving it to CWallet::CreateTransaction (João Barbosa)

Pull request description:

  Follow up to bitcoin#16257, this PR makes `bumpfee` aware of `-maxtxfee`.

  It also prevents dangling locked unspents when calling `fundrawtransaction` - because the previous check was after `LockCoin`.

ACKs for top commit:
  MarcoFalke:
    re-ACK 0d101a3, only change is small test fixup

Tree-SHA512: 3464b24ae7cd4e72ed41438c6661828ba1304af020f05da62720b23668ae734e16cf47c6d97e150cc84ef631ee099b16fc786c858f3d089905845437338fd512
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.