Merge pull request #1561 from 0xPolygonMiden/plafer-1560-fix-memory-bug

Memory handling of 2 reads at the same address in the same clock cycle

CHANGELOG.md

@@ -36,6 +36,7 @@
 - Fixed the construction of the chiplets virtual table (#1514) (#1556)
 - Fixed the construction of the chiplets bus (#1516) (#1525)
 - Decorators are now allowed in empty basic blocks (#1466)
+- Return an error if an instruction performs 2 memory accesses at the same memory address in the same cycle (#1561)
 
 ## 0.10.6 (2024-09-12) - `miden-processor` crate only
 

miden/tests/integration/operations/io_ops/mem_ops.rs

@@ -1,3 +1,6 @@
+use prover::ExecutionError;
+use test_utils::expect_exec_error;
+
 use super::{apply_permutation, build_op_test, build_test, Felt, ToElements, TRUNCATE_STACK_PROC};
 
 // LOADING SINGLE ELEMENT ONTO THE STACK (MLOAD)
@@ -228,3 +231,24 @@ fn read_after_write() {
     let test = build_op_test!("mem_storew.0 dropw mem_loadw.0", &[1, 2, 3, 4, 5, 6, 7, 8]);
     test.expect_stack(&[8, 7, 6, 5]);
 }
+
+// MISC
+// ================================================================================================
+
+/// Ensures that the processor returns an error when 2 memory operations occur in the same context,
+/// at the same address, and in the same clock cycle (which is what RCOMBBASE does when `stack[13] =
+/// stack[14] = 0`).
+#[test]
+fn mem_reads_same_clock_cycle() {
+    let asm_op = "begin rcomb_base end";
+
+    let test = build_test!(asm_op);
+    expect_exec_error!(
+        test,
+        ExecutionError::DuplicateMemoryAccess {
+            ctx: 0_u32.into(),
+            addr: 0,
+            clk: 1_u32.into()
+        }
+    );
+}

processor/src/chiplets/memory/mod.rs

@@ -11,7 +11,7 @@ use super::{
     utils::{split_element_u32_into_u16, split_u32_into_u16},
     Felt, FieldElement, RangeChecker, TraceFragment, Word, EMPTY_WORD, ONE,
 };
-use crate::system::ContextId;
+use crate::{system::ContextId, ExecutionError};
 
 mod segment;
 use segment::MemorySegmentTrace;
@@ -137,15 +137,32 @@ impl Memory {
     ///
     /// If the specified address hasn't been previously written to, four ZERO elements are
     /// returned. This effectively implies that memory is initialized to ZERO.
-    pub fn read(&mut self, ctx: ContextId, addr: u32, clk: RowIndex) -> Word {
+    ///
+    /// # Errors
+    /// - Returns an error if the same address is accessed more than once in the same clock cycle.
+    pub fn read(
+        &mut self,
+        ctx: ContextId,
+        addr: u32,
+        clk: RowIndex,
+    ) -> Result<Word, ExecutionError> {
         self.num_trace_rows += 1;
-        self.trace.entry(ctx).or_default().read(addr, Felt::from(clk))
+        self.trace.entry(ctx).or_default().read(ctx, addr, Felt::from(clk))
     }
 
     /// Writes the provided word at the specified context/address.
-    pub fn write(&mut self, ctx: ContextId, addr: u32, clk: RowIndex, value: Word) {
+    ///
+    /// # Errors
+    /// - Returns an error if the same address is accessed more than once in the same clock cycle.
+    pub fn write(
+        &mut self,
+        ctx: ContextId,
+        addr: u32,
+        clk: RowIndex,
+        value: Word,
+    ) -> Result<(), ExecutionError> {
         self.num_trace_rows += 1;
-        self.trace.entry(ctx).or_default().write(addr, Felt::from(clk), value);
+        self.trace.entry(ctx).or_default().write(ctx, addr, Felt::from(clk), value)
     }
 
     // EXECUTION TRACE GENERATION

processor/src/chiplets/memory/segment.rs

@@ -1,11 +1,15 @@
-use alloc::{collections::BTreeMap, vec::Vec};
+use alloc::{
+    collections::{btree_map::Entry, BTreeMap},
+    vec::Vec,
+};
 
 use miden_air::{
     trace::chiplets::memory::{Selectors, MEMORY_COPY_READ, MEMORY_INIT_READ, MEMORY_WRITE},
     RowIndex,
 };
 
 use super::{Felt, Word, INIT_MEM_VALUE};
+use crate::{ContextId, ExecutionError};
 
 // MEMORY SEGMENT TRACE
 // ================================================================================================
@@ -72,37 +76,66 @@ impl MemorySegmentTrace {
     ///
     /// If the specified address hasn't been previously written to, four ZERO elements are
     /// returned. This effectively implies that memory is initialized to ZERO.
-    pub fn read(&mut self, addr: u32, clk: Felt) -> Word {
+    ///
+    /// # Errors
+    /// - Returns an error if the same address is accessed more than once in the same clock cycle.
+    pub fn read(&mut self, ctx: ContextId, addr: u32, clk: Felt) -> Result<Word, ExecutionError> {
         // look up the previous value in the appropriate address trace and add (clk, prev_value)
         // to it; if this is the first time we access this address, create address trace for it
         // with entry (clk, [ZERO, 4]). in both cases, return the last value in the address trace.
-        self.0
-            .entry(addr)
-            .and_modify(|addr_trace| {
-                let last_value = addr_trace.last().expect("empty address trace").value();
-                let access = MemorySegmentAccess::new(clk, MemoryOperation::CopyRead, last_value);
-                addr_trace.push(access);
-            })
-            .or_insert_with(|| {
+        match self.0.entry(addr) {
+            Entry::Vacant(vacant_entry) => {
                 let access =
                     MemorySegmentAccess::new(clk, MemoryOperation::InitRead, INIT_MEM_VALUE);
-                vec![access]
-            })
-            .last()
-            .expect("empty address trace")
-            .value()
+                vacant_entry.insert(vec![access]);
+                Ok(INIT_MEM_VALUE)
+            },
+            Entry::Occupied(mut occupied_entry) => {
+                let addr_trace = occupied_entry.get_mut();
+                if addr_trace.last().expect("empty address trace").clk() == clk {
+                    Err(ExecutionError::DuplicateMemoryAccess { ctx, addr, clk })
+                } else {
+                    let last_value = addr_trace.last().expect("empty address trace").value();
+                    let access =
+                        MemorySegmentAccess::new(clk, MemoryOperation::CopyRead, last_value);
+                    addr_trace.push(access);
+
+                    Ok(last_value)
+                }
+            },
+        }
     }
 
     /// Writes the provided word at the specified address. The memory access is assumed to happen
     /// at the provided clock cycle.
-    pub fn write(&mut self, addr: u32, clk: Felt, value: Word) {
+    ///
+    /// # Errors
+    /// - Returns an error if the same address is accessed more than once in the same clock cycle.
+    pub fn write(
+        &mut self,
+        ctx: ContextId,
+        addr: u32,
+        clk: Felt,
+        value: Word,
+    ) -> Result<(), ExecutionError> {
         // add a memory access to the appropriate address trace; if this is the first time
         // we access this address, initialize address trace.
         let access = MemorySegmentAccess::new(clk, MemoryOperation::Write, value);
-        self.0
-            .entry(addr)
-            .and_modify(|addr_trace| addr_trace.push(access))
-            .or_insert_with(|| vec![access]);
+        match self.0.entry(addr) {
+            Entry::Vacant(vacant_entry) => {
+                vacant_entry.insert(vec![access]);
+                Ok(())
+            },
+            Entry::Occupied(mut occupied_entry) => {
+                let addr_trace = occupied_entry.get_mut();
+                if addr_trace.last().expect("empty address trace").clk() == clk {
+                    Err(ExecutionError::DuplicateMemoryAccess { ctx, addr, clk })
+                } else {
+                    addr_trace.push(access);
+                    Ok(())
+                }
+            },
+        }
     }
 
     // INNER VALUE ACCESSORS

processor/src/chiplets/memory/tests.rs

@@ -28,27 +28,27 @@ fn mem_read() {
 
     // read a value from address 0; clk = 1
     let addr0 = 0;
-    let value = mem.read(ContextId::root(), addr0, 1.into());
+    let value = mem.read(ContextId::root(), addr0, 1.into()).unwrap();
     assert_eq!(EMPTY_WORD, value);
     assert_eq!(1, mem.size());
     assert_eq!(1, mem.trace_len());
 
     // read a value from address 3; clk = 2
     let addr3 = 3;
-    let value = mem.read(ContextId::root(), addr3, 2.into());
+    let value = mem.read(ContextId::root(), addr3, 2.into()).unwrap();
     assert_eq!(EMPTY_WORD, value);
     assert_eq!(2, mem.size());
     assert_eq!(2, mem.trace_len());
 
     // read a value from address 0 again; clk = 3
-    let value = mem.read(ContextId::root(), addr0, 3.into());
+    let value = mem.read(ContextId::root(), addr0, 3.into()).unwrap();
     assert_eq!(EMPTY_WORD, value);
     assert_eq!(2, mem.size());
     assert_eq!(3, mem.trace_len());
 
     // read a value from address 2; clk = 4
     let addr2 = 2;
-    let value = mem.read(ContextId::root(), addr2, 4.into());
+    let value = mem.read(ContextId::root(), addr2, 4.into()).unwrap();
     assert_eq!(EMPTY_WORD, value);
     assert_eq!(3, mem.size());
     assert_eq!(4, mem.trace_len());
@@ -81,30 +81,30 @@ fn mem_write() {
     // write a value into address 0; clk = 1
     let addr0 = 0;
     let value1 = [ONE, ZERO, ZERO, ZERO];
-    mem.write(ContextId::root(), addr0, 1.into(), value1);
+    mem.write(ContextId::root(), addr0, 1.into(), value1).unwrap();
     assert_eq!(value1, mem.get_value(ContextId::root(), addr0).unwrap());
     assert_eq!(1, mem.size());
     assert_eq!(1, mem.trace_len());
 
     // write a value into address 2; clk = 2
     let addr2 = 2;
     let value5 = [Felt::new(5), ZERO, ZERO, ZERO];
-    mem.write(ContextId::root(), addr2, 2.into(), value5);
+    mem.write(ContextId::root(), addr2, 2.into(), value5).unwrap();
     assert_eq!(value5, mem.get_value(ContextId::root(), addr2).unwrap());
     assert_eq!(2, mem.size());
     assert_eq!(2, mem.trace_len());
 
     // write a value into address 1; clk = 3
     let addr1 = 1;
     let value7 = [Felt::new(7), ZERO, ZERO, ZERO];
-    mem.write(ContextId::root(), addr1, 3.into(), value7);
+    mem.write(ContextId::root(), addr1, 3.into(), value7).unwrap();
     assert_eq!(value7, mem.get_value(ContextId::root(), addr1).unwrap());
     assert_eq!(3, mem.size());
     assert_eq!(3, mem.trace_len());
 
     // write a value into address 0; clk = 4
     let value9 = [Felt::new(9), ZERO, ZERO, ZERO];
-    mem.write(ContextId::root(), addr0, 4.into(), value9);
+    mem.write(ContextId::root(), addr0, 4.into(), value9).unwrap();
     assert_eq!(value7, mem.get_value(ContextId::root(), addr1).unwrap());
     assert_eq!(3, mem.size());
     assert_eq!(4, mem.trace_len());
@@ -137,35 +137,35 @@ fn mem_write_read() {
     // write 1 into address 5; clk = 1
     let addr5 = 5;
     let value1 = [ONE, ZERO, ZERO, ZERO];
-    mem.write(ContextId::root(), addr5, 1.into(), value1);
+    mem.write(ContextId::root(), addr5, 1.into(), value1).unwrap();
 
     // write 4 into address 2; clk = 2
     let addr2 = 2;
     let value4 = [Felt::new(4), ZERO, ZERO, ZERO];
-    mem.write(ContextId::root(), addr2, 2.into(), value4);
+    mem.write(ContextId::root(), addr2, 2.into(), value4).unwrap();
 
     // read a value from address 5; clk = 3
-    mem.read(ContextId::root(), addr5, 3.into());
+    mem.read(ContextId::root(), addr5, 3.into()).unwrap();
 
     // write 2 into address 5; clk = 4
     let value2 = [Felt::new(2), ZERO, ZERO, ZERO];
-    mem.write(ContextId::root(), addr5, 4.into(), value2);
+    mem.write(ContextId::root(), addr5, 4.into(), value2).unwrap();
 
     // read a value from address 2; clk = 5
-    mem.read(ContextId::root(), addr2, 5.into());
+    mem.read(ContextId::root(), addr2, 5.into()).unwrap();
 
     // write 7 into address 2; clk = 6
     let value7 = [Felt::new(7), ZERO, ZERO, ZERO];
-    mem.write(ContextId::root(), addr2, 6.into(), value7);
+    mem.write(ContextId::root(), addr2, 6.into(), value7).unwrap();
 
     // read a value from address 5; clk = 7
-    mem.read(ContextId::root(), addr5, 7.into());
+    mem.read(ContextId::root(), addr5, 7.into()).unwrap();
 
     // read a value from address 2; clk = 8
-    mem.read(ContextId::root(), addr2, 8.into());
+    mem.read(ContextId::root(), addr2, 8.into()).unwrap();
 
     // read a value from address 5; clk = 9
-    mem.read(ContextId::root(), addr5, 9.into());
+    mem.read(ContextId::root(), addr5, 9.into()).unwrap();
 
     // check generated trace and memory data provided to the ChipletsBus; rows should be sorted by
     // address and then clock cycle
@@ -208,33 +208,33 @@ fn mem_multi_context() {
 
     // write a value into ctx = ContextId::root(), addr = 0; clk = 1
     let value1 = [ONE, ZERO, ZERO, ZERO];
-    mem.write(ContextId::root(), 0, 1.into(), value1);
+    mem.write(ContextId::root(), 0, 1.into(), value1).unwrap();
     assert_eq!(value1, mem.get_value(ContextId::root(), 0).unwrap());
     assert_eq!(1, mem.size());
     assert_eq!(1, mem.trace_len());
 
     // write a value into ctx = 3, addr = 1; clk = 4
     let value2 = [ZERO, ONE, ZERO, ZERO];
-    mem.write(3.into(), 1, 4.into(), value2);
+    mem.write(3.into(), 1, 4.into(), value2).unwrap();
     assert_eq!(value2, mem.get_value(3.into(), 1).unwrap());
     assert_eq!(2, mem.size());
     assert_eq!(2, mem.trace_len());
 
     // read a value from ctx = 3, addr = 1; clk = 6
-    let value = mem.read(3.into(), 1, 6.into());
+    let value = mem.read(3.into(), 1, 6.into()).unwrap();
     assert_eq!(value2, value);
     assert_eq!(2, mem.size());
     assert_eq!(3, mem.trace_len());
 
     // write a value into ctx = 3, addr = 0; clk = 7
     let value3 = [ZERO, ZERO, ONE, ZERO];
-    mem.write(3.into(), 0, 7.into(), value3);
+    mem.write(3.into(), 0, 7.into(), value3).unwrap();
     assert_eq!(value3, mem.get_value(3.into(), 0).unwrap());
     assert_eq!(3, mem.size());
     assert_eq!(4, mem.trace_len());
 
     // read a value from ctx = 0, addr = 0; clk = 9
-    let value = mem.read(ContextId::root(), 0, 9.into());
+    let value = mem.read(ContextId::root(), 0, 9.into()).unwrap();
     assert_eq!(value1, value);
     assert_eq!(3, mem.size());
     assert_eq!(5, mem.trace_len());
@@ -270,17 +270,17 @@ fn mem_get_state_at() {
     // Write 1 into (ctx = 0, addr = 5) at clk = 1.
     // This means that mem[5] = 1 at the beginning of clk = 2
     let value1 = [ONE, ZERO, ZERO, ZERO];
-    mem.write(ContextId::root(), 5, 1.into(), value1);
+    mem.write(ContextId::root(), 5, 1.into(), value1).unwrap();
 
     // Write 4 into (ctx = 0, addr = 2) at clk = 2.
     // This means that mem[2] = 4 at the beginning of clk = 3
     let value4 = [Felt::new(4), ZERO, ZERO, ZERO];
-    mem.write(ContextId::root(), 2, 2.into(), value4);
+    mem.write(ContextId::root(), 2, 2.into(), value4).unwrap();
 
     // write 7 into (ctx = 3, addr = 3) at clk = 4
     // This means that mem[3] = 7 at the beginning of clk = 4
     let value7 = [Felt::new(7), ZERO, ZERO, ZERO];
-    mem.write(3.into(), 3, 4.into(), value7);
+    mem.write(3.into(), 3, 4.into(), value7).unwrap();
 
     // Check memory state at clk = 2
     assert_eq!(mem.get_state_at(ContextId::root(), 2.into()), vec![(5, value1)]);