Skip to content

Commit 7d45600

Browse files
Adrian Nagysebastiencs
Adrian Nagy
authored and
sebastiencs
committed
feat(tx-pool): Improve error handling
1 parent a47a668 commit 7d45600

File tree

14 files changed

+199
-105
lines changed

14 files changed

+199
-105
lines changed

‎ledger/src/scan_state/transaction_logic.rs‎

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4739,12 +4739,17 @@ impl UserCommand {
47394739
}
47404740
}
47414741

4742-
#[derive(Debug, Clone, Hash, PartialEq, Eq)]
4742+
#[derive(Debug, Clone, Hash, PartialEq, Eq, thiserror::Error)]
47434743
pub enum WellFormednessError {
4744+
#[error("Insufficient Fee")]
47444745
InsufficientFee,
4746+
#[error("Zero vesting period")]
47454747
ZeroVestingPeriod,
4748+
#[error("Zkapp too big: {0}")]
47464749
ZkappTooBig(String),
4750+
#[error("Transaction type disabled")]
47474751
TransactionTypeDisabled,
4752+
#[error("Incompatible version")]
47484753
IncompatibleVersion,
47494754
}
47504755

‎ledger/src/staged_ledger/staged_ledger.rs‎

Lines changed: 0 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -1222,30 +1222,6 @@ impl StagedLedger {
12221222
verifier
12231223
.verify_commands(cs, skip_verification)
12241224
.into_iter()
1225-
.map(|x| {
1226-
use crate::verifier::VerifyCommandsResult::*;
1227-
match x {
1228-
Valid(x) => Ok(x),
1229-
InvalidKeys(invalid)
1230-
| InvalidSignature(invalid)
1231-
| MissingVerificationKey(invalid)
1232-
| UnexpectedVerificationKey(invalid)
1233-
| MismatchedVerificationKey(invalid)
1234-
| MismatchedAuthorizationKind(invalid) => {
1235-
Err(VerifierError::VerificationFailed(format!(
1236-
"verification failed on command: {:?}",
1237-
invalid
1238-
)))
1239-
}
1240-
InvalidProof(invalid) => Err(VerifierError::VerificationFailed(format!(
1241-
"verification failed on command: {:?}",
1242-
invalid
1243-
))),
1244-
ValidAssuming(_) => Err(VerifierError::VerificationFailed(
1245-
"batch verification failed".to_string(),
1246-
)),
1247-
}
1248-
})
12491225
.collect()
12501226
}
12511227

‎ledger/src/transaction_pool.rs‎

Lines changed: 54 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -21,13 +21,37 @@ use crate::{
2121
MaybeWithStatus, WithHash,
2222
},
2323
TransactionStatus::Applied,
24-
UserCommand, WithStatus,
24+
UserCommand, WellFormednessError, WithStatus,
2525
},
2626
},
27-
verifier::Verifier,
27+
verifier::{Verifier, VerifierError},
2828
Account, AccountId, BaseLedger, Mask, TokenId, VerificationKey,
2929
};
3030

