Update table - #2
Conversation
![prpal-xyz[bot]](https://avatars.githubusercontent.com/in/1456895?s=60&v=4){kind=small}
| log::debug!("received_int: {}", received_int); | ||
| FieldElement::new(received_int as u128, field) | ||
|
|
||
| FieldElement(received_int as u128, field) |
There was a problem hiding this comment.
The FieldElement::new function now correctly handles the modulo operation. However, in Channel::receive_random_field_element, FieldElement(received_int as u128, field) is used directly. While received_int (a u64) will always be less than the Goldilocks prime modulus (2^64 - 2^32 + 1), if this code were to be used with a smaller field, or if received_int could theoretically exceed u64::MAX, this direct initialization could bypass the modulo logic in FieldElement::new, leading to incorrect values. Consider consistently using FieldElement::new for safety, or add a comment explaining why direct initialization is safe here.
| .iter() | ||
| .zip(fri_merkle[..fri_merkle.len() - 1].iter()) | ||
| { | ||
| //println!("fri layer lengths: {}", layer.len()); |
There was a problem hiding this comment.
Please remove commented-out println! statements as they clutter the code. If debugging information is needed, use log::debug!.
| @@ -1,168 +1,518 @@ | |||
| use alloy::primitives::ruint::BaseConvertError; | |||
There was a problem hiding this comment.
The alloy::primitives::ruint::BaseConvertError import appears to be unused. Please remove it to keep the code clean and prevent unnecessary dependencies.
| (alpha_poly.clone() + beta_poly.clone() * x.clone().pow(d as u128)) * q.clone(), | ||
| ) | ||
| } else { | ||
| println!("processor quotient {} degree greater than degree max", i); |
There was a problem hiding this comment.
The println! statements within the combination_polynomial function (processor quotient..., memory quotient..., instruction quotient...) should be replaced with log::warn! or log::error!. println! is generally avoided in library code, especially for conditions that might indicate an issue.
| } | ||
| num | ||
| } | ||
| /*pub fn sample_index(byte_array: &[u8], size: usize) -> usize { |
There was a problem hiding this comment.
There is a large commented-out code block (sample_index, sample_indices) starting at this line. Unused or deprecated code should be removed from the codebase to improve readability and maintainability. If this code is still relevant for future use, consider documenting its purpose and storing it elsewhere (e.g., a scratchpad or separate branch).
| use std::primitive; | ||
| use chrono::Local; | ||
|
|
||
| #[allow(arithmetic_overflow)] |
There was a problem hiding this comment.
The #[allow(arithmetic_overflow)] attribute on the test_field_operations module is generally discouraged unless specific wrapping arithmetic is explicitly intended and documented. While P.wrapping_neg() is designed to wrap, operations within FieldElement methods like add_assign, mul_assign, and pow already implement explicit modulo operations. Re-evaluate if this #[allow] is truly necessary or if it's masking other potential overflow issues.
| assert_eq!(b.0, 1); | ||
| } | ||
|
|
||
| #[test] |
There was a problem hiding this comment.
The test_diff_field test case was removed, which is consistent with the removal of panic! for field mismatches in arithmetic operations. However, this also implies a design decision to no longer enforce field compatibility at the operation level, potentially leading to silent incorrect results if FieldElement instances from different fields are accidentally mixed. Ensure that field compatibility is rigorously managed at a higher architectural level if this behavior is intended.
|
The |
| let vm = VirtualMachine::new(field); | ||
| let generator = field.generator().pow((1 << 32) - 1); | ||
| let order = 1 << 32; | ||
| // let code = "++>+-[+--]++.".to_string(); |
There was a problem hiding this comment.
The Brainfuck programs used for testing are hardcoded directly in the main function. For better maintainability and reusability, these test programs should be moved to dedicated test files or loaded from a configuration, especially as the project grows.
| } | ||
| fn main() { | ||
| println!("Hello, world!"); | ||
| // std::env::set_var("RAYON_NUM_THREADS", "1024"); |
There was a problem hiding this comment.
The line std::env::set_var("RAYON_NUM_THREADS", "1024"); is commented out. If this setting is not intended to be used, it should be removed. If it's a configurable option or for specific performance tuning, consider moving it to a configuration file or making it an environment variable check within the code, along with a comment explaining its purpose.
| for i in 0..merkle_root.len(){ | ||
| print!("{}", merkle_root[i]); | ||
| } | ||
| // for i in 0..root.len() { |
There was a problem hiding this comment.
Please remove commented-out print! statements in the test function (for i in 0..root.len()...). If debugging information is needed, use log::debug! or dbg!() macros.
| @@ -1,31 +1,925 @@ | |||
| use crate::fields::FieldElement; | |||
| #![allow(unused_variables)] | |||
There was a problem hiding this comment.
The unused_variables attribute is present. Please ensure all declared variables are used or removed to maintain code clarity and prevent potential bugs.
| degree_bound, | ||
| fri_layer_length, | ||
| ); | ||
| base_idx += 8 + (4 * (fri_layer_length - 1)); |
There was a problem hiding this comment.
The hardcoded offset values in base_idx += 8 + (4 * (fri_layer_length - 1)); might be brittle. If the number of elements or the structure of the compressed_proof changes in the future, this hardcoded calculation will break. Consider deriving these offsets dynamically based on the actual proof structure to make the code more robust.
|
|
||
| pub fn extend_column( | ||
| &mut self, | ||
| _rand_field_elem: u128, |
There was a problem hiding this comment.
The rand_field_elem parameter is passed to extend_column but is unused within the function body. Please remove it from the function signature and its call site to improve clarity and avoid confusion.
| @@ -1,6 +1,7 @@ | |||
| #![allow(unused_variables)] | |||
There was a problem hiding this comment.
The unused_variables attribute is present. Please ensure all declared variables are used or removed to maintain code clarity and prevent potential bugs.
| let zero = FieldElement::zero(field); | ||
| while matrix.len() & (matrix.len() - 1) != 0 { | ||
| matrix.push(vec![zero;1]); | ||
| pub fn pad(&mut self) {} |
There was a problem hiding this comment.
The pad function for IOTable is now empty. This implies that I/O tables are not padded like other tables, or their padding is handled externally. Given that I/O length can be dynamic, this might lead to inconsistencies if non-power-of-two lengths are passed to subsequent cryptographic operations that expect padded data. If padding is not required, consider removing the pad method entirely to avoid confusion.
| let one = FieldElement::one(field); | ||
|
|
||
| // processor matrix that goes here is unpadded | ||
| pub fn derive_matrix(processor_matrix: &[Vec<FieldElement>]) -> Vec<Vec<FieldElement>> { |
There was a problem hiding this comment.
The derive_matrix function in MemoryTable no longer includes logic for inserting dummy rows based on clock jumps and memory pointer changes. The previous implementation (src/tables/memory.rs lines 70-80 in LEFT side) included a loop to insert these rows when mem_ptr was the same but cycle jumped by more than 1. If these dummy rows are crucial for the correctness of the memory permutation argument (e.g., ensuring a continuous trace for specific constraints), their removal could introduce a critical correctness issue. Please clarify if this change is intentional and how memory trace continuity is now maintained or if this aspect of the argument has been redesigned.
| - challenges[ChallengeIndices::Beta as usize]); | ||
|
|
||
| let f = |x: char| -> FieldElement { FieldElement((x as u32) as u128, self.table.field) }; | ||
| for i in 0..self.table.length - 1 { |
There was a problem hiding this comment.
The logic for updating iea (InputEvaluation) and oea (OutputEvaluation) in the extend_columns function appears to have a potential correctness issue.
For example, self.table.matrix[(i + 1) as usize][Indices::InputEvaluation as usize] = iea; assigns the current iea to the next row. Then, iea = iea * challenges[ChallengeIndices::Gamma as usize] + self.table.matrix[(i + 1) as usize][Indices::MemoryValue as usize]; updates iea using the MemoryValue from the next row, which might not be consistent with how the evaluation argument is supposed to accumulate.
Typically, the (i+1)-th evaluation argument is computed based on the i-th evaluation argument and data from the i-th row. The pattern should be new_ea = current_ea * challenge + current_value. The values self.table.matrix[(i + 1) as usize][Indices::MemoryValue as usize] are from the future row. This could lead to incorrect accumulation.
Please re-evaluate the calculation for iea and oea to ensure that the current row's values are used to compute the next row's evaluation argument, and that the assignment to the matrix is done with the newly computed value for the (i+1)-th row.
| + mv_is_zero.clone() * (ip_next.clone() - ni.clone())) | ||
| + mp_next_mp.clone() | ||
| + mv_next_mv.clone(); | ||
| air.push(trasition_i0); |
There was a problem hiding this comment.
The generate_air function for ProcessorTable pushes individual transition constraints (Transition_i0 to Transition_i7) to the air vector, and then also pushes transition_all, which appears to be a sum of general constraints. If transition_all is intended to be the comprehensive transition constraint, then the individual Transition_iX constraints added previously might be redundant or indicate a need for clearer composition. Please clarify how all transition constraints are combined or if the individual Transition_iX elements are used separately in later steps.
| let ci = domain.interpolate(values_array); | ||
| let poly = ProcessorTable::selector_polynomial('.', ci, field); | ||
| for i in 0..target_order { | ||
| println!( |
There was a problem hiding this comment.
The println! statements within the selector_polynomial function ("Selector([) value at ci:{}, ci: {}") should be replaced with log::debug! for consistency with logging practices in a library. This helps in managing output and debugging Verbosity.
|
|
||
| use crate::fields::{Field, FieldElement}; | ||
| use rayon::prelude::*; | ||
| use crate::fields::{NEG_ORDER, P}; |
There was a problem hiding this comment.
The P and NEG_ORDER constants from crate::fields are imported but not used in this module. Please remove these unused imports to maintain code clarity.
| } | ||
|
|
||
| pub fn interpolate_lagrange_polynomials(x: Vec<FieldElement>, y: Vec<FieldElement>) -> Polynomial { | ||
| println!("x ={:?}", x); |
There was a problem hiding this comment.
The println! statements in gen_polynomial_from_roots and gen_lagrange_polynomials should be replaced with log::debug! statements. Printing directly to console is typically avoided in production-ready code.
2 participants
No description provided.