-
Notifications
You must be signed in to change notification settings - Fork 281
feat: make Jolt verifier panic-free #1214
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
base: main
Are you sure you want to change the base?
Conversation
Harden the verifier to return Result<_, ProofVerifyError> instead of panicking on malformed proofs or invalid inputs. This is critical for production use where verifier crashes could be exploited. ## Core verifier hardening (verifier.rs) - Add early validation in JoltVerifier::new() for trace_length, ram_K, bytecode_K - Use checked_next_power_of_two() to prevent overflow panics - Replace slice indexing with .get() + proper error returns - Replace zip_eq with length validation + zip - Replace HashMap::remove().unwrap() with ok_or_else() - Fix serialization methods to propagate errors ## Transcripts (blake2b.rs, keccak.rs) - Make challenge_bytes() panic-free using iterator-only access - Use guarded split_at_mut() and safe zip() instead of indexing ## Opening accumulator (opening_proof.rs) - Refactor OpeningAccumulator trait to return Result - All get_*_opening and append_* methods now return Result<_, ProofVerifyError> - Add typed error variants: MissingCommittedOpening, MissingVirtualOpening, MissingAdviceOpening ## Sumcheck verification (sumcheck.rs, sumcheck_verifier.rs) - Add empty check before .max().unwrap() calls - Make SumcheckInstanceVerifier trait methods return Result - Propagate errors through verify() and cache_openings() ## Claim reductions (claim_reductions/*.rs) - All verifier-side new() constructors return Result - Remove verifier-reachable unwraps on 2.inverse() and optional fields ## Spartan/RAM/Registers verifiers - shift.rs: Remove try_into().unwrap() in ShiftSumcheckParams::new - ram/mod.rs: remap_address now returns Option instead of panicking - output_check.rs: Verifier returns errors instead of unwrapping remap_address ## Dory PCS mitigation (external dependency) - Resample transcript challenges if zero (prevents inv().expect() panics) - Wrap dory::verify in catch_unwind to convert panics to ProofVerifyError::DoryError - Note: catch_unwind only works in panic=unwind builds ## Error types (errors.rs) - Add typed variants: CountMismatch, SliceTooShort, ClaimMismatch, AddressRemapFailed, FieldArithmeticError, MissingProofComponent - Remove catch-all InvalidProofStructure(String) All 395 jolt-core tests pass. Prover-side panics intentionally unchanged (out of scope).
Code Review FindingFile: assert_eq!(self.compressed_polys.len(), num_rounds);This Suggested fix: Note: The |
0xAndoroid
left a comment
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 to me, except for some nits that I mentioned.
I'm okay with merging in right now, though it might cause quit e a bit of conflicts with ZK implementation. I think it shouldn't be too harsh tho
- Replace assert_eq! with error return in sumcheck verify - Remove zero-resample loop in dory transcript wrapper - Make challenge_bytes a trait default impl - Remove section comments from ProofVerifyError enum
|
I'm just putting this PR up for now, I think it still requires a fair bit of plumbing & cleaning up (esp. on panic freedom on Dory side). I'm okay with letting this one stay up for a while until it's fully done. Can merge main into this occasionally and fix any problems. |
|
Dory verifier is panic free (that's what @markosg04 said) |
Summary
Harden the Jolt verifier to return
Result<_, ProofVerifyError>instead of panicking on malformed proofs or invalid inputs. This is critical for production use where verifier crashes could be exploited.Status: ✅ Complete — all 395 jolt-core tests pass
Changes
Core verifier hardening (
verifier.rs)JoltVerifier::new()fortrace_length,ram_K,bytecode_Kchecked_next_power_of_two()to prevent overflow panics.get()+ proper error returnszip_eqwith length validation +zipHashMap::remove().unwrap()withok_or_else()Transcripts (
blake2b.rs,keccak.rs)challenge_bytes()panic-free using iterator-only accesssplit_at_mut()and safezip()instead of indexingOpening accumulator (
opening_proof.rs)OpeningAccumulatortrait to returnResultget_*_openingandappend_*methods now returnResult<_, ProofVerifyError>MissingCommittedOpening,MissingVirtualOpening,MissingAdviceOpeningSumcheck verification (
sumcheck.rs,sumcheck_verifier.rs).max().unwrap()callsSumcheckInstanceVerifiertrait methods returnResultverify()andcache_openings()Claim reductions (
claim_reductions/*.rs)new()constructors returnResult2.inverse()and optional fieldsSpartan/RAM/Registers verifiers
shift.rs: Removetry_into().unwrap()inShiftSumcheckParams::newram/mod.rs:remap_addressnow returnsOptioninstead of panickingoutput_check.rs: Verifier returns errors instead of unwrappingremap_addressDory PCS mitigation (external dependency)
inv().expect()panics)dory::verifyincatch_unwindto convert panics toProofVerifyError::DoryErrorcatch_unwindonly works inpanic=unwindbuildsError types (
errors.rs)CountMismatch,SliceTooShort,ClaimMismatch,AddressRemapFailed,FieldArithmeticError,MissingProofComponentInvalidProofStructure(String)Scope
dory-pcs— mitigated viacatch_unwind, but full panic-free guarantee inpanic=abortbuilds would require patching upstream