Parse arbitrary RLPs. #15
Conversation
Correct treatment of string literals. Correct treatment of phantom items if is_variable_length is true. Still need to clean up code. And change "terminal" to "phantom", it's clearer.
| ) -> RlpPrefixParsed<F>{ | ||
| let is_field = self.range.is_less_than( | ||
| ctx, | ||
| Existing(prefix), |
There was a problem hiding this comment.
We did a rust trick so specifically for Existing you don't need to type it and it will auto-infer to add Existing(). This is true for all the functions below I believe.
Explanation: the function takes in any type that implements Into<QuantumCell<F>>. We implement Into<QuantumCell<F>> for AssignedValue<F>, and what Into does it auto-wrap it with Existing.
| running_max_len += 1 + max_field_len_len + max_field_len; | ||
|
|
||
| // this is just print for debug | ||
| let mut prefix_idx_val = F::from(0u64); |
There was a problem hiding this comment.
this can be removed now?
| self.range.check_less_than_safe(ctx, field_len, (max_field_len + 1) as u64); | ||
|
|
||
|
|
||
| let field_cells = witness_subarray( |
There was a problem hiding this comment.
Just a comment: this does not affect the circuit's soundness / completeness, since we always use the adjusted field_len for RLC constraint checks. But I agree it's nice to be consistent here.
| } | ||
|
|
||
| rlc.load_rlc_cache((ctx_gate, ctx_rlc), self.gate(), bit_length(rlp_array.len() as u64)); | ||
| rlc.load_rlc_cache((ctx_gate, ctx_rlc), self.gate(), bit_length(cml_max_len as u64)); |
There was a problem hiding this comment.
This is an optimization right?
In fact after looking into it, we only use rlc_cache in constrain_rlc_concat so it seems like taking the max of all field_witness.max_field_len would work?
There was a problem hiding this comment.
This is not an optimization -- it fixes a bug. If rlp_array.len() is (much) shorter than the maximum length, it won't load enough powers of gamma, and the assertion at rlc.rs:334 will fail.
axiom-eth/axiom-eth/src/rlp/rlc.rs
Line 334 in 87a8747
Yes, I think the max of all field_witness.max_field_len would work.
There was a problem hiding this comment.
Ah, we actually resize so rlp_array is always the max length. The max_length field is actually redundant.
We'll make this clearer by using a VarLenBytes struct soon.
| } | ||
|
|
||
| // this is just print for debug | ||
| let mut prefix_idx_val = F::from(0u64); |
|
|
||
| if is_variable_len{ | ||
| // If `prefix_idx >= rlp_len`, that means we are done | ||
| // Question: can we change this to an equality check? |
There was a problem hiding this comment.
I don't think so? since you need it to be true for every non-phantom item
There was a problem hiding this comment.
Sorry -- I meant an inequality check!
I think if it's phantom, then you'll have equality prefix_idx == rlp_len, so we just have to check if they are equal or not (which is cheaper than a range check)...
| // len if short or long | ||
| // 1 if literal | ||
| // 0 if phantom | ||
| item_len = self.gate().select( |
There was a problem hiding this comment.
this can also be changed to a mul
| ); // *** unconstrained | ||
| running_max_len += 1 + max_item_len_len + max_item_len; | ||
|
|
||
|
|
There was a problem hiding this comment.
random comment: in the final version it would be nice if there was not as much whitespace (e.g., here - just one empty line separating). I understand that during development it was probably helpful for organization.
| .chain([ | ||
| (prefix, one, 1), | ||
| (len_trace.rlc_val, len_trace.len, len_trace.max_len), | ||
| // or should this be... len_trace.max_len here and below?? |
There was a problem hiding this comment.
I agree with what you have below (re: comment)
| @@ -394,6 +439,46 @@ mod rlp { | |||
| } | |||
| } | |||
|
|
|||
| #[test] // fail | |||
There was a problem hiding this comment.
tests should always "pass"
if you want to test that the MockProver is failing, you can do MockProver::run(..).verify().is_err() to get a boolean for whether it didn't pass.
There was a problem hiding this comment.
I left some comments, but overall the implementation looks good. Also thanks for adding all the comments - that's very helpful for documentation.
I'm not sure RlpOfRlp is the most appropriate naming, but I think that's because we should likely replace the current decompose_rlp_array with your decompose_rlp_of_rlp - the minor savings of the more specific decompose_rlp_array (array of only strings) is probably not worth the duplicate code.
|
|
||
|
|
||
| #[derive(Clone, Debug)] | ||
| pub struct RlpItemWitness<F: ScalarField> { // EDITING HERE ******** |
There was a problem hiding this comment.
Is this any different from RlpFieldWitness? (I do agree RlpFieldWitness is a better name)
|
Replaced by #24 |
Decompose an arbitrary RLP into a list of items.
Additionally, fix two bugs in decompose_rlp_array_phase0() and decompose_rlp_array_phase1(), and add several tests.