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

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

Merged
merged 1 commit into from
Jan 18, 2023

Conversation

0xBEEFCAF3
Copy link
Contributor

@0xBEEFCAF3 0xBEEFCAF3 commented Dec 13, 2022

Toward an Unhardened Multisig Standard

Casa and other multisig providers have learned that the use of extensive key hardening causes usability issues. This is due to additional complexities that arise from having to coordinate the use of multiple geographically distributed keys. The degradation of user experience is not offset by any security improvements because the primary security of a multisignature wallet comes from preventing any given key from being a single point of failure.

With that said, multisig coordinators should only need to harden up to the purpose path.
Mutlisig coordinators should sign at m / 45' / coin_type / account / change / address_index.

core/src/apps/common/paths.py Outdated Show resolved Hide resolved
core/src/apps/common/paths.py Outdated Show resolved Hide resolved
core/src/apps/ethereum/keychain.py Outdated Show resolved Hide resolved
@0xBEEFCAF3 0xBEEFCAF3 changed the title refactor(core): Allow eth & btc signing at m/45'/[0-1000000]/[0-1000000]/[0-1]/[0-1000000] refactor(core): Allow eth & btc signing at m / 45' / coin_type / account / change / address_index Dec 19, 2022
Copy link
Contributor

@matejcik matejcik left a comment

Choose a reason for hiding this comment

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

you probably also want to add a test for Ethereum sign-tx on your path

core/src/apps/ethereum/keychain.py Outdated Show resolved Hide resolved
@matejcik
Copy link
Contributor

please also add test for EthereumSignTx, that seems to be the more important part -- or will you not be using EthereumSignTx with the new paths?

@0xBEEFCAF3
Copy link
Contributor Author

please also add test for EthereumSignTx, that seems to be the more important part -- or will you not be using EthereumSignTx with the new paths?

We will not be using EthereumSignTx for these new paths -- only message signing

@matejcik matejcik merged commit 96b2d05 into trezor:master Jan 18, 2023
@grdddj
Copy link
Contributor

grdddj commented Jan 18, 2023

@matejcik seems like this merge has broken device/UI tests.

The addition of a new test at the second place in signmessage.json means the UI tests got out of sync - this has an easy fix of putting it as a last element and recording the fixtures.

However, a bigger issue seems that the new test doesn't even work:

E       AssertionError: assert '0x3beC5F707E...3cd089E8Ea02C' == '0x4C28fB5643...45C5B50300242'
E         - 0x4C28fB5643a2F9B03736103188845C5B50300242
E         + 0x3beC5F707Ef56057354f4c062C53cd089E8Ea02C

Gitlb job: https://gitlab.com/satoshilabs/trezor/trezor-firmware/-/jobs/3621363252

The same happens on my machine as well.

Not sure whether there is a mistake in the assertion or in the code

@0xBEEFCAF3
Copy link
Contributor Author

@grdddj Apologies I believe the assertion is incorrect. I'll fix that and move the test fixture to the last element of that list.
What do you mean by "recording the fixtures".

@0xBEEFCAF3
Copy link
Contributor Author

Drafted a PR #2753

@grdddj
Copy link
Contributor

grdddj commented Jan 19, 2023

@grdddj Apologies I believe the assertion is incorrect. I'll fix that and move the test fixture to the last element of that list. What do you mean by "recording the fixtures".

Thanks for correcting the asserted address in the PR.

I made another PR - #2754 - with that change and also with updating our UI test-suite. When the CI turns green, we will merge it into master.

That is what I meant by "recording the fixtures" - new test-case was added, but the screen recording of that test-case was not done, so the tests complained. We do that to have an overview of all screens during our testing, as when something fails later, it is a signal of something going wrong. Example of the failed test run with UI diff (from current master) is https://satoshilabs.gitlab.io/-/trezor/trezor-firmware/-/jobs/3625729086/artifacts/test_ui_report/index.html

@matejcik
Copy link
Contributor

i'm confused now.
@grdddj can you please integrate #2753 probably? so that #2754 is the only thing that needs to be merged and fixes all the problems

@grdddj
Copy link
Contributor

grdddj commented Jan 19, 2023

i'm confused now. @grdddj can you please integrate #2753 probably? so that #2754 is the only thing that needs to be merged and fixes all the problems

#2754 already contains the changes from #2753. I could have took that commit, but it is in a remote fork, so I could not push anything on that branch anyway. #2753 could be closed if #2754 is merged

@0xBEEFCAF3
Copy link
Contributor Author

0xBEEFCAF3 commented Feb 28, 2023

@matejcik Do you know roughly when these changes are going to be deployed?

@matejcik
Copy link
Contributor

matejcik commented Mar 1, 2023

we're currently aiming for March release of T1 and April for TT

@0xBEEFCAF3
Copy link
Contributor Author

Thanks for Rolling out the T1 firmware. Everything looks great! Do you have a date set for the April release for TT? We are working on a release that is dependent on this, so if you have a target date, that would be very helpful. Thank you!

@matejcik
Copy link
Contributor

the release is scheduled for April 12 (early access) and April 19 (full public release).
be aware that if there are delays, we will push the dates back by a full month

@0xBEEFCAF3
Copy link
Contributor Author

@matejcik Great! Thank you for the update

@Hannsek
Copy link
Contributor

Hannsek commented Apr 5, 2023

We won't release the fw in early access phase. Full public release is still valid.

@0xBEEFCAF3
Copy link
Contributor Author

Thanks for the update!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants