MPT Sync#81
Conversation
|
|
||
| /// Returns the random linear combination of the inputs. | ||
| /// Encoding is done as follows: v_0 * R^0 + v_1 * R^1 + ... | ||
| pub(crate) mod rlc { |
There was a problem hiding this comment.
Bit of a strange place to put, maybe just a temp location?
| // Replace a CellType_::Storage by CellType_::StoragePermutation if the later has | ||
| // better height |
There was a problem hiding this comment.
Maybe a bit optimistic to do this here because the cell manager doesn't know what cells will be requested afterwards, so could make a bad (or of course also a good one) decision by doing this? Seems like this decision could only really be moade after all cells have been queried.
There was a problem hiding this comment.
Agreed. This is from evm-circuit as well, they probably do it because there's not much copy constraints. I feel like optimization like this can be done afterwards, at least for a general purpose circuit-tool it shouldn't make such assumption.
| require!(a!(state_machine.is_start) => true); | ||
| }}; | ||
| // Main state machine | ||
| let sum_states = a!(state_machine.is_start) + a!(state_machine.is_branch) + a!(state_machine.is_account) + a!(state_machine.is_storage); |
There was a problem hiding this comment.
I think sum_states checks can be removed because already done by matchx!?
| for node in nodes.iter() { | ||
| // Assign bytes | ||
| let mut rlp_values = Vec::new(); | ||
| let mut cahced_region = CachedRegion::new( |
There was a problem hiding this comment.
| let mut cahced_region = CachedRegion::new( | |
| let mut cached_region = CachedRegion::new( |
| // println!("{}: start", offset); | ||
| assign!(region, (self.state_machine.is_start, offset) => true.scalar())?; | ||
| self.state_machine.start_config.assign( | ||
| let mut cahced_region = CachedRegion::new( |
There was a problem hiding this comment.
Instead of making a cached region per state, maybe just make one (with max height of states) and use it in each state?
| let q_enable = meta.fixed_column(); | ||
| let q_first = meta.fixed_column(); | ||
| let q_last = meta.fixed_column(); | ||
| let rows_left_in_state = meta.fixed_column(); |
There was a problem hiding this comment.
This should be advice no? (values depend on which nodes are inserted in the circuit)
| } | ||
|
|
||
| pub trait CustomTable: Clone + Copy + Debug + PartialEq + Eq + PartialOrd + Ord + Hash{ | ||
| fn matches_to(&self, other: &Self) -> bool; |
There was a problem hiding this comment.
I guess matches_to can be removed because == is used directly (and should be as flexible)?
There was a problem hiding this comment.
True, in fact CustomTable is just a marker trait because we need a type parameter for generic tables. It kind of make sense to me seeing specific tools bounded by specific table and config:
pub struct ConstraintBuilder<F, T: CustomTable> {
constraints: Vec<(&'static str, Expression<F>)>,
max_degree: usize,
conditions: Vec<Expression<F>>,
pub lookups: Vec<LookupData<F>>,
pub lookup_tables: Vec<LookupData<F>>,
pub cell_manager: Option<CellManager<F, T>>,
}
pub struct CellManager<F, T: CustomTable> {
pub(crate) width: usize,
pub(crate) height: usize,
pub(crate) cells: Vec<Cell<F>>,
pub(crate) columns: Vec<CellColumn<F, T>>,
pub(crate) phase_config: PhaseConfig<T>,
pub(crate) copy_columns: usize,
}
pub struct PhaseConfig<T> {
phase3: Vec<(T, usize)>, //lookup
phase2: usize, // rlc
phase1: usize, // byte
}
Maybe we can also move these functions in CustomTable as well.
There was a problem hiding this comment.
Or maybe the opposite, allow LookupTable to be used in the cellmanager as trait instead of CustomTable?
|
New todos:
|
3 participants
Issue Link
MPT Review
Type of change
More to come ...