feat(consensus): Prevalidate blocks in reducers not enabling conditions

tizoc · tizoc · commit 130290b47ec5 · 2024-11-23T10:27:30.000-03:00
Add more checks to prevalidation.
Also use error type for validation failure instead of boolean.
diff --git a/node/src/action_kind.rs b/node/src/action_kind.rs
@@ -162,6 +162,8 @@ pub enum ActionKind {
     CheckTimeouts,
     ConsensusBestTipUpdate,
     ConsensusBlockChainProofUpdate,
+    ConsensusBlockPrevalidateError,
+    ConsensusBlockPrevalidateSuccess,
     ConsensusBlockReceived,
     ConsensusBlockSnarkVerifyError,
     ConsensusBlockSnarkVerifyPending,
@@ -716,7 +718,7 @@ pub enum ActionKind {
 }
 
 impl ActionKind {
-    pub const COUNT: u16 = 599;
+    pub const COUNT: u16 = 601;
 }
 
 impl std::fmt::Display for ActionKind {
@@ -857,6 +859,8 @@ impl ActionKindGet for ConsensusAction {
     fn kind(&self) -> ActionKind {
         match self {
             Self::BlockReceived { .. } => ActionKind::ConsensusBlockReceived,
+            Self::BlockPrevalidateSuccess { .. } => ActionKind::ConsensusBlockPrevalidateSuccess,
+            Self::BlockPrevalidateError { .. } => ActionKind::ConsensusBlockPrevalidateError,
             Self::BlockChainProofUpdate { .. } => ActionKind::ConsensusBlockChainProofUpdate,
             Self::BlockSnarkVerifyPending { .. } => ActionKind::ConsensusBlockSnarkVerifyPending,
             Self::BlockSnarkVerifySuccess { .. } => ActionKind::ConsensusBlockSnarkVerifySuccess,
diff --git a/node/src/consensus/consensus_actions.rs b/node/src/consensus/consensus_actions.rs
@@ -9,6 +9,7 @@ use snark::block_verify::SnarkBlockVerifyError;
 
 use crate::consensus::ConsensusBlockStatus;
 use crate::snark::block_verify::SnarkBlockVerifyId;
+use crate::state::BlockPrevalidationError;
 
 pub type ConsensusActionWithMeta = redux::ActionWithMeta<ConsensusAction>;
 pub type ConsensusActionWithMetaRef<'a> = redux::ActionWithMeta<&'a ConsensusAction>;
@@ -24,6 +25,13 @@ pub enum ConsensusAction {
         block: Arc<MinaBlockBlockStableV2>,
         chain_proof: Option<(Vec<StateHash>, ArcBlockWithHash)>,
     },
+    BlockPrevalidateSuccess {
+        hash: StateHash,
+    },
+    BlockPrevalidateError {
+        hash: StateHash,
+        error: BlockPrevalidationError,
+    },
     BlockChainProofUpdate {
         hash: StateHash,
         chain_proof: (Vec<StateHash>, ArcBlockWithHash),
@@ -69,10 +77,14 @@ impl redux::EnablingCondition<crate::State> for ConsensusAction {
                     hash: hash.clone(),
                     block: block.clone()
                 };
-                !block.is_genesis()
-                    && !state.consensus.blocks.contains_key(hash)
-                    && state.prevalidate_block(&block)
+                !block.is_genesis() && !state.consensus.blocks.contains_key(hash)
             },
+            ConsensusAction::BlockPrevalidateSuccess { hash }
+            | ConsensusAction::BlockPrevalidateError { hash, .. } => state
+                .consensus
+                .blocks
+                .get(hash)
+                .map_or(false, |block| block.status.is_received()),
             ConsensusAction::BlockChainProofUpdate { hash, .. } => {
                 (state.consensus.best_tip.as_ref() == Some(hash)
                     && state.consensus.best_tip_chain_proof.is_none())
@@ -87,7 +99,7 @@ impl redux::EnablingCondition<crate::State> for ConsensusAction {
                     .consensus
                     .blocks
                     .get(hash)
-                    .map_or(false, |block| block.status.is_received())
+                    .map_or(false, |block| block.status.is_prevalidated())
                     && state.snark.block_verify.jobs.contains(*req_id)
             },
             ConsensusAction::BlockSnarkVerifySuccess { hash } => {
diff --git a/node/src/consensus/consensus_reducer.rs b/node/src/consensus/consensus_reducer.rs
@@ -1,5 +1,5 @@
 use openmina_core::{
-    block::BlockHash,
+    block::{ArcBlockWithHash, BlockHash},
     bug_condition,
     consensus::{is_short_range_fork, long_range_fork_take, short_range_fork_take},
 };
@@ -48,9 +48,33 @@ impl ConsensusState {
                 );
 
                 // Dispatch
+                let (dispatcher, state) = state_context.into_dispatcher_and_state();
+
+                let hash = hash.clone();
+                let block = ArcBlockWithHash {
+                    hash: hash.clone(),
+                    block: block.clone(),
+                };
+                match state.prevalidate_block(&block) {
+                    Ok(()) => {
+                        dispatcher.push(ConsensusAction::BlockPrevalidateSuccess { hash });
+                    }
+                    Err(error) => {
+                        dispatcher.push(ConsensusAction::BlockPrevalidateError { hash, error });
+                    }
+                }
+            }
+            ConsensusAction::BlockPrevalidateSuccess { hash } => {
+                let Some(block) = state.blocks.get_mut(hash) else {
+                    return;
+                };
+                block.status = ConsensusBlockStatus::Prevalidated;
+
+                // Dispatch
+                let block = (hash.clone(), block.block.clone()).into();
                 let dispatcher = state_context.into_dispatcher();
                 dispatcher.push(SnarkBlockVerifyAction::Init {
-                    block: (hash.clone(), block.clone()).into(),
+                    block,
                     on_init: redux::callback!(
                         on_received_block_snark_verify_init((hash: BlockHash, req_id: SnarkBlockVerifyId)) -> crate::Action {
                             ConsensusAction::BlockSnarkVerifyPending { hash, req_id }
@@ -65,6 +89,9 @@ impl ConsensusState {
                         }),
                 });
             }
+            ConsensusAction::BlockPrevalidateError { hash, .. } => {
+                state.blocks.remove(hash);
+            }
             ConsensusAction::BlockChainProofUpdate { hash, chain_proof } => {
                 if state.best_tip.as_ref() == Some(hash) {
                     state.best_tip_chain_proof = Some(chain_proof.clone());
diff --git a/node/src/consensus/consensus_state.rs b/node/src/consensus/consensus_state.rs
@@ -44,6 +44,7 @@ pub enum ConsensusBlockStatus {
     Received {
         time: redux::Timestamp,
     },
+    Prevalidated,
     SnarkVerifyPending {
         time: redux::Timestamp,
         req_id: SnarkBlockVerifyId,
@@ -73,6 +74,10 @@ impl ConsensusBlockStatus {
         matches!(self, Self::Received { .. })
     }
 
+    pub fn is_prevalidated(&self) -> bool {
+        matches!(self, Self::Prevalidated)
+    }
+
     pub fn is_snark_verify_pending(&self) -> bool {
         matches!(self, Self::SnarkVerifyPending { .. })
     }
@@ -167,6 +172,7 @@ impl ConsensusState {
         };
         match &candidate.status {
             ConsensusBlockStatus::Received { .. } => false,
+            ConsensusBlockStatus::Prevalidated => false,
             ConsensusBlockStatus::SnarkVerifyPending { .. } => false,
             ConsensusBlockStatus::SnarkVerifySuccess { .. } => false,
             ConsensusBlockStatus::ForkRangeDetected { .. } => false,
diff --git a/node/src/p2p/peer/p2p_peer_actions.rs b/node/src/p2p/peer/p2p_peer_actions.rs
@@ -2,11 +2,6 @@ use super::*;
 
 impl redux::EnablingCondition<crate::State> for P2pPeerAction {
     fn is_enabled(&self, state: &crate::State, time: redux::Timestamp) -> bool {
-        if let P2pPeerAction::BestTipUpdate { best_tip, .. } = self {
-            if !state.prevalidate_block(best_tip) {
-                return false;
-            }
-        }
         state.p2p.is_enabled(self, time)
     }
 }
diff --git a/node/src/state.rs b/node/src/state.rs
@@ -2,6 +2,7 @@ use std::sync::Arc;
 use std::time::Duration;
 
 use mina_p2p_messages::v2::{MinaBaseUserCommandStableV2, MinaBlockBlockStableV2};
+use openmina_core::constants::PROTOCOL_VERSION;
 use rand::prelude::*;
 
 use openmina_core::block::BlockWithHash;
@@ -73,8 +74,27 @@ pub struct State {
     applied_actions_count: u64,
 }
 
+#[derive(Serialize, Deserialize, Debug, Clone)]
+pub enum BlockPrevalidationError {
+    GenesisNotReady,
+    ReceivedTooEarly {
+        current_global_slot: u32,
+        block_global_slot: u32,
+    },
+    ReceivedTooLate {
+        current_global_slot: u32,
+        block_global_slot: u32,
+        delta: u32,
+    },
+    InvalidGenesisProtocolState,
+    InvalidProtocolVersion,
+    MismatchedProtocolVersion,
+    ConsantsMismatch,
+    InvalidDeltaBlockChainProof,
+}
+
 // Substate accessors that will be used in reducers
-use openmina_core::{impl_substate_access, SubstateAccess};
+use openmina_core::{bug_condition, impl_substate_access, SubstateAccess};
 
 impl_substate_access!(State, SnarkState, snark);
 impl_substate_access!(State, SnarkBlockVerifyState, snark.block_verify);
@@ -356,15 +376,19 @@ impl State {
         })
     }
 
-    pub fn prevalidate_block(&self, block: &ArcBlockWithHash) -> bool {
+    pub fn prevalidate_block(
+        &self,
+        block: &ArcBlockWithHash,
+    ) -> Result<(), BlockPrevalidationError> {
         let Some((genesis, cur_global_slot)) =
             None.or_else(|| Some((self.genesis_block()?, self.cur_global_slot()?)))
         else {
             // we don't have genesis block. This should be impossible
             // because we don't even know chain_id before we have genesis
             // block, so we can't be connected to any peers from which
             // we would receive a block.
-            return false;
+            bug_condition!("Tried to prevalidate a block before the genesis block was ready");
+            return Err(BlockPrevalidationError::GenesisNotReady);
         };
 
         // received_at_valid_time
@@ -375,23 +399,60 @@ impl State {
             let delta = genesis.constants().delta.as_u32();
             if cur_global_slot < block_global_slot {
                 // Too_early
-                return false;
+                return Err(BlockPrevalidationError::ReceivedTooEarly {
+                    current_global_slot: cur_global_slot,
+                    block_global_slot,
+                });
             } else if cur_global_slot.saturating_sub(block_global_slot) > delta {
                 // Too_late
-                return false;
+                return Err(BlockPrevalidationError::ReceivedTooLate {
+                    current_global_slot: cur_global_slot,
+                    block_global_slot,
+                    delta,
+                });
             }
         }
 
-        if block.constants() != genesis.constants() {
-            return false;
+        if block.header().genesis_state_hash() != genesis.hash() {
+            return Err(BlockPrevalidationError::InvalidGenesisProtocolState);
         }
 
-        if block.header().genesis_state_hash() != genesis.hash() {
-            return false;
+        let (protocol_versions_are_valid, protocol_version_matches_daemon) = {
+            let min_transaction_version = 1.into();
+            let v = &block.header().current_protocol_version;
+            let nv = block
+                .header()
+                .proposed_protocol_version_opt
+                .as_ref()
+                .unwrap_or(v);
+
+            // Our version values are unsigned, so there is no need to check that the
+            // other parts are not negative.
+            let valid = v.transaction >= min_transaction_version
+                && nv.transaction >= min_transaction_version;
+            let compatible = v.transaction == PROTOCOL_VERSION.transaction
+                && v.network == PROTOCOL_VERSION.network;
+
+            (valid, compatible)
+        };
+
+        if !protocol_versions_are_valid {
+            return Err(BlockPrevalidationError::InvalidProtocolVersion);
+        } else if !protocol_version_matches_daemon {
+            return Err(BlockPrevalidationError::MismatchedProtocolVersion);
+        }
+
+        // NOTE: currently these cannot change between blocks, but that
+        // may not always be true?
+        if block.constants() != genesis.constants() {
+            return Err(BlockPrevalidationError::ConsantsMismatch);
         }
 
-        // TODO(binier): more checks.
-        true
+        // TODO(tizoc): check for InvalidDeltaBlockChainProof
+        // https://github.com/MinaProtocol/mina/blob/d800da86a764d8d37ffb8964dd8d54d9f522b358/src/lib/mina_block/validation.ml#L369
+        // https://github.com/MinaProtocol/mina/blob/d800da86a764d8d37ffb8964dd8d54d9f522b358/src/lib/transition_chain_verifier/transition_chain_verifier.ml
+
+        Ok(())
     }
 
     pub fn should_log_node_id(&self) -> bool {

Original file line number	Diff line number	Diff line change
`@@ -2,11 +2,6 @@ use super::*;`
`2`	`2`
`3`	`3`	`impl redux::EnablingCondition<crate::State> for P2pPeerAction {`
`4`	`4`	`fn is_enabled(&self, state: &crate::State, time: redux::Timestamp) -> bool {`
`5`		`- if let P2pPeerAction::BestTipUpdate { best_tip, .. } = self {`
`6`		`- if !state.prevalidate_block(best_tip) {`
`7`		`- return false;`
`8`		`- }`
`9`		`- }`
`10`	`5`	`state.p2p.is_enabled(self, time)`
`11`	`6`	`}`
`12`	`7`	`}`