diff --git a/crates/apollo_consensus_orchestrator/src/test_utils.rs b/crates/apollo_consensus_orchestrator/src/test_utils.rs index 799593f380b..17f51924d75 100644 --- a/crates/apollo_consensus_orchestrator/src/test_utils.rs +++ b/crates/apollo_consensus_orchestrator/src/test_utils.rs @@ -64,7 +64,7 @@ use starknet_api::block::{ }; use starknet_api::block_hash::block_hash_calculator::{BlockHeaderCommitments, PartialBlockHash}; use starknet_api::consensus_transaction::{ConsensusTransaction, InternalConsensusTransaction}; -use starknet_api::core::{ChainId, ContractAddress, Nonce}; +use starknet_api::core::{AddressDerivationHash, ChainId, ContractAddress, Nonce}; use starknet_api::data_availability::L1DataAvailabilityMode; use starknet_api::execution_resources::GasAmount; use starknet_api::felt; @@ -115,10 +115,12 @@ pub(crate) static INTERNAL_TX_BATCH: LazyLock> .iter() .cloned() .map(|tx| { - let (internal_tx, _) = block_on( - TRANSACTION_CONVERTER.convert_consensus_tx_to_internal_consensus_tx(tx), - ) - .unwrap(); + let (internal_tx, _) = + block_on(TRANSACTION_CONVERTER.convert_consensus_tx_to_internal_consensus_tx( + tx, + AddressDerivationHash::Pedersen, + )) + .unwrap(); internal_tx }) .collect() @@ -308,9 +310,9 @@ impl TestDeps { .returning(|_| Ok(tx.clone())); self.transaction_converter .expect_convert_consensus_tx_to_internal_consensus_tx() - .withf(move |internal_tx| internal_tx == tx) + .withf(move |internal_tx, _| internal_tx == tx) // TODO(Einat): Add verification handle when client-side proving is tested in the validate proposal test. - .returning(|_| Ok((internal_tx.clone(), None))); + .returning(|_, _| Ok((internal_tx.clone(), None))); } } diff --git a/crates/apollo_consensus_orchestrator/src/validate_proposal.rs b/crates/apollo_consensus_orchestrator/src/validate_proposal.rs index f02ecbd0dbe..f23779a950d 100644 --- a/crates/apollo_consensus_orchestrator/src/validate_proposal.rs +++ b/crates/apollo_consensus_orchestrator/src/validate_proposal.rs @@ -24,6 +24,9 @@ use apollo_state_sync_types::communication::SharedStateSyncClient; use apollo_time::time::{Clock, ClockExt, DateTime}; use apollo_transaction_converter::{TransactionConverterTrait, VerifyAndStoreProofTask}; use apollo_versioned_constants::VersionedConstants; +// The address-derivation flag lives on blockifier's (full) versioned constants, not the +// lighter apollo_versioned_constants one used above for gas margins. +use blockifier::blockifier_versioned_constants::VersionedConstants as BlockifierVersionedConstants; use futures::channel::mpsc; use futures::StreamExt; use starknet_api::block::{BlockNumber, GasPrice, StarknetVersion}; @@ -564,13 +567,19 @@ async fn handle_proposal_part( // TODO(guyn): check that the length of txs and the number of batches we receive is not // so big it would fill up the memory (in case of a malicious proposal) debug!("Received transaction batch with {} txs", txs.len()); - let conversion_results = - futures::future::join_all(txs.into_iter().map(|tx| { - transaction_converter.convert_consensus_tx_to_internal_consensus_tx(tx) - })) - .await - .into_iter() - .collect::, _>>(); + // Derive deploy-account addresses with the latest versioned constants' hash, matching + // the proposer's gateway (which also predicts the scheme via latest_constants) and the + // block's execution. Relies on the latest version being active fleet-wide -- same + // decision-A boundary hazard documented in apollo_gateway::gateway. + let address_derivation_hash = + BlockifierVersionedConstants::latest_constants().address_derivation_hash(); + let conversion_results = futures::future::join_all(txs.into_iter().map(|tx| { + transaction_converter + .convert_consensus_tx_to_internal_consensus_tx(tx, address_derivation_hash) + })) + .await + .into_iter() + .collect::, _>>(); let conversion_results = match conversion_results { Ok(results) => results, Err(e) => { diff --git a/crates/apollo_gateway/src/gateway.rs b/crates/apollo_gateway/src/gateway.rs index 0ee7e537dda..5d529f02a7b 100644 --- a/crates/apollo_gateway/src/gateway.rs +++ b/crates/apollo_gateway/src/gateway.rs @@ -28,6 +28,7 @@ use apollo_transaction_converter::{ VerificationHandle, }; use async_trait::async_trait; +use blockifier::blockifier_versioned_constants::VersionedConstants; use blockifier::state::contract_class_manager::ContractClassManager; use starknet_api::executable_transaction::AccountTransaction; use starknet_api::rpc_transaction::{ @@ -39,6 +40,7 @@ use starknet_api::rpc_transaction::{ }; use starknet_api::transaction::fields::{Proof, ProofFacts, TransactionSignature}; use starknet_api::transaction::TransactionHash; +use starknet_api::versioned_constants_logic::VersionedConstantsTrait; use starknet_types_core::felt::Felt; use tokio::task::JoinHandle; use tokio::time::timeout; @@ -418,13 +420,22 @@ impl< (InternalRpcTransaction, AccountTransaction, Option<(ProofFacts, Proof)>), StarknetError, > { - let (internal_tx, verification_handle) = - self.transaction_converter.convert_rpc_tx_to_internal_rpc_tx(tx).await.map_err( - |e| { - warn!("Failed to convert RPC transaction to internal RPC transaction: {}", e); - transaction_converter_err_to_deprecated_gw_err(tx_signature, e) - }, - )?; + // Derive deploy-account addresses with the latest versioned constants' hash. The gateway + // must predict the derivation scheme of the (not-yet-assigned) landing block; this matches + // execution once the latest version is active fleet-wide. A tx submitted across a + // derivation-scheme activation boundary (Pedersen->Blake2 at 0.14.4) may be predicted with + // the wrong scheme and rejected -- the accepted decision-A boundary hazard (no separate + // opt-in tx version). + let address_derivation_hash = + VersionedConstants::latest_constants().address_derivation_hash(); + let (internal_tx, verification_handle) = self + .transaction_converter + .convert_rpc_tx_to_internal_rpc_tx(tx, address_derivation_hash) + .await + .map_err(|e| { + warn!("Failed to convert RPC transaction to internal RPC transaction: {}", e); + transaction_converter_err_to_deprecated_gw_err(tx_signature, e) + })?; // Await the verification task immediately. let proof_data = self diff --git a/crates/apollo_gateway/src/gateway_test.rs b/crates/apollo_gateway/src/gateway_test.rs index 38c7cd20411..b3ba4029722 100644 --- a/crates/apollo_gateway/src/gateway_test.rs +++ b/crates/apollo_gateway/src/gateway_test.rs @@ -52,7 +52,7 @@ use blockifier_test_utils::contracts::FeatureContract; use clap::Command; use mempool_test_utils::starknet_api_test_utils::{contract_class, declare_tx}; use metrics_exporter_prometheus::PrometheusBuilder; -use mockall::predicate::eq; +use mockall::predicate::{always, eq}; use rstest::{fixture, rstest}; use starknet_api::core::{ClassHash, ContractAddress, Nonce}; use starknet_api::executable_transaction::AccountTransaction; @@ -299,8 +299,8 @@ fn setup_transaction_converter_mock( mock_transaction_converter .expect_convert_rpc_tx_to_internal_rpc_tx() .once() - .with(eq(rpc_tx)) - .return_once(move |_| Ok((internal_tx, verification_handle))); + .with(eq(rpc_tx), always()) + .return_once(move |_, _| Ok((internal_tx, verification_handle))); let internal_tx = tx_args.get_internal_tx(); let executable_tx = tx_args.get_executable_tx(); @@ -341,8 +341,8 @@ fn setup_transaction_converter_mock_with_failed_verification( mock_transaction_converter .expect_convert_rpc_tx_to_internal_rpc_tx() .once() - .with(eq(rpc_tx)) - .return_once(move |_| Ok((internal_tx, verification_handle))); + .with(eq(rpc_tx), always()) + .return_once(move |_, _| Ok((internal_tx, verification_handle))); // Note: Unlike in the successful case, we don't set up // expect_convert_internal_rpc_tx_to_executable_tx because the verification failure will @@ -677,7 +677,7 @@ async fn test_transaction_converter_error( mock_dependencies .mock_transaction_converter .expect_convert_rpc_tx_to_internal_rpc_tx() - .return_once(|_| expect_internal_rpc_tx_result); + .return_once(|_, _| expect_internal_rpc_tx_result); mock_dependencies .mock_transaction_converter .expect_convert_internal_rpc_tx_to_executable_tx() diff --git a/crates/apollo_transaction_converter/src/transaction_converter.rs b/crates/apollo_transaction_converter/src/transaction_converter.rs index b7f0305d303..4ea5887ddae 100644 --- a/crates/apollo_transaction_converter/src/transaction_converter.rs +++ b/crates/apollo_transaction_converter/src/transaction_converter.rs @@ -82,6 +82,7 @@ pub trait TransactionConverterTrait: Send + Sync { async fn convert_consensus_tx_to_internal_consensus_tx( &self, tx: ConsensusTransaction, + address_derivation_hash: AddressDerivationHash, ) -> TransactionConverterResult<(InternalConsensusTransaction, Option)>; async fn convert_internal_rpc_tx_to_rpc_tx( @@ -92,6 +93,7 @@ pub trait TransactionConverterTrait: Send + Sync { async fn convert_rpc_tx_to_internal_rpc_tx( &self, tx: RpcTransaction, + address_derivation_hash: AddressDerivationHash, ) -> TransactionConverterResult<(InternalRpcTransaction, Option)>; async fn convert_internal_rpc_tx_to_executable_tx( @@ -184,11 +186,13 @@ impl TransactionConverterTrait for TransactionConverter { async fn convert_consensus_tx_to_internal_consensus_tx( &self, tx: ConsensusTransaction, + address_derivation_hash: AddressDerivationHash, ) -> TransactionConverterResult<(InternalConsensusTransaction, Option)> { match tx { ConsensusTransaction::RpcTransaction(tx) => { - let (internal_tx, proof_data) = self.convert_rpc_tx_to_internal(tx).await?; + let (internal_tx, proof_data) = + self.convert_rpc_tx_to_internal(tx, address_derivation_hash).await?; let task = proof_data.map(|(proof_facts, proof)| { self.spawn_verify_and_store_proof(proof_facts, proof) }); @@ -256,8 +260,10 @@ impl TransactionConverterTrait for TransactionConverter { async fn convert_rpc_tx_to_internal_rpc_tx( &self, tx: RpcTransaction, + address_derivation_hash: AddressDerivationHash, ) -> TransactionConverterResult<(InternalRpcTransaction, Option)> { - let (internal_tx, proof_data) = self.convert_rpc_tx_to_internal(tx).await?; + let (internal_tx, proof_data) = + self.convert_rpc_tx_to_internal(tx, address_derivation_hash).await?; let verification_handle = proof_data .map(|(proof_facts, proof)| self.spawn_proof_verification(proof_facts, proof)) .transpose()?; @@ -334,6 +340,7 @@ impl TransactionConverter { async fn convert_rpc_tx_to_internal( &self, tx: RpcTransaction, + address_derivation_hash: AddressDerivationHash, ) -> TransactionConverterResult<(InternalRpcTransaction, Option<(ProofFacts, Proof)>)> { let (tx_without_hash, proof_data) = match tx { RpcTransaction::Invoke(RpcInvokeTransaction::V3(tx)) => { @@ -376,8 +383,7 @@ impl TransactionConverter { ) } RpcTransaction::DeployAccount(RpcDeployAccountTransaction::V3(tx)) => { - let contract_address = - tx.calculate_contract_address(AddressDerivationHash::Pedersen)?; + let contract_address = tx.calculate_contract_address(address_derivation_hash)?; ( InternalRpcTransactionWithoutTxHash::DeployAccount( InternalRpcDeployAccountTransaction { diff --git a/crates/apollo_transaction_converter/src/transaction_converter_test.rs b/crates/apollo_transaction_converter/src/transaction_converter_test.rs index b3d063578bd..a7be0875882 100644 --- a/crates/apollo_transaction_converter/src/transaction_converter_test.rs +++ b/crates/apollo_transaction_converter/src/transaction_converter_test.rs @@ -7,6 +7,7 @@ use blockifier::context::ChainInfo; use blockifier_test_utils::cairo_versions::{CairoVersion, RunnableCairo1}; use mempool_test_utils::starknet_api_test_utils::{ declare_tx, + deploy_account_tx, invoke_tx, invoke_tx_client_side_proving, }; @@ -14,8 +15,14 @@ use mockall::predicate::eq; use rstest::{fixture, rstest}; use starknet_api::compiled_class_hash; use starknet_api::consensus_transaction::ConsensusTransaction; +use starknet_api::core::AddressDerivationHash; use starknet_api::executable_transaction::ValidateCompiledClassHashError; -use starknet_api::rpc_transaction::{RpcDeclareTransaction, RpcTransaction}; +use starknet_api::rpc_transaction::{ + InternalRpcTransaction, + InternalRpcTransactionWithoutTxHash, + RpcDeclareTransaction, + RpcTransaction, +}; use starknet_api::test_utils::{path_in_resources, read_json_file}; use starknet_api::transaction::fields::{Proof, ProofFacts}; @@ -104,8 +111,10 @@ async fn test_compiled_class_hash_mismatch() { ChainInfo::create_for_testing().chain_id, ); - let err = - transaction_converter.convert_rpc_tx_to_internal_rpc_tx(declare_tx).await.unwrap_err(); + let err = transaction_converter + .convert_rpc_tx_to_internal_rpc_tx(declare_tx, AddressDerivationHash::Pedersen) + .await + .unwrap_err(); let expected_code = TransactionConverterError::ValidateCompiledClassHashError( ValidateCompiledClassHashError::CompiledClassHashMismatch { computed_class_hash: other_compiled_class_hash, @@ -136,8 +145,10 @@ async fn test_proof_verification_called_for_invoke_v3_with_proof_facts( let transaction_converter = create_transaction_converter(mock_proof_manager_client); - let (_internal_tx, verification_handle) = - transaction_converter.convert_rpc_tx_to_internal_rpc_tx(invoke_tx).await.unwrap(); + let (_internal_tx, verification_handle) = transaction_converter + .convert_rpc_tx_to_internal_rpc_tx(invoke_tx, AddressDerivationHash::Pedersen) + .await + .unwrap(); await_verification_handle(verification_handle).await; } @@ -151,8 +162,10 @@ async fn test_proof_verification_skipped_for_invoke_v3_without_proof_facts() { let mock_proof_manager_client = MockProofManagerClient::new(); let transaction_converter = create_transaction_converter(mock_proof_manager_client); - let (_internal_tx, verification_handle) = - transaction_converter.convert_rpc_tx_to_internal_rpc_tx(invoke_tx).await.unwrap(); + let (_internal_tx, verification_handle) = transaction_converter + .convert_rpc_tx_to_internal_rpc_tx(invoke_tx, AddressDerivationHash::Pedersen) + .await + .unwrap(); assert!(verification_handle.is_none()); } @@ -189,7 +202,10 @@ async fn test_consensus_tx_to_internal_with_proof_facts_verifies_and_sets_proof( let transaction_converter = create_transaction_converter(mock_proof_manager_client); let (_internal_tx, verify_and_store_proof_task) = transaction_converter - .convert_consensus_tx_to_internal_consensus_tx(consensus_tx) + .convert_consensus_tx_to_internal_consensus_tx( + consensus_tx, + AddressDerivationHash::Pedersen, + ) .await .unwrap(); @@ -221,8 +237,10 @@ async fn test_convert_internal_rpc_tx_to_rpc_tx_with_proof(proof_facts: ProofFac let transaction_converter = create_transaction_converter(mock_proof_manager_client); - let (internal_tx, verification_handle) = - transaction_converter.convert_rpc_tx_to_internal_rpc_tx(rpc_tx.clone()).await.unwrap(); + let (internal_tx, verification_handle) = transaction_converter + .convert_rpc_tx_to_internal_rpc_tx(rpc_tx.clone(), AddressDerivationHash::Pedersen) + .await + .unwrap(); await_verification_handle(verification_handle).await; @@ -231,3 +249,30 @@ async fn test_convert_internal_rpc_tx_to_rpc_tx_with_proof(proof_facts: ProofFac assert_eq!(rpc_tx, rpc_tx_from_internal); } + +/// The deploy-account contract address (and the tx hash that embeds it) must be derived with the +/// AddressDerivationHash passed to the conversion, not a fixed scheme. +#[rstest] +#[tokio::test] +async fn test_deploy_account_conversion_uses_address_derivation_hash() { + let rpc_tx = deploy_account_tx(); + let transaction_converter = create_transaction_converter(MockProofManagerClient::new()); + + let (pedersen_internal, _) = transaction_converter + .convert_rpc_tx_to_internal_rpc_tx(rpc_tx.clone(), AddressDerivationHash::Pedersen) + .await + .unwrap(); + let (blake_internal, _) = transaction_converter + .convert_rpc_tx_to_internal_rpc_tx(rpc_tx, AddressDerivationHash::Blake2) + .await + .unwrap(); + + let deploy_account_address = |internal: &InternalRpcTransaction| match &internal.tx { + InternalRpcTransactionWithoutTxHash::DeployAccount(tx) => tx.contract_address, + _ => panic!("Expected a deploy-account transaction."), + }; + + // The requested hash flows through to the derived address and to the tx hash that embeds it. + assert_ne!(deploy_account_address(&pedersen_internal), deploy_account_address(&blake_internal),); + assert_ne!(pedersen_internal.tx_hash, blake_internal.tx_hash); +} diff --git a/crates/starknet_api/src/rpc_transaction.rs b/crates/starknet_api/src/rpc_transaction.rs index 7ac7a656b03..39c92132dc9 100644 --- a/crates/starknet_api/src/rpc_transaction.rs +++ b/crates/starknet_api/src/rpc_transaction.rs @@ -94,10 +94,15 @@ impl TransactionHasher for InternalRpcDeployAccountTransaction { chain_id: &ChainId, transaction_version: &TransactionVersion, ) -> Result { + // Use the contract address already derived during conversion (with the version-gated hash) + // rather than recomputing it, so the tx hash embeds the same address execution will deploy. match &self.tx { - RpcDeployAccountTransaction::V3(tx) => { - tx.calculate_transaction_hash(chain_id, transaction_version) - } + RpcDeployAccountTransaction::V3(tx) => get_deploy_account_transaction_v3_hash( + tx, + chain_id, + transaction_version, + self.contract_address, + ), } } }