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

TX hash mismatch for non-canonical TXs #321

Merged
merged 2 commits into from
Mar 11, 2024
Merged

Conversation

rooooooooob
Copy link
Contributor

@rooooooooob rooooooooob commented Mar 11, 2024

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 #298 files to
utils.rs files where they should be since they aren't auto-generated.

This only concerns the free-floating `hash_transaction()`.

Fixes #314
@rooooooooob
Copy link
Contributor Author

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?

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 #298 files to
`utils.rs` files where they should be since they aren't auto-generated.
@SebastienGllmt
Copy link
Contributor

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
Copy link
Contributor Author

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
Copy link
Contributor

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

@SebastienGllmt SebastienGllmt merged commit 45d5832 into develop Mar 11, 2024
1 check passed
@SebastienGllmt SebastienGllmt deleted the hash-transaction-314 branch March 11, 2024 19:36
@SebastienGllmt
Copy link
Contributor

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
Copy link
Contributor Author

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.

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.

TransactionHash Mistmatch
2 participants