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: support Unchained p2wsh path #4271

Merged
merged 1 commit into from
Nov 5, 2024

Conversation

Shadouts
Copy link
Contributor

Unchained plans to support p2wsh using the same m/45 path we use for p2sh. This change adds the UNCHAINED_ patterns to bitcoin InputScriptType.SPENDP2SHWITNESS.

@Hannsek Hannsek requested a review from matejcik October 20, 2024 17:56
@andrewkozlik
Copy link
Contributor

andrewkozlik commented Oct 21, 2024

It is best practice to differentiate the BIP 32 path depending on script type, for example m/44'/... for P2PKH and m/84'/... for P2WPKH. As far as I can see, in Trezor firmware the only exception until now has been PATTERN_GREENADDRESS_A and PATTERN_GREENADDRESS_B.

There are two main reasons for this practice, which apply in this case:

  • Using different paths with different script types makes account discovery simpler in terms of scanning the blockchain, because the BIP 32 path then fully determines the scriptPubKey and address.
  • Reusing the same path with a different script type makes it more easy to commit public key reuse, because the wallet app needs to be aware of all possible script types and scan the blockchain accordingly. Older wallet apps may not be aware of the use of newer script types and may therefore inadvertently reuse public keys when generating new receive addresses or new change-output addresses.

Furthermore, I see that you are using the subtree m / 45' / **, but you use different paths than the one specified in BIP 45, which as far as I can tell is in violation of BIP 43. Now is a good opportunity to fix this issue by defining a distinct path for Unchained's P2WSH scheme.

That being said, I think that none of the above is a strict blocker for merging this PR.

@matejcik
Copy link
Contributor

code change ACK. @Shadouts please add a changelog entry.

@andrewkozlik
Copy link
Contributor

Also squash into a single commit and force-push.

@andrewkozlik
Copy link
Contributor

Note that for your own benefit, it would be wise to add a device test to tests/device_tests/bitcoin/test_nonstandard_paths.py. This is not a requirement, just best practice.

As you are probably aware, this PR does not modify the behavior of Trezor 1 firmware. Again, it's not a requirement from our side, I am only stating this to make sure that it's clear.

fix: move unchained paths to bitcoin multisig SPENDWITNESS

chore: add changelog
@Shadouts Shadouts force-pushed the unchained-p2wsh-paths branch from 62caf4c to c3b5dfe Compare October 31, 2024 19:02
@Shadouts
Copy link
Contributor Author

Changelog added, squashed, and force pushed.

@matejcik matejcik merged commit e30a343 into trezor:main Nov 5, 2024
93 checks passed
@Shadouts Shadouts deleted the unchained-p2wsh-paths branch November 5, 2024 18:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants