From 7fc47f17aa5ba3414316ede3591268b21bbd2d1d Mon Sep 17 00:00:00 2001 From: green Date: Mon, 23 Dec 2024 12:02:34 -0500 Subject: [PATCH] Use `rollback_to` correctly. Removed `V2` gas costs version to follow the old way of compatibility. Implemented `offset` logic in more easier way. Fixed flakiness of the `test_poa_multiple_producers` test. --- CHANGELOG.md | 1 + benches/benches/vm.rs | 3 - benches/src/lib.rs | 11 +- crates/client/assets/schema.sdl | 1 - crates/client/src/client/schema/chain.rs | 128 +------------------ crates/fuel-core/src/schema/chain.rs | 5 +- crates/fuel-core/src/state/rocks_db.rs | 24 ++-- crates/services/txpool_v2/src/tests/mocks.rs | 16 ++- crates/storage/src/kv_store.rs | 32 +++-- tests/tests/poa.rs | 23 ++-- 10 files changed, 62 insertions(+), 182 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e7a976c8a96..aa3cadb48dc 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -51,6 +51,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/). - [2233](https://github.com/FuelLabs/fuel-core/pull/2233): Introduce a new column `modification_history_v2` for storing the modification history in the historical rocksDB. Keys in this column are stored in big endian order. Changed the behaviour of the historical rocksDB to write changes for new block heights to the new column, and to perform lookup of values from the `modification_history_v2` table first, and then from the `modification_history` table, performing a migration upon access if necessary. #### Breaking +- [2438](https://github.com/FuelLabs/fuel-core/pull/2438): Updated `fuel-vm` to `0.59.1` release. Check [release notes](https://github.com/FuelLabs/fuel-vm/releases/tag/v0.59.0) for more details. - [2389](https://github.com/FuelLabs/fuel-core/pull/2258): Updated the `messageProof` GraphQL schema to return a non-nullable `MessageProof`. - [2154](https://github.com/FuelLabs/fuel-core/pull/2154): Transaction graphql endpoints use `TransactionType` instead of `fuel_tx::Transaction`. - [2446](https://github.com/FuelLabs/fuel-core/pull/2446): Use graphiql instead of graphql-playground due to known vulnerability and stale development. diff --git a/benches/benches/vm.rs b/benches/benches/vm.rs index f895290bff8..e629ccbf654 100644 --- a/benches/benches/vm.rs +++ b/benches/benches/vm.rs @@ -41,7 +41,6 @@ where let clock = quanta::Clock::new(); let original_db = vm.as_mut().database_mut().clone(); - let original_memory = vm.memory().clone(); // During block production/validation for each state, which may affect the state of the database, // we create a new storage transaction. The code here simulates the same behavior to have // the same nesting level and the same performance. @@ -53,7 +52,6 @@ where let mut total = core::time::Duration::ZERO; for _ in 0..iters { - vm.memory_mut().clone_from(&original_memory); let start = black_box(clock.raw()); match instruction { Instruction::CALL(call) => { @@ -72,7 +70,6 @@ where vm.as_mut().database_mut().reset_changes(); } *vm.as_mut().database_mut() = original_db; - *vm.memory_mut() = original_memory; total }) }); diff --git a/benches/src/lib.rs b/benches/src/lib.rs index 311db47ec3a..3bed0a964de 100644 --- a/benches/src/lib.rs +++ b/benches/src/lib.rs @@ -398,7 +398,6 @@ impl TryFrom for VmBenchPrepared { let prepare_script = prepare_script .into_iter() .chain(iter::once(op::ret(RegId::ONE))) - .chain(iter::once(instruction)) .collect(); let mut tx = TransactionBuilder::script(prepare_script, data); @@ -531,9 +530,7 @@ impl TryFrom for VmBenchPrepared { } } - let start_vm = vm.clone(); - let original_db = vm.as_mut().database_mut().clone(); - let original_memory = vm.memory().clone(); + let vm_before_first_instruction = vm.clone(); let mut vm = vm.add_recording(); match instruction { Instruction::CALL(call) => { @@ -546,13 +543,11 @@ impl TryFrom for VmBenchPrepared { } let storage_diff = vm.storage_diff(); let mut vm = vm.remove_recording(); - // TODO: Check if this is the intended use of rollback_to. - let mut diff = start_vm.rollback_to(&vm); + let mut diff = vm.rollback_to(&vm_before_first_instruction); diff += storage_diff; let diff: diff::Diff = diff.into(); vm.reset_vm_state(&diff); - *vm.as_mut().database_mut() = original_db; - *vm.memory_mut() = original_memory; + assert_eq!(vm_before_first_instruction, vm); Ok(Self { vm, diff --git a/crates/client/assets/schema.sdl b/crates/client/assets/schema.sdl index 12a40603e18..ca280ebc97e 100644 --- a/crates/client/assets/schema.sdl +++ b/crates/client/assets/schema.sdl @@ -477,7 +477,6 @@ type GasCosts { enum GasCostsVersion { V1 - V2 } type Genesis { diff --git a/crates/client/src/client/schema/chain.rs b/crates/client/src/client/schema/chain.rs index 8c34836e78e..c0efa92a27c 100644 --- a/crates/client/src/client/schema/chain.rs +++ b/crates/client/src/client/schema/chain.rs @@ -342,7 +342,6 @@ pub struct GasCosts { #[cynic(schema_path = "./assets/schema.sdl")] pub enum GasCostsVersion { V1, - V2, #[cynic(fallback)] Unknown, } @@ -353,7 +352,7 @@ impl TryFrom for fuel_core_types::fuel_tx::GasCosts { fn try_from(value: GasCosts) -> Result { match value.version { GasCostsVersion::V1 => Ok(fuel_core_types::fuel_tx::GasCosts::new( - fuel_core_types::fuel_tx::consensus_parameters::gas::GasCostsValuesV4 { + fuel_core_types::fuel_tx::consensus_parameters::gas::GasCostsValuesV5 { add: value.add.into(), addi: value.addi.into(), and: value.and.into(), @@ -439,6 +438,7 @@ impl TryFrom for fuel_core_types::fuel_tx::GasCosts { wqmm: value.wqmm.into(), xor: value.xor.into(), xori: value.xori.into(), + ecop: value.ecop.map(Into::into).unwrap_or(0), aloc: value.aloc_dependent_cost.into(), bsiz: value.bsiz.map(Into::into).unwrap_or(fuel_core_types::fuel_tx::consensus_parameters::DependentCost::free()), @@ -464,6 +464,7 @@ impl TryFrom for fuel_core_types::fuel_tx::GasCosts { smo: value.smo.into(), srwq: value.srwq.into(), swwq: value.swwq.into(), + epar: value.epar.map(Into::into).unwrap_or(fuel_core_types::fuel_tx::consensus_parameters::DependentCost::free()), contract_root: value.contract_root.into(), state_root: value.state_root.into(), vm_initialization: value.vm_initialization.into(), @@ -471,129 +472,6 @@ impl TryFrom for fuel_core_types::fuel_tx::GasCosts { } .into(), )), - GasCostsVersion::V2 => { - Ok(fuel_core_types::fuel_tx::GasCosts::new( - fuel_core_types::fuel_tx::consensus_parameters::gas::GasCostsValuesV5 { - add: value.add.into(), - addi: value.addi.into(), - and: value.and.into(), - andi: value.andi.into(), - bal: value.bal.into(), - bhei: value.bhei.into(), - bhsh: value.bhsh.into(), - burn: value.burn.into(), - cb: value.cb.into(), - cfsi: value.cfsi.into(), - div: value.div.into(), - divi: value.divi.into(), - eck1: value.eck1.into(), - ecr1: value.ecr1.into(), - eq: value.eq.into(), - exp: value.exp.into(), - expi: value.expi.into(), - flag: value.flag.into(), - gm: value.gm.into(), - gt: value.gt.into(), - gtf: value.gtf.into(), - ji: value.ji.into(), - jmp: value.jmp.into(), - jne: value.jne.into(), - jnei: value.jnei.into(), - jnzi: value.jnzi.into(), - jmpf: value.jmpf.into(), - jmpb: value.jmpb.into(), - jnzf: value.jnzf.into(), - jnzb: value.jnzb.into(), - jnef: value.jnef.into(), - jneb: value.jneb.into(), - lb: value.lb.into(), - log: value.log.into(), - lt: value.lt.into(), - lw: value.lw.into(), - mint: value.mint.into(), - mlog: value.mlog.into(), - mod_op: value.mod_op.into(), - modi: value.modi.into(), - move_op: value.move_op.into(), - movi: value.movi.into(), - mroo: value.mroo.into(), - mul: value.mul.into(), - muli: value.muli.into(), - mldv: value.mldv.into(), - noop: value.noop.into(), - not: value.not.into(), - or: value.or.into(), - ori: value.ori.into(), - poph: value.poph.into(), - popl: value.popl.into(), - pshh: value.pshh.into(), - pshl: value.pshl.into(), - ret: value.ret.into(), - rvrt: value.rvrt.into(), - sb: value.sb.into(), - sll: value.sll.into(), - slli: value.slli.into(), - srl: value.srl.into(), - srli: value.srli.into(), - srw: value.srw.into(), - sub: value.sub.into(), - subi: value.subi.into(), - sw: value.sw.into(), - sww: value.sww.into(), - time: value.time.into(), - tr: value.tr.into(), - tro: value.tro.into(), - wdcm: value.wdcm.into(), - wqcm: value.wqcm.into(), - wdop: value.wdop.into(), - wqop: value.wqop.into(), - wdml: value.wdml.into(), - wqml: value.wqml.into(), - wddv: value.wddv.into(), - wqdv: value.wqdv.into(), - wdmd: value.wdmd.into(), - wqmd: value.wqmd.into(), - wdam: value.wdam.into(), - wqam: value.wqam.into(), - wdmm: value.wdmm.into(), - wqmm: value.wqmm.into(), - xor: value.xor.into(), - xori: value.xori.into(), - ecop: value.ecop.map(Into::into).unwrap_or(0), - - aloc: value.aloc_dependent_cost.into(), - bsiz: value.bsiz.map(Into::into).unwrap_or(fuel_core_types::fuel_tx::consensus_parameters::DependentCost::free()), - bldd: value.bldd.map(Into::into).unwrap_or(fuel_core_types::fuel_tx::consensus_parameters::DependentCost::free()), - cfe: value.cfe.into(), - cfei: value.cfei_dependent_cost.into(), - call: value.call.into(), - ccp: value.ccp.into(), - croo: value.croo.into(), - csiz: value.csiz.into(), - ed19: value.ed19_dependent_cost.into(), - k256: value.k256.into(), - ldc: value.ldc.into(), - logd: value.logd.into(), - mcl: value.mcl.into(), - mcli: value.mcli.into(), - mcp: value.mcp.into(), - mcpi: value.mcpi.into(), - meq: value.meq.into(), - retd: value.retd.into(), - s256: value.s256.into(), - scwq: value.scwq.into(), - smo: value.smo.into(), - srwq: value.srwq.into(), - swwq: value.swwq.into(), - epar: value.epar.map(Into::into).unwrap_or(fuel_core_types::fuel_tx::consensus_parameters::DependentCost::free()), - contract_root: value.contract_root.into(), - state_root: value.state_root.into(), - vm_initialization: value.vm_initialization.into(), - new_storage_per_byte: value.new_storage_per_byte.into(), - } - .into(), - )) - } _ => Err(ConversionError::UnknownVariant("GasCostsVersion")), } } diff --git a/crates/fuel-core/src/schema/chain.rs b/crates/fuel-core/src/schema/chain.rs index 708c4231a98..830f281c69f 100644 --- a/crates/fuel-core/src/schema/chain.rs +++ b/crates/fuel-core/src/schema/chain.rs @@ -44,7 +44,6 @@ pub struct GasCosts(fuel_tx::GasCosts); #[derive(Clone, Copy, Debug, Enum, Eq, PartialEq)] pub enum GasCostsVersion { V1, - V2, } #[derive(Clone, Copy, Debug, Enum, Eq, PartialEq)] @@ -281,8 +280,8 @@ impl GasCosts { GasCostsValues::V1(_) | GasCostsValues::V2(_) | GasCostsValues::V3(_) - | GasCostsValues::V4(_) => GasCostsVersion::V1, - GasCostsValues::V5(_) => GasCostsVersion::V2, + | GasCostsValues::V4(_) + | GasCostsValues::V5(_) => GasCostsVersion::V1, } } diff --git a/crates/fuel-core/src/state/rocks_db.rs b/crates/fuel-core/src/state/rocks_db.rs index 8cce555fdf3..3df5c7127e5 100644 --- a/crates/fuel-core/src/state/rocks_db.rs +++ b/crates/fuel-core/src/state/rocks_db.rs @@ -760,7 +760,7 @@ where key: &[u8], column: Self::Column, offset: usize, - mut buf: &mut [u8], + buf: &mut [u8], ) -> StorageResult> { self.metrics.read_meter.inc(); let column_metrics = self.metrics.columns_read_statistic.get(&column.id()); @@ -771,15 +771,21 @@ where .get_pinned_cf_opt(&self.cf(column), key, &self.read_options) .map_err(|e| DatabaseError::Other(e.into()))? .map(|value| { - if let Some(read) = value.len().checked_sub(offset) { - std::io::Write::write_all(&mut buf, value[offset..].as_ref()) - .map_err(|e| DatabaseError::Other(anyhow::anyhow!(e)))?; - StorageResult::Ok(read) - } else { - Err(StorageError::Other(anyhow::anyhow!( - "Offset is out of bounds" - ))) + let bytes_len = value.len(); + let start = offset; + let end = offset.saturating_add(buf.len()); + + if end > bytes_len { + return Err(StorageError::Other(anyhow::anyhow!( + "Offset `{offset}` is out of bounds `{bytes_len}` \ + for key `{:?}` and column `{column:?}`", + key + ))); } + + let starting_from_offset = &value[start..end]; + buf[..].copy_from_slice(starting_from_offset); + Ok(buf.len()) }) .transpose()?; diff --git a/crates/services/txpool_v2/src/tests/mocks.rs b/crates/services/txpool_v2/src/tests/mocks.rs index cbf0ba1bad5..eed1bf5f84f 100644 --- a/crates/services/txpool_v2/src/tests/mocks.rs +++ b/crates/services/txpool_v2/src/tests/mocks.rs @@ -134,13 +134,17 @@ impl StorageRead for MockDb { bytes .map(|bytes| { - if let Some(len) = bytes.0.len().checked_sub(offset) { - // SAFETY: offset is guaranteed to be less or equal to value.len() by the check above - buf.copy_from_slice(&bytes.0[offset..]); - Ok(len) - } else { - Err(()) + let bytes_len = bytes.as_ref().len(); + let start = offset; + let end = offset.saturating_add(buf.len()); + + if end > bytes_len { + return Err(()); } + + let starting_from_offset = &bytes.as_ref()[start..end]; + buf[..].copy_from_slice(starting_from_offset); + Ok(buf.len()) }) .transpose() } diff --git a/crates/storage/src/kv_store.rs b/crates/storage/src/kv_store.rs index acea55a31c9..a7029db8d20 100644 --- a/crates/storage/src/kv_store.rs +++ b/crates/storage/src/kv_store.rs @@ -1,9 +1,6 @@ //! The module provides plain abstract definition of the key-value store. -use crate::{ - Error as StorageError, - Result as StorageResult, -}; +use crate::Result as StorageResult; #[cfg(feature = "alloc")] use alloc::{ @@ -72,20 +69,21 @@ pub trait KeyValueInspect { ) -> StorageResult> { self.get(key, column)? .map(|value| { - if let Some(read) = value.len().checked_sub(offset) { - if read != buf.len() { - return Err(StorageError::Other(anyhow::anyhow!( - "Buffer size is not equal to the value size" - ))); - }; - // SAFETY: offset is guaranteed to be less or equal to value.len() by the if let check above - buf.copy_from_slice(value[offset..].as_ref()); - Ok(buf.len()) - } else { - Err(StorageError::Other(anyhow::anyhow!( - "Offset is out of bounds" - ))) + let bytes_len = value.as_ref().len(); + let start = offset; + let end = offset.saturating_add(buf.len()); + + if end > bytes_len { + return Err(anyhow::anyhow!( + "Offset `{offset}` is out of bounds `{bytes_len}` for key `{:?}`", + key + ) + .into()); } + + let starting_from_offset = &value.as_ref()[start..end]; + buf[..].copy_from_slice(starting_from_offset); + Ok(buf.len()) }) .transpose() } diff --git a/tests/tests/poa.rs b/tests/tests/poa.rs index d043d9b4ef9..b360d0d89fa 100644 --- a/tests/tests/poa.rs +++ b/tests/tests/poa.rs @@ -183,7 +183,7 @@ mod p2p { // after the first_producer stops, second_producer should start producing blocks #[tokio::test(flavor = "multi_thread")] async fn test_poa_multiple_producers() { - const SYNC_TIMEOUT: u64 = 30; + const SYNC_TIMEOUT: u64 = 5; const TIME_UNTIL_SYNCED: u64 = SYNC_TIMEOUT + 10; let mut rng = StdRng::seed_from_u64(2222); @@ -201,7 +201,9 @@ mod p2p { let make_node_config = |name: &str| { let mut config = make_config(name.to_string(), config.clone()); config.debug = true; - config.block_production = Trigger::Never; + config.block_production = Trigger::Interval { + block_time: Duration::from_secs(1), + }; config.consensus_signer = SignMode::Key(Secret::new(secret.into())); config.p2p.as_mut().unwrap().bootstrap_nodes = bootstrap.listeners(); config.p2p.as_mut().unwrap().reserved_nodes = bootstrap.listeners(); @@ -216,7 +218,7 @@ mod p2p { let first_producer = make_node(first_producer_config, vec![]).await; - // The first producer should produce 3 blocks. + // The first producer should produce 1 block manually after `SYNC_TIMEOUT` seconds. first_producer .node .shared @@ -224,14 +226,15 @@ mod p2p { .manually_produce_blocks( None, Mode::Blocks { - number_of_blocks: 3, + number_of_blocks: 1, }, ) .await - .expect("The first should produce 3 blocks"); + .expect("The first should produce 1 block manually"); - // Start the second producer after 3 blocks. - // The second producer should synchronize 3 blocks produced by the first producer. + // After 1 manual block start the second producer. + // The first producer should produce 2 more blocks. + // The second producer should synchronize 3(1 manual and 2 produced) blocks. let second_producer = make_node(second_producer_config, vec![]).await; tokio::time::timeout( Duration::from_secs(SYNC_TIMEOUT), @@ -258,11 +261,11 @@ mod p2p { .manually_produce_blocks( None, Mode::Blocks { - number_of_blocks: 2, + number_of_blocks: 1, }, ) .await - .expect("The second should produce 2 blocks"); + .expect("The second should produce 1 blocks"); assert!(start_time.elapsed() >= Duration::from_secs(TIME_UNTIL_SYNCED)); // Restart fresh first producer. @@ -270,7 +273,7 @@ mod p2p { let first_producer = make_node(make_node_config("First Producer reborn"), vec![]).await; tokio::time::timeout( - Duration::from_secs(SYNC_TIMEOUT), + Duration::from_secs(TIME_UNTIL_SYNCED), first_producer.wait_for_blocks(5, false /* is_local */), ) .await