feat(cardano): add support for multisig

[gabrielKerekes]

This PR contains changes to support Cardano's native scripts (multi-sig) and token minting.

Important points:

• 1854 (multisig) and 1855 (token minting) seed roots have been added.
• get-public-key has also been updated to support 1854 and 1855 paths.
• A new get-native-script-hash call has been added which allows the user to verify the hash of a native script while showing all its components to the user.
• Support for script address types has been added - these are addresses which contain a script hash instead of a key hash as a payment or stake credential.
• Because of the new address types the get-address and sign_tx change output UI flow would become too complicated. The UI flow was thus refactored and changed a bit - hopefully simplifying it. You can take a look at the designs here.
• CardanoTxSigningMode.SCRIPT_TRANSACTION has been added. Only when using this signing mode can script hashes and 1854 witness paths be used. 1852 witnesses are forbidden.
• Transaction certificates and withdrawals can now either contain either a key_hash or a script_hash (it was just key_hash previously). CDDL.
• Transactions can now contain the 9: mint field in the body. CDDL. Token minting is signed by keys derived from 1855 paths.
• Client libraries (Connect, Trezorlib) can now receive additional_witness_requests which will be forwarded to Trezor as a witness_request. Witness requests with "usual" 1852 paths are not shown to the user. Witness requests with 1854 and 1855 paths as well as "unusual" 1852 paths are always shown to the user.

I plan on generating the ui test fixtures as well as adding changelog entries and updating the Cardano README after the review process is finished. We have also updated Trezor Connect with respect to these changes, but I'd rather create a PR for that after this one is merged.

[gabrielKerekes]

So here goes the Cardano multi-sig PR I've mentioned already before. I know you've just finished reviewing a large Cardano PR, but do you have an idea when you will be able to review this one please? Thanks!

[matejcik]

we're freezing the September release on Monday, so this will be reviewed at some point before the October freeze

[gabrielKerekes]

@matejcik what is the status on this please? Do you have an idea when this PR could be reviewed? Do you think this can make it into the October release?

[matejcik]

It is currently unclear if we will be releasing in October.
If yes, I believe we can get this in.

[gabrielKerekes]

@matejcik any updates please?

[matejcik]

Hello, here is the review that was promised, sorry for taking so long.

As already communicated in Slack, there will not be an October release, so this will most likely be released in November.
On a related note, I am taking a vacation the next week so I will only be available in a very limited capacity.

Overall comment: what is the purpose of the distinction between "ordinary" and "script" transaction? Is there something that a script transaction is allowed to do but it's not clearly communicated to the user?

[matejcik]

The code looks nice overall.

Only thing worrying me is the growing size of the Cardano application -- there's a LOT of business logic. We might want to split up sign_tx in the future to more easily manageable "features" that make it clear that they can be treated, reviewed, and possibly enabled or disabled, separately from each other.

[gabrielKerekes]

@matejcik thanks for the review! @davidmisiak will take care of finalising this PR.

Overall comment: what is the purpose of the distinction between "ordinary" and "script" transaction? Is there something that a script transaction is allowed to do but it's not clearly communicated to the user?

True, perhaps it could have been confusing with SCRIPT_TRANSACTION. However, we have decided to rename SCRIPT_TRANSACTION to MULTISIG_TRANSACTION anyways. IMHO it is now clearer what it means.

The distinction is there to protect the user from signing something he doesn't want to sign, since the user can't verify the inputs. E.g. accidentally signing a multisig transaction with his 1852 keys and then losing some funds. In a MULTISIG_TRANSACTION only script credentials can be used in certificates and withdrawals and only 1854 and 1855 witness requests can be used.

We could perhaps use AccountPathChecker to verify the paths and an additional CredentialChecker (or something similar) to achieve similar results, I think. But the clear separation into distinct signing modes is IMHO more natural. And since all the witnesses in a multisig transaction are shown to the user, it isn't necessary to limit them to a single account.

Only thing worrying me is the growing size of the Cardano application -- there's a LOT of business logic. We might want to split up sign_tx in the future to more easily manageable "features" that make it clear that they can be treated, reviewed, and possibly enabled or disabled, separately from each other.

Indeed it's grown quite a lot. Currently I'm not sure how it could be split up.

We are planing on splitting the Cardano Ledger app into a "lite" app and a full app so that the "lite" app doesn't take up 80% of Ledger storage for ordinary users who don't need all the features. But IMHO it doesn't really apply here, it's more just FYI.

