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

Firo UXTO unspendable if there are shielded outputs #1588

Closed
hakonamatata opened this issue May 2, 2021 · 43 comments
Closed

Firo UXTO unspendable if there are shielded outputs #1588

hakonamatata opened this issue May 2, 2021 · 43 comments
Assignees
Labels
bug Something isn't working as expected

Comments

@hakonamatata
Copy link

I am starting to get a bit worried. I can not sign transactions and spend/send. I have tried one input to one output. One input to an output and a change address. Basic transactions. Nothing works, I get this firmware error every time.

Firmware version and revision

Using Trezor model T

Firmware version
2.3.6

Desktop/smartphone setup (please complete the following information):
Tried using Trezor Suite and Electrum, same issue. Does not matter what wallet I use.

To Reproduce
Any transaction with Firo it seems. Fails with Firmware error every time.

Screenshots
image

Additional context
I am using Firo https://www.firo.org/

All my coins are locked with Trezor, but if I can't spend that is a big problem.

@hakonamatata hakonamatata added the bug Something isn't working as expected label May 2, 2021
@prusnak
Copy link
Member

prusnak commented May 2, 2021

What Firo wallet is on the screenshot? Is that Electrum-Firo? Which version?

@hakonamatata
Copy link
Author

What Firo wallet is on the screenshot? Is that Electrum-Firo? Which version?

Firo Electrum Version 4.0.9.2

@prusnak
Copy link
Member

prusnak commented May 2, 2021

Firo Electrum Version 4.0.9.2

Please report the issue to their issue tracker - https://github.com/firoorg/electrum-firo/issues - they are sending coin name Zcoin even though the coin was renamed to Firo: https://github.com/firoorg/electrum-firo/blob/21052243d57b3c51fbd6aecaf3353e1babef7454/electrum_dash/plugins/trezor/trezor.py#L210

@a-bezrukov
Copy link
Contributor

@prusnak,
Pavol,
Zcoin was rebranded to Firo on March 25 #1544 (comment)
The latest firmwares for Trezor are dated 15 February 2021 (Model T) and 10 February 2021 (Trezor One). So, this change should not be in the firmwares yet.
Am I right thinking that Model T firmware 2.3.6 should have coin = "Zcoin"?

@matejcik
Copy link
Contributor

matejcik commented May 4, 2021

regardless, bad coin name should not result in "Firmware error"

@matejcik matejcik reopened this May 4, 2021
@prusnak
Copy link
Member

prusnak commented May 4, 2021

regardless, bad coin name should not result in "Firmware error"

How about a missing input value? Should that also return a more meaningful error?

@matejcik
Copy link
Contributor

matejcik commented May 4, 2021

Missing required field should raise a meaningful error from protobuf. Missing optional-but-logically-required field should be handled cleanly as well, usually by a DataError.

I'm guessing this might either be a memory error, or something fishy is going on.

@hakonamatata
Copy link
Author

Got another error message trying to send now:

Failed to send transaction
Error details: Failed to decode message: memory allocation failed, allocating 13854 bytes

@matejcik
Copy link
Contributor

matejcik commented May 4, 2021

Well, this is now up to you. Why do you need to send 13 kilobytes of protobuf to the Trezor to sign an 1-input 1-output tx?

@hakonamatata
Copy link
Author

Well, this is now up to you. Why do you need to send 13 kilobytes of protobuf to the Trezor to sign an 1-input 1-output tx?

I am just trying to send a normal transaction, no idea what protobuf is or how I prevent trezor from including this

@prusnak
Copy link
Member

prusnak commented May 4, 2021

TX size is 226 bytes according to screenshot 🤔

@matejcik
Copy link
Contributor

matejcik commented May 4, 2021

Probably a bug in Firo Electrum. Could even be Trezorlib in theory?

@hakonamatata
Copy link
Author

Probably a bug in Firo Electrum. Could even be Trezorlib in theory?

This error above with

Failed to send transaction Error details: Failed to decode message: memory allocation failed, allocating 13854 bytes

Was from using https://wallet.trezor.io

@prusnak
Copy link
Member

prusnak commented May 4, 2021

