feat: add memory trace prove entrypoint #71
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
tcoratger
reviewed
Nov 20, 2024
tcoratger
reviewed
Nov 20, 2024
Comment on lines
5
to
19
| /// The claim for the Memory component | ||
| #[derive(Debug, Eq, PartialEq)] | ||
| pub struct Claim { | ||
| pub log_size: u32, | ||
| } | ||
| impl Claim { | ||
| pub fn log_sizes(&self) -> TreeVec<Vec<u32>> { | ||
| let trace_log_sizes = vec![self.log_size; N_COLS_MEMORY_TABLE]; | ||
| TreeVec::new(vec![trace_log_sizes]) | ||
| } | ||
|
|
||
| pub fn mix_into(&self, channel: &mut impl Channel) { | ||
| channel.mix_u64(u64::from(self.log_size)); | ||
| } | ||
| } |
There was a problem hiding this comment.
Can we have header documentation for these functions to know what is their purpose?
Comment on lines
+202
to
+211
| /// Number of columns in the memory table | ||
| pub const N_COLS_MEMORY_TABLE: usize = 4; | ||
| /// Index of the `clk` register column in the Memory trace. | ||
| const CLK_COL_INDEX: usize = 0; | ||
| /// Index of the `mp` register column in the Memory trace. | ||
| const MP_COL_INDEX: usize = 1; | ||
| /// Index of the `mv` register column in the Memory trace. | ||
| const MV_COL_INDEX: usize = 2; | ||
| /// Index of the `d` register column in the Memory trace. | ||
| const D_COL_INDEX: usize = 3; |
There was a problem hiding this comment.
Maybe (just a suggestion, totally fine to do in a follow up or not if you don't like it) to avoid the number of constants into the code, can't we transform that into an enum?
/// Enum representing the column indices in the Memory trace
#[derive(Debug, Clone, Copy, PartialEq, Eq)]
pub enum MemoryColumn {
Clk,
Mp,
Mv,
D,
}
impl MemoryColumn {
/// Returns the index of the column in the Memory table
pub fn index(self) -> usize {
match self {
MemoryColumn::Clk => 0,
MemoryColumn::Mp => 1,
MemoryColumn::Mv => 2,
MemoryColumn::D => 3,
}
}
/// Returns the total number of columns in the Memory table
pub fn count() -> usize {
std::mem::variant_count::<Self>()
}
}There was a problem hiding this comment.
I'm not opinionated on const/enum, the goal is to reduce the number of public variables: having a single enum rather than 5 constants, + the number of columns in the table is not a magic number but calculated from the number of columns defined in the enum
From this angle, having an enum seems better
There was a problem hiding this comment.
let us do in a follow up if you want so that we don't block things here
There was a problem hiding this comment.
okay, i'll open an issue then
Comment on lines
213
to
247
| /// Transforms the [`MemoryTable`] into [`super::super::TraceEval`], to be commited when generating | ||
| /// a STARK proof. | ||
| /// | ||
| /// The [`MemoryTable`] is transformed from an array of rows (one element = one step of all | ||
| /// registers) to an array of columns (one element = all steps of one register). | ||
| /// It is then evaluated on the circle domain. | ||
| /// | ||
| /// # Arguments | ||
| /// * memory - The [`MemoryTable`] containing the sorted and padded trace as an array of rows. | ||
| pub fn get_trace_evaluation(memory: &MemoryTable) -> Result<(TraceEval, Claim), TraceError> { | ||
| let table = memory.table(); | ||
| let n_rows = table.len() as u32; | ||
| if n_rows == 0 { | ||
| return Err(TraceError::EmptyTraceError); | ||
| } | ||
| let log_n_rows = n_rows.ilog2(); | ||
| // TODO: Confirm that the log_size used for evaluation on Circle domain is the log_size of the | ||
| // table plus the SIMD lanes | ||
| let log_size = log_n_rows + LOG_N_LANES; | ||
| let mut trace: Vec<BaseColumn> = | ||
| (0..N_COLS_MEMORY_TABLE).map(|_| BaseColumn::zeros(1 << log_size)).collect(); | ||
|
|
||
| for (vec_row, row) in table.iter().enumerate().take(1 << log_n_rows) { | ||
| trace[CLK_COL_INDEX].data[vec_row] = row.clk().into(); | ||
| trace[MP_COL_INDEX].data[vec_row] = row.mp().into(); | ||
| trace[MV_COL_INDEX].data[vec_row] = row.mv().into(); | ||
| trace[D_COL_INDEX].data[vec_row] = row.d().into(); | ||
| } | ||
|
|
||
| let domain = CanonicCoset::new(log_size).circle_domain(); | ||
| let trace = trace.into_iter().map(|col| CircleEvaluation::new(domain, col)).collect(); | ||
|
|
||
| // TODO: Confirm that the log_size in `Claim` is `log_size`, including the SIMD lanes | ||
| Ok((trace, Claim { log_size })) | ||
| } |
There was a problem hiding this comment.
can't we integrate this directy into impl MemoryTable in order to have:
pub fn get_trace_evaluation(&self) -> Result<(TraceEval, Claim), TraceError> {
...
}There was a problem hiding this comment.
Effectively, refactoring it to MemoryTable
tcoratger
approved these changes
Nov 20, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes #55
The
write_tracefunction is in thetable.rsfile to access the private fields ofMemoryTableandMemoryTableRow, but it might be better to refactor it into its own file?The conversion from
MemoryTabletoVec<BaseColumn>and the evaluation on the circle domain could be refactored into two separate functions, to be used in thewrite_tracefunction.The name
write_tracemight be misleading as we're not writing the trace to a tree but simply returning the Trace evaluation and the Claim on its size. (We could add the tree to which the trace evaluation should be written to as parameter and do the write part in this function, it is currently in theprove_brainfuckastree_builder.extend_evals(trace)I'm assuming that trying to convert an empty
MemoryTableto aTracefor the Memory component should not be possible, it means that the execution trace is empty (compared to the I/O tables where it would simply mean no inputs nor output instruction)About the
log_size, when using the SimdBackend, theLOG_N_LANESbe appended to the size of the trace, but I'm not so sure, to be confirmed.The
Claimshould be on the log_size of theMemoryTable, or thelog_sizeappended withLOG_N_LANES?I think that it is the MemoryTable one, but still unsure.