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

[hotshot-state-prover] proof generation #2055

Merged
merged 15 commits into from
Nov 17, 2023
Merged

Conversation

mrain
Copy link
Contributor

@mrain mrain commented Nov 14, 2023

closes: #1985

I tried but cannot make functions generics because of the StakeTable trait bounds. It's too complicated.

The tests are not implemented yet for srs problem. I'm handling it now.

@mrain mrain marked this pull request as draft November 14, 2023 16:10
crates/hotshot-state-prover/src/circuit.rs Outdated Show resolved Hide resolved
crates/hotshot-state-prover/src/circuit.rs Outdated Show resolved Hide resolved
crates/hotshot-state-prover/src/lib.rs Outdated Show resolved Hide resolved
crates/hotshot-state-prover/src/lib.rs Show resolved Hide resolved
crates/hotshot-state-prover/src/circuit.rs Outdated Show resolved Hide resolved
crates/hotshot-state-prover/src/circuit.rs Outdated Show resolved Hide resolved
crates/hotshot-state-prover/src/circuit.rs Outdated Show resolved Hide resolved
@mrain mrain marked this pull request as ready for review November 16, 2023 02:34
@mrain
Copy link
Contributor Author

mrain commented Nov 16, 2023

This PR is ready for view. cc @alxiong @chancharles92

  1. padding is done in the circuit building function to avoid more memory copies.
  2. It's hard to make proof generation function generic. There are too many trait bounds. Although, this is really tempting to me.
  3. Only minimal test for proof generation, many bad cases are checked inside the circuit building tests.
  4. Other cleanup tasks will be handled during integration (e.g. struct alignment with consensus implementation)

@mrain mrain requested a review from chancharles92 November 16, 2023 02:42
Copy link
Contributor

@alxiong alxiong left a comment

Choose a reason for hiding this comment

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

👍 good work!

Apart from concrete comments, I'd like to see better/clearer organization of bad path in circuit tests.
e.g. let bit_masked_sig = |sigs| { .. } could be an internal lambda function, and can summarize some helper function to make the test code reads more smoothly.

^^ just my opinion, improve at @mrain's discretion.

crates/hotshot-state-prover/src/lib.rs Show resolved Hide resolved
crates/hotshot-state-prover/src/lib.rs Outdated Show resolved Hide resolved
crates/hotshot-state-prover/src/circuit.rs Outdated Show resolved Hide resolved
crates/hotshot-state-prover/src/circuit.rs Show resolved Hide resolved
crates/hotshot-state-prover/src/circuit.rs Outdated Show resolved Hide resolved
crates/hotshot-state-prover/src/circuit.rs Show resolved Hide resolved
crates/hotshot-state-prover/src/lib.rs Outdated Show resolved Hide resolved
crates/hotshot-state-prover/src/circuit.rs Outdated Show resolved Hide resolved
crates/hotshot-state-prover/src/lib.rs Outdated Show resolved Hide resolved
@mrain mrain requested a review from alxiong November 17, 2023 04:25
Copy link
Contributor

@alxiong alxiong left a comment

Choose a reason for hiding this comment

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

we need to change block_comm name as it is wrong, (both in this PR and in types/traits/state.rs

suggestions:

  • block_comms_root
  • block_comm_history_root

Copy link
Contributor

@alxiong alxiong left a comment

Choose a reason for hiding this comment

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

LGTM! 🚀

@elliedavidson elliedavidson merged commit 296be1b into develop Nov 17, 2023
4 of 5 checks passed
@elliedavidson elliedavidson deleted the chengyu-proofgen branch November 17, 2023 17:52
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.

[hotshot-state-prover] proof generation
4 participants