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: sdk improvements #580

Merged
merged 35 commits into from
Apr 27, 2024
Merged

refactor: sdk improvements #580

merged 35 commits into from
Apr 27, 2024

Conversation

ctian1
Copy link
Member

@ctian1 ctian1 commented Apr 24, 2024

No description provided.

Base automatically changed from chris/reduce-program to main April 24, 2024 02:34
@ctian1 ctian1 force-pushed the chris/sdk-improvements branch from e53aede to 61b3ea6 Compare April 24, 2024 02:39
let config = CoreSC::default();
let machine = RiscvAir::machine(config);
let (pk, vk) = machine.setup(&program);
let (pk, vk) = self.core_machine.setup(&program);
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we including the ELF in the pk now? I feel like it should go in there

Copy link
Member Author

Choose a reason for hiding this comment

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

yea program is in SP1ProvingKey already actually

reduced_proof: SP1ReduceProof<InnerSC>,
) -> ShardProof<OuterSC> {
// Get verify_start_challenger from the reduce proof's public values.
let pv = RecursionPublicValues::from_vec(reduced_proof.proof.public_values.clone());
Copy link
Contributor

Choose a reason for hiding this comment

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

So do we have to do this in verification as well? (Like similar initialization of the challenger)

Copy link
Member Author

Choose a reason for hiding this comment

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

no, it's checked in the program when is_complete=1 is committed, so we can just check that


// Convert pv.vkey_digest to a bn254 field element
let mut vkey_hash = Bn254Fr::zero();
for (i, word) in pv.sp1_vk_digest.iter().enumerate() {
Copy link
Contributor

Choose a reason for hiding this comment

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

lol can we have more comments as to what is going on here and why (also maybe a link to the corresponding parts in the groth16 circuit code)

Copy link
Member Author

Choose a reason for hiding this comment

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

this is not stable yet, it's not constrained in circuit

prover/src/lib.rs Outdated Show resolved Hide resolved
sdk/src/types.rs Outdated
}

impl Prover for LocalProver {
fn prove(&self, elf: &[u8], stdin: SP1Stdin) -> Result<SP1DefaultProof> {
Copy link
Contributor

Choose a reason for hiding this comment

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

In the future, it might be nice to have a way to setup a prover with just a pk instead of the ELF, so we don't have to regenerate all of the tables/commitments.

@ctian1 ctian1 marked this pull request as ready for review April 25, 2024 23:17
@ctian1 ctian1 changed the title wip: sdk improvements refactor: sdk improvements Apr 27, 2024
@ctian1 ctian1 merged commit 79ce243 into main Apr 27, 2024
3 of 4 checks passed
@ctian1 ctian1 deleted the chris/sdk-improvements branch April 27, 2024 02:58
@matthiasgoergens
Copy link
Contributor

This should probably have been squash-merged, or its history been cleaned up before a merge.

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.

6 participants