Wrong TxHash sporadically calculated by TransactionUtil class

[nemo83]

There seems to be an issue that sporadically manifest in the method:

TransactionUtil.getTxHash(Transaction transaction)

From time to time a wrong hash is created.

[nemo83]

Some additional info.

I've tested the public static String getTxHash(byte[] transactionBytes) in the TransactionUtil class and seems to be working better (I haven't done extensive tests) BUT in the few cases I've observed, when this and the other return something different, it looks like this one is the correct, while the other TransactionUtil.getTxHash(Transaction transaction) is wrong.

This suggests that the Transaction.serialize() might contain a bug?

To support this hypothesis there is also another thing I've observed, when I submit a tx, I also print the hash, and sporadically, I get:

[...] submitting tx: 5c74f67e1b144e612a7ed1af6d67fcf3d012b851e40f297ec78ffccdfcdeaa632024
[...] accepted tx: c60765cacf3e96561a950b3d10a8f656791305cb8b3f4a08510c86771a0ecb162024

I.e. for a transaction I craft in the code, the hash calculated might differ from the one returned by the code. While I write this I also realise that "some things" might be "reordered". Like for instance, inputs get reordered lexicographycally.

Inability to reliably compute a tx hash for a transaction, prevents code from creating chained transactions.

[satran004]

@nemo83
The Javadocs for these two methods highlight their differences.

There's a known issue where deserializing and serializing a transaction using different tools can result in varying byte sequences. CCL employs canonical ordering (e.g. in maps) during serialization. Therefore, if you serialize, deserialize, and then serialize a transaction created by CCL, you will consistently obtain the same result.

However, this may not hold true for transactions generated by other tools that do not adhere to canonical ordering. But, both types of transactions are valid during execution in node.

This is why it's advisable to avoid fully deserializing and serializing a transaction when calculating its Tx hash.

In the first method, TransactionUtil.getTransaction(Transaction), it is assumed that the transaction object is constructed by CCL. This is because it must undergo the serialize method of CCL, which orders keys in a map.

However, if you're using a transaction built by an external tool or an already executed transaction to obtain the Tx hash, it's better to use getTxHash(byte[] transactionBytes). This method doesn't involve full deserialization into a CCL object for Tx hash calculation. Instead, it deserializes to a DataItem and then serializes again with canonical order set to false, preserving the original order.

This method should be correct most of the time, but we've encountered issues with only few transactions on the mainnet when using this approach with Yaci. So in Yaci, we avoid deserializing to DataItem altogether. The parser simply extracts the transaction body bytes from the full transaction bytes to create the hash.

I think we should update getTxHash(byte[] transaction) to adopt Yaci's approach. The other method, getTxHash(Transaction), should not be used for external or already executed transactions.

[satran004]

@nemo83
It's fixed now. PR #356

Can you please verify the fix with your use case ? You can take the code from the below branch. Thanks

https://github.com/bloxbean/cardano-client-lib/tree/fix_353_main

[nemo83]

Changes look good! Thank you