Signed Ethereum network and token definitions from host #2410

marnova · 2022-07-25T09:47:50Z

Enables to send signed Ethereum network and token definitions from host.
Related to #15 (high-level plan described in its comment #15 (comment)).

matejcik

partial review for now

common/defs/ethereum/eth_builtin_networks.json

common/defs/README.md

python/src/trezorlib/ethereum.py

core/tests/test_apps.ethereum.tokens.py

core/tests/test_apps.ethereum.layout.py

common/protob/messages-ethereum.proto

grdddj · 2022-10-13T16:08:41Z

I have some possibly good news including binary size.

So far we have considered the size of tokens.py and networks.py definitions around 90 KB in total - that is their size connected with src/apps/ethereum/tokens.py and networks,py respectively.

Now I tried to actually remove these two files from the build and got incredible results:

...
.flash2    -8_148  1    L-8_148  D0       mp_qstr_frozen_const_pool
.flash2    -13_068 135  L-10_956 D-2_112  src/apps/ethereum/networks.py                                _networks_iterator()
.flash2    -78_727 2054 L-45_911 D-32_816 src/apps/ethereum/tokens.py                                  token_by_chain_address()
.flash2    -87_716 1    L-87_716 D0       [section .flash2]
SUMMARY: 41 rows, -193_024 bytes in total (L-156_640 D-36_384).

There is that ~90 kb size decrease in src/apps/ethereum, but also other big decrease in mp_qstr_frozen_const_pool and mostly .flash2 section - str1 symbol in particular.

This means that if we can get rid of the most information in them, we are looking at a HUGE size decrease.

Holding the thumbs we can really replace it!

grdddj · 2022-10-13T17:05:34Z

I also tried to replace tokens.py with tokens.rs and exposing trezorui2.token_by_chain_address(chain_id: int, address: bytes) - it seems to be around 70 kB (say 45 %) smaller.

We could theoretically try to use this with coininfo.py, could save some kilobytes.

matejcik

legacy part review done.

feel free to rebase before you start working on these changes

legacy/firmware/ethereum_definitions.c

legacy/firmware/ethereum_networks.c.mako

legacy/firmware/ethereum_definitions.h

legacy/firmware/fsm_msg_ethereum.h

matejcik

I have rebased on master and squashed the commit history somewhat.
Resolved all comments from before, most of them seem to have been resolved, the others we'll find and fix again. Only thing I'm leaving up is the most recent pending review.

common/protob/protocol.md

core/.changelog.d/15.removed

core/src/apps/ethereum/definitions.py

core/src/apps/ethereum/keychain.py

core/src/apps/ethereum/layout.py

core/tests/test_apps.ethereum.keychain.py

core/tests/test_apps.ethereum.definitions.py

common/defs/README.md

common/tools/coin_info.py

grdddj

Some comments from reviewing core and python changes

core/src/apps/ethereum/definitions.py

core/src/apps/ethereum/get_public_key.py

python/src/trezorlib/ethereum.py

python/tools/eth_defs_unpack.py

matejcik · 2023-03-15T13:08:30Z

@mmilata please review mainly, in order of priority:

legacy implementation
core implementation
documentation

device tests are currently broken, that is a known problem

mroz22 · 2023-03-17T08:37:35Z

docs/common/ethereum-definitions.md

+`<TOKEN_ADDRESS>` is all lowercase token address hex without the `0x` prefix.
+
+E.g., this is the URL for Görli TST token:
+[https://data.trezor.io/firmware/eth-definitions/chain-id/5/token-7af963cf6d228e564e2a0aa0ddbf06210b38615d.dat]


this does not work
https://data.trezor.io/firmware/eth-definitions/chain-id/5/token-7af963cf6d228e564e2a0aa0ddbf06210b38615d.dat

this does
https://data.trezor.io/firmware/eth-definitions/by_chain_id/5/token_7af963cf6d228e564e2a0aa0ddbf06210b38615d.dat

expected behavior. also the version on your old-style URL will be rejected by current code in this branch.
i'll try to get the new links up ASAP

onvej-sl · 2023-03-23T18:03:49Z

By the way, what is the motivation in this case for implementing a k-out-of-n signature using aggregated public keys and aggregated signatures instead of verifying each of k signatures separately?

matejcik · 2023-03-24T09:00:58Z

By the way, what is the motivation in this case for implementing a k-out-of-n signature using aggregated public keys and aggregated signatures instead of verifying each of k signatures separately?

the main motivation is that CoSi is a thing we know well and we can piggy-back on the same functionality as firmware signing; n.b., it's the only way to sign arbitrary data on Trezor, otherwise we'd need to go with something like T1's SignMessage method.
the other (smaller) reason is space saving, because it's more practical to make the definition blobs as small as possible

also fixes #2759

matejcik · 2023-03-24T10:52:10Z

For now i am leaving the signing format as is and I'd like to merge, unless you disagree @andrewkozlik @onvej-sl ?
If you feel like we should change it, please file an issue and we'll do that separately.

andrewkozlik · 2023-03-24T11:24:15Z

By the way, what is the motivation in this case for implementing a k-out-of-n signature using aggregated public keys and aggregated signatures instead of verifying each of k signatures separately?

the main motivation is that CoSi is a thing we know well and we can piggy-back on the same functionality as firmware signing; n.b., it's the only way to sign arbitrary data on Trezor, otherwise we'd need to go with something like T1's SignMessage method. the other (smaller) reason is space saving, because it's more practical to make the definition blobs as small as possible

The problem with CoSi is that it's difficult to use correctly. There is the already mentioned rogue public key attack. In addition, the process of combining the "commitments" is also prone to incorrect usage, which can be exploited to leak the private key.

For now i am leaving the signing format as is and I'd like to merge, unless you disagree @andrewkozlik @onvej-sl ?
If you feel like we should change it, please file an issue and we'll do that separately.

Based on @onvej-sl's evaluation, I don't see any obvious problem with not signing the sigmask.

fixes #2535

Previously, an amount whose decimal representation with suffix does not fit in buffer would not be rendered at all. This looks weird in "Send to address 0xblabla". Enlarging the buffer lets the amount be stringified. Then it won't fit on screen so this is not a perfect fix, but "Send 25000000... to address" is better than before.

In a very weird situation, our declaration of `shutdown()` shadows a function `shutdown(int, int)` from sys/socket, which _just happens_ to be called by libxcb when closing the sdl window. This calls `main_clean_exit` which calls into micropython and causes at best an uncaught NLR and at worst an outright segfault because by that time the micropython environment doesn't exist anymore. I didn't think this sort of thing would be possible but here we are?? Fixed by removing `__shutdown()` and replacing `shutdown` with `trezor_shutdown`

prusnak · 2023-03-24T11:53:48Z

By the way, what is the motivation in this case for implementing a k-out-of-n signature using aggregated public keys and aggregated signatures instead of verifying each of k signatures separately?

The main motivation was speed of verification. It is faster to verify just one signature instead of verifying 3 signatures. This is very much pronounced in the original T1 bootloader where it takes around 1 second to verify 3 signatures prolonging the boot time, while TT bootloader phase is "instant" (according to naked eye).

bosomt · 2023-04-18T16:53:54Z

QA OK

trezor/trezor-suite#6442 (comment)

marnova self-assigned this Jul 25, 2022

trezor-ci added this to To be reviewed in Pull Requests via automation Jul 25, 2022

marnova linked an issue Jul 25, 2022 that may be closed by this pull request

SignTx should contain the signed definition of the used coin/token #15

Closed

matejcik reviewed Sep 29, 2022

View reviewed changes

marnova mentioned this pull request Oct 3, 2022

Ethereum definitions send from host trezor/trezor-suite#6442

Closed

marnova marked this pull request as ready for review November 24, 2022 13:28

marnova requested a review from prusnak as a code owner November 24, 2022 13:28

matejcik reviewed Nov 30, 2022

View reviewed changes

marnova force-pushed the marnova/ethereum_defs_from_host branch from 79f325a to e89e453 Compare December 7, 2022 13:18

This was referenced Dec 13, 2022

Delist tokens marked with redFlags #1779

Closed

Duplicates in coins_details.json #1780

Closed

refactor(core): Allow eth & btc signing at m / 45' / coin_type / account / change / address_index #2682

Merged

Hannsek assigned matejcik and unassigned marnova Feb 3, 2023

matejcik force-pushed the marnova/ethereum_defs_from_host branch from 7466199 to 472a04a Compare February 3, 2023 13:56

matejcik reviewed Feb 3, 2023

View reviewed changes

matejcik mentioned this pull request Feb 8, 2023

Fix definitions downloader script trezor/definitions#1

Closed

matejcik force-pushed the marnova/ethereum_defs_from_host branch from 472a04a to 17d55cd Compare February 10, 2023 16:04

grdddj reviewed Feb 10, 2023

View reviewed changes

common/tools/coin_info.py Outdated Show resolved Hide resolved

grdddj reviewed Feb 15, 2023

View reviewed changes

Hannsek assigned grdddj Feb 16, 2023

mmilata mentioned this pull request Mar 2, 2023

Fix defintions of ethereum testnets #2759

Closed

matejcik force-pushed the marnova/ethereum_defs_from_host branch from 8c6850c to 0813e48 Compare March 15, 2023 11:42

matejcik requested review from mmilata and removed request for prusnak March 15, 2023 13:07

matejcik force-pushed the marnova/ethereum_defs_from_host branch from facbadc to d73247c Compare March 16, 2023 15:39

mroz22 reviewed Mar 17, 2023

View reviewed changes

matejcik and others added 13 commits March 24, 2023 11:34

refactor(core): delegate EthereumGetPublicKey to bitcoin GetPublicKey

4994ae3

docs(common): how to add new protobuf message

ab6cf72

feat(common): protobuf messages for Ethereum definitions

ee72762

docs: document Ethereum definition binary format

2d1d4f4

feat(common): switch to external Ethereum definitions, add generators

d09e0ad

fix(common): deal with duplicity code leftovers

40e3283

also fixes #2759

feat(core): external Ethereum definitions

7028552

feat(legacy): support external Ethereum definitions

4571c2e

feat(python): support external Ethereum definitions

202cf0c

feat(core): implement CoSi module and CoSi verification

0ae454b

feat(core): update Ethereum definitions to verify using CoSi

e35b005

feat(legacy): implement CoSi verification

105bace

feat(legacy): update Ethereum definitions to verify using CoSi

9ea9cb5

matejcik force-pushed the marnova/ethereum_defs_from_host branch from 4aa7289 to 8609f48 Compare March 24, 2023 10:47

matejcik added 6 commits March 24, 2023 12:24

tests: update device_tests for external definitions

2d0c4e8

fix(python/trezorctl): fix colliding option shortcut in ethereum sign-tx

8c77bd4

fixes #2535

fix(tests): only override left clicks in index custom script

b05115c

fix(legacy): limit EthereumGetPublicKey allowable paths

ccd4bf6

matejcik force-pushed the marnova/ethereum_defs_from_host branch from 8609f48 to 5300790 Compare March 24, 2023 11:24

matejcik merged commit 9244522 into master Mar 24, 2023
7 checks passed

Pull Requests automation moved this from Finalizing to Merged Mar 24, 2023

matejcik deleted the marnova/ethereum_defs_from_host branch March 24, 2023 12:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Signed Ethereum network and token definitions from host #2410

Signed Ethereum network and token definitions from host #2410

marnova commented Jul 25, 2022

matejcik left a comment

grdddj commented Oct 13, 2022

grdddj commented Oct 13, 2022 •

edited

matejcik left a comment

matejcik left a comment

grdddj left a comment

matejcik commented Mar 15, 2023

mroz22 Mar 17, 2023 •

edited

matejcik Mar 17, 2023

onvej-sl commented Mar 23, 2023

matejcik commented Mar 24, 2023

matejcik commented Mar 24, 2023

andrewkozlik commented Mar 24, 2023

prusnak commented Mar 24, 2023 •

edited

bosomt commented Apr 18, 2023

Signed Ethereum network and token definitions from host #2410

Signed Ethereum network and token definitions from host #2410

Conversation

marnova commented Jul 25, 2022

matejcik left a comment

Choose a reason for hiding this comment

grdddj commented Oct 13, 2022

grdddj commented Oct 13, 2022 • edited

matejcik left a comment

Choose a reason for hiding this comment

matejcik left a comment

Choose a reason for hiding this comment

grdddj left a comment

Choose a reason for hiding this comment

matejcik commented Mar 15, 2023

mroz22 Mar 17, 2023 • edited

Choose a reason for hiding this comment

matejcik Mar 17, 2023

Choose a reason for hiding this comment

onvej-sl commented Mar 23, 2023

matejcik commented Mar 24, 2023

matejcik commented Mar 24, 2023

andrewkozlik commented Mar 24, 2023

prusnak commented Mar 24, 2023 • edited

bosomt commented Apr 18, 2023

grdddj commented Oct 13, 2022 •

edited

mroz22 Mar 17, 2023 •

edited

prusnak commented Mar 24, 2023 •

edited