Update table by manojkgorle · Pull Request #2 · manojkgorle/brainfuckvm

manojkgorle · 2025-07-23T14:30:52Z

No description provided.

table

prpal-xyz · 2025-07-23T14:32:35Z

src/channel/mod.rs

-        log::debug!("received_int: {}", received_int);
-        FieldElement::new(received_int as u128, field)
+
+        FieldElement(received_int as u128, field)


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.

prpal-xyz · 2025-07-23T14:32:38Z

src/fri/mod.rs

+        .iter()
+        .zip(fri_merkle[..fri_merkle.len() - 1].iter())
+    {
+        //println!("fri layer lengths: {}", layer.len());


Please remove commented-out println! statements as they clutter the code. If debugging information is needed, use log::debug!.

prpal-xyz · 2025-07-23T14:32:40Z

src/fri/mod.rs

@@ -1,168 +1,518 @@
+use alloy::primitives::ruint::BaseConvertError;


The alloy::primitives::ruint::BaseConvertError import appears to be unused. Please remove it to keep the code clean and prevent unnecessary dependencies.

prpal-xyz · 2025-07-23T14:32:44Z

src/fri/mod.rs

+                    (alpha_poly.clone() + beta_poly.clone() * x.clone().pow(d as u128)) * q.clone(),
+                )
+            } else {
+                println!("processor quotient {} degree greater than degree max", i);


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.

prpal-xyz · 2025-07-23T14:32:46Z

src/fri/mod.rs

-        } 
-        num
-    }
-    /*pub fn sample_index(byte_array: &[u8], size: usize) -> usize {


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).

prpal-xyz · 2025-07-23T14:32:49Z

src/fields/mod.rs

-    use std::primitive;
+    use chrono::Local;

+    #[allow(arithmetic_overflow)]


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.

prpal-xyz · 2025-07-23T14:32:52Z

src/fields/mod.rs

        assert_eq!(b.0, 1);
    }

-    #[test]


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.

prpal-xyz · 2025-07-23T14:32:59Z

The unused_imports attribute is used in main.rs. It's best practice to remove all unused imports to keep the code clean and reduce compilation time. Please identify and remove any unused imports.

prpal-xyz · 2025-07-23T14:33:01Z

src/main.rs

+    let vm = VirtualMachine::new(field);
+    let generator = field.generator().pow((1 << 32) - 1);
+    let order = 1 << 32;
+    // let code = "++>+-[+--]++.".to_string();


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.

prpal-xyz · 2025-07-23T14:33:04Z

src/main.rs

+}
 fn main() {
-    println!("Hello, world!");
+    // std::env::set_var("RAYON_NUM_THREADS", "1024");


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.

prpal-xyz · 2025-07-23T14:33:07Z

src/merkle/mod.rs

-        for i in 0..merkle_root.len(){
-            print!("{}", merkle_root[i]);
-        }
+        // for i in 0..root.len() {


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.

prpal-xyz · 2025-07-23T14:33:10Z

src/stark/mod.rs

@@ -1,31 +1,925 @@
-use crate::fields::FieldElement;
+#![allow(unused_variables)]


The unused_variables attribute is present. Please ensure all declared variables are used or removed to maintain code clarity and prevent potential bugs.

prpal-xyz · 2025-07-23T14:33:14Z

src/stark/mod.rs

+            degree_bound,
+            fri_layer_length,
+        );
+        base_idx += 8 + (4 * (fri_layer_length - 1));


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.

prpal-xyz · 2025-07-23T14:33:16Z

src/tables/instruction.rs

+
+    pub fn extend_column(
+        &mut self,
+        _rand_field_elem: u128,


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.

prpal-xyz · 2025-07-23T14:33:19Z

src/tables/io.rs

@@ -1,6 +1,7 @@
+#![allow(unused_variables)]


The unused_variables attribute is present. Please ensure all declared variables are used or removed to maintain code clarity and prevent potential bugs.

prpal-xyz · 2025-07-23T14:33:22Z

src/tables/io.rs

-        let zero = FieldElement::zero(field);
-        while matrix.len() & (matrix.len() - 1) != 0 {
-            matrix.push(vec![zero;1]);
+    pub fn pad(&mut self) {}


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.

prpal-xyz · 2025-07-23T14:33:25Z

src/tables/memory.rs

-        let one = FieldElement::one(field);
-
+    // processor matrix that goes here is unpadded
+    pub fn derive_matrix(processor_matrix: &[Vec<FieldElement>]) -> Vec<Vec<FieldElement>> {


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.

prpal-xyz · 2025-07-23T14:33:28Z

src/tables/processor.rs

+                - 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 {


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.

prpal-xyz · 2025-07-23T14:33:31Z

src/tables/processor.rs

+            + mv_is_zero.clone() * (ip_next.clone() - ni.clone()))
+            + mp_next_mp.clone()
+            + mv_next_mv.clone();
+        air.push(trasition_i0);


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.

prpal-xyz · 2025-07-23T14:33:34Z

src/tables/processor.rs

+        let ci = domain.interpolate(values_array);
+        let poly = ProcessorTable::selector_polynomial('.', ci, field);
+        for i in 0..target_order {
+            println!(


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.

prpal-xyz · 2025-07-23T14:33:37Z

src/univariate_polynomial/mod.rs


 use crate::fields::{Field, FieldElement};
-use rayon::prelude::*;
+use crate::fields::{NEG_ORDER, P};


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.

prpal-xyz · 2025-07-23T14:33:40Z

src/univariate_polynomial/mod.rs

 }

 pub fn interpolate_lagrange_polynomials(x: Vec<FieldElement>, y: Vec<FieldElement>) -> Polynomial {
-    println!("x ={:?}", x);


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.

manojkgorle and others added 30 commits December 5, 2024 21:18

Merge pull request #1 from manojkgorle/table

022539e

table

pulling

a187ec4

Merge branch 'main' of github.com:manojkgorle/brainfuckvm

e2886e3

final_removed

532ab68

choumya

04d9174

Merge branch 'main' of github.com:manojkgorle/brainfuckvm

fda86b1

memory constraints

a4de0b6

selector_polynomial

5551484

Merge branch 'main' of github.com:manojkgorle/brainfuckvm

0cb8421

zerofier

cb866f0

Merge branch 'main' of github.com:manojkgorle/brainfuckvm

495184f

enum

89012c2

gorilla

3d70040

Merge branch 'main' of github.com:manojkgorle/brainfuckvm

d17abe0

next_interpolated

dd94928

doing_interpolation

484e728

chuski

c487143

memory and lint

1d979eb

merge

2941a72

zerofier

638185b

processor_const

0ae15b6

Merge branch 'main' of github.com:manojkgorle/brainfuckvm

db1ee1c

mem

4b03256

ppa

ac4ff98

Merge branch 'main' of github.com:manojkgorle/brainfuckvm

6626b38

ppa

fe91dd1

tables

640f0b5

remove unnecessary test

8b78443

Merge branch 'main' of github.com:manojkgorle/brainfuckvm

39e8b22

.

190d6f6

soumyathakur44 and others added 8 commits December 21, 2024 19:50

fib run

fa50974

readme_added

e0432d6

Merge branch 'main' of github.com:manojkgorle/brainfuckvm

4a83b72

removed

b31bc7c

cleanup

c9e4c78

update

5b8ad1b

update comments

da2bdb9

added_comments

5c58571

prpal-xyz bot reviewed Jul 23, 2025

View reviewed changes

		@@ -1,168 +1,518 @@
		use alloy::primitives::ruint::BaseConvertError;

		@@ -1,31 +1,925 @@
		use crate::fields::FieldElement;
		#![allow(unused_variables)]

Conversation

manojkgorle commented Jul 23, 2025

Uh oh!

prpal-xyz bot Jul 23, 2025

Choose a reason for hiding this comment

Uh oh!

prpal-xyz bot Jul 23, 2025

Choose a reason for hiding this comment

Uh oh!

prpal-xyz bot Jul 23, 2025

Choose a reason for hiding this comment

Uh oh!

prpal-xyz bot Jul 23, 2025

Choose a reason for hiding this comment

Uh oh!

prpal-xyz bot Jul 23, 2025

Choose a reason for hiding this comment

Uh oh!

prpal-xyz bot Jul 23, 2025

Choose a reason for hiding this comment

Uh oh!

prpal-xyz bot Jul 23, 2025

Choose a reason for hiding this comment

Uh oh!

prpal-xyz bot commented Jul 23, 2025

Uh oh!

prpal-xyz bot Jul 23, 2025

Choose a reason for hiding this comment

Uh oh!

prpal-xyz bot Jul 23, 2025

Choose a reason for hiding this comment

Uh oh!

prpal-xyz bot Jul 23, 2025

Choose a reason for hiding this comment

Uh oh!

prpal-xyz bot Jul 23, 2025

Choose a reason for hiding this comment

Uh oh!

prpal-xyz bot Jul 23, 2025

Choose a reason for hiding this comment

Uh oh!

prpal-xyz bot Jul 23, 2025

Choose a reason for hiding this comment

Uh oh!

prpal-xyz bot Jul 23, 2025

Choose a reason for hiding this comment

Uh oh!

prpal-xyz bot Jul 23, 2025

Choose a reason for hiding this comment

Uh oh!

prpal-xyz bot Jul 23, 2025

Choose a reason for hiding this comment

Uh oh!

prpal-xyz bot Jul 23, 2025

Choose a reason for hiding this comment

Uh oh!

prpal-xyz bot Jul 23, 2025

Choose a reason for hiding this comment

Uh oh!

prpal-xyz bot Jul 23, 2025

Choose a reason for hiding this comment

Uh oh!

prpal-xyz bot Jul 23, 2025

Choose a reason for hiding this comment

Uh oh!

prpal-xyz bot Jul 23, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants