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

Multi-era exts #298

Merged
merged 3 commits into from
Jan 30, 2024
Merged

Multi-era exts #298

merged 3 commits into from
Jan 30, 2024

Conversation

gostkin
Copy link
Contributor

@gostkin gostkin commented Jan 23, 2024

Adds better support for multi-era. Mostly hash functions are added to tx bodies / blocks. Fixed pool naming as well

pub fn hash(&self) -> BlockHeaderHash {
let bytes = match self {
Self::Byron(block) => match block {
// why there's no to_bytes or sth for byron block headers?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gostkin There should be. I think it's the ToBytes trait? We don't implement the same traits as the other eras since we can't do encoding preservation/etc so it directly uses cbor_event's, but byron/cip25 should still have that serialization::ToBytes (in core/rust/src/serialization.rs). There's a TODO about regeneration there so we might want to think about if we want to keep this long-term or if there was a way to use the other traits for non-preserve types. For Byron it's not a big deal since everything is canonical anyway but for cip25 it's weird since some things are subsets but maybe those aren't implementing any of the traits.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

More context: we had these ToBytes/FromBytes traits from before we did preserve-encodings. (possibly named differently e.g.ToFromBytes or ToCBORBytes/FromCBORBytes - I don't remember). The new CML's Deserialize which now has those to/from bytes methods now has the ability to preserve encodings and/or force canonical encoding which byron/cip25 don't.

Byron could kind of fake it since it's all guaranteed to be canonical anyway so both methods would do the same thing. This would have to be changed in cddl-codegen though since when you're not preserving encodings in general we don't make any assumptions about all inputs being canonical so we don't implement these traits and instead directly implement cbor_event::Serialize like we did before. You couldn't blanket impl it like with ToBytes since it would blanket impl it for everything not just the byron types.

CIP25 is weird since it's not canonical but skips some data e.g. parses a subset of objects so round-tripping isn't preserved. We could maybe regen cip25 and have it impl those types with the exception of the outermost object (the one equivalent to a tx metadatum) which IIRC doesn't implement ToBytes anyway.

@gostkin gostkin force-pushed the egostkin/multiera-extensions branch 6 times, most recently from e60f987 to d8b2893 Compare January 29, 2024 10:15
@gostkin gostkin force-pushed the egostkin/multiera-extensions branch from d8b2893 to c574039 Compare January 29, 2024 11:54
@gostkin gostkin marked this pull request as ready for review January 29, 2024 19:22
Copy link
Contributor

@rooooooooob rooooooooob left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM although I think that comment was meant to be removed? since it calls to_bytes() below it.

pub fn hash(&self) -> [u8; 32] {
let bytes = match self {
Self::Byron(block) => match block {
// why there's no to_bytes or sth for byron block headers?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was this comment meant to be removed?

@@ -375,6 +376,10 @@ impl TransactionBody {
encodings: None,
}
}

pub fn hash(&self) -> [u8; 32] {
blake2b256(&self.to_cbor_bytes())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[u8; u32] is more general but less consistent than TransactionHash. I guess on the rust side there's an Into impl so it's okay.

@gostkin gostkin merged commit fda2228 into develop Jan 30, 2024
1 check passed
@SebastienGllmt SebastienGllmt deleted the egostkin/multiera-extensions branch January 30, 2024 07:54
rooooooooob added a commit that referenced this pull request Mar 11, 2024
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 pushed a commit that referenced this pull request Mar 11, 2024
* TX hash mismatch for non-canonical TXs

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

Fixes #314

* *TxBody.hash() for multi-era rework

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.
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.

None yet

2 participants