feat: serialize trusted trace proving inputs - #3284
Conversation
|
@plafer Thanks, both points were right.
|
c8d1bda to
b661a6f
Compare
54df782 to
cf221e4
Compare
Question: what would happen if someone sends a malformed MAST forest (i.e., with incorrect hashes) - would the prover error out or crush? The reason for the question is that we have two separate settings for this:
|
The prover would not crash. The bad digest is accepted into the sparse forest. From the PR description:
The closest signal for this scope is code documentation, not an API boundary strong enough to prevent misuse. I opened #3303 for the untrusted serialization. |
cf221e4 to
0c4c6f4
Compare
bobbinth
left a comment
There was a problem hiding this comment.
Thank you! Looks good. Not a full review but I left some comments inline.
| "TraceProvingInputs byte budget is smaller than payload length".into(), | ||
| )); | ||
| } | ||
| let allocation_budget = budget.min(bytes.len().saturating_mul(4)); |
There was a problem hiding this comment.
Would be good to add a comment why multiplying by 4 is appropriate/sufficient here.
There was a problem hiding this comment.
Addressed in #3314, which names the multiplier and documents the split between the byte budget and the allocation budget. It's the same pattern as the MastForest, UntrustedMastForest, Package, etc.
| if reader.has_more_bytes() { | ||
| return Err(SerdeDeserializationError::InvalidValue( | ||
| "TraceProvingInputs payload has trailing bytes".into(), | ||
| )); | ||
| } |
There was a problem hiding this comment.
Do we need this check? Having it implies that we wouldn't be able to serialize TraceProvingInputs as a part of some bigger object, right?
There was a problem hiding this comment.
The check is in read_from_bytes() because that function reads one full standalone payload. A larger wrapper should call TraceProvingInputs::read_from(reader) inside its own reader, then the wrapper owns the final end-of-input check. #3314 documents that, and it is the same pattern used e.g. by serde_json or bincode for whole-slice vs reader-based deserialization.
There was a problem hiding this comment.
This file has grown too large - let's split it up and move the content under src/trace/trace_state directory. I would split it up along the component lines. For example:
src/trace/trace_state/system.rssrc/trace/trace_state/stack.rssrc/trace/trace_state/block_stack.rs- etc.
There was a problem hiding this comment.
I agree that trace_state.rs is large. I have found large mechanical diffs have a negative impact on some reviewers, so I would rather keep that split out of this stack. Happy to have a component split as a separate cleanup PR.
| pub struct TraceBuildInputs { | ||
| trace_output: TraceBuildOutput, | ||
| trace_generation_context: TraceGenerationContext, | ||
| program_info: ProgramInfo, | ||
| } | ||
|
|
||
| #[derive(Debug)] | ||
| pub(crate) struct TraceBuildOutput { | ||
| stack_outputs: StackOutputs, | ||
| final_precompile_transcript: PrecompileTranscript, | ||
| precompile_requests: Vec<PrecompileRequest>, | ||
| precompile_requests_digest: [u8; 32], | ||
| } |
There was a problem hiding this comment.
Not related to this PR, but I find the structure of TraceBuildInputs and related structs pretty confusing. For example:
Do we actually need TraceBuildOutput? It seems like 3 out of 4 fields there are about precompiles and probably should be put into a dedicated struct. And once this is done, maybe it would make sense to dissolve this struct entirely?
Also, TraceBuildOutput doesn't need to be pub(crate) as it is used only within this module (and its submodules).
There was a problem hiding this comment.
#3314 makes TraceBuildOutput private. I would leave the larger shape change for a follow-up because it is not needed for serialization.
| /// | ||
| /// This uses the same wire shape as `Vec<T>`: a length prefix followed by items in iteration | ||
| /// order. | ||
| pub struct SerializableVecDeque<'a, T>(pub &'a VecDeque<T>); |
There was a problem hiding this comment.
We are doing this because we don't have have serialization implemented for VecDeque in miden-crypto? If so, should we just implement it it there?
If we'd rather keep it here for now, I'd move it src/utils/mod.rs.
There was a problem hiding this comment.
#3314 moves the VecDeque helper to core::utils. I did not add it to miden-crypto for now because (besides release propagation delays) the only current caller is trace replay.
| fn read_from<R: crate::serde::ByteReader>( | ||
| source: &mut R, | ||
| ) -> Result<Self, DeserializationError> { | ||
| Ok(Self(<u32 as Deserializable>::read_from(source)?)) |
There was a problem hiding this comment.
I'm not sure this is safe. I remember previously we've always avoided deserializing MAST node IDs directly and preferred using from_u32_with_node_count(). I believe this was because MAST node ID values were limited to
If this is not a concern now, we should explicitly document this. But also, I'm a bit weary of adding this serialization option as it could be easily misused.
There was a problem hiding this comment.
- 1/3 Serialize sparse MAST forests #3313 keeps
MastNodeIdserializable but removes the blanketDeserializableimpl, - The sparse reader reads raw IDs only through a local helper that checks the source node count
- 2/3 Serialize trace proving inputs #3314 uses local trace replay readers instead of a global
MastNodeId::read_from,
|
Closing this version because the change is too large as one PR. It has been replaced by three smaller PRs:
These keep the same code changes, split by scope. |
The transaction executor can now serialize
TraceProvingInputsand send them to a trusted prover. The prover can deserialize the payload, build the trace, prove it, and return a proof.This adds binary serialization for the trace replay state, sparse MAST replay data,
TraceBuildInputs,ProvingOptions, andTraceProvingInputs.Sparse MAST serialization is a trusted replay format. It preserves the source node IDs, roots, advice data, digest entries, and source forest commitment. It does not implement the untrusted hashless MAST read path (and does not verify node hashes).
Tests cover sparse round trips, malformed sparse payloads, bad serialized forest IDs, trace summary equality after
TraceBuildInputsround trip, proof generation afterTraceProvingInputsround trip, and fuzz smoke runs for the new deserialize targets.Closes #3235