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

Do not skip Block::id during ser/de #505

Merged
merged 1 commit into from
Nov 3, 2023
Merged

Do not skip Block::id during ser/de #505

merged 1 commit into from
Nov 3, 2023

Conversation

zeegomo
Copy link
Contributor

@zeegomo zeegomo commented Nov 2, 2023

Block.id is a necessary piece of information in the context of the consensus engine since there it's not possible to recover the id of the block since the contents are not available. Instead, we should only skip that field when serializing/deserializing a full block.

@zeegomo zeegomo added the bug Something isn't working label Nov 2, 2023
@zeegomo zeegomo added this to the Nomos testnet (playground) milestone Nov 2, 2023
@zeegomo zeegomo self-assigned this Nov 2, 2023
Block.id is a necessary piece of information in the context
of the consensus engine since there it's not possible to recover
the id of the block since the contents are not available.
Instead, we should only skip that field when serializing/deserializing
a full block.
Copy link
Contributor

@youngjoon-lee youngjoon-lee left a comment

Choose a reason for hiding this comment

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

LGTM!

Can I ask why we should kip the block id when ser/deser a "full" block?

@danielSanchezQ
Copy link
Collaborator

I see the point. Is this because we check within the consensus API and then it goes missing right?

@zeegomo
Copy link
Contributor Author

zeegomo commented Nov 3, 2023

Can I ask why we should kip the block id when ser/deser a "full" block?

Because you can get it by hashing the contents of the block.
But if you only send the header (as in the consensus API), this is not something you can do and you need to 'trust' the node to return the correct id

@zeegomo zeegomo merged commit 350620b into master Nov 3, 2023
6 checks passed
zeegomo added a commit that referenced this pull request Nov 3, 2023
Block.id is a necessary piece of information in the context
of the consensus engine since there it's not possible to recover
the id of the block since the contents are not available.
Instead, we should only skip that field when serializing/deserializing
a full block.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants