From 80cc9e793e491b9a7744aa01ad331368fb34e7e9 Mon Sep 17 00:00:00 2001 From: Svyatoslav Nikolsky Date: Fri, 31 Jul 2020 16:32:22 +0300 Subject: [PATCH] Fix gas_used fields in receipts (#261) * gas_used should be cumulative_gas_used!!! * more runtime traces * improve logs --- bridges/modules/currency-exchange/src/lib.rs | 8 +- bridges/modules/ethereum/src/lib.rs | 75 +++++++++++++++++-- bridges/modules/ethereum/src/validators.rs | 2 +- bridges/modules/ethereum/src/verification.rs | 2 +- bridges/primitives/ethereum-poa/src/lib.rs | 37 ++++++--- .../relays/ethereum/src/ethereum_client.rs | 7 +- bridges/relays/ethereum/src/ethereum_types.rs | 4 - bridges/relays/ethereum/src/rpc_errors.rs | 5 -- .../relays/ethereum/src/substrate_types.rs | 3 +- 9 files changed, 107 insertions(+), 36 deletions(-) diff --git a/bridges/modules/currency-exchange/src/lib.rs b/bridges/modules/currency-exchange/src/lib.rs index cf03239c8a..967fbb66cb 100644 --- a/bridges/modules/currency-exchange/src/lib.rs +++ b/bridges/modules/currency-exchange/src/lib.rs @@ -147,7 +147,13 @@ impl, I: Instance> Module { /// Returns true if currency exchange module is able to import given transaction proof in /// its current state. pub fn filter_transaction_proof(proof: &::TransactionInclusionProof) -> bool { - if prepare_deposit_details::(proof).is_err() { + if let Err(err) = prepare_deposit_details::(proof) { + frame_support::debug::trace!( + target: "runtime", + "Can't accept exchange transaction: {:?}", + err, + ); + return false; } diff --git a/bridges/modules/ethereum/src/lib.rs b/bridges/modules/ethereum/src/lib.rs index ae6399f002..cb8305543d 100644 --- a/bridges/modules/ethereum/src/lib.rs +++ b/bridges/modules/ethereum/src/lib.rs @@ -893,17 +893,40 @@ pub fn verify_transaction_finalized( proof: &[(RawTransaction, RawTransactionReceipt)], ) -> bool { if tx_index >= proof.len() as _ { + frame_support::debug::trace!( + target: "runtime", + "Tx finality check failed: transaction index ({}) is larger than number of transactions ({})", + tx_index, + proof.len(), + ); + return false; } let header = match storage.header(&block) { Some((header, _)) => header, - None => return false, + None => { + frame_support::debug::trace!( + target: "runtime", + "Tx finality check failed: can't find header in the storage: {}", + block, + ); + + return false; + } }; let finalized = storage.finalized_block(); // if header is not yet finalized => return if header.number > finalized.number { + frame_support::debug::trace!( + target: "runtime", + "Tx finality check failed: header {}/{} is not finalized. Best finalized: {}", + header.number, + block, + finalized.number, + ); + return false; } @@ -915,24 +938,62 @@ pub fn verify_transaction_finalized( false => block == finalized.hash, }; if !is_finalized { + frame_support::debug::trace!( + target: "runtime", + "Tx finality check failed: header {} is not finalized: no canonical path to best finalized block {}", + block, + finalized.hash, + ); + return false; } // verify that transaction is included in the block - if !header.verify_transactions_root(proof.iter().map(|(tx, _)| tx)) { + if let Err(computed_root) = header.check_transactions_root(proof.iter().map(|(tx, _)| tx)) { + frame_support::debug::trace!( + target: "runtime", + "Tx finality check failed: transactions root mismatch. Expected: {}, computed: {}", + header.transactions_root, + computed_root, + ); + return false; } // verify that transaction receipt is included in the block - if !header.verify_raw_receipts_root(proof.iter().map(|(_, r)| r)) { + if let Err(computed_root) = header.check_raw_receipts_root(proof.iter().map(|(_, r)| r)) { + frame_support::debug::trace!( + target: "runtime", + "Tx finality check failed: receipts root mismatch. Expected: {}, computed: {}", + header.receipts_root, + computed_root, + ); + return false; } // check that transaction has completed successfully - matches!( - Receipt::is_successful_raw_receipt(&proof[tx_index as usize].1), - Ok(true) - ) + let is_successful_raw_receipt = Receipt::is_successful_raw_receipt(&proof[tx_index as usize].1); + match is_successful_raw_receipt { + Ok(true) => true, + Ok(false) => { + frame_support::debug::trace!( + target: "runtime", + "Tx finality check failed: receipt shows that transaction has failed", + ); + + false + } + Err(err) => { + frame_support::debug::trace!( + target: "runtime", + "Tx finality check failed: receipt check has failed: {}", + err, + ); + + false + } + } } /// Transaction pool configuration. diff --git a/bridges/modules/ethereum/src/validators.rs b/bridges/modules/ethereum/src/validators.rs index 9695e9a11b..8253ab2def 100644 --- a/bridges/modules/ethereum/src/validators.rs +++ b/bridges/modules/ethereum/src/validators.rs @@ -132,7 +132,7 @@ impl<'a> Validators<'a> { } let receipts = receipts.ok_or(Error::MissingTransactionsReceipts)?; - if !header.verify_receipts_root(&receipts) { + if header.check_receipts_root(&receipts).is_err() { return Err(Error::TransactionsReceiptsMismatch); } diff --git a/bridges/modules/ethereum/src/verification.rs b/bridges/modules/ethereum/src/verification.rs index 31f9c2e553..8e028edbbc 100644 --- a/bridges/modules/ethereum/src/verification.rs +++ b/bridges/modules/ethereum/src/verification.rs @@ -144,7 +144,7 @@ pub fn accept_aura_header_into_pool( // the heaviest, but rare operation - we do not want invalid receipts in the pool if let Some(receipts) = receipts { frame_support::debug::trace!(target: "runtime", "Got receipts! {:?}", receipts); - if !header.verify_receipts_root(receipts) { + if header.check_receipts_root(receipts).is_err() { return Err(Error::TransactionsReceiptsMismatch); } } diff --git a/bridges/primitives/ethereum-poa/src/lib.rs b/bridges/primitives/ethereum-poa/src/lib.rs index ada3fe9a1a..a2a2b9366b 100644 --- a/bridges/primitives/ethereum-poa/src/lib.rs +++ b/bridges/primitives/ethereum-poa/src/lib.rs @@ -205,18 +205,30 @@ impl Header { } /// Check if passed transactions receipts are matching receipts root in this header. - pub fn verify_receipts_root(&self, receipts: &[Receipt]) -> bool { - verify_merkle_proof(self.receipts_root, receipts.iter().map(|r| r.rlp())) + /// Returns Ok(computed-root) if check succeeds. + /// Returns Err(computed-root) if check fails. + pub fn check_receipts_root(&self, receipts: &[Receipt]) -> Result { + check_merkle_proof(self.receipts_root, receipts.iter().map(|r| r.rlp())) } /// Check if passed raw transactions receipts are matching receipts root in this header. - pub fn verify_raw_receipts_root<'a>(&self, receipts: impl IntoIterator) -> bool { - verify_merkle_proof(self.receipts_root, receipts.into_iter()) + /// Returns Ok(computed-root) if check succeeds. + /// Returns Err(computed-root) if check fails. + pub fn check_raw_receipts_root<'a>( + &self, + receipts: impl IntoIterator, + ) -> Result { + check_merkle_proof(self.receipts_root, receipts.into_iter()) } /// Check if passed transactions are matching transactions root in this header. - pub fn verify_transactions_root<'a>(&self, transactions: impl IntoIterator) -> bool { - verify_merkle_proof(self.transactions_root, transactions.into_iter()) + /// Returns Ok(computed-root) if check succeeds. + /// Returns Err(computed-root) if check fails. + pub fn check_transactions_root<'a>( + &self, + transactions: impl IntoIterator, + ) -> Result { + check_merkle_proof(self.transactions_root, transactions.into_iter()) } /// Gets the seal hash of this header. @@ -502,9 +514,16 @@ pub fn public_to_address(public: &[u8; 64]) -> Address { result } -/// Verify ethereum merkle proof. -fn verify_merkle_proof>(expected_root: H256, items: impl Iterator) -> bool { - compute_merkle_root(items) == expected_root +/// Check ethereum merkle proof. +/// Returns Ok(computed-root) if check succeeds. +/// Returns Err(computed-root) if check fails. +fn check_merkle_proof>(expected_root: H256, items: impl Iterator) -> Result { + let computed_root = compute_merkle_root(items); + if computed_root == expected_root { + Ok(computed_root) + } else { + Err(computed_root) + } } /// Compute ethereum merkle root. diff --git a/bridges/relays/ethereum/src/ethereum_client.rs b/bridges/relays/ethereum/src/ethereum_client.rs index df4945afca..b489cab522 100644 --- a/bridges/relays/ethereum/src/ethereum_client.rs +++ b/bridges/relays/ethereum/src/ethereum_client.rs @@ -169,12 +169,7 @@ impl EthereumRpc for EthereumRpcClient { } async fn transaction_receipt(&self, transaction_hash: H256) -> Result { - let receipt = Ethereum::get_transaction_receipt(&self.client, transaction_hash).await?; - - match receipt.gas_used { - Some(_) => Ok(receipt), - None => Err(RpcError::Ethereum(EthereumNodeError::IncompleteReceipt)), - } + Ok(Ethereum::get_transaction_receipt(&self.client, transaction_hash).await?) } async fn account_nonce(&self, address: Address) -> Result { diff --git a/bridges/relays/ethereum/src/ethereum_types.rs b/bridges/relays/ethereum/src/ethereum_types.rs index 58aa349701..8dcfb00fc9 100644 --- a/bridges/relays/ethereum/src/ethereum_types.rs +++ b/bridges/relays/ethereum/src/ethereum_types.rs @@ -24,10 +24,6 @@ pub use web3::types::{Address, Bytes, CallRequest, H256, U128, U256, U64}; /// both number and hash fields filled. pub const HEADER_ID_PROOF: &str = "checked on retrieval; qed"; -/// When receipt is just received from the Ethereum node, we check that it has -/// gas_used field filled. -pub const RECEIPT_GAS_USED_PROOF: &str = "checked on retrieval; qed"; - /// Ethereum transaction hash type. pub type TransactionHash = H256; diff --git a/bridges/relays/ethereum/src/rpc_errors.rs b/bridges/relays/ethereum/src/rpc_errors.rs index dfc84192db..aa3bcbde09 100644 --- a/bridges/relays/ethereum/src/rpc_errors.rs +++ b/bridges/relays/ethereum/src/rpc_errors.rs @@ -95,8 +95,6 @@ pub enum EthereumNodeError { ResponseParseFailed(String), /// We have received a header with missing fields. IncompleteHeader, - /// We have received a receipt missing a `gas_used` field. - IncompleteReceipt, /// We have received a transaction missing a `raw` field. IncompleteTransaction, /// An invalid Substrate block number was received from @@ -112,9 +110,6 @@ impl ToString for EthereumNodeError { "Incomplete Ethereum Header Received (missing some of required fields - hash, number, logs_bloom)" .to_string() } - Self::IncompleteReceipt => { - "Incomplete Ethereum Receipt Recieved (missing required field - gas_used)".to_string() - } Self::IncompleteTransaction => "Incomplete Ethereum Transaction (missing required field - raw)".to_string(), Self::InvalidSubstrateBlockNumber => "Received an invalid Substrate block from Ethereum Node".to_string(), } diff --git a/bridges/relays/ethereum/src/substrate_types.rs b/bridges/relays/ethereum/src/substrate_types.rs index 7d761a627e..9e5ce6b732 100644 --- a/bridges/relays/ethereum/src/substrate_types.rs +++ b/bridges/relays/ethereum/src/substrate_types.rs @@ -16,7 +16,6 @@ use crate::ethereum_types::{ Header as EthereumHeader, Receipt as EthereumReceipt, HEADER_ID_PROOF as ETHEREUM_HEADER_ID_PROOF, - RECEIPT_GAS_USED_PROOF as ETHEREUM_RECEIPT_GAS_USED_PROOF, }; use crate::sync_types::{HeaderId, HeadersSyncPipeline, QueuedHeader, SourceHeader}; use codec::Encode; @@ -108,7 +107,7 @@ pub fn into_substrate_ethereum_receipts( /// Convert Ethereum transactions receipt into Ethereum transactions receipt for Substrate. pub fn into_substrate_ethereum_receipt(receipt: &EthereumReceipt) -> SubstrateEthereumReceipt { SubstrateEthereumReceipt { - gas_used: receipt.gas_used.expect(ETHEREUM_RECEIPT_GAS_USED_PROOF), + gas_used: receipt.cumulative_gas_used, log_bloom: receipt.logs_bloom.data().into(), logs: receipt .logs