I thinks there's not much that could be isolated into separate/independent features since most of the "features" can be used together in a single transaction. The separation into signing modes is something that comes the closest and perhaps it would just need to be a bit restructured. Or could you perhaps elaborate a bit on what you have in mind with the "features"?

[davidmisiak]

Hi, I'm taking over Trezor Cardano development after @gabrielKerekes.

Apart from the review fixes, we've just added two small things:

1. Outputs with reward addresses and change outputs with a script in address payment part are forbidden, see 5f7ab08.
2. We've renamed SCRIPT_TRANSACTION to MULTISIG_TRANSACTION, so that it doesn't collide with non-native (Plutus) signing mode which we will propably add. See 50ab301.

[matejcik]

I thinks there's not much that could be isolated into separate/independent features since most of the "features" can be used together in a single transaction. The separation into signing modes is something that comes the closest and perhaps it would just need to be a bit restructured. Or could you perhaps elaborate a bit on what you have in mind with the "features"?

I believe the next step is to convert sign_tx.py into a package. Then ISTM that converting each _process_* into its own submodule is a thing that would make sense -- they seem largely independent of each other.

(btw the file sign_tx.py itself is huge in RAM because of how many symbols it imports, modifying the code so that instead of from foo import x; x() to import foo; foo.x() would help. However, at this point it would probably further reduce readability -- you folks' symbol names are so big!)

No changes requested here, just thinking forward.

[matejcik]

Alright, new take on the CredentialPolicy thing.

I notice that the return value of get_*_credential_policy is used exclusively in conjunction with the show_credential method. I also notice that the same is true for CredentialParams wrapper, and that in both uses it's the same sequence:

await show_credential(CredentialParams(PAYMENT, address_params))
await show_credential(CredentialParams(STAKE, address_params))

on high level, i'd suggest:

1. convert to show_credentials that will internally call _show_credential twice
2. instead of CredentialParams.__init__ taking type argument, create a @classmethod def payment_params(address_params) -> CredentialParams and dtto for staking, accept path, key_hash, script_hash, pointer, type_name as arguments to __init__
3. move get_*_credential_policy functionality so that it works on top of CredentialParams instances. Probably can be filled by the constructors. Presumably boolean flags like CredentialParams.is_reward, staking_path_mismatch, unusual_path, etc. would make sense?

the only thing that doesn't seem clear to me is what to do about the usage in fail_if_unusual. But my guess is that is_unusual(address_params) would be a function separate from what is now get_credential_policy, and could be called separately too?

I think this sort of converges on your "very explicit" idea of should_show_reward_warning -- but I'm proposing that the semantic level is is_reward_address(), and the decision to "show warning" is only made in the layout function.

[gabrielKerekes]

Alright, new take on the CredentialPolicy thing.

The changes should be implemented in 248328f. I think CredentialParams could now simply be called Credential, what do you think?

[matejcik]

Very nice, it seems much easier to follow this way. A couple small nits.

[matejcik]

think CredentialParams could now simply be called Credential, what do you think?

probably, yes. feel free to make that change.

[gabrielKerekes]

probably, yes. feel free to make that change.

CredentialParams renamed to Credential in 0027ea9.

[matejcik]

looking good.
please autosquash fixups & rebase, add a changelog entry, make sure style checks pass, etc.
perhaps expand the README?
i'll do a final once-over tomorrow.

[gabrielKerekes]

Squashed and rebased. Changelog, README and ui test fixtures updated in 497d639. Hopefully everything ends up green.

[matejcik]

@pytest.mark.skip_t1 and altcoin and cardano markers are missing here

[gabrielKerekes]

Oh. Added in 813eb72.

[matejcik]

it appears that get_native_script_hash ui fixtures are missing?

[gabrielKerekes]

it appears that get_native_script_hash ui fixtures are missing?

Yes, indeed, sorry. I ran pytest tests/device_tests -m cardano --ui=record which apparently didn't add them. I think adding --ui-check-missing also didn't include them - but I might have just overlooked it. I had to run poetry run make -C core test_emu_ui_record. Should be fixed in 813eb72.

In addition I added display format to the tests so that something is actually captured by the ui tests and it also makes sense to test with the display formats.

[matejcik]

which apparently didn't add them

that's because the cardano marker was missing :)

[gabrielKerekes]

that's because the cardano marker was missing :)

Well, that makes sense 🤦

[matejcik]

and we're done.
thanks for your work!

[davidmisiak]

@matejcik just to make sure - will there be the November release please?

[matejcik]

@davidmisiak unfortunately no. we decided to postpone the release until December

[davidmisiak]

Ok, thanks for info.