diff --git a/crates/rpc/src/server/api.rs b/crates/rpc/src/server/api.rs index 915e5643a..7be85f0b7 100644 --- a/crates/rpc/src/server/api.rs +++ b/crates/rpc/src/server/api.rs @@ -1,3 +1,4 @@ +use std::num::NonZeroUsize; use std::sync::LazyLock; use std::time::Duration; @@ -25,6 +26,7 @@ use miden_node_utils::limiter::{ QueryParamNullifierLimit, QueryParamStorageMapKeyTotalLimit, }; +use miden_node_utils::lru_cache::LruCache; use miden_node_utils::tracing::OpenTelemetrySpanExt; use miden_protocol::batch::{ProposedBatch, ProvenBatch}; use miden_protocol::block::{BlockHeader, BlockNumber}; @@ -53,6 +55,7 @@ pub struct RpcService { validator: ValidatorClient, ntx_builder: Option, genesis_commitment: Option, + block_commitment_cache: LruCache, } impl RpcService { @@ -61,6 +64,7 @@ impl RpcService { block_producer_url: Option, validator_url: Url, ntx_builder_url: Option, + commitment_cache_capacity: NonZeroUsize, ) -> Self { let store = { info!(target: COMPONENT, store_endpoint = %store_url, "Initializing store client"); @@ -124,6 +128,7 @@ impl RpcService { validator, ntx_builder, genesis_commitment: None, + block_commitment_cache: LruCache::new(commitment_cache_capacity), } } @@ -186,6 +191,52 @@ impl RpcService { } } } + + /// Returns the given block's onchain commitment. + /// + /// This is retrieved from the local LRU cache, or otherwise from the store on cache miss. + #[tracing::instrument(target = COMPONENT, name = "get_block_commitment", skip_all, fields(block.number = %block))] + async fn get_block_commitment(&self, block: BlockNumber) -> Result { + if let Some(commitment) = self.block_commitment_cache.get(&block).await { + return Ok(commitment); + } + + let header = self + .store + .clone() + .get_block_header_by_number(Request::new(proto::rpc::BlockHeaderByNumberRequest { + block_num: Some(block.as_u32()), + include_mmr_proof: false.into(), + })) + .await? + .into_inner() + .block_header + .map(BlockHeader::try_from) + .transpose()? + .ok_or_else(|| Status::invalid_argument(format!("unknown block {block}")))?; + + let commitment = header.commitment(); + self.block_commitment_cache.put(block, commitment).await; + + Ok(commitment) + } + + /// Returns an error if the provided block's commitment does not match the one on chain. + async fn verify_reference_commitment( + &self, + block: BlockNumber, + commitment: Word, + ) -> Result<(), Status> { + let onchain = self.get_block_commitment(block).await?; + + if onchain != commitment { + return Err(Status::invalid_argument(format!( + "reference block's commitment {commitment} at block {block} does not match the chain's commitment of {onchain}", + ))); + } + + Ok(()) + } } // API IMPLEMENTATION @@ -430,6 +481,10 @@ impl api_server::Api for RpcService { span.set_attribute("transaction.reference_block.number", tx.ref_block_num()); span.set_attribute("transaction.reference_block.commitment", tx.ref_block_commitment()); + // Verify the reference block is actually part of the chain. + self.verify_reference_commitment(tx.ref_block_num(), tx.ref_block_commitment()) + .await?; + // Rebuild a new ProvenTransaction with decorators removed from output notes let account_update = TxAccountUpdate::new( tx.account_id(), @@ -465,7 +520,6 @@ impl api_server::Api for RpcService { } let tx_verifier = TransactionVerifier::new(MIN_PROOF_SECURITY_LEVEL); - tx_verifier.verify(&tx).map_err(|err| { Status::invalid_argument(format!( "Invalid proof for transaction {}: {}", @@ -520,6 +574,13 @@ impl api_server::Api for RpcService { })? .ok_or(Status::invalid_argument("missing `proposed_batch` field"))?; + // Verify the reference block is actually part of the chain. + self.verify_reference_commitment( + proven_batch.reference_block_num(), + proven_batch.reference_block_commitment(), + ) + .await?; + // Perform this check here since its cheap. If this passes we can safely zip inputs and // transactions. if request.transaction_inputs.len() != proposed_batch.transactions().len() { @@ -555,32 +616,6 @@ impl api_server::Api for RpcService { return Err(Status::invalid_argument("batch proof did not match proposed batch")); } - // Verify the reference header matches the canonical chain. - let reference_header = self - .get_block_header_by_number(Request::new(proto::rpc::BlockHeaderByNumberRequest { - block_num: expected_proof.reference_block_num().as_u32().into(), - include_mmr_proof: false.into(), - })) - .await? - .into_inner() - .block_header - .map(BlockHeader::try_from) - .transpose()? - .ok_or_else(|| { - Status::invalid_argument(format!( - "unknown reference block {}", - expected_proof.reference_block_num() - )) - })?; - if reference_header.commitment() != expected_proof.reference_block_commitment() { - return Err(Status::invalid_argument(format!( - "batch reference commitment {} at block {} does not match canonical chain's commitment of {}", - expected_proof.reference_block_commitment(), - expected_proof.reference_block_num(), - reference_header.commitment() - ))); - } - // Submit each transaction to the validator. // // SAFETY: We checked earlier that the two iterators are the same length. diff --git a/crates/rpc/src/server/mod.rs b/crates/rpc/src/server/mod.rs index 2e4c1caa7..f788a8364 100644 --- a/crates/rpc/src/server/mod.rs +++ b/crates/rpc/src/server/mod.rs @@ -1,3 +1,5 @@ +use std::num::NonZeroUsize; + use accept::AcceptHeaderLayer; use anyhow::Context; use miden_node_proto::generated::rpc::api_server; @@ -48,6 +50,7 @@ impl Rpc { self.block_producer_url.clone(), self.validator_url, self.ntx_builder_url.clone(), + NonZeroUsize::new(1_000_000).unwrap(), ); let genesis = api diff --git a/crates/rpc/src/tests.rs b/crates/rpc/src/tests.rs index 794d9b604..29613be0d 100644 --- a/crates/rpc/src/tests.rs +++ b/crates/rpc/src/tests.rs @@ -66,7 +66,11 @@ fn build_test_account(seed: [u8; 32]) -> (Account, AccountDelta) { /// /// This uses `ExecutionProof::new_dummy()` and is intended for tests that /// need to test validation logic. -fn build_test_proven_tx(account: &Account, delta: &AccountDelta) -> ProvenTransaction { +fn build_test_proven_tx( + account: &Account, + delta: &AccountDelta, + genesis: Word, +) -> ProvenTransaction { let account_id = AccountId::dummy( [0; 15], AccountIdVersion::Version0, @@ -88,7 +92,7 @@ fn build_test_proven_tx(account: &Account, delta: &AccountDelta) -> ProvenTransa Vec::::new(), Vec::::new(), 0.into(), - Word::default(), + genesis, test_fee(), u32::MAX.into(), ExecutionProof::new_dummy(), @@ -305,7 +309,7 @@ async fn rpc_server_rejects_proven_transactions_with_invalid_commitment() { // Build a valid proven transaction let (account, account_delta) = build_test_account([0; 32]); - let tx = build_test_proven_tx(&account, &account_delta); + let tx = build_test_proven_tx(&account, &account_delta, genesis); // Create an incorrect delta commitment from a different account let (other_account, _) = build_test_account([1; 32]); @@ -338,11 +342,56 @@ async fn rpc_server_rejects_proven_transactions_with_invalid_commitment() { shutdown_store(store_runtime).await; } +#[tokio::test] +async fn rpc_server_rejects_proven_transactions_with_invalid_reference_block() { + // Start the RPC. + let (_, rpc_addr, store_listener) = start_rpc().await; + let (store_runtime, _data_directory, genesis, _store_addr) = start_store(store_listener).await; + + // Wait for the store to be ready before sending requests. + tokio::time::sleep(Duration::from_millis(100)).await; + + // Override the client so that the ACCEPT header is not set. + let mut rpc_client = + miden_node_proto::clients::Builder::new(Url::parse(&format!("http://{rpc_addr}")).unwrap()) + .without_tls() + .with_timeout(Duration::from_secs(5)) + .without_metadata_version() + .with_metadata_genesis(genesis.to_hex()) + .without_otel_context_injection() + .connect_lazy::(); + + // Build a valid proven transaction but with the incorrect hash (empty). + let invalid = Word::empty(); + let (account, account_delta) = build_test_account([0; 32]); + let tx = build_test_proven_tx(&account, &account_delta, invalid); + + let request = proto::transaction::ProvenTransaction { + transaction: tx.to_bytes(), + transaction_inputs: None, + }; + + let response = rpc_client.submit_proven_transaction(request).await; + + // Assert that the server rejected our request. + assert!(response.is_err()); + + // Rejection should be from invalid reference block. + let err = response.as_ref().unwrap_err().message(); + assert!( + err.contains("does not match the chain's commitment of"), + "expected error message to contain reference block error but got: {err}" + ); + + // Shutdown to avoid runtime drop error. + shutdown_store(store_runtime).await; +} + #[tokio::test] async fn rpc_server_rejects_tx_submissions_without_genesis() { // Start the RPC. let (_, rpc_addr, store_listener) = start_rpc().await; - let (store_runtime, _data_directory, _genesis, _store_addr) = start_store(store_listener).await; + let (store_runtime, _data_directory, genesis, _store_addr) = start_store(store_listener).await; // Override the client so that the ACCEPT header is not set. let mut rpc_client = @@ -355,7 +404,7 @@ async fn rpc_server_rejects_tx_submissions_without_genesis() { .connect_lazy::(); let (account, account_delta) = build_test_account([0; 32]); - let tx = build_test_proven_tx(&account, &account_delta); + let tx = build_test_proven_tx(&account, &account_delta, genesis); let request = proto::transaction::ProvenTransaction { transaction: tx.to_bytes(),