31+
#[derive(Debug, thiserror::Error)]
32+
pub enum TransactionPoolErrors {
33+
/// Invalid transactions, rejeceted diffs, etc...
34+
#[error("Transaction pool errors: {0:?}")]
35+
BatchedErrors(Vec<TransactionError>),
36+
/// Errors that should panic the node (bugs in implementation)
37+
#[error("Unexpected error: {0}")]
38+
Unexpected(String),
39+
}
40+
41+
#[derive(Debug, thiserror::Error)]
42+
pub enum TransactionError {
43+
#[error(transparent)]
44+
Verifier(#[from] VerifierError),
45+
#[error(transparent)]
46+
WellFormedness(#[from] WellFormednessError),
47+
}
48+
49+
impl From<String> for TransactionPoolErrors {
50+
fn from(value: String) -> Self {
51+
Self::Unexpected(value)
52+
}
53+
}
54+
3155
mod consensus {
3256
use crate::scan_state::currency::{BlockTimeSpan, Epoch, Length};
3357

@@ -475,6 +499,7 @@ enum Batch {
475499
Of(usize),
476500
}
477501

502+
#[derive(Debug)]
478503
pub enum CommandError {
479504
InvalidNonce {
480505
account_nonce: Nonce,
@@ -729,6 +754,7 @@ impl IndexedPool {
729754
if unchecked.expected_target_nonce()
730755
!= first_queued.data.forget_check().applicable_at_nonce()
731756
{
757+
// Ocaml panics here as well
732758
panic!("indexed pool nonces inconsistent when adding from backtrack.")
733759
}
734760

@@ -963,7 +989,9 @@ impl IndexedPool {
963989
self.check_expiry(global_slot_since_genesis, &unchecked)?;
964990
let consumed = currency_consumed(&unchecked).ok_or(CommandError::Overflow)?;
965991
if !unchecked.fee_token().is_default() {
966-
return Err(CommandError::BadToken);
992+
return Err(CommandError::UnwantedFeeToken {
993+
token_id: unchecked.fee_token(),
994+
});
967995
}
968996
consumed
969997
};
@@ -2072,7 +2100,7 @@ impl TransactionPool {
20722100
&self,
20732101
diff: diff::Diff,
20742102
accounts: &BTreeMap<AccountId, Account>,
2075-
) -> Result<Vec<valid::UserCommand>, String> {
2103+
) -> Result<Vec<valid::UserCommand>, TransactionPoolErrors> {
20762104
let well_formedness_errors: HashSet<_> = diff
20772105
.list
20782106
.iter()
@@ -2083,9 +2111,11 @@ impl TransactionPool {
20832111
.collect();
20842112

20852113
if !well_formedness_errors.is_empty() {
2086-
return Err(format!(
2087-
"Some commands have one or more well-formedness errors: {:?}",
2114+
return Err(TransactionPoolErrors::BatchedErrors(
20882115
well_formedness_errors
2116+
.into_iter()
2117+
.map(TransactionError::WellFormedness)
2118+
.collect_vec(),
20892119
));
20902120
}
20912121

@@ -2132,7 +2162,7 @@ impl TransactionPool {
21322162

21332163
from_unapplied_sequence::Cache::new(merged)
21342164
})
2135-
.map_err(|e| format!("Invalid {:?}", e))?;
2165+
.map_err(TransactionPoolErrors::Unexpected)?;
21362166

21372167
let diff = diff
21382168
.into_iter()
@@ -2142,17 +2172,25 @@ impl TransactionPool {
21422172
})
21432173
.collect::<Vec<_>>();
21442174

2145-
Verifier
2175+
let (verified, invalid): (Vec<_>, Vec<_>) = Verifier
21462176
.verify_commands(diff, None)
21472177
.into_iter()
2148-
.map(|cmd| {
2149-
// TODO: Handle invalids
2150-
match cmd {
2151-
crate::verifier::VerifyCommandsResult::Valid(cmd) => Ok(cmd),
2152-
e => Err(format!("invalid tx: {:?}", e)),
2153-
}
2154-
})
2155-
.collect()
2178+
.partition(Result::is_ok);
2179+
2180+
let verified: Vec<_> = verified.into_iter().map(Result::unwrap).collect();
2181+
let invalid: Vec<_> = invalid.into_iter().map(Result::unwrap_err).collect();
2182+
2183+
if !invalid.is_empty() {
2184+
let transaction_pool_errors = invalid
2185+
.into_iter()
2186+
.map(TransactionError::Verifier)
2187+
.collect();
2188+
Err(TransactionPoolErrors::BatchedErrors(
2189+
transaction_pool_errors,
2190+
))
2191+
} else {
2192+
Ok(verified)
2193+
}
21562194
}
21572195

21582196
fn get_rebroadcastable<F>(&mut self, has_timed_out: F) -> Vec<Vec<UserCommand>>

‎ledger/src/verifier/mod.rs‎

Lines changed: 27 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -103,22 +103,32 @@ fn verify_digest_only(ts: Vec<(LedgerProof, SokMessage)>) -> Result<(), String>
103103
}
104104

105105
/// https://github.com/MinaProtocol/mina/blob/bfd1009abdbee78979ff0343cc73a3480e862f58/src/lib/verifier/verifier_intf.ml#L10C1-L36C29
106-
#[derive(Debug)]
107-
pub enum VerifyCommandsResult {
108-
Valid(valid::UserCommand),
106+
pub type VerifyCommandsResult = Result<valid::UserCommand, VerifierError>;
107+
108+
#[derive(Debug, thiserror::Error)]
109+
pub enum VerifierError {
110+
// TODO(adonagy): print something here as well?
111+
#[error("Batch verification failed")]
109112
ValidAssuming(
110113
Vec<(
111114
VerificationKey,
112115
ZkappStatement,
113116
Arc<PicklesProofProofsVerifiedMaxStableV2>,
114117
)>,
115118
),
119+
#[error("Invalid keys: {0:?}")]
116120
InvalidKeys(Vec<CompressedPubKey>),
121+
#[error("Invalid signature: {0:?}")]
117122
InvalidSignature(Vec<CompressedPubKey>),
123+
#[error("Invalid proof: {0}")]
118124
InvalidProof(String),
125+
#[error("Missing verification key: {0:?}")]
119126
MissingVerificationKey(Vec<CompressedPubKey>),
127+
#[error("Unexpected verification key: {0:?}")]
120128
UnexpectedVerificationKey(Vec<CompressedPubKey>),
129+
#[error("Mismatched verification key: {0:?}")]
121130
MismatchedVerificationKey(Vec<CompressedPubKey>),
131+
#[error("Authorization kind does not match the authorization - Keys {0:?}")]
122132
MismatchedAuthorizationKind(Vec<CompressedPubKey>),
123133
}
124134

@@ -176,36 +186,36 @@ impl Verifier {
176186

177187
cs.into_iter()
178188
.map(|c| match c {
179-
CheckResult::Valid(c) => VerifyCommandsResult::Valid(c),
189+
CheckResult::Valid(c) => Ok(c),
180190
CheckResult::ValidAssuming((c, xs)) => {
181191
if all_verified {
182-
VerifyCommandsResult::Valid(c)
192+
Ok(c)
183193
} else {
184-
VerifyCommandsResult::ValidAssuming(xs)
194+
Err(VerifierError::ValidAssuming(xs))
185195
}
186196
}
187-
CheckResult::InvalidKeys(keys) => VerifyCommandsResult::InvalidKeys(keys),
188-
CheckResult::InvalidSignature(keys) => VerifyCommandsResult::InvalidSignature(keys),
189-
CheckResult::InvalidProof(s) => VerifyCommandsResult::InvalidProof(s),
197+
CheckResult::InvalidKeys(keys) => Err(VerifierError::InvalidKeys(keys)),
198+
CheckResult::InvalidSignature(keys) => Err(VerifierError::InvalidSignature(keys)),
199+
CheckResult::InvalidProof(s) => Err(VerifierError::InvalidProof(s)),
190200
CheckResult::MissingVerificationKey(keys) => {
191-
VerifyCommandsResult::MissingVerificationKey(keys)
201+
Err(VerifierError::MissingVerificationKey(keys))
192202
}
193203
CheckResult::UnexpectedVerificationKey(keys) => {
194-
VerifyCommandsResult::UnexpectedVerificationKey(keys)
204+
Err(VerifierError::UnexpectedVerificationKey(keys))
195205
}
196206
CheckResult::MismatchedAuthorizationKind(keys) => {
197-
VerifyCommandsResult::MismatchedAuthorizationKind(keys)
207+
Err(VerifierError::MismatchedAuthorizationKind(keys))
198208
}
199209
})
200210
.collect()
201211
}
202212
}
203213

204-
#[derive(Debug, derive_more::From)]
205-
pub enum VerifierError {
206-
CheckError(CheckResult),
207-
VerificationFailed(String),
208-
}
214+
// #[derive(Debug, derive_more::From)]
215+
// pub enum VerifierError {
216+
// CheckError(CheckResult),
217+
// VerificationFailed(String),
218+
// }
209219

210220
pub mod common {
211221
use std::sync::Arc;

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

Lines changed: 5 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,8 @@ use node::rpc::{
88
RpcDiscoveryRoutingTableResponse, RpcHealthCheckResponse, RpcLedgerAccountsResponse,
99
RpcMessageProgressResponse, RpcPeersGetResponse, RpcReadinessCheckResponse, RpcRequest,
1010
RpcStateGetError, RpcStatusGetResponse, RpcTransactionInjectFailure,
11-
RpcTransactionPoolResponse, RpcTransitionFrontierUserCommandsResponse,
11+
RpcTransactionInjectRejected, RpcTransactionInjectResponse, RpcTransactionPoolResponse,
12+
RpcTransitionFrontierUserCommandsResponse,
1213
};
1314
use serde::{Deserialize, Serialize};
1415

@@ -19,7 +20,7 @@ pub use node::rpc::{
1920
ActionStatsResponse, RespondError, RpcActionStatsGetResponse, RpcId, RpcIdType,
2021
RpcP2pConnectionOutgoingResponse, RpcScanStateSummaryGetResponse, RpcSnarkPoolGetResponse,
2122
RpcSnarkerJobCommitResponse, RpcSnarkerJobSpecResponse, RpcStateGetResponse,
22-
RpcSyncStatsGetResponse, RpcTransactionInjectResponse,
23+
RpcSyncStatsGetResponse, RpcTransactionInjectSuccess,
2324
};
2425
use node::State;
2526
use node::{event_source::Event, rpc::RpcSnarkPoolJobGetResponse};
@@ -96,9 +97,9 @@ macro_rules! rpc_service_impl {
9697
let chan = entry.ok_or(RespondError::UnknownRpcId)?;
9798
let chan = chan
9899
.downcast::<oneshot::Sender<$ty>>()
99-
.or(Err(RespondError::UnexpectedResponseType))?;
100+
.map_err(|_| RespondError::UnexpectedResponseType)?;
100101
chan.send(response)
101-
.or(Err(RespondError::RespondingFailed))?;
102+
.map_err(|_| RespondError::RespondingFailed)?;
102103
Ok(())
103104
}
104105
};
@@ -287,10 +288,6 @@ impl node::rpc::RpcService for NodeService {
287288
rpc_service_impl!(respond_transaction_pool, RpcTransactionPoolResponse);
288289
rpc_service_impl!(respond_ledger_accounts, RpcLedgerAccountsResponse);
289290
rpc_service_impl!(respond_transaction_inject, RpcTransactionInjectResponse);
290-
rpc_service_impl!(
291-
respond_transaction_inject_failed,
292-
RpcTransactionInjectFailure
293-
);
294291
rpc_service_impl!(
295292
respond_transition_frontier_commands,
296293
RpcTransitionFrontierUserCommandsResponse

‎node/src/action_kind.rs‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -395,6 +395,7 @@ pub enum ActionKind {
395395
RpcTransactionInjectFailure,
396396
RpcTransactionInjectInit,
397397
RpcTransactionInjectPending,
398+
RpcTransactionInjectRejected,
398399
RpcTransactionInjectSuccess,
399400
RpcTransactionPool,
400401
RpcTransitionFrontierUserCommandsGet,
@@ -447,6 +448,7 @@ pub enum ActionKind {
447448
TransactionPoolRebroadcast,
448449
TransactionPoolStartVerify,
449450
TransactionPoolStartVerifyWithAccounts,
451+
TransactionPoolVerifyError,
450452
TransactionPoolEffectfulFetchAccounts,
451453
TransitionFrontierGenesisInject,
452454
TransitionFrontierSynced,
@@ -685,6 +687,7 @@ impl ActionKindGet for TransactionPoolAction {
685687
Self::StartVerifyWithAccounts { .. } => {
686688
ActionKind::TransactionPoolStartVerifyWithAccounts
687689
}
690+
Self::VerifyError { .. } => ActionKind::TransactionPoolVerifyError,
688691
Self::BestTipChanged { .. } => ActionKind::TransactionPoolBestTipChanged,
689692
Self::BestTipChangedWithAccounts { .. } => {
690693
ActionKind::TransactionPoolBestTipChangedWithAccounts
@@ -817,6 +820,7 @@ impl ActionKindGet for RpcAction {
817820
Self::TransactionInjectInit { .. } => ActionKind::RpcTransactionInjectInit,
818821
Self::TransactionInjectPending { .. } => ActionKind::RpcTransactionInjectPending,
819822
Self::TransactionInjectSuccess { .. } => ActionKind::RpcTransactionInjectSuccess,
823+
Self::TransactionInjectRejected { .. } => ActionKind::RpcTransactionInjectRejected,
820824
Self::TransactionInjectFailure { .. } => ActionKind::RpcTransactionInjectFailure,
821825
Self::TransitionFrontierUserCommandsGet { .. } => {
822826
ActionKind::RpcTransitionFrontierUserCommandsGet

‎node/src/rpc/mod.rs‎

Lines changed: 12 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -354,8 +354,18 @@ pub enum RpcTransactionInjectedCommand {
354354
Zkapp,
355355
}
356356

357-
pub type RpcTransactionInjectResponse = Vec<RpcTransactionInjectedCommand>;
358-
pub type RpcTransactionInjectFailure = Vec<(RpcTransactionInjectedCommand, diff::Error)>;
357+
pub type RpcTransactionInjectSuccess = Vec<RpcTransactionInjectedCommand>;
358+
pub type RpcTransactionInjectRejected = Vec<(RpcTransactionInjectedCommand, diff::Error)>;
359+
/// Errors
360+
pub type RpcTransactionInjectFailure = Vec<String>;
361+
362+
#[derive(Serialize, Deserialize, Debug, Clone)]
363+
#[serde(untagged)]
364+
pub enum RpcTransactionInjectResponse {
365+
Success(RpcTransactionInjectSuccess),
366+
Rejected(RpcTransactionInjectRejected),
367+
Failure(RpcTransactionInjectFailure),
368+
}
359369

360370
impl From<ValidCommandWithHash> for RpcTransactionInjectedCommand {
361371
fn from(value: ValidCommandWithHash) -> Self {

‎node/src/rpc/rpc_actions.rs‎

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -170,10 +170,15 @@ pub enum RpcAction {
170170
response: Vec<ValidCommandWithHash>,
171171
},
172172
#[action_event(level = info)]
173-
TransactionInjectFailure {
173+
TransactionInjectRejected {
174174
rpc_id: RpcId,
175175
response: Vec<(ValidCommandWithHash, diff::Error)>,
176176
},
177+
#[action_event(level = warn)]
178+
TransactionInjectFailure {
179+
rpc_id: RpcId,
180+
errors: Vec<String>,
181+
},
177182
#[action_event(level = info)]
178183
TransitionFrontierUserCommandsGet {
179184
rpc_id: RpcId,
@@ -287,6 +292,11 @@ impl redux::EnablingCondition<crate::State> for RpcAction {
287292
.requests
288293
.get(rpc_id)
289294
.map_or(false, |v| v.status.is_pending()),
295+
RpcAction::TransactionInjectRejected { rpc_id, .. } => state
296+
.rpc
297+
.requests
298+
.get(rpc_id)
299+
.map_or(false, |v| v.status.is_pending()),
290300
RpcAction::TransactionInjectFailure { rpc_id, .. } => state
291301
.rpc
292302
.requests

0 commit comments

Comments
 (0)