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.

Sign up for GitHub

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

Jump to bottom

Type-checking for Ethereum app #1794

Merged
merged 12 commits into from
Sep 10, 2021
Merged

Type-checking for Ethereum app #1794

merged 12 commits into from
Sep 10, 2021

Conversation

matejcik
Copy link
Contributor

@matejcik matejcik commented Sep 2, 2021 •
edited
Loading

includes #1771 which was already reviewed (2b3b75d)

includes and supersedes #1781 which already had a review from @prusnak

there are following protobuf changes:

  • from feat(core,legacy): add support for Ethereum 64-bit chain_id #1771, chain_id is enlarged to uint64. This is fully backwards-compatible.
  • chain_id is set to required. This removes support for non-EIP-155 networks. Our support database is keyed to EIP-155 chain_id, so we do not officially support any non-EIP-155 network. It's unclear if any exist at all.
    • TODO: core should probably disallow chain_id==0. It's not invalid per se, far as I could find out, but per this comment it will probably simply not work with libraries. T1 already disallows it.
  • gas_price and gas_limit set to required. Both T1 and TT had checks that would reject messages with gas_price or gas_limit unset, so while it's an incompatible change on the protobuf level, we know that no app could possibly have been sending such messages.
  • a bunch of other fields are set to required that we know were required already (parameters to sign/verify, data chunk in TxAck...)

UI diff: https://satoshilabs.gitlab.io/-/trezor/trezor-firmware/-/jobs/1556349947/artifacts/master_diff/index.html
the data_streaming case differs because we're specifying chain_id 1 so the transfer registers as Ethereum (previously UNKN)

TODO:

  • add restriction that chain_id >= 1
  • add correct changelogs
  • enable Palm network

@matejcik
Copy link
Contributor Author

matejcik commented Sep 2, 2021

  • core should probably disallow chain_id==0. It's not invalid per se, far as I could find out, but per this comment it will probably simply not work with libraries. T1 already disallows it.

ISTM the reason for disallowing chain_id==0 was to simplify implementation, so that 0 internally indicates "EIP-155 disabled". I'm inclined to simplify code and enable chain_id 0 for T1 as well.

@matejcik matejcik mentioned this pull request Sep 2, 2021
Closed
@matejcik matejcik force-pushed the matejcik/ethereum-typing branch from 836d619 to e282662 Compare September 2, 2021 11:16
@matejcik matejcik marked this pull request as ready for review September 2, 2021 11:45
@matejcik matejcik requested review from mmilata and removed request for onvej-sl and andrewkozlik September 2, 2021 11:45
mmilata
mmilata approved these changes Sep 7, 2021
View reviewed changes
Copy link
Member

@mmilata mmilata left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't been able to spot any problem. Together with the Stellar PR this makes core more than 70% typed:ok_hand: What about the zero chain_id?

@matejcik
Copy link
Contributor Author

matejcik commented Sep 7, 2021

i'm trying to ask around if there are reasons for/against chain_id 0

@matejcik
Copy link
Contributor Author

matejcik commented Sep 7, 2021

further information on chain_id 0:
https://ethereum-magicians.org/t/discussion-to-eip-3788-strict-enforcement-of-chainid/7001
ethereum/go-ethereum#23152
https://eips.ethereum.org/EIPS/eip-3788

haven't read through it all yet

@matejcik
Copy link
Contributor Author

matejcik commented Sep 7, 2021

Conclusion seems to be: geth also uses chain_id == 0 to indicate pre-EIP-155 transactions (codebase confirms this), so while nothing explicitly disallows this value, realistically nothing works with that value either.

Pre-EIP-155 transactions can currently be used deliberately to produce cross-chain-replayable transactions. So by forcing EIP-155 on Trezor, we disallow the cross-chain usecase.
The above linked EIP-3788 aims to disallow this practice, but it's very new so it's unclear if it's going anywhere.

@arbitrarylink
Copy link
Contributor

If this PR gets approved, will the next release of the Firmware also include an updated version of Trezor Connect?

@matejcik
Copy link
Contributor Author

matejcik commented Sep 9, 2021 •
edited
Loading

Final decision about chain_id:

  • we are keeping chain_id as required field, thus disallowing non-EIP-155 cross chain replayable transactions.
  • we are explicitly disallowing chain_id == 0 to prevent confusion if someone wanted to try signing a non-EIP-155 transaction

I also reorganized the sanity checks somewhat, so that chain_id == 0 is checked in EIP1559 as well (arguably it's unnecessary there, but better safe than sorry)

@mmilata PTAL at the new commits

@matejcik matejcik requested a review from mmilata September 9, 2021 09:11
mmilata
mmilata approved these changes Sep 9, 2021
View reviewed changes
Copy link
Member

@mmilata mmilata left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

New changes ACK.

arbitrarylink and others added 10 commits September 10, 2021 13:55
@arbitrarylink @matejcik
feat(core,legacy): add support for Ethereum 64-bit chain_id
9579d25 
* Changes from original PR

* Now that we are rejecting chain_ids of 0, we need to have the tests set the chain_ids to at least 1.

* Ran 'make gen' and uploaded changed files.

* Ran make style_check and fixed reported errors

* Added changelog files

* Reverted changes concerning chain_id 0 being rejected.

* Adds tests for MAX_CHAIN_ID and MAX_CHAIN_ID+1.  Also reverts MAX_CHAIN_ID to the previous value.

* Added missing whitespace around arithmetic operator.

Co-authored-by: Michael Hatton <michaelhatton@Michaels-Mini.fios-router.home>
@matejcik
feat(all): make chain_id mandatory
24720e8
@matejcik
refactor(core,legacy): remove Wanchain special handling
d670ab0 
now that Wanchain has proper chainID and we don't need to detect it by
magic.

[no changelog]
@matejcik
feat(common): add default values to EthereumSignTx fields
7dafac8 
so that we can avoid Optional types for fields
@matejcik
fix(core): allow AnyStr as parameter for unhexlify
33ff002
@matejcik
feat(core/ethereum): use TokenInfo object instead of tuples
2050eef
@matejcik
feat(core/ethereum): enable type-checking for Ethereum app
70224ab
@matejcik
fix(legacy): make use of Ethereum required fields
bcf95f6
@matejcik
chore(tests): regenerate UI test fixtures
077918b
@matejcik
chore(common): enable Palm network
1e10a78
@matejcik matejcik force-pushed the matejcik/ethereum-typing branch from 16df0d6 to 800ae5c Compare September 10, 2021 11:55
@matejcik matejcik merged commit d318a29 into master Sep 10, 2021
@matejcik matejcik deleted the matejcik/ethereum-typing branch September 10, 2021 13:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mmilata mmilata mmilata approved these changes

@prusnak prusnak Awaiting requested review from prusnak

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@matejcik @arbitrarylink @mmilata