From 9586e192afd2659ef14afb1cf166f4ab529f89b8 Mon Sep 17 00:00:00 2001 From: green Date: Tue, 24 Sep 2024 19:49:35 +0200 Subject: [PATCH 1/2] Proper support for MemoryInstance rollback --- fuel-vm/src/interpreter/diff.rs | 91 +++++------------- fuel-vm/src/interpreter/diff/tests.rs | 68 +++++++++++-- fuel-vm/src/interpreter/memory.rs | 132 ++++++++++++++++++++++---- 3 files changed, 200 insertions(+), 91 deletions(-) diff --git a/fuel-vm/src/interpreter/diff.rs b/fuel-vm/src/interpreter/diff.rs index 6cf7d87577..6a9ad95d00 100644 --- a/fuel-vm/src/interpreter/diff.rs +++ b/fuel-vm/src/interpreter/diff.rs @@ -49,6 +49,7 @@ use super::{ Memory, PanicContext, }; +use crate::interpreter::memory::MemoryRollbackData; use storage::*; mod storage; @@ -74,7 +75,7 @@ enum Change { /// Holds a snapshot of register state. Register(T::State>), /// Holds a snapshot of memory state. - Memory(T::State), + Memory(MemoryRollbackData), /// Holds a snapshot of storage state. Storage(T::State), /// Holds a snapshot of the call stack. @@ -167,31 +168,6 @@ where value: V, } -#[derive(Clone)] -/// The state of a memory region. -struct MemoryRegion { - /// The start of the memory region. - start: usize, - /// The region of bytes. - bytes: Vec, -} - -impl Debug for MemoryRegion { - fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result { - if f.alternate() { - f.debug_struct("Memory") - .field("start", &self.start) - .field("bytes", &self.bytes) - .finish() - } else { - f.debug_struct("Memory") - .field("start", &self.start) - .field("bytes", &self.bytes.len()) - .finish() - } - } -} - fn capture_buffer_state<'iter, I, T>( a: I, b: I, @@ -323,7 +299,7 @@ where { /// The diff function generates a diff of VM state, represented by the Diff struct, /// between two VMs internal states. - pub fn diff(&self, other: &Self) -> Diff + pub fn diff(&self, desired_state: &Self) -> Diff where Tx: PartialEq + Clone + Debug + 'static, { @@ -332,66 +308,53 @@ where }; let registers = capture_buffer_state( self.registers.iter(), - other.registers.iter(), + desired_state.registers.iter(), Change::Register, ); diff.changes.extend(registers); - let frames = - capture_vec_state(self.frames.iter(), other.frames.iter(), Change::Frame); + let frames = capture_vec_state( + self.frames.iter(), + desired_state.frames.iter(), + Change::Frame, + ); diff.changes.extend(frames); let receipts = capture_vec_state( self.receipts.as_ref().iter(), - other.receipts.as_ref().iter(), + desired_state.receipts.as_ref().iter(), Change::Receipt, ); diff.changes.extend(receipts); let balances = capture_map_state( self.balances.as_ref(), - other.balances.as_ref(), + desired_state.balances.as_ref(), Change::Balance, ); diff.changes.extend(balances); - let other_memory = other.memory().clone().into_linear_memory(); - let this_memory = self.memory().clone().into_linear_memory(); - - let mut memory = this_memory.iter().enumerate().zip(other_memory.iter()); - - while let Some(((start, s_from), s_to)) = memory - .by_ref() - .find(|((_, a), b)| a != b) - .map(|((n, a), b)| ((n, *a), *b)) - { - let (mut from, mut to): (Vec<_>, Vec<_>) = memory - .by_ref() - .take_while(|((_, a), b)| a != b) - .map(|((_, a), b)| (*a, *b)) - .unzip(); - from.splice(..0, core::iter::once(s_from)).next(); - to.splice(..0, core::iter::once(s_to)).next(); - diff.changes.push(Change::Memory(Delta { - from: MemoryRegion { start, bytes: from }, - to: MemoryRegion { start, bytes: to }, - })); + let memory_rollback_data = + self.memory().collect_rollback_data(desired_state.memory()); + + if let Some(memory_rollback_data) = memory_rollback_data { + diff.changes.push(Change::Memory(memory_rollback_data)); } - if self.context != other.context { + if self.context != desired_state.context { diff.changes.push(Change::Context(Delta { from: self.context.clone(), - to: other.context.clone(), + to: desired_state.context.clone(), })) } - if self.panic_context != other.panic_context { + if self.panic_context != desired_state.panic_context { diff.changes.push(Change::PanicContext(Delta { from: self.panic_context.clone(), - to: other.panic_context.clone(), + to: desired_state.panic_context.clone(), })) } - if self.tx != other.tx { + if self.tx != desired_state.tx { let from: Arc = Arc::new(self.tx.clone()); - let to: Arc = Arc::new(other.tx.clone()); + let to: Arc = Arc::new(desired_state.tx.clone()); diff.changes.push(Change::Txn(Delta { from, to })) } @@ -416,11 +379,9 @@ where invert_receipts_ctx(&mut self.receipts, value) } Change::Balance(Previous(value)) => invert_map(self.balances.as_mut(), value), - Change::Memory(Previous(MemoryRegion { start, bytes })) => self - .memory_mut() - .write_noownerchecks(*start, bytes.len()) - .expect("Memory must exist here") - .copy_from_slice(&bytes[..]), + Change::Memory(memory_rollback_data) => { + self.memory_mut().rollback(memory_rollback_data) + } Change::Context(Previous(value)) => self.context = value.clone(), Change::PanicContext(Previous(value)) => self.panic_context = value.clone(), Change::Txn(Previous(tx)) => { @@ -510,7 +471,7 @@ impl From> for Diff { .into_iter() .map(|c| match c { Change::Register(v) => Change::Register(v.into()), - Change::Memory(v) => Change::Memory(v.into()), + Change::Memory(v) => Change::Memory(v), Change::Storage(v) => Change::Storage(v.into()), Change::Frame(v) => Change::Frame(v.into()), Change::Receipt(v) => Change::Receipt(v.into()), diff --git a/fuel-vm/src/interpreter/diff/tests.rs b/fuel-vm/src/interpreter/diff/tests.rs index 599fd9c9cd..a5f985ddde 100644 --- a/fuel-vm/src/interpreter/diff/tests.rs +++ b/fuel-vm/src/interpreter/diff/tests.rs @@ -16,6 +16,12 @@ use fuel_types::{ use test_case::test_case; use crate::{ + constraints::reg_key::{ + Reg, + RegMut, + HP, + SP, + }, consts::*, storage::MemoryStorage, }; @@ -216,15 +222,59 @@ fn test_invert_map(v: &[(u32, u32)], key: u32, value: Option) -> Vec<(u32, } #[test] -fn reset_vm_memory() { - let mut a = Interpreter::<_, _, Script>::with_memory_storage(); - a.memory_mut().grow_stack(132).unwrap(); - let mut b = a.clone(); - b.memory_mut()[100..132].copy_from_slice(&[1u8; 32]); - let diff: Diff = a.diff(&b).into(); - assert_ne!(a, b); - b.reset_vm_state(&diff); - assert_eq!(a, b); +fn reset_vm_memory_grow_stack() { + let mut latest = Interpreter::<_, _, Script>::with_memory_storage(); + let desired = latest.clone(); + latest.memory_mut().grow_stack(132).unwrap(); + let diff: Diff = latest.diff(&desired).into(); + assert_ne!(latest, desired); + latest.reset_vm_state(&diff); + assert_eq!(latest, desired); +} + +#[test] +fn reset_vm_memory_grow_heap() { + let mut latest = Interpreter::<_, _, Script>::with_memory_storage(); + let desired = latest.clone(); + let sp = 0; + let mut hp = MEM_SIZE as u64; + latest + .memory_mut() + .grow_heap_by(Reg::::new(&sp), RegMut::::new(&mut hp), 132) + .unwrap(); + let diff: Diff = latest.diff(&desired).into(); + assert_ne!(latest, desired); + latest.reset_vm_state(&diff); + assert_eq!(latest, desired); +} + +#[test] +fn reset_vm_memory_range_write_stack() { + let mut latest = Interpreter::<_, _, Script>::with_memory_storage(); + latest.memory_mut().grow_stack(132).unwrap(); + let desired = latest.clone(); + latest.memory_mut()[100..132].copy_from_slice(&[1u8; 32]); + let diff: Diff = latest.diff(&desired).into(); + assert_ne!(latest, desired); + latest.reset_vm_state(&diff); + assert_eq!(latest, desired); +} + +#[test] +fn reset_vm_memory_range_write_heap() { + let mut latest = Interpreter::<_, _, Script>::with_memory_storage(); + let sp = 0; + let mut hp = MEM_SIZE as u64; + latest + .memory_mut() + .grow_heap_by(Reg::::new(&sp), RegMut::::new(&mut hp), 132) + .unwrap(); + let desired = latest.clone(); + latest.memory_mut()[MEM_SIZE - 32..MEM_SIZE].copy_from_slice(&[1u8; 32]); + let diff: Diff = latest.diff(&desired).into(); + assert_ne!(latest, desired); + latest.reset_vm_state(&diff); + assert_eq!(latest, desired); } #[test] diff --git a/fuel-vm/src/interpreter/memory.rs b/fuel-vm/src/interpreter/memory.rs index eb7ff51ac3..a6b16a569e 100644 --- a/fuel-vm/src/interpreter/memory.rs +++ b/fuel-vm/src/interpreter/memory.rs @@ -35,10 +35,7 @@ use core::ops::{ RangeTo, }; -use alloc::{ - vec, - vec::Vec, -}; +use alloc::vec::Vec; #[cfg(test)] mod tests; @@ -130,19 +127,6 @@ impl MemoryInstance { MEM_SIZE.saturating_sub(self.heap.len()) } - /// Returns a linear memory representation where stack is at the beginning and heap is - /// at the end. - pub fn into_linear_memory(self) -> Vec { - let uninit_memory_size = MEM_SIZE - .saturating_sub(self.stack.len()) - .saturating_sub(self.heap.len()); - let uninit_memory = vec![0u8; uninit_memory_size]; - let mut memory = self.stack; - memory.extend(uninit_memory); - memory.extend(self.heap); - memory - } - /// Grows the stack to be at least `new_sp` bytes. pub fn grow_stack(&mut self, new_sp: Word) -> Result<(), PanicReason> { if new_sp > VM_MAX_RAM { @@ -359,6 +343,120 @@ impl MemoryInstance { pub fn heap_raw(&self) -> &[u8] { &self.heap } + + /// Returns a `MemoryRollbackData` that can be used to achieve the state of the + /// `desired_memory_state` instance. + pub fn collect_rollback_data( + &self, + desired_memory_state: &MemoryInstance, + ) -> Option { + if self == desired_memory_state { + return None + } + + let sp = desired_memory_state.stack.len(); + let hp = desired_memory_state.hp; + + assert!( + hp >= self.hp, + "We only allow shrinking of the heap during rollback" + ); + + let stack_changes = + get_changes(&self.stack[..sp], &desired_memory_state.stack[..sp], 0); + + let heap_start = hp + .checked_sub(self.heap_offset()) + .expect("Memory is invalid, hp is out of bounds"); + let heap = &self.heap[heap_start..]; + let desired_heap_start = hp + .checked_sub(desired_memory_state.heap_offset()) + .expect("Memory is invalid, hp is out of bounds"); + let desired_heap = &desired_memory_state.heap[desired_heap_start..]; + + let heap_changes = get_changes(heap, desired_heap, hp); + + Some(MemoryRollbackData { + sp, + hp, + stack_changes, + heap_changes, + }) + } + + /// Rollbacks the memory changes returning the memory to the old state. + pub fn rollback(&mut self, data: &MemoryRollbackData) { + self.stack.resize(data.sp, 0); + assert!( + data.hp >= self.hp, + "We only allow shrinking of the heap during rollback" + ); + self.hp = data.hp; + + for change in &data.stack_changes { + self.stack[change.global_start + ..change.global_start.saturating_add(change.data.len())] + .copy_from_slice(&change.data); + } + + let offset = self.heap_offset(); + for change in &data.heap_changes { + let local_start = change + .global_start + .checked_sub(offset) + .expect("Invalid offset"); + self.heap[local_start..local_start.saturating_add(change.data.len())] + .copy_from_slice(&change.data); + } + } +} + +fn get_changes( + latest_array: &[u8], + desired_array: &[u8], + offset: usize, +) -> Vec { + let mut changes = Vec::new(); + let mut range = None; + for (i, (old, new)) in latest_array.iter().zip(desired_array.iter()).enumerate() { + if old != new { + range = match range { + None => Some((i, 1usize)), + Some((start, count)) => Some((start, count.saturating_add(1))), + }; + } else if let Some((start, count)) = range.take() { + changes.push(MemorySliceChange { + global_start: offset.saturating_add(start), + data: desired_array[start..start.saturating_add(count)].to_vec(), + }); + } + } + if let Some((start, count)) = range.take() { + changes.push(MemorySliceChange { + global_start: offset.saturating_add(start), + data: desired_array[start..start.saturating_add(count)].to_vec(), + }); + } + changes +} + +#[derive(Debug, Clone)] +struct MemorySliceChange { + global_start: usize, + data: Vec, +} + +/// The container for the data used to rollback memory changes. +#[derive(Debug, Clone)] +pub struct MemoryRollbackData { + /// Desired stack pointer. + sp: usize, + /// Desired heap pointer. Desired heap pointer can't be less than the current one. + hp: usize, + /// Changes to the stack to achieve the desired state of the stack. + stack_changes: Vec, + /// Changes to the heap to achieve the desired state of the heap. + heap_changes: Vec, } #[cfg(feature = "test-helpers")] From 7d525a141c856b6c9fa71d482aec253cac0a6489 Mon Sep 17 00:00:00 2001 From: green Date: Tue, 24 Sep 2024 20:16:44 +0200 Subject: [PATCH 2/2] Changed the flow how `Diff` is generated --- fuel-vm/src/interpreter/diff.rs | 9 +-- fuel-vm/src/interpreter/diff/tests.rs | 97 ++++++++++++++------------- 2 files changed, 55 insertions(+), 51 deletions(-) diff --git a/fuel-vm/src/interpreter/diff.rs b/fuel-vm/src/interpreter/diff.rs index 6a9ad95d00..ffdb0005b1 100644 --- a/fuel-vm/src/interpreter/diff.rs +++ b/fuel-vm/src/interpreter/diff.rs @@ -297,9 +297,10 @@ impl Interpreter where M: Memory, { - /// The diff function generates a diff of VM state, represented by the Diff struct, - /// between two VMs internal states. - pub fn diff(&self, desired_state: &Self) -> Diff + /// The function generates a diff of VM state, represented by the Diff struct, + /// between two VMs internal states. The `desired_state` is the desired state + /// that we expect after rollback is done. + pub fn rollback_to(&self, desired_state: &Self) -> Diff where Tx: PartialEq + Clone + Debug + 'static, { @@ -487,7 +488,7 @@ impl From> for Diff { impl From> for Previous { fn from(d: Delta) -> Self { - Self(d.from) + Self(d.to) } } diff --git a/fuel-vm/src/interpreter/diff/tests.rs b/fuel-vm/src/interpreter/diff/tests.rs index a5f985ddde..48206ab44c 100644 --- a/fuel-vm/src/interpreter/diff/tests.rs +++ b/fuel-vm/src/interpreter/diff/tests.rs @@ -32,21 +32,21 @@ use super::*; fn identity() { let a = Interpreter::<_, _, Script>::without_storage(); let b = Interpreter::<_, _, Script>::without_storage(); - let diff = a.diff(&b); + let diff = a.rollback_to(&b); assert!(diff.changes.is_empty()); assert_eq!(a, b); } #[test] fn reset_vm_state() { - let a = Interpreter::<_, _, Script>::with_memory_storage(); - let mut b = Interpreter::<_, _, Script>::with_memory_storage(); - b.set_gas(1_000_000); - b.instruction(op::addi(0x10, 0x11, 1)).unwrap(); - let diff: Diff = a.diff(&b).into(); - assert_ne!(a, b); - b.reset_vm_state(&diff); - assert_eq!(a, b); + let desired = Interpreter::<_, _, Script>::with_memory_storage(); + let mut latest = Interpreter::<_, _, Script>::with_memory_storage(); + latest.set_gas(1_000_000); + latest.instruction(op::addi(0x10, 0x11, 1)).unwrap(); + let diff: Diff = latest.rollback_to(&desired).into(); + assert_ne!(desired, latest); + latest.reset_vm_state(&diff); + assert_eq!(desired, latest); } use crate::interpreter::InterpreterParams; @@ -57,33 +57,33 @@ fn record_and_invert_storage() { let interpreter_params = InterpreterParams::new(arb_gas_price, ConsensusParameters::standard()); - let a = Interpreter::<_, _, Script>::with_storage( + let desired = Interpreter::<_, _, Script>::with_storage( crate::interpreter::MemoryInstance::new(), Record::new(MemoryStorage::default()), interpreter_params.clone(), ); - let mut b = Interpreter::<_, _, Script>::with_storage( + let mut latest = Interpreter::<_, _, Script>::with_storage( crate::interpreter::MemoryInstance::new(), Record::new(MemoryStorage::default()), interpreter_params, ); as StorageMutate>::insert( - &mut b.storage, + &mut latest.storage, &(&ContractId::default(), &AssetId::default()).into(), &1u64, ) .unwrap(); - b.set_gas(1_000_000); - b.instruction(op::addi(0x10, 0x11, 1)).unwrap(); + latest.set_gas(1_000_000); + latest.instruction(op::addi(0x10, 0x11, 1)).unwrap(); - let storage_diff: Diff = b.storage_diff().into(); - let mut diff: Diff = a.diff(&b).into(); + let storage_diff: Diff = latest.storage_diff().into(); + let mut diff: Diff = latest.rollback_to(&desired).into(); diff.changes.extend(storage_diff.changes); - assert_ne!(a, b); - b.reset_vm_state(&diff); - assert_eq!(a, b); + assert_ne!(desired, latest); + latest.reset_vm_state(&diff); + assert_eq!(desired, latest); let c = Interpreter::<_, _, Script>::with_memory_storage(); let mut d = Interpreter::<_, _, Script>::with_memory_storage(); @@ -104,8 +104,8 @@ fn record_and_invert_storage() { #[test] fn reset_vm_state_frame() { - let a = Interpreter::<_, _, Script>::with_memory_storage(); - let mut b = Interpreter::<_, _, Script>::with_memory_storage(); + let desired = Interpreter::<_, _, Script>::with_memory_storage(); + let mut latest = Interpreter::<_, _, Script>::with_memory_storage(); let frame = CallFrame::new( Default::default(), Default::default(), @@ -115,17 +115,17 @@ fn reset_vm_state_frame() { Default::default(), ) .unwrap(); - b.frames.push(frame); - assert_ne!(a.frames, b.frames); - let diff: Diff = a.diff(&b).into(); - b.reset_vm_state(&diff); - assert_eq!(a.frames, b.frames); + latest.frames.push(frame); + assert_ne!(desired.frames, latest.frames); + let diff: Diff = latest.rollback_to(&desired).into(); + latest.reset_vm_state(&diff); + assert_eq!(desired.frames, latest.frames); } #[test] fn reset_vm_state_receipts() { - let a = Interpreter::<_, _, Script>::with_memory_storage(); - let mut b = Interpreter::<_, _, Script>::with_memory_storage(); + let desired = Interpreter::<_, _, Script>::with_memory_storage(); + let mut latest = Interpreter::<_, _, Script>::with_memory_storage(); let receipt = Receipt::call( Default::default(), Default::default(), @@ -137,11 +137,11 @@ fn reset_vm_state_receipts() { Default::default(), Default::default(), ); - b.receipts.push(receipt).expect("not full"); - assert_ne!(a.receipts, b.receipts); - let diff: Diff = a.diff(&b).into(); - b.reset_vm_state(&diff); - assert_eq!(a.receipts, b.receipts); + latest.receipts.push(receipt).expect("not full"); + assert_ne!(desired.receipts, latest.receipts); + let diff: Diff = latest.rollback_to(&desired).into(); + latest.reset_vm_state(&diff); + assert_eq!(desired.receipts, latest.receipts); } #[test_case(&[], &[] => it empty)] @@ -226,7 +226,7 @@ fn reset_vm_memory_grow_stack() { let mut latest = Interpreter::<_, _, Script>::with_memory_storage(); let desired = latest.clone(); latest.memory_mut().grow_stack(132).unwrap(); - let diff: Diff = latest.diff(&desired).into(); + let diff: Diff = latest.rollback_to(&desired).into(); assert_ne!(latest, desired); latest.reset_vm_state(&diff); assert_eq!(latest, desired); @@ -242,7 +242,7 @@ fn reset_vm_memory_grow_heap() { .memory_mut() .grow_heap_by(Reg::::new(&sp), RegMut::::new(&mut hp), 132) .unwrap(); - let diff: Diff = latest.diff(&desired).into(); + let diff: Diff = latest.rollback_to(&desired).into(); assert_ne!(latest, desired); latest.reset_vm_state(&diff); assert_eq!(latest, desired); @@ -254,7 +254,7 @@ fn reset_vm_memory_range_write_stack() { latest.memory_mut().grow_stack(132).unwrap(); let desired = latest.clone(); latest.memory_mut()[100..132].copy_from_slice(&[1u8; 32]); - let diff: Diff = latest.diff(&desired).into(); + let diff: Diff = latest.rollback_to(&desired).into(); assert_ne!(latest, desired); latest.reset_vm_state(&diff); assert_eq!(latest, desired); @@ -271,7 +271,7 @@ fn reset_vm_memory_range_write_heap() { .unwrap(); let desired = latest.clone(); latest.memory_mut()[MEM_SIZE - 32..MEM_SIZE].copy_from_slice(&[1u8; 32]); - let diff: Diff = latest.diff(&desired).into(); + let diff: Diff = latest.rollback_to(&desired).into(); assert_ne!(latest, desired); latest.reset_vm_state(&diff); assert_eq!(latest, desired); @@ -280,14 +280,17 @@ fn reset_vm_memory_range_write_heap() { #[test] fn reset_vm_txns() { use fuel_tx::field::Outputs; - let a = Interpreter::<_, _, Script>::with_memory_storage(); - let mut b = Interpreter::<_, _, Script>::with_memory_storage(); - b.tx.outputs_mut().push(fuel_tx::Output::ContractCreated { - contract_id: Default::default(), - state_root: Default::default(), - }); - let diff: Diff = a.diff(&b).into(); - assert_ne!(a, b); - b.reset_vm_state(&diff); - assert_eq!(a, b); + let desired = Interpreter::<_, _, Script>::with_memory_storage(); + let mut latest = Interpreter::<_, _, Script>::with_memory_storage(); + latest + .tx + .outputs_mut() + .push(fuel_tx::Output::ContractCreated { + contract_id: Default::default(), + state_root: Default::default(), + }); + let diff: Diff = latest.rollback_to(&desired).into(); + assert_ne!(desired, latest); + latest.reset_vm_state(&diff); + assert_eq!(desired, latest); }