-
Notifications
You must be signed in to change notification settings - Fork 125
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
Plutus Datums: Respect deserialized bytes #317
Conversation
This should resolve #313 Plutus datums, unlike other data (e.g. tx body), are NOT canonicized before hashing so in order to help with interoperability with other tooling we will store the serialized bytes of plutus datums when deserialized to be re-used to avoid causing problems in hash differences.
@@ -1211,4 +1270,12 @@ mod tests { | |||
witness_set.set_plutus_data(&list); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kodemill why was the witness set checked in this test? Was the mismatch causing an issue anywhere or was it just added in conservatively? From what I know the transaction body's cbor is canonicized before hashing to compute the id, and the plutus datums are not canonicized so we must not change their representation like we were. Your PR #228 made us in line with the CLI for lists but this turned out to be more of a band-aid. Does the CBOR format for the rest of the transaction matter anywhere?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the witness set change was just a conservative addition, to be in line with the CLI, feel free to revert it!
@@ -1178,12 +1236,13 @@ mod tests { | |||
let constr_0_hash = hex::encode(hash_plutus_data(&constr_0).to_bytes()); | |||
assert_eq!(constr_0_hash, "923918e403bf43c34b4ef6b48eb2ee04babed17320d8d1b9ff9ad086e86f44ec"); | |||
let constr_0_roundtrip = PlutusData::from_bytes(constr_0.to_bytes()).unwrap(); | |||
assert_eq!(constr_0, constr_0_roundtrip); | |||
// TODO: do we want semantic equality or bytewise equality? | |||
//assert_eq!(constr_0, constr_0_roundtrip); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How do we want equality/comparison to work for plutus datums? Semantic (ignoring original_bytes
) or bytewise?
} | ||
} | ||
|
||
impl cbor_event::se::Serialize for PlutusList { | ||
fn serialize<'se, W: Write>(&self, serializer: &'se mut Serializer<W>) -> cbor_event::Result<&'se mut Serializer<W>> { | ||
if self.0.len() == 0 { | ||
return Ok(serializer.write_array(cbor_event::Len::Len(0))?); | ||
let use_definite_encoding = match self.definite_encoding { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I decided to, at least right now, leave the adaption from #228's behavior in. I think it was breaking a test or two otherwise. One argument (besides saving 1 byte for non-empty lists) could be that it would cause things to break right away to stop people from making assumptions and not respecting the given byte encodings since lists are pretty fundamental and would be noticed early on (which happened in #227), instead of having it be noticed later on by some other inequality in some other part of code between us/cardano cli or even some other tool. There are other areas that it could vary like even with integer encodings or key orderings.
Should we keep this as changed in #228, or just always use definite like before?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you, @rooooooooob !
This should resolve #313
Plutus datums, unlike other data (e.g. tx body), are NOT canonicized
before hashing so in order to help with interoperability with other
tooling we will store the serialized bytes of plutus datums when
deserialized to be re-used to avoid causing problems in hash
differences.
This is related to #228 where the format for lists was changed to match
cardano-cli, but as can be seen in #313 this is still causing issues with
other parts of the datum not matching.