Merge pull request #985 from openmina/fix/tx-snark-verify

fix(txpool): Don't verify zkapp proofs in the main thread

Author: tizoc

ledger/src/transaction_pool.rs

@@ -15,7 +15,7 @@ use crate::{
         currency::{Amount, Balance, BlockTime, Fee, Magnitude, Nonce, Slot},
         fee_rate::FeeRate,
         transaction_logic::{
-            valid,
+            valid, verifiable,
             zkapp_command::{
                 from_unapplied_sequence::{self, FromUnappliedSequence},
                 MaybeWithStatus, WithHash,
@@ -2095,11 +2095,7 @@ impl TransactionPool {
         }
     }
 
-    pub fn verify(
-        &self,
-        diff: diff::Diff,
-        accounts: &BTreeMap<AccountId, Account>,
-    ) -> Result<Vec<valid::UserCommand>, TransactionPoolErrors> {
+    pub fn prevalidate(&self, diff: diff::Diff) -> Result<diff::Diff, TransactionPoolErrors> {
         let well_formedness_errors: HashSet<_> = diff
             .list
             .iter()
@@ -2118,6 +2114,14 @@ impl TransactionPool {
             ));
         }
 
+        Ok(diff)
+    }
+
+    pub fn convert_diff_to_verifiable(
+        &self,
+        diff: diff::Diff,
+        accounts: &BTreeMap<AccountId, Account>,
+    ) -> Result<Vec<WithStatus<verifiable::UserCommand>>, TransactionPoolErrors> {
         let cs = diff
             .list
             .iter()
@@ -2167,10 +2171,18 @@ impl TransactionPool {
             .into_iter()
             .map(|MaybeWithStatus { cmd, status: _ }| WithStatus {
                 data: cmd,
+                // TODO: is this correct?
                 status: Applied,
             })
             .collect::<Vec<_>>();
 
+        Ok(diff)
+    }
+
+    pub fn verify_proofs(
+        &self,
+        diff: Vec<WithStatus<verifiable::UserCommand>>,
+    ) -> Result<Vec<valid::UserCommand>, TransactionPoolErrors> {
         let (verified, invalid): (Vec<_>, Vec<_>) = Verifier
             .verify_commands(diff, None)
             .into_iter()

node/common/src/service/snarks.rs

@@ -1,7 +1,13 @@
 use std::sync::Arc;
 
 use ark_ff::fields::arithmetic::InvalidBigInt;
-use ledger::scan_state::scan_state::transaction_snark::{SokDigest, Statement};
+use ledger::{
+    scan_state::{
+        scan_state::transaction_snark::{SokDigest, Statement},
+        transaction_logic::WithStatus,
+    },
+    transaction_pool::{TransactionError, TransactionPoolErrors},
+};
 use mina_p2p_messages::v2;
 use node::{
     core::{
@@ -104,14 +110,39 @@ impl node::service::SnarkWorkVerifyService for NodeService {
 impl node::service::SnarkUserCommandVerifyService for NodeService {
     fn verify_init(
         &mut self,
-        _req_id: node::snark::user_command_verify::SnarkUserCommandVerifyId,
-        _verifier_index: TransactionVerifier,
-        _verifier_srs: Arc<VerifierSRS>,
-        _commands: mina_p2p_messages::list::List<
-            mina_p2p_messages::v2::MinaBaseUserCommandStableV2,
-        >,
+        req_id: node::snark::user_command_verify::SnarkUserCommandVerifyId,
+        commands: Vec<WithStatus<ledger::scan_state::transaction_logic::verifiable::UserCommand>>,
     ) {
-        todo!()
+        if self.replayer.is_some() {
+            return;
+        }
+
+        let tx = self.event_sender().clone();
+        let result = {
+            let (verified, invalid): (Vec<_>, Vec<_>) = ledger::verifier::Verifier
+                .verify_commands(commands, None)
+                .into_iter()
+                .partition(Result::is_ok);
+
+            let verified: Vec<_> = verified.into_iter().map(Result::unwrap).collect();
+            let invalid: Vec<_> = invalid.into_iter().map(Result::unwrap_err).collect();
+
+            if !invalid.is_empty() {
+                let transaction_pool_errors = invalid
+                    .into_iter()
+                    .map(TransactionError::Verifier)
+                    .collect();
+                Err(TransactionPoolErrors::BatchedErrors(
+                    transaction_pool_errors,
+                ))
+            } else {
+                Ok(verified)
+            }
+        };
+
+        let result = result.map_err(|err| err.to_string());
+
+        let _ = tx.send(SnarkEvent::UserCommandVerify(req_id, result).into());
     }
 }
 

node/src/action_kind.rs

@@ -603,6 +603,7 @@ pub enum ActionKind {
     TransactionPoolStartVerify,
     TransactionPoolStartVerifyWithAccounts,
     TransactionPoolVerifyError,
+    TransactionPoolVerifySuccess,
     TransactionPoolCandidateFetchAll,
     TransactionPoolCandidateFetchError,
     TransactionPoolCandidateFetchInit,
@@ -706,7 +707,7 @@ pub enum ActionKind {
 }
 
 impl ActionKind {
-    pub const COUNT: u16 = 596;
+    pub const COUNT: u16 = 597;
 }
 
 impl std::fmt::Display for ActionKind {
@@ -916,6 +917,7 @@ impl ActionKindGet for TransactionPoolAction {
             Self::StartVerifyWithAccounts { .. } => {
                 ActionKind::TransactionPoolStartVerifyWithAccounts
             }
+            Self::VerifySuccess { .. } => ActionKind::TransactionPoolVerifySuccess,
             Self::VerifyError { .. } => ActionKind::TransactionPoolVerifyError,
             Self::BestTipChanged { .. } => ActionKind::TransactionPoolBestTipChanged,
             Self::BestTipChangedWithAccounts { .. } => {

node/src/event_source/event_source_effects.rs

@@ -290,13 +290,13 @@ pub fn event_source_effects<S: Service>(store: &mut Store<S>, action: EventSourc
                     }
                 },
                 SnarkEvent::UserCommandVerify(req_id, result) => {
-                    if result.iter().any(|res| res.is_err()) {
+                    if let Ok(commands) = result {
+                        store.dispatch(SnarkUserCommandVerifyAction::Success { req_id, commands });
+                    } else {
                         store.dispatch(SnarkUserCommandVerifyAction::Error {
                             req_id,
                             error: SnarkUserCommandVerifyError::VerificationFailed,
                         });
-                    } else {
-                        store.dispatch(SnarkUserCommandVerifyAction::Success { req_id });
                     }
                 }
             },

node/src/logger/logger_effects.rs

@@ -117,6 +117,7 @@ pub fn logger_effects<S: Service>(store: &Store<S>, action: ActionWithMetaRef<'_
         Action::ExternalSnarkWorker(action) => action.action_event(&context),
         Action::SnarkPool(action) => action.action_event(&context),
         Action::Snark(SnarkAction::WorkVerify(a)) => a.action_event(&context),
+        Action::Snark(SnarkAction::UserCommandVerify(a)) => a.action_event(&context),
         Action::Consensus(a) => a.action_event(&context),
         Action::TransitionFrontier(a) => match a {
             TransitionFrontierAction::Synced { .. } => {

node/src/state.rs

@@ -3,7 +3,7 @@ use std::time::Duration;
 
 use mina_p2p_messages::v2;
 use openmina_core::constants::PROTOCOL_VERSION;
-use openmina_core::transaction::{Transaction, TransactionInfo, TransactionWithHash};
+use openmina_core::transaction::{TransactionInfo, TransactionWithHash};
 use rand::prelude::*;
 
 use openmina_core::block::BlockWithHash;
@@ -534,10 +534,7 @@ impl P2p {
             on_p2p_channels_transaction_libp2p_received: Some(redux::callback!(
                 on_p2p_channels_transaction_libp2p_received(transaction: Box<TransactionWithHash>) -> crate::Action {
                     TransactionPoolAction::StartVerify {
-                        commands: std::iter::once(*transaction)
-                            // TODO(SEC): remove once proof verification is done outside state machine.
-                            .filter(|v| !matches!(v.body(), Transaction::ZkappCommand(_)))
-                            .collect(),
+                        commands: std::iter::once(*transaction).collect(),
                         from_rpc: None
                     }
                 }

node/src/transaction_pool/transaction_pool_actions.rs

@@ -1,6 +1,7 @@
 use std::collections::{BTreeMap, BTreeSet};
 
 use ledger::{
+    scan_state::transaction_logic::valid,
     transaction_pool::{
         diff::{self, BestTipDiff, DiffVerified},
         ValidCommandWithHash,
@@ -30,6 +31,10 @@ pub enum TransactionPoolAction {
         pending_id: PendingId,
         from_rpc: Option<RpcId>,
     },
+    VerifySuccess {
+        valids: Vec<valid::UserCommand>,
+        from_rpc: Option<RpcId>,
+    },
     #[action_event(level = warn, fields(debug(errors)))]
     VerifyError {
         errors: Vec<String>,

node/src/transaction_pool/transaction_pool_reducer.rs

@@ -1,5 +1,5 @@
 use ledger::{
-    scan_state::transaction_logic::{GenericCommand, UserCommand},
+    scan_state::transaction_logic::{valid, GenericCommand, UserCommand},
     transaction_pool::{
         diff::{self, DiffVerified},
         transaction_hash, ApplyDecision, TransactionPoolErrors,
@@ -14,6 +14,7 @@ use openmina_core::{
 };
 use p2p::channels::transaction::P2pChannelsTransactionAction;
 use redux::callback;
+use snark::user_command_verify::{SnarkUserCommandVerifyAction, SnarkUserCommandVerifyId};
 use std::collections::{BTreeMap, BTreeSet};
 
 use crate::{BlockProducerAction, RpcAction};
@@ -107,21 +108,38 @@ impl TransactionPoolState {
                 };
                 let diff = diff::Diff { list: commands };
 
-                match substate.pool.verify(diff, accounts) {
-                    Ok(valids) => {
-                        let valids = valids
-                            .into_iter()
-                            .map(transaction_hash::hash_command)
-                            .collect::<Vec<_>>();
-                        let best_tip_hash = substate.best_tip_hash.clone().unwrap();
-                        let diff = DiffVerified { list: valids };
+                match substate
+                    .pool
+                    .prevalidate(diff)
+                    .and_then(|diff| substate.pool.convert_diff_to_verifiable(diff, accounts))
+                {
+                    Ok(verifiable) => {
+                        let (dispatcher, global_state) = state.into_dispatcher_and_state();
+                        let req_id = global_state.snark.user_command_verify.next_req_id();
 
-                        let dispatcher = state.into_dispatcher();
-                        dispatcher.push(TransactionPoolAction::ApplyVerifiedDiff {
-                            best_tip_hash,
-                            diff,
-                            is_sender_local: from_rpc.is_some(),
+                        dispatcher.push(SnarkUserCommandVerifyAction::Init {
+                            req_id,
+                            commands: verifiable,
                             from_rpc: *from_rpc,
+                            on_success: callback!(
+                                on_snark_user_command_verify_success(
+                                    (req_id: SnarkUserCommandVerifyId, valids: Vec<valid::UserCommand>, from_rpc: Option<RpcId>)
+                                ) -> crate::Action {
+                                    TransactionPoolAction::VerifySuccess {
+                                        valids,
+                                        from_rpc,
+                                    }
+                                }
+                            ),
+                            on_error: callback!(
+                                on_snark_user_command_verify_error(
+                                    (req_id: SnarkUserCommandVerifyId, errors: Vec<String>)
+                                ) -> crate::Action {
+                                    TransactionPoolAction::VerifyError {
+                                        errors
+                                    }
+                                }
+                            )
                         });
                     }
                     Err(e) => {
@@ -151,6 +169,23 @@ impl TransactionPoolState {
                     }
                 }
             }
+            TransactionPoolAction::VerifySuccess { valids, from_rpc } => {
+                let valids = valids
+                    .iter()
+                    .cloned()
+                    .map(transaction_hash::hash_command)
+                    .collect::<Vec<_>>();
+                let best_tip_hash = substate.best_tip_hash.clone().unwrap();
+                let diff = DiffVerified { list: valids };
+
+                let dispatcher = state.into_dispatcher();
+                dispatcher.push(TransactionPoolAction::ApplyVerifiedDiff {
+                    best_tip_hash,
+                    diff,
+                    is_sender_local: from_rpc.is_some(),
+                    from_rpc: *from_rpc,
+                });
+            }
             TransactionPoolAction::VerifyError { .. } => {
                 // just logging the errors
             }

node/testing/src/service/mod.rs

@@ -8,11 +8,11 @@ use std::{collections::BTreeMap, sync::Arc};
 use ledger::dummy::dummy_transaction_proof;
 use ledger::proofs::transaction::ProofError;
 use ledger::scan_state::scan_state::transaction_snark::SokMessage;
+use ledger::scan_state::transaction_logic::{verifiable, WithStatus};
 use ledger::Mask;
-use mina_p2p_messages::list::List;
 use mina_p2p_messages::string::ByteString;
 use mina_p2p_messages::v2::{
-    self, CurrencyFeeStableV1, LedgerHash, LedgerProofProdStableV2, MinaBaseProofStableV2,
+    CurrencyFeeStableV1, LedgerHash, LedgerProofProdStableV2, MinaBaseProofStableV2,
     MinaStateSnarkedLedgerStateWithSokStableV2, NonZeroCurvePoint,
     ProverExtendBlockchainInputStableV2, SnarkWorkerWorkerRpcsVersionedGetWorkV2TResponseA0Single,
     StateHash, TransactionSnarkStableV2, TransactionSnarkWorkTStableV2Proofs,
@@ -451,17 +451,9 @@ impl SnarkUserCommandVerifyService for NodeTestingService {
     fn verify_init(
         &mut self,
         req_id: SnarkUserCommandVerifyId,
-        verifier_index: TransactionVerifier,
-        verifier_srs: Arc<VerifierSRS>,
-        commands: List<v2::MinaBaseUserCommandStableV2>,
+        commands: Vec<WithStatus<verifiable::UserCommand>>,
     ) {
-        SnarkUserCommandVerifyService::verify_init(
-            &mut self.real,
-            req_id,
-            verifier_index,
-            verifier_srs,
-            commands,
-        )
+        SnarkUserCommandVerifyService::verify_init(&mut self.real, req_id, commands)
     }
 }
 

snark/src/snark_event.rs

@@ -11,7 +11,7 @@ pub enum SnarkEvent {
     WorkVerify(SnarkWorkVerifyId, Result<(), SnarkWorkVerifyError>),
     UserCommandVerify(
         SnarkUserCommandVerifyId,
-        Vec<Result<valid::UserCommand, String>>,
+        Result<Vec<valid::UserCommand>, String>,
     ),
 }
 
@@ -33,13 +33,9 @@ impl std::fmt::Display for SnarkEvent {
                 write!(f, "WorkVerify, {id}, {}", res_kind(res))
             }
             Self::UserCommandVerify(id, res) => {
-                let n_failed = res.iter().filter(|res| res.is_err()).count();
-                let n_success = res.len() - n_failed;
-                write!(
-                    f,
-                    "UserCommandVerify, {id}, n_success={} n_failed={}",
-                    n_success, n_failed
-                )
+                //let n_failed = res.iter().filter(|res| res.is_err()).count();
+                //let n_success = res.len() - n_failed;
+                write!(f, "UserCommandVerify, {id}, success={}", res.is_err())
             }
         }
     }