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

refactor: decouple object methods into reusable primitives #339

Merged
merged 90 commits into from
Jul 30, 2024

Conversation

xavierdmello
Copy link
Contributor

Decouples object methods into standalone primitives to enable usage in restricted environments without async and SystemTime, like ZKVMs. This allows critical functions (e.g., verify_update) to run in synchronous ZK proving environments, facilitating on-chain verification of beacon chain state.

Copy link
Collaborator

@ncitron ncitron left a comment

Choose a reason for hiding this comment

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

This is a great start! My comments focus on two main areas.

  1. Avoid SP1 specific functionality. We want to ensure any prover can use this and would like to stay neutral. Because of this I don't love the SP1 patched crates being directly by Helios.

  2. Increasing the size of the common crate. If the major goal here is just to have access to the core consensus functions in a separate crate, I think the best way to do so would be by moving those into some new consensus-core crate and deeply assess how to avoid dragging things out of config and execution into this new crate or common.

Copy link
Collaborator

@ncitron ncitron left a comment

Choose a reason for hiding this comment

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

This is looking really good. Left a few comments and questions and think we should be good to merge after these things have been addressed.

Also can you address the fmt and clippy issues we are running into in CI?

@xavierdmello
Copy link
Contributor Author

Addressed comments & CI. One notable change was going from .clone() to clone_from() to make clippy happy here: https://github.com/succinctlabs/helios/blob/6e0e4f122038d8b570f9c5af9a834c79ec597c29/consensus-core/src/consensus_core.rs#L149-L155

Copy link
Collaborator

@ncitron ncitron left a comment

Choose a reason for hiding this comment

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

LGTM! Amazing work!

@ncitron ncitron merged commit 3099e30 into a16z:master Jul 30, 2024
4 checks passed
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.

2 participants