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

feat(trezor): add HF15 support, BP+ #8299

Merged
merged 1 commit into from
Aug 23, 2022

Conversation

ph4r05
Copy link
Contributor

@ph4r05 ph4r05 commented Apr 27, 2022

  • HP15 changes implemented to the Trezor client in Monero wallet
  • BP+ support added for Trezor
  • old Trezor firmware version support removed, code cleanup

Trezor firmware changes: trezor/trezor-firmware#2232

@ph4r05
Copy link
Contributor Author

ph4r05 commented Apr 28, 2022

Pinging @sethforprivacy that we have all code ready in PRs to support HF15

@sethforprivacy
Copy link
Contributor

Pinging @sethforprivacy that we have all code ready in PRs to support HF15

Wow, that was quick, thank you! Will forward this on and update the checklist.

@ph4r05
Copy link
Contributor Author

ph4r05 commented May 26, 2022

Just pinging that Trezor PRs were merged

@sethforprivacy
Copy link
Contributor

Thanks, @ph4r05, I pinged the Monero dev channel that this should be ready for review etc. yesterday, and will be sure we get it in ASAP.

Thank you for all of the hard work on this!

@kayabaNerve
Copy link

Just to confirm, does the have the inv-8 reversion? #8277 (only merged 17 days ago)

I remember hearing that concern. The only branch with a commit since then (besides a version bump on the same day, present in this branch) is the "hackathon results" which seems to be cleaning up.

Any test on the current master which confirms TXs work would be sufficient to confirm :)

@ph4r05
Copy link
Contributor Author

ph4r05 commented May 31, 2022

@kayabaNerve thats a good point, let me check

@ph4r05
Copy link
Contributor Author

ph4r05 commented May 31, 2022

@kayabaNerve you were right, I've incorporated the #8277 change. Trezor firmware needs the change as well, I will create the PR now.

@ph4r05 ph4r05 closed this May 31, 2022
@ph4r05 ph4r05 reopened this May 31, 2022
@kayabaNerve
Copy link

Thanks, and sorry you're only hearing about this now after Trezor already did a merge. Happy we're catching it though, and thanks again for working on this :)

@ph4r05
Copy link
Contributor Author

ph4r05 commented May 31, 2022

No problem @kayabaNerve! I am glad we caught it before release! :)

@ph4r05
Copy link
Contributor Author

ph4r05 commented Jun 13, 2022

hello @moneromooo-monero, if you pls find a minute, I would be grateful for PR review for upcoming HF upgrade, thanks a lot! :)

@selsta
Copy link
Collaborator

selsta commented Jun 13, 2022

We will have a second release before the HF that will include this patch.

@selsta
Copy link
Collaborator

selsta commented Jun 20, 2022

@ph4r05 Just for completeness, did you test this on the current testnet? Are view tags working correctly in addition to BP+? Also is the firmware already out so that I can test this myself? If not, any approx release date?

@ph4r05
Copy link
Contributor Author

ph4r05 commented Jun 20, 2022

@selsta

  • ad testnet - I never test it on testnet. Trezor-tests are passing, they generate a synthetic blockchain, sign several transactions and daemon validates them with full node logic. So PR should work properly (this approach worked perfectly so far)
  • I have no clue what view tags are - have to look into it more
  • ad trezor release date: @matejcik

@selsta
Copy link
Collaborator

selsta commented Jun 20, 2022

View tags: #8061

@ph4r05
Copy link
Contributor Author

ph4r05 commented Jun 20, 2022

There is definitely no view tags support added in Trezor. I will check it out this week and figure out required changes.

@selsta
Copy link
Collaborator

selsta commented Jun 20, 2022

I asked @j-berman and he said

16:19 <jberman[m]> AFAICT trezor should be fine because they lean on the default `generate_output_ephemeral_keys` which handles view tags, but I'd need to test to know for sure and I don't have a trezor

but he only took a quick look himself. I think the easiest way would be to make a transaction on testnet, or change the fork height locally and try to construct a transaction.

- BP+ support added for Trezor
- old Trezor firmware version support removed, code cleanup
@ph4r05
Copy link
Contributor Author

ph4r05 commented Jun 20, 2022

thanks @selsta for the ping! I've added view tag support to the trezor firmware: trezor/trezor-firmware#2345. This PR is also updated. As wallet is using view_key to derive view tags, tx receiving code on the wallet side does not require changes.

