From 4207d947e00b37f102b259baa61cd49e7695158f Mon Sep 17 00:00:00 2001 From: Aaryamann Challani <43716372+rymnc@users.noreply.github.com> Date: Wed, 22 Jan 2025 15:46:04 +0530 Subject: [PATCH] fix(da_compression): invalid decompression of utxo id and CoinConfig fix (#2593) ## Linked Issues/PRs some investigation related to why transaction ids were mismatching before compression and after decompression ## Description - Fixed CoinConfig generate function to create appropriate tx_id and output_index - Fixed utxo id lookup - da compression integ test asserts tx ids are the same AI generated: ### Code Organization: * [`crates/chain-config/src/config/coin.rs`](diffhunk://#diff-942b8fa635eb049417e89ef88024e08422c209c2952e0f5612e45531ee03a8b1R124-R171): Moved `CoinConfigGenerator` to a new `coin_config_helpers` module and updated its implementation to use a `CoinCount` type. * Updated references to `CoinConfigGenerator` across multiple files to use the new `coin_config_helpers` module. [[1]](diffhunk://#diff-942b8fa635eb049417e89ef88024e08422c209c2952e0f5612e45531ee03a8b1L170-R182) [[2]](diffhunk://#diff-942b8fa635eb049417e89ef88024e08422c209c2952e0f5612e45531ee03a8b1L183-R195) [[3]](diffhunk://#diff-beed0a09969286877b6b2569d2e3775a9526d5328bfebe9ef04da44e16dc8492L49-R49) [[4]](diffhunk://#diff-aceecd6cf4d806ddbb7fe3edc5d757cc292c1c8b81f9df2601cdc2103cca16f2R5-L6) [[5]](diffhunk://#diff-e83ddeaff43cbea45b7b2a6c5b688054cf32fbcd7c5010fc5059d0812b1f413eR3-L4) [[6]](diffhunk://#diff-12758c6f607876b04a2fd230ac5b6c16c275c65e82390f867404ddb62345119aR27-L28) [[7]](diffhunk://#diff-12758c6f607876b04a2fd230ac5b6c16c275c65e82390f867404ddb62345119aL527-R527) ### Decompression Logic: * [`crates/compression/src/decompress.rs`](diffhunk://#diff-d0b271dacdbc77c5808aab48fee0fa85f338d593e1adc25e8bd0205c370dd824L71-R93): Modified the decompression logic to handle mint transactions correctly and ensure the TxPointer is set appropriately. [[1]](diffhunk://#diff-d0b271dacdbc77c5808aab48fee0fa85f338d593e1adc25e8bd0205c370dd824L71-R93) [[2]](diffhunk://#diff-d0b271dacdbc77c5808aab48fee0fa85f338d593e1adc25e8bd0205c370dd824R231-R232) ### Testing and Configuration: * [`tests/tests/da_compression.rs`](diffhunk://#diff-b7fc0c13495dc1a6096e1822748a1d512e84510d50d2ac1d56b1f9a839646342R66-R92): Enhanced the test for fetching DA compressed blocks from GraphQL by adding additional checks and ensuring the transactions' IDs match. [[1]](diffhunk://#diff-b7fc0c13495dc1a6096e1822748a1d512e84510d50d2ac1d56b1f9a839646342R66-R92) [[2]](diffhunk://#diff-b7fc0c13495dc1a6096e1822748a1d512e84510d50d2ac1d56b1f9a839646342R129-R157) ### Miscellaneous: * Various files: Updated imports and removed unnecessary `where` clauses to clean up the codebase. [[1]](diffhunk://#diff-942b8fa635eb049417e89ef88024e08422c209c2952e0f5612e45531ee03a8b1L23) [[2]](diffhunk://#diff-d0b271dacdbc77c5808aab48fee0fa85f338d593e1adc25e8bd0205c370dd824R21) [[3]](diffhunk://#diff-d0b271dacdbc77c5808aab48fee0fa85f338d593e1adc25e8bd0205c370dd824R39) [[4]](diffhunk://#diff-715d143d62233118cd663c107e340e6d32aeb6446c02c167893656316ce13cbeL276-R276) [[5]](diffhunk://#diff-715d143d62233118cd663c107e340e6d32aeb6446c02c167893656316ce13cbeL304) [[6]](diffhunk://#diff-715d143d62233118cd663c107e340e6d32aeb6446c02c167893656316ce13cbeL317-R321) ## Checklist - [ ] Breaking changes are clearly marked as such in the PR description and changelog - [ ] New behavior is reflected in tests - [ ] [The specification](https://github.com/FuelLabs/fuel-specs/) matches the implemented behavior (link update PR if changes are needed) ### Before requesting review - [ ] I have reviewed the code myself - [ ] I have created follow-up issues caused by this PR and linked them here ### After merging, notify other teams [Add or remove entries as needed] - [ ] [Rust SDK](https://github.com/FuelLabs/fuels-rs/) - [ ] [Sway compiler](https://github.com/FuelLabs/sway/) - [ ] [Platform documentation](https://github.com/FuelLabs/devrel-requests/issues/new?assignees=&labels=new+request&projects=&template=NEW-REQUEST.yml&title=%5BRequest%5D%3A+) (for out-of-organization contributors, the person merging the PR will do this) - [ ] Someone else? --- CHANGELOG.md | 1 + crates/chain-config/src/config/coin.rs | 96 +++++++++++-------- crates/chain-config/src/config/state.rs | 2 +- crates/compression/src/decompress.rs | 23 ++++- .../src/graphql_api/da_compression.rs | 21 ++-- crates/fuel-core/src/p2p_test_helpers.rs | 4 +- tests/tests/balances.rs | 4 +- tests/tests/coin.rs | 2 +- tests/tests/coins.rs | 4 +- tests/tests/da_compression.rs | 38 +++++++- 10 files changed, 129 insertions(+), 66 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ead95e97d77..fbbebc2c2c8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/). ### Fixed - [2609](https://github.com/FuelLabs/fuel-core/pull/2609): Check response before trying to deserialize, return error instead - [2599](https://github.com/FuelLabs/fuel-core/pull/2599): Use the proper `url` apis to construct full url path in `BlockCommitterHttpApi` client +- [2593](https://github.com/FuelLabs/fuel-core/pull/2593): Fixed utxo id decompression ## [Version 0.41.0] diff --git a/crates/chain-config/src/config/coin.rs b/crates/chain-config/src/config/coin.rs index d24297a54f2..d38717e7d53 100644 --- a/crates/chain-config/src/config/coin.rs +++ b/crates/chain-config/src/config/coin.rs @@ -20,7 +20,6 @@ use fuel_core_types::{ BlockHeight, Bytes32, }, - fuel_vm::SecretKey, }; use serde::{ Deserialize, @@ -75,41 +74,6 @@ impl From for TableEntry { } } -/// Generates a new coin config with a unique utxo id for testing -#[derive(Default, Debug)] -pub struct CoinConfigGenerator { - count: usize, -} - -impl CoinConfigGenerator { - pub fn new() -> Self { - Self { count: 0 } - } - - pub fn generate(&mut self) -> CoinConfig { - let mut bytes = [0u8; 32]; - bytes[..std::mem::size_of::()].copy_from_slice(&self.count.to_be_bytes()); - - let config = CoinConfig { - tx_id: Bytes32::from(bytes), - ..Default::default() - }; - self.count = self.count.checked_add(1).expect("Max coin count reached"); - - config - } - - pub fn generate_with(&mut self, secret: SecretKey, amount: u64) -> CoinConfig { - let owner = Address::from(*secret.public_key().hash()); - - CoinConfig { - amount, - owner, - ..self.generate() - } - } -} - impl CoinConfig { pub fn utxo_id(&self) -> UtxoId { UtxoId::new(self.tx_id, self.output_index) @@ -157,6 +121,62 @@ impl GenesisCommitment for Coin { } } +#[cfg(feature = "test-helpers")] +pub mod coin_config_helpers { + use crate::CoinConfig; + use fuel_core_types::{ + fuel_types::{ + Address, + Bytes32, + }, + fuel_vm::SecretKey, + }; + + type CoinCount = u16; + + /// Generates a new coin config with a unique utxo id for testing + #[derive(Default, Debug)] + pub struct CoinConfigGenerator { + count: CoinCount, + } + + pub fn tx_id(count: CoinCount) -> Bytes32 { + let mut bytes = [0u8; 32]; + bytes[..size_of::()].copy_from_slice(&count.to_be_bytes()); + bytes.into() + } + + impl CoinConfigGenerator { + pub fn new() -> Self { + Self { count: 0 } + } + + pub fn generate(&mut self) -> CoinConfig { + let tx_id = tx_id(self.count); + + let config = CoinConfig { + tx_id, + output_index: self.count, + ..Default::default() + }; + + self.count = self.count.checked_add(1).expect("Max coin count reached"); + + config + } + + pub fn generate_with(&mut self, secret: SecretKey, amount: u64) -> CoinConfig { + let owner = Address::from(*secret.public_key().hash()); + + CoinConfig { + amount, + owner, + ..self.generate() + } + } + } +} + #[cfg(test)] mod tests { use super::*; @@ -167,7 +187,7 @@ mod tests { #[test] fn test_generate_unique_utxo_id() { - let mut generator = CoinConfigGenerator::new(); + let mut generator = coin_config_helpers::CoinConfigGenerator::new(); let config1 = generator.generate(); let config2 = generator.generate(); @@ -180,7 +200,7 @@ mod tests { let secret = SecretKey::random(&mut rng); let amount = 1000; - let mut generator = CoinConfigGenerator::new(); + let mut generator = coin_config_helpers::CoinConfigGenerator::new(); let config = generator.generate_with(secret, amount); assert_eq!(config.owner, Address::from(*secret.public_key().hash())); diff --git a/crates/chain-config/src/config/state.rs b/crates/chain-config/src/config/state.rs index c8fee0468fd..06d4f605283 100644 --- a/crates/chain-config/src/config/state.rs +++ b/crates/chain-config/src/config/state.rs @@ -46,7 +46,7 @@ use serde::{ use crate::SnapshotMetadata; #[cfg(feature = "test-helpers")] -use crate::CoinConfigGenerator; +use crate::coin_config_helpers::CoinConfigGenerator; #[cfg(feature = "test-helpers")] use bech32::{ ToBase32, diff --git a/crates/compression/src/decompress.rs b/crates/compression/src/decompress.rs index 082124e9161..bb4f66d651a 100644 --- a/crates/compression/src/decompress.rs +++ b/crates/compression/src/decompress.rs @@ -18,6 +18,7 @@ use fuel_core_types::{ RegistryKey, }, fuel_tx::{ + field::TxPointer, input::{ coin::{ Coin, @@ -35,6 +36,7 @@ use fuel_core_types::{ Mint, ScriptCode, Transaction, + TxPointer as FuelTxPointer, UtxoId, }, fuel_types::{ @@ -68,12 +70,29 @@ where db, }; - let transactions = as DecompressibleBy<_>>::decompress_with( + let mut transactions = as DecompressibleBy<_>>::decompress_with( block.transactions(), &ctx, ) .await?; + let transaction_count = transactions.len(); + + // patch mint transaction + let mint_tx = transactions + .last_mut() + .ok_or_else(|| anyhow::anyhow!("No transactions"))?; + if let Transaction::Mint(mint) = mint_tx { + let tx_pointer = mint.tx_pointer_mut(); + *tx_pointer = FuelTxPointer::new( + block.consensus_header().height, + #[allow(clippy::arithmetic_side_effects)] + u16::try_from(transaction_count - 1)?, + ); + } else { + anyhow::bail!("Last transaction is not a mint"); + } + Ok(PartialFuelBlock { header: block.partial_block_header(), transactions, @@ -211,7 +230,7 @@ where ctx: &DecompressCtx, ) -> anyhow::Result { Ok(Transaction::mint( - Default::default(), // TODO: what should this we do with this? + Default::default(), // TODO: what should we do with this? c.input_contract.decompress(ctx).await?, c.output_contract.decompress(ctx).await?, c.mint_amount.decompress(ctx).await?, diff --git a/crates/fuel-core/src/graphql_api/da_compression.rs b/crates/fuel-core/src/graphql_api/da_compression.rs index 722f63b5080..bde6497859e 100644 --- a/crates/fuel-core/src/graphql_api/da_compression.rs +++ b/crates/fuel-core/src/graphql_api/da_compression.rs @@ -273,10 +273,7 @@ impl_temporal_registry!(ContractId); impl_temporal_registry!(ScriptCode); impl_temporal_registry!(PredicateCode); -impl<'a, Tx> UtxoIdToPointer for CompressDbTx<'a, Tx> -where - Tx: OffChainDatabaseTransaction, -{ +impl<'a, Tx> UtxoIdToPointer for CompressDbTx<'a, Tx> { fn lookup( &self, utxo_id: fuel_core_types::fuel_tx::UtxoId, @@ -301,7 +298,6 @@ where impl<'a, Tx, Onchain> HistoryLookup for DecompressDbTx<'a, Tx, Onchain> where - Tx: OffChainDatabaseTransaction, Onchain: StorageInspect + StorageInspect + StorageInspect, @@ -310,17 +306,16 @@ where &self, c: fuel_core_types::fuel_tx::CompressedUtxoId, ) -> anyhow::Result { + #[cfg(feature = "test-helpers")] if c.tx_pointer.block_height() == 0u32.into() { // This is a genesis coin, which is handled differently. // See CoinConfigGenerator::generate which generates the genesis coins. - let mut bytes = [0u8; 32]; - let tx_index = c.tx_pointer.tx_index(); - bytes[..std::mem::size_of_val(&tx_index)] - .copy_from_slice(&tx_index.to_be_bytes()); - return Ok(fuel_core_types::fuel_tx::UtxoId::new( - fuel_core_types::fuel_tx::TxId::from(bytes), - 0, - )); + let tx_id = + fuel_core_chain_config::coin_config_helpers::tx_id(c.output_index); + + let utxo_id = fuel_core_types::fuel_tx::UtxoId::new(tx_id, c.output_index); + + return Ok(utxo_id); } let block_info = self diff --git a/crates/fuel-core/src/p2p_test_helpers.rs b/crates/fuel-core/src/p2p_test_helpers.rs index 684e6b06a54..eb392dcf260 100644 --- a/crates/fuel-core/src/p2p_test_helpers.rs +++ b/crates/fuel-core/src/p2p_test_helpers.rs @@ -2,8 +2,8 @@ use crate::{ chain_config::{ + coin_config_helpers::CoinConfigGenerator, CoinConfig, - CoinConfigGenerator, }, combined_database::CombinedDatabase, database::{ @@ -246,7 +246,7 @@ pub async fn make_nodes( let initial_coin = CoinConfig { // set idx to prevent overlapping utxo_ids when // merging with existing coins from config - output_index: 2, + output_index: 10, ..coin_generator.generate_with(secret, 10000) }; let tx = TransactionBuilder::script( diff --git a/tests/tests/balances.rs b/tests/tests/balances.rs index 7c264fcffce..8b486be1f6d 100644 --- a/tests/tests/balances.rs +++ b/tests/tests/balances.rs @@ -1,7 +1,7 @@ use fuel_core::{ chain_config::{ + coin_config_helpers::CoinConfigGenerator, CoinConfig, - CoinConfigGenerator, MessageConfig, StateConfig, }, @@ -334,9 +334,9 @@ async fn first_5_balances() { mod pagination { use fuel_core::{ chain_config::{ + coin_config_helpers::CoinConfigGenerator, ChainConfig, CoinConfig, - CoinConfigGenerator, MessageConfig, StateConfig, }, diff --git a/tests/tests/coin.rs b/tests/tests/coin.rs index 024e7211cd8..9057ca8355c 100644 --- a/tests/tests/coin.rs +++ b/tests/tests/coin.rs @@ -1,7 +1,7 @@ use fuel_core::{ chain_config::{ + coin_config_helpers::CoinConfigGenerator, CoinConfig, - CoinConfigGenerator, StateConfig, }, database::Database, diff --git a/tests/tests/coins.rs b/tests/tests/coins.rs index bfab743814b..0e90d22af78 100644 --- a/tests/tests/coins.rs +++ b/tests/tests/coins.rs @@ -24,8 +24,8 @@ use rand::{ mod coin { use super::*; use fuel_core::chain_config::{ + coin_config_helpers::CoinConfigGenerator, ChainConfig, - CoinConfigGenerator, }; use fuel_core_client::client::types::CoinType; use fuel_core_types::fuel_crypto::SecretKey; @@ -524,7 +524,7 @@ mod message_coin { // It is combination of coins and deposit coins test cases. mod all_coins { - use fuel_core::chain_config::CoinConfigGenerator; + use fuel_core::chain_config::coin_config_helpers::CoinConfigGenerator; use fuel_core_client::client::types::CoinType; use fuel_core_types::blockchain::primitives::DaBlockHeight; diff --git a/tests/tests/da_compression.rs b/tests/tests/da_compression.rs index cd0d1430375..409d064a25b 100644 --- a/tests/tests/da_compression.rs +++ b/tests/tests/da_compression.rs @@ -24,6 +24,7 @@ use fuel_core_compression::{ VersionedCompressedBlock, }; use fuel_core_storage::transactional::{ + AtomicView, HistoricalView, IntoTransaction, }; @@ -39,6 +40,7 @@ use fuel_core_types::{ Input, TransactionBuilder, TxPointer, + UniqueIdentifier, }, secrecy::Secret, signer::SignMode, @@ -61,6 +63,11 @@ async fn can_fetch_da_compressed_block_from_graphql() { temporal_registry_retention: Duration::from_secs(3600), }; config.da_compression = DaCompressionConfig::Enabled(compression_config); + let chain_id = config + .snapshot_reader + .chain_config() + .consensus_parameters + .chain_id(); let srv = FuelService::new_node(config).await.unwrap(); let client = FuelClient::from(srv.bound_address); @@ -68,19 +75,21 @@ async fn can_fetch_da_compressed_block_from_graphql() { SecretKey::from_str(TESTNET_WALLET_SECRETS[1]).expect("Expected valid secret"); let wallet_address = Address::from(*wallet_secret.public_key().hash()); - let coin = client + let coins = client .coins( &wallet_address, None, PaginationRequest { cursor: None, - results: 1, + results: 10, direction: fuel_core_client::client::pagination::PageDirection::Forward, }, ) .await .expect("Unable to get coins") - .results + .results; + + let coin = coins .into_iter() .next() .expect("Expected at least one coin"); @@ -117,16 +126,35 @@ async fn can_fetch_da_compressed_block_from_graphql() { // Reuse the existing offchain db to decompress the block let db = &srv.shared.database; + + let on_chain_before_execution = db.on_chain().view_at(&0u32.into()).unwrap(); let mut tx_inner = db.off_chain().clone().into_transaction(); let db_tx = DecompressDbTx { db_tx: DbTx { db_tx: &mut tx_inner, }, - onchain_db: db.on_chain().view_at(&0u32.into()).unwrap(), + onchain_db: on_chain_before_execution, }; let decompressed = decompress(compression_config, db_tx, block).await.unwrap(); - assert!(decompressed.transactions.len() == 2); + let block_from_on_chain_db = db + .on_chain() + .latest_view() + .unwrap() + .get_full_block(&block_height) + .unwrap() + .unwrap(); + + let db_transactions = block_from_on_chain_db.transactions(); + let decompressed_transactions = decompressed.transactions; + + assert_eq!(decompressed_transactions.len(), 2); + for (db_tx, decompressed_tx) in + db_transactions.iter().zip(decompressed_transactions.iter()) + { + // ensure tx ids match + assert_eq!(db_tx.id(&chain_id), decompressed_tx.id(&chain_id)); + } } #[tokio::test(flavor = "multi_thread")]