Legacy firmware support for Unchained paths

[Shadouts]

This change supports new Unchained paths for p2wsh addresses on legacy devices.

[onvej-sl]

I found two differences to how this is implemented in core:

• This pull request completely disables using the paths m/45'/coin_type'/account'/[0-1000000]/change/address_index and m/45'/coin_type/account/[0-1000000]/change/address_index for singlesig.
• This pull request shows warning when using the paths m/45'/coin_type'/account'/[0-1000000]/change/address_index and m/45'/coin_type/account/[0-1000000]/change/address_index with SPENDADDRESS and multisig.

Is it intentional?

[Shadouts]

I found two differences to how this is implemented in core:

• This pull request completely disables using the paths m/45'/coin_type'/account'/[0-1000000]/change/address_index and m/45'/coin_type/account/[0-1000000]/change/address_index for singlesig.
• This pull request shows warning when using the paths m/45'/coin_type'/account'/[0-1000000]/change/address_index and m/45'/coin_type/account/[0-1000000]/change/address_index with SPENDADDRESS and multisig.

Is it intentional?

Yes. Unchained does not support p2pkh spends. Also, how would a spend ever be InputScriptType_SPENDADDRESS and has_multisig at the same time? My interpretation is that InputScriptType_SPENDADDRESS indicates p2pkh and therefore it will never be has_multisig == true.

[onvej-sl]

I'm not entirely sure, but I believe that if has_multisig == true, then InputScriptType_SPENDADDRESS is interpreted the same as InputScriptType_SPENDMULTISIG. See for example this.

However, my point is not that this is incorrect. The point is that I think the behaviour implemented in this pull request for the legacy firmware differs from the behaviour implemented here. I am uncertain whether the difference is intentional.

[Shadouts]

@onvej-sl I've pushed a change which I think restores the logic from previous, however, it's still not clear to me how this pathway would ever evaluate to true if InputScriptType_SPENDADDRESS and has_multisig, but at least it's there to address any possible regression.

[matejcik]

so i'm curious: the PR says "support unchained paths", but you seem to be adding restrictions here. Previously the same paths on a Trezor One would be supported on any script type, now we're discussing which script types to keep allowed?

[onvej-sl]

so i'm curious: the PR says "support unchained paths", but you seem to be adding restrictions here. Previously the same paths on a Trezor One would be supported on any script type, now we're discussing which script types to keep allowed?

It depends on what you mean by "supported". If "supported" means you can use the path with safety checks enabled and no warning is displayed, then the paths weren't previously supported for all script types. If "supported" means you can use the path with safety checks turned off or you don't care about the warning, then this pull request restricts the paths to multisig (see this and this), which differs from how this was implemented in core and I believe this is not intentional.

[Shadouts]

What restrictions are being added? This increases the scope of the Unchained paths check to include InputScriptType_SPENDWITNESS

The only conditional difference is on line 577 where InputScriptType_SPENDWITNESS is now allowed. All other conditional checks should be identical for address_n[0] == PATH_HARDENED + 45 && address_n_count == 6

[onvej-sl]

The other difference is here. However, I don't know if this is what @matejcik meant.

[matejcik]

yeah, my bad. i didn't notice that there was a common "if full" block in the previous version.
in that case, yes, the current version of the code looks fine to me.

it's still not clear to me how this pathway would ever evaluate to true if InputScriptType_SPENDADDRESS and has_multisig

again, callers are explicitly allowed to use SPENDADDRESS and provide multisig data, which sets has_multisig flag and is treated as multisig. (this is actually the same behavior as SPENDWITNESS, where you also indicate multisig-ness by providing the multisig data and not by using a special script type. the existence of SPENDMULTISIG is a historical artifact)

[onvej-sl]

A got a bit lost in this discussion.

I would like to ask @Shadouts, whether you want to allow the use of m/45'/coin_type'/account'/[0-1000000]/change/address_index and m/45'/coin_type/account/[0-1000000]/change/address_index for singlesig provided the user ignored this warning.

I'm asking because it is allowed in core but not in this pull request.

[Shadouts]

@onvej-sl To answer your question, Unchained has no use for singlesig at those paths. Our only concern is with multisig.

But, I don't see how it differs from core. Both conditions are checking has_multisig in legacy and multisig in core.

[onvej-sl]

But, I don't see how it differs from core. Both conditions are checking has_multisig in legacy and multisig in core.

Please see #4382:

• 135ceb6 adopts this pull request.
• df1c857 introduces a test that attempts to retrieve a singlesig P2WSH address.

The test succeeds in core, while it fails with "Forbidden key path" in legacy.

[Shadouts]

• df1c857 introduces a test that attempts to retrieve a singlesig P2WSH address.

The test succeeds in core, while it fails with "Forbidden key path" in legacy.

Maybe there is a confusion on terminology here. In this context, what does singlesig mean? Does it mean an input that requires a signature from only a single pubkey? Commonly singlesig refers to p2wpkh, but I suppose it could also refer to a p2wsh script which requires only one signature.

Unchained only utilizes p2wsh inputs which require signatures from more than one pubkey to spend. If this case is provided without a warning, then it is acceptable for our needs.

[matejcik]

yes, we'd consider "singlesig" to be "p2wpkh". what @onvej-sl is pointing at is that if you use p2wpkh with the unchained path, it is allowed on core but disallowed on legacy.

that said, i'm surprised the above test is passing. that seems to be a bug, seeing as the if condition specifies and multisig

[matejcik]

ah, the test "passes" but the warning is there, hidden by the ConfirmAllWarnings input flow.
core makes it possible to accept that address instead of hard-raising "Forbidden key path." That may or may not be a bug, but it's a behavior that we've had for some time. The path m/45'/... is a part of Bitcoin's keychain, and the script type mismatch is a softer kind of warning. Legacy does not make this distinction.

It seems to me that we can merge this PR regardless of the above.

[matejcik]

drop the leading empty line please

[Shadouts]

Removed in 8ae3a80

[onvej-sl]

Maybe there is a confusion on terminology here. In this context, what does singlesig mean? Does it mean an input that requires a signature from only a single pubkey? Commonly singlesig refers to p2wpkh, but I suppose it could also refer to a p2wsh script which requires only one signature.

Sorry for the confusion. The test I created generated a p2wsh address but I believe the result will be the same if I use p2wpkh.

core makes it possible to accept that address instead of hard-raising "Forbidden key path." That may or may not be a bug, but it's a behavior that we've had for some time. The path m/45'/... is a part of Bitcoin's keychain, and the script type mismatch is a softer kind of warning. Legacy does not make this distinction.

This is the difference I'm talking about. However, I believe legacy is able to make this distinction, please read this comment. Only thing that we need to do for it is to remove this line.

[Shadouts]

This is the difference I'm talking about. However, I believe legacy is able to make this distinction, please read this comment. Only thing that we need to do for it is to remove this line.

Line removed in 8e12441. That was a check that did not exist in the original validation. I most likely added it to ensure that it was a transaction specifically for Unchained application. I suppose making it operate similar to core is no problem, but again, we don't really care about singlesig at these paths.

[matejcik]

ACK from me
@onvej-sl please squash-merge if you are also happy with the change (we don't need the 3 extra commits that are all "address some feedback" in history)