Skip to content

Commit

Permalink
Merge pull request #2414 from eqlabs/chris/defer-deployed_contract_ad…
Browse files Browse the repository at this point in the history 
…dress

fix(sync): deployed_contract_address computed in async context
  • Loading branch information
@CHr15F0x
CHr15F0x authored Nov 29, 2024
2 parents e82985e + 9536824 commit 4306d3b
Show file tree
Hide file tree
Showing 5 changed files with 120 additions and 25 deletions.
50 changes: 50 additions & 0 deletions crates/common/src/transaction.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<DeclareTransactionV2> for TransactionVariant {
Expand Down
32 changes: 12 additions & 20 deletions crates/p2p/src/client/conv.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -443,6 +443,8 @@ impl ToDto<p2p_proto::common::L1DataAvailabilityMode> for L1DataAvailabilityMode
}

impl TryFromDto<p2p_proto::transaction::TransactionVariant> for TransactionVariant {
/// Caller must take care to compute the contract address for deploy and
/// deploy account transactions separately in a non-async context.
fn try_from_dto(dto: p2p_proto::transaction::TransactionVariant) -> anyhow::Result<Self>
where
Self: Sized,
Expand Down Expand Up @@ -535,11 +537,8 @@ impl TryFromDto<p2p_proto::transaction::TransactionVariant> 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,
Expand All @@ -552,11 +551,8 @@ impl TryFromDto<p2p_proto::transaction::TransactionVariant> 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,
Expand All @@ -570,11 +566,8 @@ impl TryFromDto<p2p_proto::transaction::TransactionVariant> 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
Expand All @@ -595,11 +588,8 @@ impl TryFromDto<p2p_proto::transaction::TransactionVariant> 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
Expand Down Expand Up @@ -690,6 +680,8 @@ impl TryFromDto<p2p_proto::transaction::TransactionVariant> for TransactionVaria
}

impl TryFromDto<p2p_proto::transaction::Transaction> for Transaction {
/// Caller must take care to compute the contract address for deploy and
/// deploy account transactions separately in a non-async context.
fn try_from_dto(dto: p2p_proto::transaction::Transaction) -> anyhow::Result<Self>
where
Self: Sized,
Expand Down
42 changes: 40 additions & 2 deletions crates/p2p/src/client/peer_agnostic/fixtures.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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<T> Dummy<T> for TestTxn {
fn dummy_with_rng<R: rand::Rng + ?Sized>(_: &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);

Expand Down
4 changes: 3 additions & 1 deletion crates/pathfinder/src/p2p_network/sync_handlers/tests.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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::<Vec<_>>();
Expand Down
17 changes: 15 additions & 2 deletions crates/pathfinder/src/sync/transactions.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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");
Expand All @@ -110,6 +122,7 @@ impl ProcessStage for CalculateHashes {
}
})
.collect::<Result<Vec<_>, _>>()?;

Ok(UnverifiedTransactions {
expected_commitment,
transactions,
Expand Down

0 comments on commit 4306d3b

Please sign in to comment.