From 01a3f78b09a75bfe29c921e318ac9c533611829e Mon Sep 17 00:00:00 2001 From: Krzysztof Lis Date: Wed, 27 Nov 2024 12:41:22 +0100 Subject: [PATCH 1/2] fix(sync): deployed_contract_address computed in async context --- crates/common/src/transaction.rs | 50 +++++++++++++++++++ crates/p2p/src/client/conv.rs | 32 +++++------- .../p2p/src/client/peer_agnostic/fixtures.rs | 42 +++++++++++++++- .../src/p2p_network/sync_handlers/tests.rs | 4 +- crates/pathfinder/src/sync/transactions.rs | 17 ++++++- 5 files changed, 120 insertions(+), 25 deletions(-) diff --git a/crates/common/src/transaction.rs b/crates/common/src/transaction.rs index 4cac3c877..4490650d2 100644 --- a/crates/common/src/transaction.rs +++ b/crates/common/src/transaction.rs @@ -145,6 +145,56 @@ impl TransactionVariant { Some(hash) } + + /// Compute contract address for deploy and deploy account transactions. Do + /// nothing in case of other transactions. + /// + /// ### Important + /// + /// This function should not be called in async context. + pub fn calculate_contract_address(&mut self) { + match self { + TransactionVariant::DeployV0(DeployTransactionV0 { + class_hash, + constructor_calldata, + contract_address, + contract_address_salt, + }) + | TransactionVariant::DeployV1(DeployTransactionV1 { + class_hash, + constructor_calldata, + contract_address, + contract_address_salt, + }) => { + *contract_address = ContractAddress::deployed_contract_address( + constructor_calldata.iter().map(|d| CallParam(d.0)), + contract_address_salt, + class_hash, + ); + } + TransactionVariant::DeployAccountV1(DeployAccountTransactionV1 { + class_hash, + constructor_calldata, + contract_address, + contract_address_salt, + .. + }) + | TransactionVariant::DeployAccountV3(DeployAccountTransactionV3 { + class_hash, + constructor_calldata, + contract_address, + contract_address_salt, + .. + }) => { + *contract_address = ContractAddress::deployed_contract_address( + constructor_calldata.iter().copied(), + contract_address_salt, + class_hash, + ); + } + _ => {} + } + } } impl From for TransactionVariant { diff --git a/crates/p2p/src/client/conv.rs b/crates/p2p/src/client/conv.rs index 5e8e08a00..8611ba019 100644 --- a/crates/p2p/src/client/conv.rs +++ b/crates/p2p/src/client/conv.rs @@ -443,6 +443,8 @@ impl ToDto for L1DataAvailabilityMode } impl TryFromDto for TransactionVariant { + /// Caller must take care to compute the contract address for deploy and + /// deploy account transactions separately is a non-async context. fn try_from_dto(dto: p2p_proto::transaction::TransactionVariant) -> anyhow::Result where Self: Sized, @@ -535,11 +537,8 @@ impl TryFromDto for TransactionVaria let class_hash = ClassHash(x.class_hash.0); Self::DeployV0(DeployTransactionV0 { - contract_address: ContractAddress::deployed_contract_address( - constructor_calldata.iter().map(|d| CallParam(d.0)), - &contract_address_salt, - &class_hash, - ), + // Computing the address is CPU intensive, so we do it later on. + contract_address: ContractAddress::ZERO, contract_address_salt, class_hash, constructor_calldata, @@ -552,11 +551,8 @@ impl TryFromDto for TransactionVaria let class_hash = ClassHash(x.class_hash.0); Self::DeployV1(DeployTransactionV1 { - contract_address: ContractAddress::deployed_contract_address( - constructor_calldata.iter().map(|d| CallParam(d.0)), - &contract_address_salt, - &class_hash, - ), + // Computing the address is CPU intensive, so we do it later on. + contract_address: ContractAddress::ZERO, contract_address_salt, class_hash, constructor_calldata, @@ -570,11 +566,8 @@ impl TryFromDto for TransactionVaria let class_hash = ClassHash(x.class_hash.0); Self::DeployAccountV1(DeployAccountTransactionV1 { - contract_address: ContractAddress::deployed_contract_address( - constructor_calldata.iter().map(|d| CallParam(d.0)), - &contract_address_salt, - &class_hash, - ), + // Computing the address is CPU intensive, so we do it later on. + contract_address: ContractAddress::ZERO, max_fee: Fee(x.max_fee), signature: x .signature @@ -595,11 +588,8 @@ impl TryFromDto for TransactionVaria let class_hash = ClassHash(x.class_hash.0); Self::DeployAccountV3(DeployAccountTransactionV3 { - contract_address: ContractAddress::deployed_contract_address( - constructor_calldata.iter().map(|d| CallParam(d.0)), - &contract_address_salt, - &class_hash, - ), + // Computing the address is CPU intensive, so we do it later on. + contract_address: ContractAddress::ZERO, signature: x .signature .parts @@ -690,6 +680,8 @@ impl TryFromDto for TransactionVaria } impl TryFromDto for Transaction { + /// Caller must take care to compute the contract address for deploy and + /// deploy account transactions separately is a non-async context. fn try_from_dto(dto: p2p_proto::transaction::Transaction) -> anyhow::Result where Self: Sized, diff --git a/crates/p2p/src/client/peer_agnostic/fixtures.rs b/crates/p2p/src/client/peer_agnostic/fixtures.rs index 0bdbee8cb..aa3fde777 100644 --- a/crates/p2p/src/client/peer_agnostic/fixtures.rs +++ b/crates/p2p/src/client/peer_agnostic/fixtures.rs @@ -13,7 +13,13 @@ use p2p_proto::state::{ContractDiff, ContractStoredValue, DeclaredClass, StateDi use p2p_proto::transaction::{TransactionWithReceipt, TransactionsResponse}; use pathfinder_common::event::Event; use pathfinder_common::state_update::{ContractClassUpdate, ContractUpdate, StateUpdateData}; -use pathfinder_common::transaction::TransactionVariant; +use pathfinder_common::transaction::{ + DeployAccountTransactionV1, + DeployAccountTransactionV3, + DeployTransactionV0, + DeployTransactionV1, + TransactionVariant, +}; use pathfinder_common::{ BlockHeader, BlockNumber, @@ -37,12 +43,44 @@ use crate::client::peer_agnostic::Receipt; #[derive(Clone, PartialEq, TaggedDebug)] pub struct TestPeer(pub PeerId); -#[derive(Clone, Dummy, PartialEq, TaggedDebug)] +#[derive(Clone, PartialEq, TaggedDebug)] pub struct TestTxn { pub t: TransactionVariant, pub r: Receipt, } +/// We want to simulate transactions as they're incoming via P2P, where +/// contract_address for deploy and deploy account transactions is not +/// propagated. +impl Dummy for TestTxn { + fn dummy_with_rng(_: &T, rng: &mut R) -> Self { + let mut t = Faker.fake_with_rng(rng); + match &mut t { + TransactionVariant::DeployV0(DeployTransactionV0 { + contract_address, .. + }) + | TransactionVariant::DeployV1(DeployTransactionV1 { + contract_address, .. + }) + | TransactionVariant::DeployAccountV1(DeployAccountTransactionV1 { + contract_address, + .. + }) + | TransactionVariant::DeployAccountV3(DeployAccountTransactionV3 { + contract_address, + .. + }) => { + *contract_address = ContractAddress::ZERO; + } + _ => {} + }; + Self { + t, + r: Faker.fake_with_rng(rng), + } + } +} + #[derive(Copy, Clone, Dummy, PartialEq, TaggedDebug)] pub struct TaggedTransactionHash(pub TransactionHash); diff --git a/crates/pathfinder/src/p2p_network/sync_handlers/tests.rs b/crates/pathfinder/src/p2p_network/sync_handlers/tests.rs index ee8a12260..36a287c1b 100644 --- a/crates/pathfinder/src/p2p_network/sync_handlers/tests.rs +++ b/crates/pathfinder/src/p2p_network/sync_handlers/tests.rs @@ -455,7 +455,9 @@ mod prop { // Check the rest let mut actual = responses.into_iter().map(|response| match response { TransactionsResponse::TransactionWithReceipt(TransactionWithReceipt { transaction, receipt }) => { - (TransactionVariant::try_from_dto(transaction.txn).unwrap(), Receipt::try_from((receipt, TransactionIndex::new_or_panic(0))).unwrap()) + let mut txn_variant = TransactionVariant::try_from_dto(transaction.txn).unwrap(); + txn_variant.calculate_contract_address(); + (txn_variant, Receipt::try_from((receipt, TransactionIndex::new_or_panic(0))).unwrap()) } _ => panic!("unexpected response"), }).collect::>(); diff --git a/crates/pathfinder/src/sync/transactions.rs b/crates/pathfinder/src/sync/transactions.rs index 91ad92314..c947ec948 100644 --- a/crates/pathfinder/src/sync/transactions.rs +++ b/crates/pathfinder/src/sync/transactions.rs @@ -6,11 +6,20 @@ use p2p::client::types::TransactionData; use p2p::libp2p::PeerId; use p2p::PeerData; use pathfinder_common::receipt::Receipt; -use pathfinder_common::transaction::{Transaction, TransactionVariant}; +use pathfinder_common::transaction::{ + DeployAccountTransactionV1, + DeployAccountTransactionV3, + DeployTransactionV0, + DeployTransactionV1, + Transaction, + TransactionVariant, +}; use pathfinder_common::{ BlockHeader, BlockNumber, + CallParam, ChainId, + ContractAddress, StarknetVersion, TransactionCommitment, TransactionHash, @@ -92,7 +101,10 @@ impl ProcessStage for CalculateHashes { let transactions = transactions .into_par_iter() - .map(|(tx, r)| { + .map(|(mut tx, r)| { + // Contract address for deploy and deploy account transactions is not propagated via p2p + tx.variant.calculate_contract_address(); + let computed_hash = tx.variant.calculate_hash(self.0, false); if tx.hash != computed_hash { tracing::debug!(%peer, %block_number, expected_hash=%tx.hash, %computed_hash, "Transaction hash mismatch"); @@ -110,6 +122,7 @@ impl ProcessStage for CalculateHashes { } }) .collect::, _>>()?; + Ok(UnverifiedTransactions { expected_commitment, transactions, From 9536824ad4b09d8fc393cc3d0b4d92c17d737892 Mon Sep 17 00:00:00 2001 From: Krzysztof Lis Date: Fri, 29 Nov 2024 12:55:03 +0100 Subject: [PATCH 2/2] doc: fix typo --- crates/p2p/src/client/conv.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/p2p/src/client/conv.rs b/crates/p2p/src/client/conv.rs index 8611ba019..43f104915 100644 --- a/crates/p2p/src/client/conv.rs +++ b/crates/p2p/src/client/conv.rs @@ -444,7 +444,7 @@ impl ToDto for L1DataAvailabilityMode impl TryFromDto for TransactionVariant { /// Caller must take care to compute the contract address for deploy and - /// deploy account transactions separately is a non-async context. + /// deploy account transactions separately in a non-async context. fn try_from_dto(dto: p2p_proto::transaction::TransactionVariant) -> anyhow::Result where Self: Sized, @@ -681,7 +681,7 @@ impl TryFromDto for TransactionVaria impl TryFromDto for Transaction { /// Caller must take care to compute the contract address for deploy and - /// deploy account transactions separately is a non-async context. + /// deploy account transactions separately in a non-async context. fn try_from_dto(dto: p2p_proto::transaction::Transaction) -> anyhow::Result where Self: Sized,