trezor_tests are passing so I am pretty confident view_tags will work also on testnet / mainnet when HF15 is activated.

@selsta
Copy link
Collaborator

selsta commented Jun 22, 2022

@matejcik It would be good to have a date for the Trezor firmware release as it's something that has to be done before we hardfork.

The current HF date is 2022-07-16, but we will likely push it back 2 weeks to around 2022-08-01. Is that enough time for Trezor to release a firmware update?

@matejcik
Copy link

matejcik commented Jun 27, 2022

@selsta We will not be releasing firmware sooner than August 17-ish.
Does that mean breakage in the interim?

@ph4r05
Copy link
Contributor Author

ph4r05 commented Jun 27, 2022

@selsta are pls HF15 changes enforced on activation or is it still possible to send HF14 transactions to the network (normal Bulletproofs) for some time after HF15 activation?

If HF15 rules are enforced, Trezor won't be able to sign new transactions until new trezor-firmware update.

@kayabaNerve
Copy link

I believe there will be a 24 hour interim period where both transaction types are valid. After those 24 hours, only the new ones will be.

@selsta
Copy link
Collaborator

selsta commented Jun 28, 2022

@matejcik Would it be possible to split the firmware update in two? One for monero and one for the other planned changes? If not we will have to ask the monero community if they prefer to delay the hardfork or if they are okay with leaving Trezor users unable to transact for about 2 weeks. Both are not ideal :/

@matejcik
Copy link

@selsta thing is, there are no other planned changes right now .)
but good news is, we considered this and decided to go ahead with July release. So the new firmware should be out by July 20.

@matejcik
Copy link

matejcik commented Jul 4, 2022

Bad news everyone.
We will not be able to do a firmware release this month. The originally announced date, August 17, is back in the game.
I've seen that you pushed back HF activation to August 13, so that is only four days of service disruption.

@selsta
Copy link
Collaborator

selsta commented Jul 6, 2022

Thank you for the update, four days shouldn't be too bad.

@ph4r05 Is there a way for me to install the firmware before the update is out? A nightly firmware build or something similar? I want to do some additional testing on testnet.

@matejcik
Copy link

matejcik commented Jul 8, 2022

@selsta you should be able to pick a firmware image from our CI
pick a pipeline, the task you're looking for is core fw regular build, then browse the artifacts.
here is latest master build

@selsta
Copy link
Collaborator

selsta commented Aug 3, 2022

Did some testing with the firmware image from CI, tx creation and receiving worked well on testnet.

@selsta
Copy link
Collaborator

selsta commented Aug 3, 2022

@ph4r05 please also open against release-v0.18

Copy link
Collaborator

@j-berman j-berman left a comment

Choose a reason for hiding this comment

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

Changes LGTM. I see the majority of this PR's changes get rid of dead code for now unusable versions, thank you :)

The one thing I didn't fully wrap my around is the minimal versioning setting, but I figure you've got it how you want it

if (trezor_version <= pack_version(2, 0, 10)){
throw exc::TrezorException("Trezor firmware 2.0.10 and lower are not supported. Please update.");
if (trezor_version < pack_version(2, 4, 3)){
throw exc::TrezorException("Minimal Trezor firmware version is 2.4.3. Please update.");
Copy link
Collaborator

Choose a reason for hiding this comment

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

From these comments:

Another thing, I hard-coded to the monero wallet that firmware version 2.5.1 supports HF15

We are releasing 2.5.1 which does not include this PR. Please bump to 2.5.2 in the monero PR.

I'm assuming only Trezor v2.5.2 will support bp+ and view tags. Wouldn't that mean the minimal Trezor version should be v2.5.2? Is the minimum set at v2.4.3 to support tx construction pre-fork?

Copy link
Collaborator

@j-berman j-berman Aug 5, 2022

Choose a reason for hiding this comment

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

Nevermind, I see. Once this ships, Trezors v2.4.3 and up will be able to use updated wallets until the fork height

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the review @j-berman! That was the idea, so old Trezors could still use the wallet even if they did not update yet. We can later enforce minimal version 2.5.2.

@ph4r05
Copy link
Contributor Author

ph4r05 commented Aug 5, 2022

@ph4r05 please also open against release-v0.18

Done @selsta: #8299 :)
thanks!

@luigi1111 luigi1111 merged commit 080fc69 into monero-project:master Aug 23, 2022
@ph4r05 ph4r05 deleted the pr/001-trezor-hf15 branch March 19, 2023 11:26
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.

7 participants