From 856d5da980efce99a1b63cdd8e3f08bb95e42d60 Mon Sep 17 00:00:00 2001 From: AurelienFT <32803821+AurelienFT@users.noreply.github.com> Date: Thu, 12 Dec 2024 21:31:39 +0100 Subject: [PATCH] Ignore receipts from failed transactions in `message_receipts_proof` (#2478) Resolves #2421 In this PR we changed the `message_receipts_proof` to ignore the receipts from the failed transactions. ## Checklist - [x] Breaking changes are clearly marked as such in the PR description and changelog - [x] New behavior is reflected in tests - [x] [The specification](https://github.com/FuelLabs/fuel-specs/) matches the implemented behavior (link update PR if changes are needed) ### Before requesting review - [x] I have reviewed the code myself - [x] I have created follow-up issues caused by this PR and linked them here --- CHANGELOG.md | 1 + crates/fuel-core/src/query/message.rs | 235 ++++++++++++++++--- crates/fuel-core/src/query/message/test.rs | 40 ++-- crates/types/src/entities/relayer/message.rs | 1 + 4 files changed, 219 insertions(+), 58 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 071fd4d68a4..b37c9a7b3b5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -36,6 +36,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/). - [2369](https://github.com/FuelLabs/fuel-core/pull/2369): The `transaction_insertion_time_in_thread_pool_milliseconds` metric is properly collected. - [2413](https://github.com/FuelLabs/fuel-core/issues/2413): block production immediately errors if unable to lock the mutex. - [2389](https://github.com/FuelLabs/fuel-core/pull/2389): Fix construction of reverse iterator in RocksDB. +- [2478](https://github.com/FuelLabs/fuel-core/pull/2478): Fix proof created by `message_receipts_proof` function by ignoring the receipts from failed transactions to match `message_outbox_root`. - [2485](https://github.com/FuelLabs/fuel-core/pull/2485): Hardcode the timestamp of the genesis block and version of `tai64` to avoid breaking changes for us. ### Changed diff --git a/crates/fuel-core/src/query/message.rs b/crates/fuel-core/src/query/message.rs index 07d2cd9ab56..03b92d4c652 100644 --- a/crates/fuel-core/src/query/message.rs +++ b/crates/fuel-core/src/query/message.rs @@ -115,9 +115,6 @@ pub trait MessageProofData { /// Get the block. fn block(&self, id: &BlockHeight) -> StorageResult; - /// Return all receipts in the given transaction. - fn receipts(&self, transaction_id: &TxId) -> StorageResult>; - /// Get the status of a transaction. fn transaction_status( &self, @@ -138,10 +135,6 @@ impl MessageProofData for ReadView { self.block(id) } - fn receipts(&self, transaction_id: &TxId) -> StorageResult> { - self.receipts(transaction_id) - } - fn transaction_status( &self, transaction_id: &TxId, @@ -165,34 +158,28 @@ pub fn message_proof( desired_nonce: Nonce, commit_block_height: BlockHeight, ) -> StorageResult { - // Check if the receipts for this transaction actually contain this nonce or exit. - let (sender, recipient, nonce, amount, data) = database - .receipts(&transaction_id)? - .into_iter() - .find_map(|r| match r { - Receipt::MessageOut { - sender, - recipient, - nonce, - amount, - data, - .. - } if r.nonce() == Some(&desired_nonce) => { - Some((sender, recipient, nonce, amount, data)) - } - _ => None, - }) - .ok_or::( - anyhow::anyhow!("Desired `nonce` missing in transaction receipts").into(), - )?; - - let Some(data) = data else { - return Err(anyhow::anyhow!("Output message doesn't contain any `data`").into()) - }; - // Get the block id from the transaction status if it's ready. - let message_block_height = match database.transaction_status(&transaction_id) { - Ok(TransactionStatus::Success { block_height, .. }) => block_height, + let (message_block_height, (sender, recipient, nonce, amount, data)) = match database.transaction_status(&transaction_id) { + Ok(TransactionStatus::Success { block_height, receipts, .. }) => ( + block_height, + receipts.into_iter() + .find_map(|r| match r { + Receipt::MessageOut { + sender, + recipient, + nonce, + amount, + data, + .. + } if r.nonce() == Some(&desired_nonce) => { + Some((sender, recipient, nonce, amount, data)) + } + _ => None, + }) + .ok_or::( + anyhow::anyhow!("Desired `nonce` missing in transaction receipts").into(), + )? + ), Ok(TransactionStatus::Submitted { .. }) => { return Err(anyhow::anyhow!( "Unable to obtain the message block height. The transaction has not been processed yet" @@ -219,6 +206,10 @@ pub fn message_proof( } }; + let Some(data) = data else { + return Err(anyhow::anyhow!("Output message doesn't contain any `data`").into()) + }; + // Get the message fuel block header. let (message_block_header, message_block_txs) = match database.block(&message_block_height) { @@ -278,7 +269,11 @@ fn message_receipts_proof( // Get the message receipts from the block. let leaves: Vec> = message_block_txs .iter() - .map(|id| database.receipts(id)) + .filter_map(|id| match database.transaction_status(id) { + Ok(TransactionStatus::Success { receipts, .. }) => Some(Ok(receipts)), + Ok(_) => None, + Err(err) => Some(Err(err)), + }) .try_collect()?; let leaves = leaves.into_iter() // Flatten the receipts after filtering on output messages @@ -337,3 +332,173 @@ pub fn message_status( Ok(MessageStatus::not_found()) } } + +#[cfg(test)] +mod tests { + use std::collections::HashMap; + + use fuel_core_storage::not_found; + use fuel_core_types::{ + blockchain::block::CompressedBlock, + entities::relayer::message::MerkleProof, + fuel_tx::{ + Address, + Bytes32, + Receipt, + TxId, + }, + fuel_types::{ + BlockHeight, + Nonce, + }, + services::txpool::TransactionStatus, + tai64::Tai64, + }; + + use super::{ + message_proof, + MessageProofData, + }; + + pub struct FakeDB { + pub blocks: HashMap, + pub transaction_statuses: HashMap, + pub receipts: HashMap>, + } + + impl FakeDB { + fn new() -> Self { + Self { + blocks: HashMap::new(), + transaction_statuses: HashMap::new(), + receipts: HashMap::new(), + } + } + + fn insert_block(&mut self, block_height: BlockHeight, block: CompressedBlock) { + self.blocks.insert(block_height, block); + } + + fn insert_transaction_status( + &mut self, + transaction_id: TxId, + status: TransactionStatus, + ) { + self.transaction_statuses.insert(transaction_id, status); + } + + fn insert_receipts(&mut self, transaction_id: TxId, receipts: Vec) { + self.receipts.insert(transaction_id, receipts); + } + } + + impl MessageProofData for FakeDB { + fn block(&self, id: &BlockHeight) -> fuel_core_storage::Result { + self.blocks.get(id).cloned().ok_or(not_found!("Block")) + } + + fn transaction_status( + &self, + transaction_id: &TxId, + ) -> fuel_core_storage::Result { + self.transaction_statuses + .get(transaction_id) + .cloned() + .ok_or(not_found!("Transaction status")) + } + + fn block_history_proof( + &self, + _message_block_height: &BlockHeight, + _commit_block_height: &BlockHeight, + ) -> fuel_core_storage::Result { + // Unused in current tests + Ok(MerkleProof::default()) + } + } + + // Test will try to get the message receipt proof with a block with only valid transactions + // Then add an invalid transaction and check if the proof is still the same (meaning the invalid transaction was ignored) + #[test] + fn test_message_proof_ignore_failed() { + // Create a fake database + let mut database = FakeDB::new(); + + // Given + // Create a block with a valid transaction and receipts + let mut block = CompressedBlock::default(); + let block_height: BlockHeight = BlockHeight::new(1); + block.header_mut().set_block_height(block_height); + let valid_tx_id = Bytes32::new([1; 32]); + let mut valid_tx_receipts = vec![]; + for i in 0..100 { + valid_tx_receipts.push(Receipt::MessageOut { + sender: Address::default(), + recipient: Address::default(), + amount: 0, + nonce: 0.into(), + len: 32, + digest: Bytes32::default(), + data: Some(vec![i; 32]), + }); + } + block.transactions_mut().push(valid_tx_id); + database.insert_block(block_height, block.clone()); + database.insert_transaction_status( + valid_tx_id, + TransactionStatus::Success { + time: Tai64::UNIX_EPOCH, + block_height, + receipts: valid_tx_receipts.clone(), + total_fee: 0, + total_gas: 0, + result: None, + }, + ); + database.insert_receipts(valid_tx_id, valid_tx_receipts.clone()); + + // Get the message proof with the valid transaction + let message_proof_valid_tx = + message_proof(&database, valid_tx_id, Nonce::default(), block_height) + .unwrap(); + + // Add an invalid transaction with receipts to the block + let invalid_tx_id = Bytes32::new([2; 32]); + block.transactions_mut().push(invalid_tx_id); + database.insert_block(block_height, block.clone()); + let mut invalid_tx_receipts = vec![]; + for i in 0..100 { + invalid_tx_receipts.push(Receipt::MessageOut { + sender: Address::default(), + recipient: Address::default(), + amount: 0, + nonce: 0.into(), + len: 33, + digest: Bytes32::default(), + data: Some(vec![i; 33]), + }); + } + database.insert_transaction_status( + invalid_tx_id, + TransactionStatus::Failed { + time: Tai64::UNIX_EPOCH, + block_height, + result: None, + total_fee: 0, + total_gas: 0, + receipts: invalid_tx_receipts.clone(), + }, + ); + database.insert_receipts(invalid_tx_id, invalid_tx_receipts.clone()); + + // When + // Get the message proof with the same message id + let message_proof_invalid_tx = + message_proof(&database, valid_tx_id, Nonce::default(), block_height) + .unwrap(); + + // Then + // The proof should be the same because the invalid transaction was ignored + assert_eq!(message_proof_valid_tx, message_proof_invalid_tx); + } +} diff --git a/crates/fuel-core/src/query/message/test.rs b/crates/fuel-core/src/query/message/test.rs index 447e1c01a78..7c9b217aa44 100644 --- a/crates/fuel-core/src/query/message/test.rs +++ b/crates/fuel-core/src/query/message/test.rs @@ -65,7 +65,6 @@ mockall::mock! { message_block_height: &BlockHeight, commit_block_height: &BlockHeight, ) -> StorageResult; - fn receipts(&self, transaction_id: &TxId) -> StorageResult>; fn transaction_status(&self, transaction_id: &TxId) -> StorageResult; } } @@ -97,16 +96,6 @@ async fn can_build_message_proof() { let mut data = MockProofDataStorage::new(); let mut count = 0; - data.expect_receipts().returning(move |txn_id| { - if *txn_id == transaction_id { - Ok(receipts.to_vec()) - } else { - let r = other_receipts[count..=count].to_vec(); - count += 1; - Ok(r) - } - }); - let commit_block_header = PartialBlockHeader { application: ApplicationHeader { da_height: 0u64.into(), @@ -158,18 +147,23 @@ async fn can_build_message_proof() { }); let message_block_height = *message_block.header().height(); - data.expect_transaction_status() - .with(eq(transaction_id)) - .returning(move |_| { - Ok(TransactionStatus::Success { - block_height: message_block_height, - time: Tai64::UNIX_EPOCH, - result: None, - receipts: vec![], - total_gas: 0, - total_fee: 0, - }) - }); + data.expect_transaction_status().returning(move |tx_id| { + let receipts = if *tx_id == transaction_id { + receipts.to_vec() + } else { + let r = other_receipts[count..=count].to_vec(); + count += 1; + r + }; + Ok(TransactionStatus::Success { + block_height: message_block_height, + time: Tai64::UNIX_EPOCH, + result: None, + receipts, + total_gas: 0, + total_fee: 0, + }) + }); data.expect_block().times(2).returning({ let commit_block = commit_block.clone(); diff --git a/crates/types/src/entities/relayer/message.rs b/crates/types/src/entities/relayer/message.rs index 5be4cbda490..6e8e39b53c0 100644 --- a/crates/types/src/entities/relayer/message.rs +++ b/crates/types/src/entities/relayer/message.rs @@ -249,6 +249,7 @@ pub struct MerkleProof { pub proof_index: u64, } +#[derive(Debug, PartialEq, Eq)] /// Proves to da layer that this message was included in a Fuel block. pub struct MessageProof { /// Proof that message is contained within the provided block header.