From 7328c0fdd9d21cf596abb75b8b311264bb5c089c Mon Sep 17 00:00:00 2001 From: sistemd Date: Wed, 10 Apr 2024 14:23:57 +0200 Subject: [PATCH 1/2] pathfinder_getProof rpc error if proof missing --- crates/merkle-tree/src/contract.rs | 8 +-- crates/merkle-tree/src/tree.rs | 13 ++-- crates/rpc/src/context.rs | 15 +++- crates/rpc/src/error.rs | 4 ++ crates/rpc/src/lib.rs | 4 +- .../rpc/src/pathfinder/methods/get_proof.rs | 70 +++++++++++++++---- 6 files changed, 85 insertions(+), 29 deletions(-) diff --git a/crates/merkle-tree/src/contract.rs b/crates/merkle-tree/src/contract.rs index 4813267ed3..7a42a9731c 100644 --- a/crates/merkle-tree/src/contract.rs +++ b/crates/merkle-tree/src/contract.rs @@ -74,13 +74,13 @@ impl<'tx> ContractsStorageTree<'tx> { contract: ContractAddress, block: BlockNumber, key: &BitSlice, - ) -> anyhow::Result> { + ) -> anyhow::Result>> { let root = tx .contract_root_index(block, contract) .context("Querying contract root index")?; let Some(root) = root else { - return Ok(Vec::new()); + return Ok(None); }; let storage = ContractStorage { @@ -183,13 +183,13 @@ impl<'tx> StorageCommitmentTree<'tx> { tx: &'tx Transaction<'tx>, block: BlockNumber, address: &ContractAddress, - ) -> anyhow::Result> { + ) -> anyhow::Result>> { let root = tx .storage_root_index(block) .context("Querying storage root index")?; let Some(root) = root else { - return Ok(Vec::new()); + return Ok(None); }; let storage = StorageTrieStorage { diff --git a/crates/merkle-tree/src/tree.rs b/crates/merkle-tree/src/tree.rs index 15ea772ceb..e6d1a53d8b 100644 --- a/crates/merkle-tree/src/tree.rs +++ b/crates/merkle-tree/src/tree.rs @@ -510,17 +510,16 @@ impl MerkleTree { root: u64, storage: &impl Storage, key: &BitSlice, - ) -> anyhow::Result> { + ) -> anyhow::Result>> { // Manually traverse towards the key. let mut nodes = Vec::new(); let mut next = Some(root); let mut height = 0; while let Some(index) = next.take() { - let node = storage - .get(index) - .context("Resolving node")? - .context("Node is missing from storage")?; + let Some(node) = storage.get(index).context("Resolving node")? else { + return Ok(None); + }; let node = match node { StoredNode::Binary { left, right } => { @@ -595,7 +594,7 @@ impl MerkleTree { nodes.push(node); } - Ok(nodes) + Ok(Some(nodes)) } /// Traverses from the current root towards destination node. @@ -1944,7 +1943,7 @@ mod tests { storage: &impl Storage, ) -> anyhow::Result>> { keys.iter() - .map(|k| TestTree::get_proof(root, storage, k)) + .map(|k| TestTree::get_proof(root, storage, k).map(Option::unwrap)) .collect() } diff --git a/crates/rpc/src/context.rs b/crates/rpc/src/context.rs index bf7c10f7eb..2237399bfd 100644 --- a/crates/rpc/src/context.rs +++ b/crates/rpc/src/context.rs @@ -60,8 +60,21 @@ impl RpcContext { Self::for_tests_on(pathfinder_common::Chain::SepoliaTestnet) } + #[cfg(test)] + pub fn for_tests_with_trie_pruning(trie_prune_mode: pathfinder_storage::TriePruneMode) -> Self { + Self::for_tests_impl(pathfinder_common::Chain::SepoliaTestnet, trie_prune_mode) + } + #[cfg(test)] pub fn for_tests_on(chain: pathfinder_common::Chain) -> Self { + Self::for_tests_impl(chain, pathfinder_storage::TriePruneMode::Archive) + } + + #[cfg(test)] + pub fn for_tests_impl( + chain: pathfinder_common::Chain, + trie_prune_mode: pathfinder_storage::TriePruneMode, + ) -> Self { use pathfinder_common::Chain; use starknet_gateway_client::test_utils::GATEWAY_TIMEOUT; @@ -78,7 +91,7 @@ impl RpcContext { Chain::Custom => unreachable!("Should not be testing with custom chain"), }; - let storage = super::test_utils::setup_storage(); + let storage = super::test_utils::setup_storage(trie_prune_mode); let sync_state = Arc::new(SyncState::default()); let (_, rx) = tokio_watch::channel(Default::default()); diff --git a/crates/rpc/src/error.rs b/crates/rpc/src/error.rs index b16a9c9c51..0f2af981a3 100644 --- a/crates/rpc/src/error.rs +++ b/crates/rpc/src/error.rs @@ -83,6 +83,8 @@ pub enum ApplicationError { transaction_index: usize, error: String, }, + #[error("Proof is missing")] + ProofMissing, /// Internal errors are errors whose details we don't want to show to the end user. /// These are logged, and a simple "internal error" message is shown to the end /// user. @@ -131,6 +133,7 @@ impl ApplicationError { ApplicationError::UnexpectedError { .. } => 63, // doc/rpc/pathfinder_rpc_api.json ApplicationError::ProofLimitExceeded { .. } => 10000, + ApplicationError::ProofMissing => 10001, // https://www.jsonrpc.org/specification#error_object ApplicationError::GatewayError(_) | ApplicationError::Internal(_) @@ -204,6 +207,7 @@ impl ApplicationError { "limit": limit, "requested": requested, })), + ApplicationError::ProofMissing => None, ApplicationError::ValidationFailureV06(error) => Some(json!(error)), } } diff --git a/crates/rpc/src/lib.rs b/crates/rpc/src/lib.rs index 803af3b3db..f58dfcb7c5 100644 --- a/crates/rpc/src/lib.rs +++ b/crates/rpc/src/lib.rs @@ -252,10 +252,10 @@ pub mod test_utils { use std::collections::HashMap; // Creates storage for tests - pub fn setup_storage() -> Storage { + pub fn setup_storage(trie_prune_mode: pathfinder_storage::TriePruneMode) -> Storage { use pathfinder_merkle_tree::contract_state::update_contract_state; - let storage = StorageBuilder::in_memory().unwrap(); + let storage = StorageBuilder::in_memory_with_trie_pruning(trie_prune_mode).unwrap(); let mut connection = storage.connection().unwrap(); let db_txn = connection.transaction().unwrap(); diff --git a/crates/rpc/src/pathfinder/methods/get_proof.rs b/crates/rpc/src/pathfinder/methods/get_proof.rs index c7ee1563d3..cb853a51d1 100644 --- a/crates/rpc/src/pathfinder/methods/get_proof.rs +++ b/crates/rpc/src/pathfinder/methods/get_proof.rs @@ -21,6 +21,7 @@ pub enum GetProofError { Internal(anyhow::Error), BlockNotFound, ProofLimitExceeded { limit: u32, requested: u32 }, + ProofMissing, } impl From for GetProofError { @@ -37,6 +38,7 @@ impl From for crate::error::ApplicationError { } GetProofError::BlockNotFound => Self::BlockNotFound, GetProofError::Internal(internal) => Self::Internal(internal), + GetProofError::ProofMissing => Self::ProofMissing, } } } @@ -193,7 +195,8 @@ pub async fn get_proof( // be a "non membership" proof. let contract_proof = StorageCommitmentTree::get_proof(&tx, header.number, &input.contract_address) - .context("Creating contract proof")?; + .context("Creating contract proof")? + .ok_or(GetProofError::ProofMissing)?; let contract_proof = ProofNodes(contract_proof); let contract_state_hash = tx @@ -224,20 +227,18 @@ pub async fn get_proof( .context("Querying contract's nonce")? .unwrap_or_default(); - let storage_proofs = input - .keys - .iter() - .map(|k| { - ContractsStorageTree::get_proof( - &tx, - input.contract_address, - header.number, - k.view_bits(), - ) - .map(ProofNodes) - }) - .collect::>>() - .context("Get proof from contract state treee")?; + let mut storage_proofs = Vec::new(); + for k in &input.keys { + let proof = ContractsStorageTree::get_proof( + &tx, + input.contract_address, + header.number, + k.view_bits(), + ) + .context("Get proof from contract state tree")? + .ok_or(GetProofError::ProofMissing)?; + storage_proofs.push(ProofNodes(proof)); + } let contract_data = ContractData { class_hash, @@ -278,4 +279,43 @@ mod tests { let err = get_proof(context, input).await.unwrap_err(); assert_matches::assert_matches!(err, GetProofError::ProofLimitExceeded { .. }); } + + #[tokio::test] + async fn proof_pruned() { + let context = + RpcContext::for_tests_with_trie_pruning(pathfinder_storage::TriePruneMode::Prune { + num_blocks_kept: 0, + }); + let mut conn = context.storage.connection().unwrap(); + let tx = conn.transaction().unwrap(); + + // Ensure that all storage tries are pruned, hence the node does not store historic proofs. + tx.insert_storage_trie( + &pathfinder_storage::TrieUpdate { + nodes_added: vec![(Felt::from_u64(0), pathfinder_storage::Node::LeafBinary)], + nodes_removed: (0..100).collect(), + }, + BlockNumber::GENESIS + 3, + ) + .unwrap(); + tx.commit().unwrap(); + let tx = conn.transaction().unwrap(); + tx.insert_storage_trie( + &pathfinder_storage::TrieUpdate { + nodes_added: vec![(Felt::from_u64(1), pathfinder_storage::Node::LeafBinary)], + nodes_removed: vec![], + }, + BlockNumber::GENESIS + 4, + ) + .unwrap(); + tx.commit().unwrap(); + + let input = GetProofInput { + block_id: BlockId::Latest, + contract_address: contract_address!("0xdeadbeef"), + keys: vec![storage_address_bytes!(b"storage addr 0")], + }; + let err = get_proof(context, input).await.unwrap_err(); + assert_matches::assert_matches!(err, GetProofError::ProofMissing); + } } From 0f0645aeaac80111d36f4d7b979eeafffca09629 Mon Sep 17 00:00:00 2001 From: sistemd Date: Thu, 11 Apr 2024 17:01:29 +0200 Subject: [PATCH 2/2] handle comments --- crates/rpc/src/pathfinder/methods/get_proof.rs | 9 ++++++++- doc/rpc/pathfinder_rpc_api.json | 4 ++++ 2 files changed, 12 insertions(+), 1 deletion(-) diff --git a/crates/rpc/src/pathfinder/methods/get_proof.rs b/crates/rpc/src/pathfinder/methods/get_proof.rs index cb853a51d1..d8f108ce58 100644 --- a/crates/rpc/src/pathfinder/methods/get_proof.rs +++ b/crates/rpc/src/pathfinder/methods/get_proof.rs @@ -236,7 +236,14 @@ pub async fn get_proof( k.view_bits(), ) .context("Get proof from contract state tree")? - .ok_or(GetProofError::ProofMissing)?; + .ok_or_else(|| { + let e = anyhow!( + "Storage proof missing for key {:?}, but should be present", + k + ); + tracing::warn!("{e}"); + e + })?; storage_proofs.push(ProofNodes(proof)); } diff --git a/doc/rpc/pathfinder_rpc_api.json b/doc/rpc/pathfinder_rpc_api.json index f6aed1180b..ac1ad08ca6 100644 --- a/doc/rpc/pathfinder_rpc_api.json +++ b/doc/rpc/pathfinder_rpc_api.json @@ -291,6 +291,10 @@ }, "required": ["limit", "requested"] } + }, + "PROOF_MISSING": { + "code": 10000, + "message": "Merkle trie proof is not available" } } }