@matejcik wallet.trezor.io doing it too means it has been streaming a rather big prev-tx

@hakonamatata try removing your homescreen and reboot the device, this could help

@matejcik
Copy link
Contributor

matejcik commented May 4, 2021

Prev-txes are also streamed piece-by-piece though - precisely to avoid this problem. I have no idea how you can even get to 13 kilobytes! The only thing comes to mind is sending more data than Trezor is in fact requesting.

@matejcik
Copy link
Contributor

matejcik commented May 4, 2021

@hakonamatata are you able to reproduce the problem with a testnet account?

@matejcik
Copy link
Contributor

matejcik commented May 4, 2021

I set up a testnet account and cannot reproduce the problem. Could be indeed something about the previous transaction, but in that case we would need a reproducer.

@hakonamatata
Copy link
Author

A previous transaction can look like this:

https://explorer.firo.org/tx/bc9f34adcd45944fc1cb3c6b806f3cacd291e1ef75441691a59758fd368f75d1

Lots of inputs and outputs from mining.

There are also transactions like this, with "Extra payload"

image

@yura-pakhuchiy
Copy link
Contributor

yura-pakhuchiy commented May 4, 2021

@hakonamatata is one of the prev tx is lelantus/sigma/zerocoin spend? Privacy transactions are bigger in size, attempt to stream them would explain memory allocation problem

@prusnak
Copy link
Member

prusnak commented May 4, 2021

@hakonamatata is one of the prev tx is lelantus/sigma/zerocoin spend? Privacy transactions are bigger in size, attempt to stream them would explain memory allocation problem

Yes, it is - see firoorg/electrum-firo#3 (comment)

@yura-pakhuchiy
Copy link
Contributor

@prusnak How this case can be supported in Trezor? Would it be acceptable solution to allow lifting streaming prev tx requirement for Firo transactions? Eg. Electrum-Firo will check are the any privacy tx in prev tx:

  1. If there are none, then stream all prev tx and sign transaction normally.
  2. If there some privacy tx, then Electrum-Firo passes flag to lift streaming requirement and Trezor shows extra warning that transaction amount is not validated, user should trust wallet software for this tx.

@prusnak
Copy link
Member

prusnak commented May 4, 2021

Would it be acceptable solution to allow lifting streaming prev tx

That's not needed really. For Zcash we also stream the extra data via the extra_data field - Trezor controls how much data it is requesting, so there should be no memory issue.

@yura-pakhuchiy
Copy link
Contributor

yura-pakhuchiy commented May 4, 2021

For Zcash we also stream the extra data via the extra_data field

Unlike Zcash, in Firo the long part placed not after transaction, but inside scriptSig, eg. check this transaction out:
https://blockbook.firo.org/tx/a89769cc65c8404d0fe863f6b8a777be66ecb062f4dc6cdda4f09ba324bcffca
It seems extra_data field will not help in this case. Or am I missing something about how extra_data works?

@matejcik
Copy link
Contributor

matejcik commented May 5, 2021

the long part placed not after transaction, but inside scriptSig

I see.
For reference, the above linked transaction has 29 kB of scriptSig. This also means that such transaction cannot be signed on T1, as the scriptSig size limit is 1650 bytes. There is no explicit limit on TT, but this will fail for huge txes anyway because you need two consecutive 29kB blocks of RAM, which most likely won't be available.

There is currently no solution for streaming contents of scriptSig. I suppose we could implement one, but such feature is going to the backlog from the outset.

I'm afraid that turning off prevtx validation is out of the question, unless Firo signs over both the amount and the scriptSig of the previous transaction.

@yura-pakhuchiy
Copy link
Contributor

yura-pakhuchiy commented May 5, 2021

For reference, the above linked transaction has 29 kB of scriptSig. This also means that such transaction cannot be signed on T1

Will 29 kB transaction work on both TT and T1 if the long part will be moved away from scriptSig to extra data?

I'm afraid that turning off prevtx validation is out of the question, unless Firo signs over both the amount and the scriptSig of the previous transaction.

Firo uses pre segwit signing, so it does not sign amount, but AFAIK even segwit signing requires streaming now due to possibility of some clever attack.

