-
Notifications
You must be signed in to change notification settings - Fork 31
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
Refactor demo and block payload implementations #1770
Conversation
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.
Looks good!
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.
LGTM
crates/hotshot/src/block_impl.rs
Outdated
// TODO: Use use VID block commitment. | ||
// <https://github.com/EspressoSystems/HotShot/issues/1730> | ||
commit::RawCommitmentBuilder::new("Txn Comm").finalize() |
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 does the transaction commitment relate to the block commitment? Does it at all? My understanding is VID just treats the whole block as a blob, without regard for transaction boundaries. Then I think the only use of the transaction hash would be as a unique index, say for filtering duplicate transactions and stuff, peripheral to the core of consensus? In that case I think this can just be a regular Keccak commitment (ie using the commit library as we normally do)
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.
Agree! Ideally, this function could just return the keccak hash, but this would need updates to the Committable
trait and some other structs that implement Committable
, so as a short-term fix, I calculated the Keccak256
hash and then converted it into a Commitment
.
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.
Made an issue for commitment refactoring: #1774.
vdemo
(for validating consensus).sdemo
to the upper crate, and renames it todemo
since it's now the only demo.demo
toblock_impl
.Closes #1673.