Merge pull request #597 from openmina/feat/tx-pool-error-handling

Improve tx-pool error handling

Author: tizoc

ledger/src/scan_state/transaction_logic.rs

@@ -4181,6 +4181,12 @@ pub mod zkapp_command {
                 cache
                     .find(account_id, &vk_hash)
                     .cloned()
+                    .or_else(|| {
+                        cmd.extract_vks()
+                            .iter()
+                            .find(|(id, _)| account_id == id)
+                            .map(|(_, key)| key.clone())
+                    })
                     .ok_or_else(|| "verification key not found in cache".to_string())
             })?;
             if !is_failed {
@@ -4733,12 +4739,17 @@ impl UserCommand {
     }
 }
 
-#[derive(Debug, Clone, Hash, PartialEq, Eq)]
+#[derive(Debug, Clone, Hash, PartialEq, Eq, thiserror::Error)]
 pub enum WellFormednessError {
+    #[error("Insufficient Fee")]
     InsufficientFee,
+    #[error("Zero vesting period")]
     ZeroVestingPeriod,
+    #[error("Zkapp too big: {0}")]
     ZkappTooBig(String),
+    #[error("Transaction type disabled")]
     TransactionTypeDisabled,
+    #[error("Incompatible version")]
     IncompatibleVersion,
 }
 

ledger/src/staged_ledger/staged_ledger.rs

@@ -1222,30 +1222,6 @@ impl StagedLedger {
         verifier
             .verify_commands(cs, skip_verification)
             .into_iter()
-            .map(|x| {
-                use crate::verifier::VerifyCommandsResult::*;
-                match x {
-                    Valid(x) => Ok(x),
-                    InvalidKeys(invalid)
-                    | InvalidSignature(invalid)
-                    | MissingVerificationKey(invalid)
-                    | UnexpectedVerificationKey(invalid)
-                    | MismatchedVerificationKey(invalid)
-                    | MismatchedAuthorizationKind(invalid) => {
-                        Err(VerifierError::VerificationFailed(format!(
-                            "verification failed on command: {:?}",
-                            invalid
-                        )))
-                    }
-                    InvalidProof(invalid) => Err(VerifierError::VerificationFailed(format!(
-                        "verification failed on command: {:?}",
-                        invalid
-                    ))),
-                    ValidAssuming(_) => Err(VerifierError::VerificationFailed(
-                        "batch verification failed".to_string(),
-                    )),
-                }
-            })
             .collect()
     }
 

ledger/src/transaction_pool.rs

@@ -21,13 +21,37 @@ use crate::{
                 MaybeWithStatus, WithHash,
             },
             TransactionStatus::Applied,
-            UserCommand, WithStatus,
+            UserCommand, WellFormednessError, WithStatus,
         },
     },
-    verifier::Verifier,
+    verifier::{Verifier, VerifierError},
     Account, AccountId, BaseLedger, Mask, TokenId, VerificationKey,
 };
 
+#[derive(Debug, thiserror::Error)]
+pub enum TransactionPoolErrors {
+    /// Invalid transactions, rejeceted diffs, etc...
+    #[error("Transaction pool errors: {0:?}")]
+    BatchedErrors(Vec<TransactionError>),
+    /// Errors that should panic the node (bugs in implementation)
+    #[error("Unexpected error: {0}")]
+    Unexpected(String),
+}
+
+#[derive(Debug, thiserror::Error)]
+pub enum TransactionError {
+    #[error(transparent)]
+    Verifier(#[from] VerifierError),
+    #[error(transparent)]
+    WellFormedness(#[from] WellFormednessError),
+}
+
+impl From<String> for TransactionPoolErrors {
+    fn from(value: String) -> Self {
+        Self::Unexpected(value)
+    }
+}
+
 mod consensus {
     use crate::scan_state::currency::{BlockTimeSpan, Epoch, Length};
 
@@ -475,6 +499,7 @@ enum Batch {
     Of(usize),
 }
 
+#[derive(Debug)]
 pub enum CommandError {
     InvalidNonce {
         account_nonce: Nonce,
@@ -729,6 +754,7 @@ impl IndexedPool {
                 if unchecked.expected_target_nonce()
                     != first_queued.data.forget_check().applicable_at_nonce()
                 {
+                    // Ocaml panics here as well
                     panic!("indexed pool nonces inconsistent when adding from backtrack.")
                 }
 
@@ -833,8 +859,8 @@ impl IndexedPool {
             })
             .unwrap();
 
-        let keep_queue = sender_queue.split_off(cmd_index);
-        let drop_queue = sender_queue.clone();
+        let drop_queue = sender_queue.split_off(cmd_index);
+        let keep_queue = sender_queue;
         assert!(!drop_queue.is_empty());
 
         let currency_to_remove = drop_queue.iter().fold(Amount::zero(), |acc, cmd| {
@@ -963,7 +989,9 @@ impl IndexedPool {
             self.check_expiry(global_slot_since_genesis, &unchecked)?;
             let consumed = currency_consumed(&unchecked).ok_or(CommandError::Overflow)?;
             if !unchecked.fee_token().is_default() {
-                return Err(CommandError::BadToken);
+                return Err(CommandError::UnwantedFeeToken {
+                    token_id: unchecked.fee_token(),
+                });
             }
             consumed
         };
@@ -1042,8 +1070,7 @@ impl IndexedPool {
                         })
                         .unwrap();
 
-                    let _ = queued_cmds.split_off(replacement_index);
-                    let drop_queue = queued_cmds.clone();
+                    let drop_queue = queued_cmds.split_off(replacement_index);
 
                     let to_drop = drop_queue.front().unwrap().data.forget_check();
                     assert!(cmd_applicable_at_nonce <= to_drop.applicable_at_nonce());
@@ -2072,7 +2099,7 @@ impl TransactionPool {
         &self,
         diff: diff::Diff,
         accounts: &BTreeMap<AccountId, Account>,
-    ) -> Result<Vec<valid::UserCommand>, String> {
+    ) -> Result<Vec<valid::UserCommand>, TransactionPoolErrors> {
         let well_formedness_errors: HashSet<_> = diff
             .list
             .iter()
@@ -2083,9 +2110,11 @@ impl TransactionPool {
             .collect();
 
         if !well_formedness_errors.is_empty() {
-            return Err(format!(
-                "Some commands have one or more well-formedness errors: {:?}",
+            return Err(TransactionPoolErrors::BatchedErrors(
                 well_formedness_errors
+                    .into_iter()
+                    .map(TransactionError::WellFormedness)
+                    .collect_vec(),
             ));
         }
 
@@ -2132,7 +2161,7 @@ impl TransactionPool {
 
             from_unapplied_sequence::Cache::new(merged)
         })
-        .map_err(|e| format!("Invalid {:?}", e))?;
+        .map_err(TransactionPoolErrors::Unexpected)?;
 
         let diff = diff
             .into_iter()
@@ -2142,17 +2171,25 @@ impl TransactionPool {
             })
             .collect::<Vec<_>>();
 
-        Verifier
+        let (verified, invalid): (Vec<_>, Vec<_>) = Verifier
             .verify_commands(diff, None)
             .into_iter()
-            .map(|cmd| {
-                // TODO: Handle invalids
-                match cmd {
-                    crate::verifier::VerifyCommandsResult::Valid(cmd) => Ok(cmd),
-                    e => Err(format!("invalid tx: {:?}", e)),
-                }
-            })
-            .collect()
+            .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)
+        }
     }
 
     fn get_rebroadcastable<F>(&mut self, has_timed_out: F) -> Vec<Vec<UserCommand>>

ledger/src/verifier/mod.rs

@@ -103,22 +103,32 @@ fn verify_digest_only(ts: Vec<(LedgerProof, SokMessage)>) -> Result<(), String>
 }
 
 /// https://github.com/MinaProtocol/mina/blob/bfd1009abdbee78979ff0343cc73a3480e862f58/src/lib/verifier/verifier_intf.ml#L10C1-L36C29
-#[derive(Debug)]
-pub enum VerifyCommandsResult {
-    Valid(valid::UserCommand),
+pub type VerifyCommandsResult = Result<valid::UserCommand, VerifierError>;
+
+#[derive(Debug, thiserror::Error)]
+pub enum VerifierError {
+    // TODO(adonagy): print something here as well?
+    #[error("Batch verification failed")]
     ValidAssuming(
         Vec<(
             VerificationKey,
             ZkappStatement,
             Arc<PicklesProofProofsVerifiedMaxStableV2>,
         )>,
     ),
+    #[error("Invalid keys: {0:?}")]
     InvalidKeys(Vec<CompressedPubKey>),
+    #[error("Invalid signature: {0:?}")]
     InvalidSignature(Vec<CompressedPubKey>),
+    #[error("Invalid proof: {0}")]
     InvalidProof(String),
+    #[error("Missing verification key: {0:?}")]
     MissingVerificationKey(Vec<CompressedPubKey>),
+    #[error("Unexpected verification key: {0:?}")]
     UnexpectedVerificationKey(Vec<CompressedPubKey>),
+    #[error("Mismatched verification key: {0:?}")]
     MismatchedVerificationKey(Vec<CompressedPubKey>),
+    #[error("Authorization kind does not match the authorization - Keys {0:?}")]
     MismatchedAuthorizationKind(Vec<CompressedPubKey>),
 }
 
@@ -176,36 +186,36 @@ impl Verifier {
 
         cs.into_iter()
             .map(|c| match c {
-                CheckResult::Valid(c) => VerifyCommandsResult::Valid(c),
+                CheckResult::Valid(c) => Ok(c),
                 CheckResult::ValidAssuming((c, xs)) => {
                     if all_verified {
-                        VerifyCommandsResult::Valid(c)
+                        Ok(c)
                     } else {
-                        VerifyCommandsResult::ValidAssuming(xs)
+                        Err(VerifierError::ValidAssuming(xs))
                     }
                 }
-                CheckResult::InvalidKeys(keys) => VerifyCommandsResult::InvalidKeys(keys),
-                CheckResult::InvalidSignature(keys) => VerifyCommandsResult::InvalidSignature(keys),
-                CheckResult::InvalidProof(s) => VerifyCommandsResult::InvalidProof(s),
+                CheckResult::InvalidKeys(keys) => Err(VerifierError::InvalidKeys(keys)),
+                CheckResult::InvalidSignature(keys) => Err(VerifierError::InvalidSignature(keys)),
+                CheckResult::InvalidProof(s) => Err(VerifierError::InvalidProof(s)),
                 CheckResult::MissingVerificationKey(keys) => {
-                    VerifyCommandsResult::MissingVerificationKey(keys)
+                    Err(VerifierError::MissingVerificationKey(keys))
                 }
                 CheckResult::UnexpectedVerificationKey(keys) => {
-                    VerifyCommandsResult::UnexpectedVerificationKey(keys)
+                    Err(VerifierError::UnexpectedVerificationKey(keys))
                 }
                 CheckResult::MismatchedAuthorizationKind(keys) => {
-                    VerifyCommandsResult::MismatchedAuthorizationKind(keys)
+                    Err(VerifierError::MismatchedAuthorizationKind(keys))
                 }
             })
             .collect()
     }
 }
 
-#[derive(Debug, derive_more::From)]
-pub enum VerifierError {
-    CheckError(CheckResult),
-    VerificationFailed(String),
-}
+// #[derive(Debug, derive_more::From)]
+// pub enum VerifierError {
+//     CheckError(CheckResult),
+//     VerificationFailed(String),
+// }
 
 pub mod common {
     use std::sync::Arc;

node/common/src/service/rpc/mod.rs

@@ -7,7 +7,7 @@ use node::rpc::{
     RpcBlockProducerStatsGetResponse, RpcDiscoveryBoostrapStatsResponse,
     RpcDiscoveryRoutingTableResponse, RpcHealthCheckResponse, RpcLedgerAccountsResponse,
     RpcMessageProgressResponse, RpcPeersGetResponse, RpcReadinessCheckResponse, RpcRequest,
-    RpcStateGetError, RpcStatusGetResponse, RpcTransactionInjectFailure,
+    RpcStateGetError, RpcStatusGetResponse, RpcTransactionInjectResponse,
     RpcTransactionPoolResponse, RpcTransitionFrontierUserCommandsResponse,
 };
 use serde::{Deserialize, Serialize};
@@ -19,7 +19,7 @@ pub use node::rpc::{
     ActionStatsResponse, RespondError, RpcActionStatsGetResponse, RpcId, RpcIdType,
     RpcP2pConnectionOutgoingResponse, RpcScanStateSummaryGetResponse, RpcSnarkPoolGetResponse,
     RpcSnarkerJobCommitResponse, RpcSnarkerJobSpecResponse, RpcStateGetResponse,
-    RpcSyncStatsGetResponse, RpcTransactionInjectResponse,
+    RpcSyncStatsGetResponse, RpcTransactionInjectSuccess,
 };
 use node::State;
 use node::{event_source::Event, rpc::RpcSnarkPoolJobGetResponse};
@@ -96,9 +96,9 @@ macro_rules! rpc_service_impl {
             let chan = entry.ok_or(RespondError::UnknownRpcId)?;
             let chan = chan
                 .downcast::<oneshot::Sender<$ty>>()
-                .or(Err(RespondError::UnexpectedResponseType))?;
+                .map_err(|_| RespondError::UnexpectedResponseType)?;
             chan.send(response)
-                .or(Err(RespondError::RespondingFailed))?;
+                .map_err(|_| RespondError::RespondingFailed)?;
             Ok(())
         }
     };
@@ -287,10 +287,6 @@ impl node::rpc::RpcService for NodeService {
     rpc_service_impl!(respond_transaction_pool, RpcTransactionPoolResponse);
     rpc_service_impl!(respond_ledger_accounts, RpcLedgerAccountsResponse);
     rpc_service_impl!(respond_transaction_inject, RpcTransactionInjectResponse);
-    rpc_service_impl!(
-        respond_transaction_inject_failed,
-        RpcTransactionInjectFailure
-    );
     rpc_service_impl!(
         respond_transition_frontier_commands,
         RpcTransitionFrontierUserCommandsResponse

node/src/action_kind.rs

@@ -395,6 +395,7 @@ pub enum ActionKind {
     RpcTransactionInjectFailure,
     RpcTransactionInjectInit,
     RpcTransactionInjectPending,
+    RpcTransactionInjectRejected,
     RpcTransactionInjectSuccess,
     RpcTransactionPool,
     RpcTransitionFrontierUserCommandsGet,
@@ -447,6 +448,7 @@ pub enum ActionKind {
     TransactionPoolRebroadcast,
     TransactionPoolStartVerify,
     TransactionPoolStartVerifyWithAccounts,
+    TransactionPoolVerifyError,
     TransactionPoolEffectfulFetchAccounts,
     TransitionFrontierGenesisInject,
     TransitionFrontierSynced,
@@ -536,7 +538,7 @@ pub enum ActionKind {
 }
 
 impl ActionKind {
-    pub const COUNT: u16 = 444;
+    pub const COUNT: u16 = 446;
 }
 
 impl std::fmt::Display for ActionKind {
@@ -685,6 +687,7 @@ impl ActionKindGet for TransactionPoolAction {
             Self::StartVerifyWithAccounts { .. } => {
                 ActionKind::TransactionPoolStartVerifyWithAccounts
             }
+            Self::VerifyError { .. } => ActionKind::TransactionPoolVerifyError,
             Self::BestTipChanged { .. } => ActionKind::TransactionPoolBestTipChanged,
             Self::BestTipChangedWithAccounts { .. } => {
                 ActionKind::TransactionPoolBestTipChangedWithAccounts
@@ -817,6 +820,7 @@ impl ActionKindGet for RpcAction {
             Self::TransactionInjectInit { .. } => ActionKind::RpcTransactionInjectInit,
             Self::TransactionInjectPending { .. } => ActionKind::RpcTransactionInjectPending,
             Self::TransactionInjectSuccess { .. } => ActionKind::RpcTransactionInjectSuccess,
+            Self::TransactionInjectRejected { .. } => ActionKind::RpcTransactionInjectRejected,
             Self::TransactionInjectFailure { .. } => ActionKind::RpcTransactionInjectFailure,
             Self::TransitionFrontierUserCommandsGet { .. } => {
                 ActionKind::RpcTransitionFrontierUserCommandsGet