-
Notifications
You must be signed in to change notification settings - Fork 136
Fix plutus list serialization #228
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
Fix plutus list serialization #228
Conversation
|
This is the same root cause as #217 where the root cause is actually a bug the cardano-ledger. Doesn't mean we can't merge PRs to unblock people on this in the meantime though |
|
@SebastienGllmt how does changing the wire format for cardano at this point play out? Not sure what that process looks like or how it would effect downstream tooling |
If the plutus data serialization is correct in the serialization-lib and just wrong in the cli, I would leave it as it is. #217 was necessary to change because the LanguageViews encoding is created on-chain. The plutus data on the other hand are just off-chain and I personally rely on the serialization-lib already with my dApp. |
Do the specs for encoding Plutus data mention that it should be in canonical CBOR form (and thus definite), or is it ambiguous? |
|
Our hope in the ledger is to fix the serialization bug in the next hard fork IntersectMBO/cardano-ledger#2512 |
|
Just a casual observer here, how do we know if our datum hash is correct if it differs between the cli and the serialization lib? Is there no way to test this until the issue gets resolved in the next hard fork? |
Unfortunately that's why hashing is so useful - small changes in the input cause huge changes in the output so you can't predict it. This makes debugging not very nice about why the hash is wrong. You just know that something is different/wrong. If you can export the data that the cardano-node hashes that can help a lot since we can manually examine that and compare it with the cardano-serialization-lib's structure. Any difference is probably something small like the CBOR encoding differences (e.g. definite vs indefinite list length encoding) here or in #217 for example. Or even just having any similar sample data from the node vs ours can help since you can look out for small differences in the CBOR encoding like that. That's how I figured it out in #217 by looking at sample data from the cardano-node and noticing the indefinite CBOR list/map length encoding instead of definite. |
Am I getting it right that if we merge this change right now for the hardfork we will have to change it back? |
Plutus V1 is domed to have to always use this sad script integrity hash, but our hope is to have Plutus V2 use what we had intended. |
|
@kodemill , can you plz pull latest master and also run the I am getting this failure when checking your branch locally, and it does not happen on master: |
|
@vsubhuman I've fixed the failing test and added a couple more to cover the changes introduced in this PR. |
Thank you, @kodemill ! |
#227
PlutusListas indefinite length array instead of fixed length array, with the exception of 0-length arraysPlutusData(PlutusList) in the TransactionWitnessSet as fixed length arrays