Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
66 changes: 43 additions & 23 deletions magicblock-committor-service/src/intent_executor/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,8 @@ pub enum IntentExecutorError {
EmptyIntentError,
#[error("User supplied actions are ill-formed: {0}. {:?}", .1)]
ActionsError(#[source] TransactionError, Option<Signature>),
#[error("Invalid undelegation: {0}. {:?}", .1)]
UndelegationError(#[source] TransactionError, Option<Signature>),
#[error("Accounts committed with an invalid Commit id: {0}. {:?}", .1)]
CommitIDError(#[source] TransactionError, Option<Signature>),
#[error("Max instruction trace length exceeded: {0}. {:?}", .1)]
Expand Down Expand Up @@ -94,6 +96,10 @@ impl IntentExecutorError {
err,
signature,
) => IntentExecutorError::CommitIDError(err, signature),
TransactionStrategyExecutionError::UndelegationError(
err,
signature,
) => IntentExecutorError::UndelegationError(err, signature),
TransactionStrategyExecutionError::InternalError(err) => {
converter(err)
}
Expand All @@ -117,6 +123,8 @@ impl metrics::LabelValue for IntentExecutorError {
pub enum TransactionStrategyExecutionError {
#[error("User supplied actions are ill-formed: {0}. {:?}", .1)]
ActionsError(#[source] TransactionError, Option<Signature>),
#[error("Invalid undelegation: {0}. {:?}", .1)]
UndelegationError(#[source] TransactionError, Option<Signature>),
#[error("Accounts committed with an invalid Commit id: {0}. {:?}", .1)]
CommitIDError(#[source] TransactionError, Option<Signature>),
#[error("Max instruction trace length exceeded: {0}. {:?}", .1)]
Expand Down Expand Up @@ -146,14 +154,6 @@ impl TransactionStrategyExecutionError {
dlp::error::DlpError::NonceOutOfOrder as u32;

match err {
// Filter CommitIdError by custom error code
transaction_err @ TransactionError::InstructionError(
_,
InstructionError::Custom(NONCE_OUT_OF_ORDER),
) => Ok(TransactionStrategyExecutionError::CommitIDError(
transaction_err,
signature,
)),
// Some tx may use too much CPIs and we can handle it in certain cases
transaction_err @ TransactionError::InstructionError(
_,
Expand All @@ -162,25 +162,45 @@ impl TransactionStrategyExecutionError {
transaction_err,
signature,
)),
// Filter ActionError, we can attempt recovery by stripping away actions
transaction_err @ TransactionError::InstructionError(index, _) => {
// Map per-task InstructionError into CommitID / Actions / Undelegation errors when possible
TransactionError::InstructionError(index, instruction_err) => {
let tx_err_helper = |instruction_err| -> TransactionError {
TransactionError::InstructionError(index, instruction_err)
};
let Some(action_index) = index.checked_sub(OFFSET) else {
return Err(transaction_err);
return Err(tx_err_helper(instruction_err));
};

let Some(task_type) = tasks
.get(action_index as usize)
.map(|task| task.task_type())
else {
return Err(tx_err_helper(instruction_err));
};

// If index corresponds to an Action -> ActionsError; otherwise -> InternalError.
if matches!(
tasks
.get(action_index as usize)
.map(|task| task.task_type()),
Some(TaskType::Action)
) {
Ok(TransactionStrategyExecutionError::ActionsError(
transaction_err,
match (task_type, instruction_err) {
(
TaskType::Commit,
InstructionError::Custom(NONCE_OUT_OF_ORDER),
) => Ok(TransactionStrategyExecutionError::CommitIDError(
tx_err_helper(InstructionError::Custom(
NONCE_OUT_OF_ORDER,
)),
signature,
))
} else {
Err(transaction_err)
)),
(TaskType::Action, instruction_err) => {
Ok(TransactionStrategyExecutionError::ActionsError(
tx_err_helper(instruction_err),
signature,
))
}
(TaskType::Undelegate, instruction_err) => Ok(
TransactionStrategyExecutionError::UndelegationError(
tx_err_helper(instruction_err),
signature,
),
),
(_, instruction_err) => Err(tx_err_helper(instruction_err)),
}
}
// This means transaction failed to other reasons that we don't handle - propagate
Expand Down
34 changes: 33 additions & 1 deletion magicblock-committor-service/src/intent_executor/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -445,6 +445,35 @@ where
(commit_strategy, finalize_strategy, to_cleanup)
}

/// Handles actions error, stripping away actions
/// Returns [`TransactionStrategy`] to be cleaned up
Comment on lines +448 to +449
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Fix copy-paste error in documentation.

The comment says "Handles actions error, stripping away actions" but this method handles undelegation errors.

Apply this diff:

-    /// Handles actions error, stripping away actions
+    /// Handles undelegation error, stripping away undelegation and subsequent tasks
     /// Returns [`TransactionStrategy`] to be cleaned up
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
/// Handles actions error, stripping away actions
/// Returns [`TransactionStrategy`] to be cleaned up
/// Handles undelegation error, stripping away undelegation and subsequent tasks
/// Returns [`TransactionStrategy`] to be cleaned up
🤖 Prompt for AI Agents
In magicblock-committor-service/src/intent_executor/mod.rs around lines 448 to
449, the doc comment incorrectly states "Handles actions error, stripping away
actions" though the function actually handles undelegation errors; update the
documentation comment to accurately describe that this method handles
undelegation errors (e.g., "Handles undelegation error, stripping away
undelegations") and ensure wording matches the function behavior and terminology
used elsewhere in the module.

fn handle_undelegation_error(
&self,
strategy: &mut TransactionStrategy,
) -> TransactionStrategy {
let position = strategy
.optimized_tasks
.iter()
.position(|el| el.task_type() == TaskType::Undelegate);

if let Some(position) = position {
// Remove everything after undelegation including post undelegation actions
let removed_task =
strategy.optimized_tasks.drain(position..).collect();
let old_alts =
strategy.dummy_revaluate_alts(&self.authority.pubkey());
TransactionStrategy {
optimized_tasks: removed_task,
lookup_tables_keys: old_alts,
}
} else {
TransactionStrategy {
optimized_tasks: vec![],
lookup_tables_keys: vec![],
}
}
}

/// Shared helper for sending transactions
async fn send_prepared_message(
&self,
Expand Down Expand Up @@ -520,7 +549,8 @@ where
}
Err(IntentExecutorError::CommitIDError(_, _))
| Err(IntentExecutorError::ActionsError(_, _))
| Err(IntentExecutorError::CpiLimitError(_, _)) => None,
| Err(IntentExecutorError::CpiLimitError(_, _))
| Err(IntentExecutorError::UndelegationError(_, _)) => None,
Err(IntentExecutorError::EmptyIntentError)
| Err(IntentExecutorError::FailedToFitError)
| Err(IntentExecutorError::TaskBuilderError(_))
Expand Down Expand Up @@ -758,6 +788,8 @@ where
let result = self.execute_inner(base_intent, &persister).await;
if let Some(pubkeys) = pubkeys {
// Reset TaskInfoFetcher, as cache could become invalid
// NOTE: if undelegation was removed - we still reset
// We assume its safe since all consecutive commits will fail
if result.is_err() || is_undelegate {
self.task_info_fetcher.reset(ResetType::Specific(&pubkeys));
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -175,6 +175,13 @@ where
.await?;
Ok(ControlFlow::Continue(to_cleanup))
}
TransactionStrategyExecutionError::UndelegationError(_, _) => {
// Here we patch strategy for it to be retried in next iteration
// & we also record data that has to be cleaned up after patch
let to_cleanup =
self.handle_undelegation_error(transaction_strategy);
Ok(ControlFlow::Continue(to_cleanup))
}
TransactionStrategyExecutionError::CpiLimitError(_, _) => {
// Can't be handled in scope of single stage execution
// We signal flow break
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -174,7 +174,13 @@ where
TransactionStrategyExecutionError::ActionsError(_, _) => {
// Unexpected in Two Stage commit
// That would mean that Two Stage executes Standalone commit
error!("Unexpected error in Two stage commit flow: {}", err);
error!("Unexpected error in two stage commit flow: {}", err);
Ok(ControlFlow::Break(()))
}
TransactionStrategyExecutionError::UndelegationError(_, _) => {
// Unexpected in Two Stage commit
// That would mean that Two Stage executes undelegation in commit phase
error!("Unexpected error in two stage commit flow: {}", err);
Ok(ControlFlow::Break(()))
}
TransactionStrategyExecutionError::CpiLimitError(_, _) => {
Expand Down Expand Up @@ -213,6 +219,13 @@ where
let to_cleanup = self.handle_actions_error(finalize_strategy);
Ok(ControlFlow::Continue(to_cleanup))
}
TransactionStrategyExecutionError::UndelegationError(_, _) => {
// Here we patch strategy for it to be retried in next iteration
// & we also record data that has to be cleaned up after patch
let to_cleanup =
self.handle_undelegation_error(finalize_strategy);
Ok(ControlFlow::Continue(to_cleanup))
}
TransactionStrategyExecutionError::CpiLimitError(_, _) => {
// Can't be handled
warn!("Finalization tasks exceeded CpiLimitError: {}", err);
Expand Down
Loading
Loading