Could you give your reasoning why turning off prevtx validation is out of the question? It seems to me that showing warning on Trezor and turning off prevtx validation only for Firo (not all coins) and only if there is huge prev tx present, is a lot better solution than forcing user to enter his seed in software wallet (and compromising security for all coins/accounts forever, rather then only 1 Firo account for 1 transaction). This can be protected by safety-checks trezorctl option as well, though I think just showing warning on Trezor is enough since it does not affect security of other coins.

@matejcik
Copy link
Contributor

matejcik commented May 6, 2021

Will 29 kB transaction work on both TT and T1 if the long part will be moved away from scriptSig to extra data?

Yes. The length of extra data is effectively unlimited.

since it does not affect security of other coins.

We do not want this feature configurable per-coin, for complexity & auditability reasons; and given that, we currently do not want the feature present in the firmware at all.

I will note that we might revisit this decision in the future if there are more usecases.


I will also note that with the recent work on RAM pressure (#1565), it might very well be possible to sign big scriptSigs on TT in the upcoming firmware; and if not on the official one, hand-compiling a firmware without the FIDO module is a relatively straightforward way to gain more RAM. This would at least allow users to spend their funds without needing to input the seed on a software wallet.

@prusnak
Copy link
Member

prusnak commented May 6, 2021

we currently do not want the feature present in the firmware at all.

In that case I'd suggest removing Firo from the firmware completely so people will not run into problems when using Trezor (unspendable coins) - #1602

prusnak added a commit that referenced this issue May 6, 2021
using Firo leads to unspendable coins, let's remove this coin
until we can spend the coins again

context:
- #1588
- firoorg/electrum-firo#3
@yura-pakhuchiy
Copy link
Contributor

@prusnak This will be even worse, because it will lockout all current masternode users who use Trezor to secure their stake. I think it is better to add warning to Firo Core (the only wallet with support of privacy transactions) that privacy funds should not be sent directly to Trezor wallet, but spent to Firo Core (or another software wallet) managed address first and only then sent to Trezor.

Also Firo probably can change privacy tx format in the future to reduce scriptSig length and use extra data. I will open issue in theirs repo.

@hakonamatata
Copy link
Author

@prusnak This will be even worse, because it will lockout all current masternode users who use Trezor to secure their stake. I think it is better to add warning to Firo Core (the only wallet with support of privacy transactions) that privacy funds should not be sent directly to Trezor wallet, but spent to Firo Core (or another software wallet) managed address first and only then sent to Trezor.

Also Firo probably can change privacy tx format in the future to reduce scriptSig length and use extra data. I will open issue in theirs repo.

I agree with this, please don't remove Firo from Trezor. Lots of people already use Trezor for their masternodes, and depend on this. It still works, as long as they don't use Lelantus with Trezor. Hopefully this can be fixed by the Firo team.

@yura-pakhuchiy
Copy link
Contributor

I would like to add that privacy transactions are opt-in in Firo and if user make privacy transaction, then by default Firo Core will send funds to Core controlled address. Sending to external address is rarely used feature.
At the same time, every masternode requires 1000 FIRO collateral (which is probably much larger amount than average privacy transaction). Firo supported by Trezor since 2018 and a lot of people use it for masternodes, if support will be removed it will lockout much larger amounts of funds for masternode owners, than protect few people who will ignore warning and make Lelantus spend to Trezor directly.

@reubenyap
Copy link

Hi @prusnak @matejcik , I'm from the Firo team, our developers are considering the best way to deal with this and as many of our existing users rely on Trezor for securing their existing masternodes, would appreciate some time for us to resolve this (esp since this requires a hard fork).

As @yura-pakhuchiy mentioned, users can still make valid tx and just don't use Lelantus when sending to Trezor but obviously we would like to resolve this issue permanently.

@alex-jerechinsky alex-jerechinsky modified the milestone: backlog May 10, 2021
@matejcik
Copy link
Contributor

Hello @reubenyap @yura-pakhuchiy,
after an internal discussion, we decided to drop the support flag for Firo for the time being.
This means that the coin definition will remain in Trezor repository, and we intend to keep them in the upcoming T1 FW 1.10.0 and TT 2.4.0, but the coin will be delisted from trezor.io/coins, Trezor Connect, and possibly Trezor Wallet.
The coin will also be removed from future firmware versions beyond the above.

We will flip the support flag back on when this situation is resolved. In the meantime, please instruct your users to not upgrade beyond 1.10.0 / 2.4.0; alternately, it should be possible to safely downgrade to these versions in order to transact Firo.

@a-bezrukov
Copy link
Contributor

@matejcik,
We are investigating on how to solve it. What are the memory size limits that a client can rely on when passing data with extra payload?

@yura-pakhuchiy
Copy link
Contributor

yura-pakhuchiy commented May 10, 2021

@a-bezrukov there is no limit. #1588 (comment)

Will 29 kB transaction work on both TT and T1 if the long part will be moved away from scriptSig to extra data?

Yes. The length of extra data is effectively unlimited.

@matejcik
Copy link
Contributor

We are investigating on how to solve it. What are the memory size limits that a client can rely on when passing data with extra payload?

extra_data, i.e., data that are after the last output and locktime value, can be of any length. Trezor requests those in 1024-byte chunks (or whatever the TxRequest message specifies) and they are not stored, only hashed.

Data in a single script_sig are limited to 1650 bytes, script_pubkey can be 520 bytes. We could raise these limits to about twice that, but definitely not tens of kB.

@a-bezrukov
Copy link
Contributor

Hi Guys,
We are reworking these large transactions and moving the long data into the extra payload area. It's hard to provide any ETA on this, but this work is starting right now.
Thanks.

@hakonamatata
Copy link
Author

Hi Guys,
We are reworking these large transactions and moving the long data into the extra payload area. It's hard to provide any ETA on this, but this work is starting right now.
Thanks.

That is good to hear! Thanks

@hakonamatata
Copy link
Author

Updated to latest version of Trezor T, 4.2.0. Now you can no longer see your accounts in the trezor wallet. I does still work show up with Electrum.

image

@prusnak
Copy link
Member

prusnak commented Jul 10, 2021

@hakonamatata please contact our support via https://trezor.io/support/technical/ form, so they can get information for you to track down the issue

@hakonamatata
Copy link
Author

hakonamatata commented Jul 14, 2021

@hakonamatata please contact our support via https://trezor.io/support/technical/ form, so they can get information for you to track down the issue

Maybe I was not clear, everything works fine except for Firo, when I change the "backend server url" to "https://explorer.firo.org/", then I get this error. Before it would show all my Firo wallets.

@andrewkozlik
Copy link
Contributor

Updated to latest version of Trezor T, 4.2.0. Now you can no longer see your accounts in the trezor wallet. I does still work show up with Electrum.

This is a result of 2fac964, which disables Firo not only in Trezor but also in trezor-connect and Suite. So at the moment this is expected behavior.

  • Trezor Suite and Trezor Wallet will no longer work with Firo for any firmware version of Trezor.
  • You need to use Electrum and firmware versions 1.10.0 (Trezor 1) or 2.4.0 (Trezor T) or earlier.
  • Newer firmware versions won't work for Firo, even with Electrum!

This state will last at least until the above issue is resolved, i.e. until Firo coin moves the long scriptSig data into the extra data area of the transaction.

@matejcik correct me if I am wrong.

@reubenyap
Copy link

We have moved the long scriptSig data and has been released. in v14.8.0
https://github.com/firoorg/firo/releases/tag/v0.14.8.0

Awaiting hard fork that would activate at block 401580 (approximately August 26 2021).

Please see:
firoorg/firo#1063
firoorg/firo@2cfa8e9

@matejcik @prusnak @andrewkozlik

@matejcik matejcik changed the title RuntimeError('FirmwareError: firmware error') when trying to sign a simple transaction Firo UXTO unspendable if there are shielded outputs Aug 19, 2021
@matejcik
Copy link
Contributor

@reubenyap thanks for the info. Please open a new issue detailing what needs to be done on firmware side. I believe with this change it should be enough to re-enable Firo support?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working as expected
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants