-
-
Notifications
You must be signed in to change notification settings - Fork 655
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
Signed Ethereum network and token definitions from host #2410
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
partial review for now
I have some possibly good news including binary size. So far we have considered the size of Now I tried to actually remove these two files from the build and got incredible results:
There is that 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! |
I also tried to replace We could theoretically try to use this with |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
legacy part review done.
feel free to rebase before you start working on these changes
79f325a
to
e89e453
Compare
7466199
to
472a04a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
472a04a
to
17d55cd
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some comments from reviewing core
and python
changes
8c6850c
to
0813e48
Compare
@mmilata please review mainly, in order of priority:
device tests are currently broken, that is a known problem |
facbadc
to
d73247c
Compare
`<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] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
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. |
4aa7289
to
8609f48
Compare
For now i am leaving the signing format as is and I'd like to merge, unless you disagree @andrewkozlik @onvej-sl ? |
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.
Based on @onvej-sl's evaluation, I don't see any obvious problem with not signing the |
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`
8609f48
to
5300790
Compare
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). |
Enables to send signed Ethereum network and token definitions from host.
Related to #15 (high-level plan described in its comment #15 (comment)).