Merge branch 'grarco/opt-validation-calls' (#3473)

* origin/grarco/opt-validation-calls:
  Changelog #3473
  Renames process proposal cache
  LRU process proposal cache
  Custom type for process proposal result cache
  Cache of process proposal uses results
  Refactors process proposal check in a separate function
  Avoids full clone of the entire block in process proposal
  Cache check in `try_recheck_process_proposal`
  Cache result of `process_proposal`
  New type for process proposal rechecks
  Removes redundant `votes` field from `FinalizeBlock` request
  Rechecks with `process_proposal` instead of `process_txs`

.changelog/unreleased/improvements/3473-opt-validation-calls.md

@@ -0,0 +1,4 @@
+- Modified rechecks of process proposal to actually use `process_proposal`
+  instead of `process_txs`. Added a caching mechanism to avoid
+  running the check for a given proposed block more than once.
+  ([\#3473](https://github.com/anoma/namada/pull/3473))

crates/core/src/hash.rs

@@ -29,6 +29,8 @@ pub enum Error {
     ConversionFailed(std::array::TryFromSliceError),
     #[error("Failed to convert string into a hash: {0}")]
     FromStringError(data_encoding::DecodeError),
+    #[error("Cannot convert an empty CometBFT hash")]
+    FromCometError,
 }
 
 /// Result for functions that may fail
@@ -178,6 +180,17 @@ impl From<Hash> for crate::tendermint::Hash {
     }
 }
 
+impl TryFrom<crate::tendermint::Hash> for Hash {
+    type Error = self::Error;
+
+    fn try_from(value: crate::tendermint::Hash) -> Result<Self, Self::Error> {
+        match value {
+            crate::tendermint::Hash::Sha256(hash) => Ok(Self(hash)),
+            crate::tendermint::Hash::None => Err(Error::FromCometError),
+        }
+    }
+}
+
 impl From<Hash> for H256 {
     fn from(hash: Hash) -> Self {
         hash.0.into()

crates/node/src/lib.rs

@@ -35,12 +35,10 @@ use futures::future::TryFutureExt;
 use namada::core::storage::BlockHeight;
 use namada::core::time::DateTimeUtc;
 use namada::eth_bridge::ethers::providers::{Http, Provider};
-use namada::key::tm_raw_hash_to_string;
-use namada::ledger::pos::find_validator_by_raw_hash;
-use namada::state::DB;
+use namada::state::{ProcessProposalCachedResult, DB};
 use namada::storage::DbColFam;
 use namada::tendermint::abci::request::CheckTxKind;
-use namada::tx::data::ResultCode;
+use namada::tendermint::abci::response::ProcessProposal;
 use namada_apps_lib::cli::args;
 use namada_apps_lib::config::utils::{
     convert_tm_addr_to_socket_addr, num_of_threads,
@@ -128,8 +126,33 @@ impl Shell {
             Request::ProcessProposal(block) => {
                 tracing::debug!("Request ProcessProposal");
                 // TODO: use TM domain type in the handler
-                let (response, _tx_results) =
+                // NOTE: make sure to put any checks inside process_proposal
+                // since that function is called in other places to rerun the
+                // checks if (when) needed. Every check living outside that
+                // function will not be correctly replicated in the other
+                // locations
+                let block_hash = block.hash.try_into();
+                let (response, tx_results) =
                     self.process_proposal(block.into());
+                // Cache the response in case of future calls from Namada. If
+                // hash conversion fails avoid caching
+                if let Ok(block_hash) = block_hash {
+                    let result = if let ProcessProposal::Accept = response {
+                        ProcessProposalCachedResult::Accepted(
+                            tx_results
+                                .into_iter()
+                                .map(|res| res.into())
+                                .collect(),
+                        )
+                    } else {
+                        ProcessProposalCachedResult::Rejected
+                    };
+
+                    self.state
+                        .in_mem_mut()
+                        .block_proposals_cache
+                        .put(block_hash, result);
+                }
                 Ok(Response::ProcessProposal(response))
             }
             Request::RevertProposal(_req) => {
@@ -173,9 +196,9 @@ impl Shell {
     }
 
     // Checks if a run of process proposal is required before finalize block
-    // (recheck) and, in case, performs it
+    // (recheck) and, in case, performs it. Clears the cache before returning
     fn try_recheck_process_proposal(
-        &self,
+        &mut self,
         finalize_req: &shims::abcipp_shim_types::shim::request::FinalizeBlock,
     ) -> Result<(), Error> {
         let recheck_process_proposal = match self.mode {
@@ -193,37 +216,42 @@ impl Shell {
         };
 
         if recheck_process_proposal {
-            let tm_raw_hash_string =
-                tm_raw_hash_to_string(&finalize_req.proposer_address);
-            let block_proposer =
-                find_validator_by_raw_hash(&self.state, tm_raw_hash_string)
-                    .unwrap()
-                    .expect(
-                        "Unable to find native validator address of block \
-                         proposer from tendermint raw hash",
-                    );
-
-            let check_result = self.process_txs(
-                &finalize_req
-                    .txs
-                    .iter()
-                    .map(|ptx| ptx.tx.clone())
-                    .collect::<Vec<_>>(),
-                finalize_req.header.time,
-                &block_proposer,
-            );
-
-            let invalid_txs = check_result.iter().any(|res| {
-                let error = ResultCode::from_u32(res.code)
-                    .expect("Failed to cast result code");
-                !error.is_recoverable()
-            });
+            let process_proposal_result = match self
+                .state
+                .in_mem_mut()
+                .block_proposals_cache
+                .get(&finalize_req.block_hash)
+            {
+                // We already have the result of process proposal for this block
+                // cached in memory
+                Some(res) => res.to_owned(),
+                None => {
+                    let process_req = finalize_req
+                        .clone()
+                        .cast_to_process_proposal_req()
+                        .map_err(|_| Error::InvalidBlockProposal)?;
+                    // No need to cache the result since this is the last step
+                    // before finalizing the block
+                    if let ProcessProposal::Accept =
+                        self.process_proposal(process_req.into()).0
+                    {
+                        ProcessProposalCachedResult::Accepted(vec![])
+                    } else {
+                        ProcessProposalCachedResult::Rejected
+                    }
+                }
+            };
 
-            if invalid_txs {
-                return Err(Error::InvalidBlockProposal);
+            if let ProcessProposalCachedResult::Rejected =
+                process_proposal_result
+            {
+                return Err(Error::RejectedBlockProposal);
             }
         }
 
+        // Clear the cache of proposed blocks' results
+        self.state.in_mem_mut().block_proposals_cache.clear();
+
         Ok(())
     }
 }

crates/node/src/shell/finalize_block.rs

@@ -83,7 +83,8 @@ where
 
         let emit_events = &mut response.events;
         // Get the actual votes from cometBFT in the preferred format
-        let votes = pos_votes_from_abci(&self.state, &req.votes);
+        let votes =
+            pos_votes_from_abci(&self.state, &req.decided_last_commit.votes);
         let validator_set_update_epoch =
             self.get_validator_set_update_epoch(current_epoch);
 
@@ -1893,7 +1894,10 @@ mod test_finalize_block {
             let req = FinalizeBlock {
                 txs,
                 proposer_address: proposer_address.clone(),
-                votes: votes.clone(),
+                decided_last_commit: tendermint::abci::types::CommitInfo {
+                    round: 0u8.into(),
+                    votes: votes.clone(),
+                },
                 ..Default::default()
             };
             // merkle tree root before finalize_block

crates/node/src/shell/mod.rs

@@ -126,6 +126,11 @@ pub enum Error {
     ReplayAttempt(String),
     #[error("Error with snapshots: {0}")]
     Snapshot(std::io::Error),
+    #[error(
+        "Received a finalize request for a block that was rejected by process \
+         proposal"
+    )]
+    RejectedBlockProposal,
     #[error("Received an invalid block proposal")]
     InvalidBlockProposal,
 }
@@ -1902,6 +1907,7 @@ mod test_utils {
                     time: DateTimeUtc::now(),
                     next_validators_hash: Hash([0; 32]),
                 },
+                block_hash: Hash([0; 32]),
                 byzantine_validators: vec![],
                 txs: vec![],
                 proposer_address: HEXUPPER
@@ -1912,7 +1918,11 @@ mod test_utils {
                             .as_bytes(),
                     )
                     .unwrap(),
-                votes: vec![],
+                height: 0u8.into(),
+                decided_last_commit: tendermint::abci::types::CommitInfo {
+                    round: 0u8.into(),
+                    votes: vec![],
+                },
             }
         }
     }
@@ -1959,7 +1969,10 @@ mod test_utils {
         let mut req = FinalizeBlock {
             header,
             proposer_address,
-            votes,
+            decided_last_commit: tendermint::abci::types::CommitInfo {
+                round: 0u8.into(),
+                votes,
+            },
             ..Default::default()
         };
         if let Some(byz_vals) = byzantine_validators {

crates/node/src/shell/prepare_proposal.rs

@@ -687,7 +687,10 @@ mod test_prepare_proposal {
         ];
         let req = FinalizeBlock {
             proposer_address: pkh1.to_vec(),
-            votes,
+            decided_last_commit: namada::tendermint::abci::types::CommitInfo {
+                round: 0u8.into(),
+                votes,
+            },
             ..Default::default()
         };
         shell.start_new_epoch(Some(req));

crates/node/src/shell/queries.rs

@@ -163,7 +163,10 @@ mod test_queries {
                     }];
                     let req = FinalizeBlock {
                         proposer_address: pkh1.to_vec(),
-                        votes,
+                        decided_last_commit: namada::tendermint::abci::types::CommitInfo{
+                            round: 0u8.into(),
+                            votes
+                        },
                         ..Default::default()
                     };
                     shell.finalize_and_commit(Some(req));

crates/node/src/shell/testing/node.rs

@@ -475,10 +475,15 @@ impl MockNode {
                 time: DateTimeUtc::now(),
                 next_validators_hash: Hash([0; 32]),
             },
+            block_hash: Hash([0; 32]),
             byzantine_validators: vec![],
             txs: txs.clone(),
             proposer_address,
-            votes,
+            height: height.try_into().unwrap(),
+            decided_last_commit: tendermint::abci::types::CommitInfo {
+                round: 0u8.into(),
+                votes,
+            },
         };
 
         let resp = locked.finalize_block(req).expect("Test failed");
@@ -589,6 +594,7 @@ impl MockNode {
                 time: DateTimeUtc::now(),
                 next_validators_hash: Hash([0; 32]),
             },
+            block_hash: Hash([0; 32]),
             byzantine_validators: vec![],
             txs: txs
                 .clone()
@@ -600,7 +606,11 @@ impl MockNode {
                 })
                 .collect(),
             proposer_address,
-            votes,
+            height: height.try_into().unwrap(),
+            decided_last_commit: tendermint::abci::types::CommitInfo {
+                round: 0u8.into(),
+                votes,
+            },
         };
 
         // process the results

crates/node/src/shell/vote_extensions/bridge_pool_vext.rs

@@ -145,7 +145,10 @@ mod test_bp_vote_extensions {
         }];
         let req = FinalizeBlock {
             proposer_address: pkh1.to_vec(),
-            votes,
+            decided_last_commit: namada::tendermint::abci::types::CommitInfo {
+                round: 0u8.into(),
+                votes,
+            },
             ..Default::default()
         };
         assert_eq!(shell.start_new_epoch(Some(req)).0, 1);

crates/node/src/shell/vote_extensions/eth_events.rs

@@ -471,7 +471,10 @@ mod test_vote_extensions {
         }];
         let req = FinalizeBlock {
             proposer_address: pkh1.to_vec(),
-            votes,
+            decided_last_commit: namada::tendermint::abci::types::CommitInfo {
+                round: 0u8.into(),
+                votes,
+            },
             ..Default::default()
         };
         assert_eq!(shell.start_new_epoch(Some(req)).0, 1);

crates/node/src/shell/vote_extensions/val_set_update.rs

@@ -323,7 +323,10 @@ mod test_vote_extensions {
         }];
         let req = FinalizeBlock {
             proposer_address: pkh1.to_vec(),
-            votes,
+            decided_last_commit: namada::tendermint::abci::types::CommitInfo {
+                round: 0u8.into(),
+                votes,
+            },
             ..Default::default()
         };
         assert_eq!(shell.start_new_epoch(Some(req)).0, 1);