-
Notifications
You must be signed in to change notification settings - Fork 145
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
feat(jolt-sdk): Write ELF and Proof to project directory based on environment variable + refactors for over-wire proof verification. #387
Conversation
|
||
pub type CommitmentScheme = HyraxScheme<G>; | ||
|
||
#[derive(CanonicalSerialize, CanonicalDeserialize)] | ||
pub struct Proof { | ||
pub proof: RV32IJoltProof<F, CommitmentScheme>, | ||
pub commitments: JoltCommitments<CommitmentScheme>, | ||
} |
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.
can we leave this where it is? You can enable the host
feature of this package to make it std
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.
I moved this due to issues with compiling Jolt behind rust -> go ffi bindings with the host feature enabled.
jolt-sdk/macros/src/lib.rs
Outdated
@@ -443,6 +446,18 @@ impl MacroBuilder { | |||
} | |||
} | |||
|
|||
fn make_write_to_file(&self) -> TokenStream2 { |
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.
I'm not a huge fan of using an env variable here; I'd rather just call save_to_file
directly from the host
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.
The issue I was hoping to address with the env var is in regards to saving the elf file when it is compiled to a temporary directory at run time. Since the program isn't directly accessible in the host I thought this was the least intrusive. Open to suggestions.
jolt-core/src/host/mod.rs
Outdated
fs::create_dir_all("elf/").expect("could not create elf directory"); | ||
} | ||
let elf = self.elf.as_ref().unwrap(); | ||
fs::copy(elf, format!("elf/{}.elf", self.guest)) |
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.
Could you explain the rationale of this function? If we already have the ELF at the self.elf
path, why copy it to a new directory?
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.
The elf file is saved to a temporary directory during proving and is then removed at the end of execution. The intention here was to have a method that could be called during execution to save the elf file to a non temporary directory. In retrospect there is not a need for a new directory to be created the project current directory is fine.
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.
The elf file isn't removed at the end of execution, it stays in the target
directory, no?
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.
Running the default example it does not appear to persist.
https://github.com/a16z/jolt/blob/main/jolt-core/src/host/mod.rs#L123
51733b8
to
cb1588b
Compare
@moodlezoup comments addressed |
@PatStiles could you resolve the merge conflict? Otherwise looks good to merge! |
@moodlezoup resolved! |
@PatStiles could your run cargo fmt? last request 🙏 |
d15b293
to
c66784b
Compare
093bccc
to
8e76d85
Compare
Done! @moodlezoup |
"JOLT_SAVE"
env variable.Proof
->RV32IHyraxProof
and relocates it tojolt-core/src/jolt/vm/rv32i_vm.rs
making it std. This was done as for my purposes having it be no_std lead to issues when compiling ffi bindings for rust -> go.