search

dcSpark / cardano-multiplatform-lib

Rust implementation of Cardano
MIT License
98 stars 36 forks source link

TX hash mismatch for non-canonical TXs #321

Closed rooooooooob closed 7 months ago

rooooooooob commented 7 months ago

This only concerns the free-floating hash_transaction().

Fixes #314

Use TransactionBody instead of of [u8; 32] to be consistent with hash_transaction().

Moves all the functions that were added directly to mod.rs in https://github.com/dcSpark/cardano-multiplatform-lib/pull/298 files to utils.rs files where they should be since they aren't auto-generated.

rooooooooob commented 7 months ago

Why do we have this redundant functionality (both free floating hash_*() and *::hash()) for everything?

At least now one just refers to the other but it's still a bit weird. Was it easier to map a function over values from WASM or some other WASM-related ease of use? I vaguely remember hearing something about that years ago but I could be wrong. @SebastienGllmt If there's no good reason to have both I would prefer we just have one or the other to make the API simpler.

Do we want to provide some functionality for ensuring the entire tx is canonical? We could probably auto-gen or write a make_canonical() to structs but that could cause fees to not line up or maybe some other issues for anything in the intermediate construction that referenced hashes or sizes of components. If you're creating entirely from within CML in the first place then everything should already be canonical so maybe it's fine as is?

SebastienGllmt commented 7 months ago

Was it easier to map a function over values from WASM or some other WASM-related ease of use

If I remember correctly, it had to do with supporting multiple eras where the function on the type itself would always use the current era, but the standalone utility functions would be provided for old eras as well. I don't think we need this anymore with our new project configuration though

rooooooooob commented 7 months ago

It looks like in new CML we only had the standalone and the methods were added in #298 when adding the extra multi-era functionality needed to use in Carp. I moved those to their utils.rs files instead of the mod.rs they were in.

Do we prefer the standalone or the attached methods if we pick to just have one?

What about the other hash functionality (plutus datums, metadatums).

SebastienGllmt commented 7 months ago

The way you did it in this PR is my preferred way 👍

SebastienGllmt commented 7 months ago

We could probably auto-gen or write a make_canonical()

There are some key things we shouldn't change like the tx metadata and the plutus datum. I think trying to make things canonical could make sense, but it could also introduce subtle issues and so I'm not really sure it's worth touching

rooooooooob commented 7 months ago

I am reading CIP21, just to make sure I understand this:

HW wallets do not serialize auxiliary data because of their complex structure. They only include the given auxiliary data hash in the transaction body.

Does this mean that the tx body has to be canonical - e.g. the hashes are canonical bytes, but were computed on arbitrary CBOR?

And the outer transaction could be encoded as arbitrary CBOR it's just the transaction_body that must be canonical (since that's what's signed)?

Earlier in the CIP it says:

Transactions must be serialized in line with suggestions from Section 3.9 of CBOR specification RFC. In particular: [insert canonical CBOR description here]

which I first interpreted as the whole transaction before (thus metadatums/etc too), not just the body, but maybe it's just ambiguous wording and it just means the body?

I guess we can just keep things how they are and assume anyone implementing for HW wallets fully understands CIP21 and CBOR encodings? I'll add a line to the CBOR section of the docs stating that all structures default to canonical CBOR unless created from deserialized bytes just to make it clear.