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

Introduce hard limit on transaction fee (core) #1087

Closed
tsusanka opened this issue Jun 30, 2020 · 15 comments
Closed

Introduce hard limit on transaction fee (core) #1087

tsusanka opened this issue Jun 30, 2020 · 15 comments
Assignees
Labels
core Trezor Core firmware. Runs on Trezor Model T and T2B1. feature Product related issue visible for end user good first issue Issue for newbie developers who want to participate

Comments

@tsusanka
Copy link
Contributor

tsusanka commented Jun 30, 2020

We currently have soft warning when transaction fee seems too high. For a usual small segwit transaction the limit is around 0.003 BTC which is ~30 USD at the current price (~9000 USD).

When there is a really big fee (by accident or malware) we are just a user's click away from spending a huge fee. As requested by Product, let's introduce a hard limit which is 10x more than the soft limit and may be turned of using advanced setting introduced in #1064. By hard limit I mean we refuse to sign and raise an error.

@tsusanka tsusanka added core Trezor Core firmware. Runs on Trezor Model T and T2B1. feature labels Jun 30, 2020
@tsusanka tsusanka added this to the backlog milestone Jun 30, 2020
@destrys

This comment has been minimized.

@tsusanka

This comment has been minimized.

@tsusanka tsusanka added the good first issue Issue for newbie developers who want to participate label Jul 16, 2020
@tsusanka tsusanka modified the milestones: backlog, 2020-09 Jul 24, 2020
@tsusanka
Copy link
Contributor Author

tsusanka commented Aug 3, 2020

@mmilata would you be interested in a small research? There are two things we would like to look at.


First let's have a look on the current fee limits. That means create a table for top10 Bitcoin-like coins that we support (or simpler way: use the ones that we currently support in wallet.trezor.io) and list for each one of them:

  1. The current maxfee_kb.
  2. The current fee_threshold for a "traditional" transaction (let's say something the user commonly deals with, 2 inputs, 2 outputs, doesn't have to be exact).
  3. The current fee_threshold in USD. Basically (2) but in USD.
  4. The hard limit we suggest (that is 10x (3) but it would be nice to see it in one table).

And second, let's investigate how we deal with fee thresholds in the other non-Bitcoin-like coins because I am not sure we do.


Do you think you could look at it at some point in future? The Port has definitely a priority.

@mmilata
Copy link
Member

mmilata commented Aug 3, 2020

@tsusanka sure, can do that

@mmilata
Copy link
Member

mmilata commented Aug 11, 2020

@tsusanka For some of the bitcoin-based coins, the threshold for 250B transaction is following:

Coin maxfee_kb fee_threshold USD rate fee_threshold (USD) hard limit (USD)
BTC 2000000 500000 sats = 0.005 11500.0 57.5 575
BCH 500000 125000 sats = 0.00125 280.0 0.4 4.0
BTG 500000 125000 sats = 0.00125 10.0 0.01 0.1
DASH 100000 25000 sats = 0.00025 90.0 0.02 0.2
DGB 500000 125000 sats = 0.00125 0.03 0.00004 0.0004
DOGE 1000000000 250000000 sats = 2.5 0.003 0.008 0.08
LTC 40000000 10000000 sats = 0.1 55.0 5.5 55.0
NMC 10000000 2500000 sats = 0.025 0.4 0.01 0.1
VTC 40000000 10000000 sats = 0.1 0.3 0.03 0.3
ZEC 1000000 250000 sats = 0.0025 85.0 0.2 2.0

Other coins such as ETH, ETC, NEM, XLM, ADA, XTZ don't have any warning or hard limit implemented.

@tsusanka
Copy link
Contributor Author

Closed by #1155 with two follow-up issues #1191 and #1192.

@tsusanka
Copy link
Contributor Author

tsusanka commented Aug 17, 2020

QA

  • Please test BTC transaction signing (both Wallet and Suite).
    • With normal fee (as suggested by Wallet/Suite).
    • With fee around 100 USD. Warning should be shown. (I am not sure you can test that?)
    • WIth ridiculous fee around 1000 USD. Error should be raised. (I am not sure you can test that?)

@tsusanka
Copy link
Contributor Author

@bosomt please include a picture of Suite/Wallet when you are testing it so support can have a look.

@bosomt
Copy link

bosomt commented Aug 25, 2020

maximum fee in suite is 2000 sat/b
i will check for another ways to test hard limit

@bosomt
Copy link

bosomt commented Aug 28, 2020

QA OK

tested 2.3.3, no warning in older FW
@tsusanka is this feature core specific or is this legacy T1 included ?

FW version 2.3.3 df5421e
Wallet Git Commit: ec6b4d36cbae2ec1c3f479eb3c1891090790b7c0

wallet log :

["log","Fri, 28 Aug 2020 07:40:33 GMT","[trezor.js] [call] Sending","ButtonAck",{}]
["log","Fri, 28 Aug 2020 07:40:37 GMT","[trezor.js] [call] Received","Failure",{"code":"Failure_DataError","message":"The fee is unexpectedly large"}]
["log","Fri, 28 Aug 2020 07:40:37 GMT","[trezor.js] [session] releasing"]
["error","Fri, 28 Aug 2020 07:40:37 GMT","[AccountSendCtrl] Failed to send transaction","Error: The fee is unexpectedly large"]

image

@tsusanka
Copy link
Contributor Author

TT only for now. We will introduce it to T1 via Port.

@bosomt
Copy link

bosomt commented Aug 28, 2020

tried Fee: $211.06

image
image

@bosomt
Copy link

bosomt commented Aug 28, 2020

test results for Electrum 4.0.2:

  1. treshold: same results, warning on device and in Electrum
  2. hard limit:

image

maybe we will need fix for Electrum to properly handle FW error returned by device when you hit hard limit ? @matejcik

@tsusanka
Copy link
Contributor Author

This is a huge edge case let's not worry about that. (Unless this happens for every wire.DataErorr then it might be worth the fix.)

@prusnak
Copy link
Member

prusnak commented Mar 21, 2022

Let's implement this for Legacy too.

@prusnak prusnak added T1B1 legacy Trezor One and removed core Trezor Core firmware. Runs on Trezor Model T and T2B1. labels Mar 21, 2022
@prusnak prusnak removed this from the 2020-09 milestone Mar 21, 2022
@hynek-jina hynek-jina added the feature Product related issue visible for end user label Mar 23, 2022
@hynek-jina hynek-jina changed the title Introduce hard limit on transaction fee Introduce hard limit on transaction fee (core) Mar 28, 2022
@hynek-jina hynek-jina added core Trezor Core firmware. Runs on Trezor Model T and T2B1. and removed T1B1 legacy Trezor One labels Mar 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Trezor Core firmware. Runs on Trezor Model T and T2B1. feature Product related issue visible for end user good first issue Issue for newbie developers who want to participate
Projects
None yet
Development

No branches or pull requests

7 participants