From 725774037f1a7324fc9cc641f2eb38b06a1e1e96 Mon Sep 17 00:00:00 2001 From: Turner Date: Sun, 8 Oct 2023 13:35:35 -0700 Subject: [PATCH] Revert behavior to cost for entire contrat, fix test, add tests, fix CHANGELOG --- CHANGELOG.md | 2 +- fuel-vm/src/interpreter/blockchain.rs | 33 +++++---- fuel-vm/src/interpreter/contract.rs | 20 ------ .../src/interpreter/executors/instruction.rs | 6 +- fuel-vm/src/tests/blockchain.rs | 71 ++++++++++++++----- 5 files changed, 77 insertions(+), 55 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6e7f83eb7f..eb50d6b7d9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,12 +8,12 @@ and this project adheres to [Semantic Versioning](http://semver.org/). ### Changed -- [#598](https://github.com/FuelLabs/fuel-vm/pull/598): Update cost model for `ldc` opcode to take into account contract size. - [#595](https://github.com/FuelLabs/fuel-vm/pull/595): Removed `wee_alloc` dependency from `fuel-asm`. It now uses the builtin allocator on web targets as well. #### Breaking +- [#598](https://github.com/FuelLabs/fuel-vm/pull/598): Update cost model for `ldc` opcode to take into account contract size. - [#594](https://github.com/FuelLabs/fuel-vm/pull/594): Add new predicate input validation tests. Also improves error propagation so that predicate error message better reflects the reason for invalidity. - [#596](https://github.com/FuelLabs/fuel-vm/pull/596): Remove `core::ops::{Add, Sub}` impls from `BlockHeight`. Use `succ` and `pred` to access adjacent blocks, or perform arithmetic directly on the wrapped integer instead. diff --git a/fuel-vm/src/interpreter/blockchain.rs b/fuel-vm/src/interpreter/blockchain.rs index 1e122dc817..e283e5a7d3 100644 --- a/fuel-vm/src/interpreter/blockchain.rs +++ b/fuel-vm/src/interpreter/blockchain.rs @@ -1,5 +1,3 @@ -use alloc::vec::Vec; - use super::{ contract::{ balance, @@ -57,6 +55,7 @@ use crate::{ InterpreterStorage, }, }; +use alloc::vec::Vec; use fuel_asm::PanicReason; use fuel_storage::StorageSize; use fuel_tx::{ @@ -101,7 +100,7 @@ where a: Word, b: Word, c: Word, - ) -> IoResult<(), S::DataError> { + ) -> IoResult { let contract_max_size = self.contract_max_size(); let ( SystemRegisters { @@ -461,12 +460,13 @@ where /// contract_code = contracts[contract_id] /// mem[$ssp, $rC] = contract_code[$rB, $rC] /// ``` + /// Returns the total length of the contract code that was loaded from storage. pub(crate) fn load_contract_code( mut self, contract_id_addr: Word, contract_offset: Word, length_unpadded: Word, - ) -> IoResult<(), S::DataError> + ) -> IoResult where I: Iterator, S: InterpreterStorage, @@ -498,14 +498,21 @@ where self.input_contracts.check(&contract_id)?; + let length_unpadded: usize = length_unpadded + .try_into() + .map_err(|_| PanicReason::MemoryOverflow)?; + // Fetch the storage contract - // If $rC is greater than the code size, zero bytes are filled in. - let contract = super::contract::contract_subsection( - self.storage, - &contract_id, - length_unpadded as usize, - )?; - let contract = contract.as_ref(); + let contract_bytes: Vec = + super::contract::contract(self.storage, &contract_id)? + .into_owned() + .into(); + let contract_len = contract_bytes.len(); + let start = contract_offset; + let finish = contract_offset + .checked_add(length_unpadded) + .ok_or(PanicReason::MemoryOverflow)?; + let contract_sub_bytes = &contract_bytes[start..finish]; // Mark stack space as allocated let new_stack = dst_range.words().end; @@ -515,7 +522,7 @@ where // Copy the code. Ownership checks are not used as the stack is adjusted above. copy_from_slice_zero_fill_noownerchecks( self.memory, - contract, + contract_sub_bytes, dst_range.start, contract_offset, length, @@ -546,7 +553,7 @@ where inc_pc(self.pc)?; - Ok(()) + Ok(contract_len) } } diff --git a/fuel-vm/src/interpreter/contract.rs b/fuel-vm/src/interpreter/contract.rs index 16c286e6c5..5f4c579240 100644 --- a/fuel-vm/src/interpreter/contract.rs +++ b/fuel-vm/src/interpreter/contract.rs @@ -154,26 +154,6 @@ where .ok_or_else(|| PanicReason::ContractNotFound.into()) } -/// Gets the bytes of the contract code from storage. If the contract is longer than -/// specified `size` the contract will be shortened to `size` bytes. If the contract is -/// shorter than `size` bytes, the contract will be padded with zeros. -pub(crate) fn contract_subsection( - storage: &S, - contract: &ContractId, - size: usize, -) -> IoResult -where - S: InterpreterStorage, -{ - let mut buf = alloc::vec![0; size]; - let _ = storage - .read_contract(contract, &mut buf) - .map_err(RuntimeError::Storage)? - .ok_or(PanicReason::ContractNotFound)?; - let contract = Contract::from(buf); - Ok(contract) -} - struct ContractBalanceCtx<'vm, S, I> { storage: &'vm S, memory: &'vm mut [u8; MEM_SIZE], diff --git a/fuel-vm/src/interpreter/executors/instruction.rs b/fuel-vm/src/interpreter/executors/instruction.rs index 87535d35ec..c14a2e0ec0 100644 --- a/fuel-vm/src/interpreter/executors/instruction.rs +++ b/fuel-vm/src/interpreter/executors/instruction.rs @@ -759,9 +759,9 @@ where Instruction::LDC(ldc) => { let (a, b, c) = ldc.unpack(); - let len = r!(c); - self.load_contract_code(r!(a), r!(b), len)?; - self.dependent_gas_charge(self.gas_costs().ldc, len)?; + // returns length of internal contract that was loaded + let len = self.load_contract_code(r!(a), r!(b), r!(c))?; + self.dependent_gas_charge(self.gas_costs().ldc, len as u64)?; } Instruction::LOG(log) => { diff --git a/fuel-vm/src/tests/blockchain.rs b/fuel-vm/src/tests/blockchain.rs index f8ab80e047..afdb73ed89 100644 --- a/fuel-vm/src/tests/blockchain.rs +++ b/fuel-vm/src/tests/blockchain.rs @@ -305,7 +305,7 @@ fn ldc__load_external_contract_code() { } #[test] -fn ldc__gas_cost_is_dependent_on_size_of_rC() { +fn ldc__gas_cost_is_not_dependent_on_rC() { let rng = &mut StdRng::seed_from_u64(2322u64); let salt: Salt = rng.gen(); @@ -313,23 +313,57 @@ fn ldc__gas_cost_is_dependent_on_size_of_rC() { let gas_costs = client.gas_costs(); let ldc_cost = gas_costs.ldc; - let bytes_per_gas_increase = ldc_cost.dep_per_unit; + let ldc_dep_cost = ldc_cost.dep_per_unit; let noop_cost = gas_costs.noop; - let starting_len = 0; - let starting_gas_amount = ldc__gas_cost_for_len(&mut client, rng, salt, starting_len); + let contract_size = 1000; + let starting_rC = 0; + let starting_gas_amount = + ldc__gas_cost_for_len(&mut client, rng, salt, contract_size, starting_rC); for i in 1..10 { - // Increase by bytes_per_gas_increase for each attempt - let len_diff = i * bytes_per_gas_increase as u16; - let len = starting_len + i * bytes_per_gas_increase as u16; - // The gas should go up 1 per bytes_per_gas_increase and noop_cost every 4 bytes - // (noop is 4 bytes) - let cost_of_copy = i as u64; - let cost_of_noops = len_diff as u64 / 4 * noop_cost; - let expected_gas_used = starting_gas_amount + cost_of_copy + cost_of_noops; - - let actual_gas_used = ldc__gas_cost_for_len(&mut client, rng, salt, len); + // Increase by ldc_dep_cost for each attempt + let rC_diff = (i * ldc_dep_cost) as u16; + let rC = starting_rC + rC_diff; + // The gas should go up 0 per ldc_dep_cost and 1 per noop_cost every 4 + // bytes (noop is 4 bytes) + let cost_of_noops = rC_diff as u64 / 4 * noop_cost; + let expected_gas_used = starting_gas_amount + cost_of_noops; + + let actual_gas_used = + ldc__gas_cost_for_len(&mut client, rng, salt, contract_size, rC); + assert_eq!(actual_gas_used, expected_gas_used); + } +} + +#[test] +fn ldc__cost_is_proportional_to_total_contracts_size_not_rC() { + let rng = &mut StdRng::seed_from_u64(2322u64); + let salt: Salt = rng.gen(); + + let mut client = MemoryClient::default(); + + let gas_costs = client.gas_costs(); + let ldc_cost = gas_costs.ldc; + let ldc_dep_cost = ldc_cost.dep_per_unit; + + let contract_size = 0; + let rC = 0; + let starting_gas_amount = + ldc__gas_cost_for_len(&mut client, rng, salt, contract_size, rC); + + let bytes_per_op = 4; + + for i in 1..10 { + let contract_size = contract_size + (i * ldc_dep_cost / bytes_per_op) as usize; + + let cost_of_ldc = i; + + // The gas should go up 1 per every ldc_dep_cost even when rC is 0 + let expected_gas_used = starting_gas_amount + cost_of_ldc; + + let actual_gas_used = + ldc__gas_cost_for_len(&mut client, rng, salt, contract_size, rC); assert_eq!(actual_gas_used, expected_gas_used); } } @@ -338,18 +372,19 @@ fn ldc__gas_cost_for_len( client: &mut MemoryClient, rng: &mut StdRng, salt: Salt, - len: u16, + // in number of opcodes + target_contract_size: usize, + rC: u16, ) -> Word { let mut target_contract = vec![]; - for _ in 0..1000 { + for _ in 0..target_contract_size { target_contract.push(op::noop()); } let bytes = target_contract.into_iter().collect::>(); let contract_code: Witness = bytes.into(); - let receipts = - ldc__load_len_of_target_contract(client, rng, salt, len, contract_code); + let receipts = ldc__load_len_of_target_contract(client, rng, salt, rC, contract_code); let result = receipts.last().expect("No receipt");