Skip to content

refactor(committor): remove legacy Commit and Finalize tasks #1764

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
snawaz wants to merge 2 commits into from
Open

refactor(committor): remove legacy Commit and Finalize tasks #1764

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
cc8dab4
refactor(committor): remove legacy Commit and Finalize tasks
snawaz Oct 5, 2026
7ecff47
Remove unused committor helpers and consolidate combined-commit builders
snawaz Oct 5, 2026
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
14 changes: 1 addition & 13 deletions magicblock-committor-service/src/committor_processor.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,8 +31,7 @@ use crate::{
},
},
persist::{
CommitStatusRow, IntentPersister, IntentPersisterImpl,
MessageSignatures, RecoveredIntent,
CommitStatusRow, IntentPersister, IntentPersisterImpl, RecoveredIntent,
},
};
const POISONED_MUTEX_MSG: &str =
Expand Down Expand Up @@ -143,17 +142,6 @@ impl CommittorProcessor {
Ok(commit_statuses)
}

pub fn get_commit_signature(
&self,
commit_id: u64,
pubkey: Pubkey,
) -> CommittorServiceResult<Option<MessageSignatures>> {
let signatures = self
.persister
.get_signatures_by_commit(commit_id, &pubkey)?;
Ok(signatures)
}

fn recovery_min_created_at() -> u64 {
SystemTime::now()
.duration_since(UNIX_EPOCH)
Expand Down
96 changes: 13 additions & 83 deletions magicblock-committor-service/src/intent_executor/error.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -208,8 +208,6 @@ pub enum TransactionStrategyExecutionError {
#[source] TransactionError,
Option<Signature>,
),
#[error("Unfinalized account error: {0}, {1:?}")]
UnfinalizedAccountError(#[source] TransactionError, Option<Signature>),
#[error("Transaction too large to send over the wire: {0}")]
TransactionTooLargeError(#[source] InternalError),
#[error("InternalError: {0}")]
Expand Down Expand Up @@ -245,40 +243,13 @@ impl TransactionStrategyExecutionError {
|| matches!(self, Self::TransactionTooLargeError(_))
}

pub fn task_index(&self, task_instruction_offset: u8) -> Option<u8> {
match self {
Self::CommitIDError(
TransactionError::InstructionError(index, _),
_,
)
| Self::ActionsError(
TransactionError::InstructionError(index, _),
_,
)
| Self::UndelegationError(
TransactionError::InstructionError(index, _),
_,
)
| Self::UnfinalizedAccountError(
TransactionError::InstructionError(index, _),
_,
)
| Self::CpiLimitError(
TransactionError::InstructionError(index, _),
_,
) => index.checked_sub(task_instruction_offset),
_ => None,
}
}

pub fn signature(&self) -> Option<Signature> {
match self {
Self::InternalError(err) => err.signature(),
Self::TransactionTooLargeError(err) => err.signature(),
Self::CommitIDError(_, signature)
| Self::ActionsError(_, signature)
| Self::UndelegationError(_, signature)
| Self::UnfinalizedAccountError(_, signature)
| Self::CpiLimitError(_, signature)
| Self::LoadedAccountsDataSizeExceeded(_, signature) => *signature,
}
Expand All @@ -296,16 +267,6 @@ impl TransactionStrategyExecutionError {
// Commit Nonce order error
const NONCE_OUT_OF_ORDER: u32 =
dlp_api::error::DlpError::NonceOutOfOrder as u32;
// Errors when commit state already exists
const COMMIT_STATE_INVALID_ACCOUNT_OWNER: u32 =
dlp_api::error::DlpError::CommitStateInvalidAccountOwner as u32;
const COMMIT_STATE_ALREADY_INITIALIZED: u32 =
dlp_api::error::DlpError::CommitStateAlreadyInitialized as u32;
const COMMIT_RECORD_INVALID_ACCOUNT_OWNER: u32 =
dlp_api::error::DlpError::CommitRecordInvalidAccountOwner as u32;
const COMMIT_RECORD_ALREADY_INITIALIZED: u32 =
dlp_api::error::DlpError::CommitRecordAlreadyInitialized as u32;

match err {
// Some tx may use too much CPIs and we can handle it in certain cases
transaction_err @ TransactionError::InstructionError(
Expand Down Expand Up @@ -338,8 +299,7 @@ impl TransactionStrategyExecutionError {

match (task, instruction_err) {
(
BaseTaskImpl::Commit(_)
| BaseTaskImpl::CommitFinalize(_),
BaseTaskImpl::CommitFinalize(_),
instruction_err,
) => match instruction_err {
InstructionError::Custom(NONCE_OUT_OF_ORDER) => Ok(
Expand All @@ -350,25 +310,6 @@ impl TransactionStrategyExecutionError {
signature,
),
),
// Only legacy commits use the temporary state/record
// PDAs that a separate Finalize can recover.
instruction_err @ (InstructionError::Custom(
COMMIT_STATE_INVALID_ACCOUNT_OWNER,
)
| InstructionError::Custom(
COMMIT_STATE_ALREADY_INITIALIZED,
)
| InstructionError::Custom(
COMMIT_RECORD_INVALID_ACCOUNT_OWNER,
)
| InstructionError::Custom(
COMMIT_RECORD_ALREADY_INITIALIZED,
)) if matches!(task, BaseTaskImpl::Commit(_)) => {
Ok(TransactionStrategyExecutionError::UnfinalizedAccountError(
tx_err_helper(instruction_err),
signature
))
}
err => Err(tx_err_helper(err)),
},
(BaseTaskImpl::BaseAction(_), instruction_err) => {
Expand All @@ -383,7 +324,6 @@ impl TransactionStrategyExecutionError {
signature,
),
),
(_, instruction_err) => Err(tx_err_helper(instruction_err)),
}
}
// This means transaction failed to other reasons that we don't handle - propagate
Expand All @@ -405,7 +345,6 @@ impl metrics::LabelValue for TransactionStrategyExecutionError {
}
Self::CommitIDError(_, _) => "commit_nonce_failed",
Self::UndelegationError(_, _) => "undelegation_failed",
Self::UnfinalizedAccountError(_, _) => "unfinalized_account_failed",
Self::TransactionTooLargeError(_) => "transaction_too_large",
_ => "failed",
}
Expand Down Expand Up @@ -482,7 +421,7 @@ mod tests {
const TX_TOO_LARGE_SOLANA: &str = "base64 encoded too large";

#[test]
fn combined_commits_do_not_trigger_legacy_finalization() {
fn combined_commit_errors_use_message_instruction_offset() {
use dlp_api::error::DlpError;
use magicblock_core::intent::types::CommittedAccount;
use solana_account::Account;
Expand All @@ -491,38 +430,29 @@ mod tests {
use solana_transaction_error::TransactionError;

use crate::tasks::utils::{
create_commit_finalize_task, create_commit_task, TransactionUtils,
create_commit_finalize_task, TransactionUtils,
};

let account = CommittedAccount {
pubkey: Pubkey::new_unique(),
account: Account::default(),
remote_slot: 0,
};
let legacy = create_commit_task(1, false, account.clone(), None).into();
let combined =
create_commit_finalize_task(1, false, account, None).into();
let pending_errors = [
DlpError::CommitStateInvalidAccountOwner,
DlpError::CommitStateAlreadyInitialized,
DlpError::CommitRecordInvalidAccountOwner,
DlpError::CommitRecordAlreadyInitialized,
];
for offset in [0, TransactionUtils::COMPUTE_BUDGET_INSTRUCTION_COUNT] {
for pending_error in &pending_errors {
let error = TransactionError::InstructionError(
let unrelated_error = TransactionError::InstructionError(
offset,
InstructionError::InvalidAccountData,
);
let result =
TransactionStrategyExecutionError::try_from_transaction_error(
unrelated_error.clone(),
None,
std::slice::from_ref(&combined),
offset,
InstructionError::Custom(*pending_error as u32),
);
let legacy_result = TransactionStrategyExecutionError::try_from_transaction_error(
error.clone(), None, std::slice::from_ref(&legacy), offset,
);
assert!(matches!(legacy_result, Ok(TransactionStrategyExecutionError::UnfinalizedAccountError(_, _))));
let combined_result = TransactionStrategyExecutionError::try_from_transaction_error(
error.clone(), None, std::slice::from_ref(&combined), offset,
);
assert_eq!(combined_result.unwrap_err(), error);
}
assert_eq!(result.unwrap_err(), unrelated_error);
let nonce_error = TransactionError::InstructionError(
offset,
InstructionError::Custom(DlpError::NonceOutOfOrder as u32),
Expand Down
19 changes: 10 additions & 9 deletions magicblock-committor-service/src/intent_executor/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ use crate::{
},
persist::{CommitStatus, CommitStatusSignatures, IntentPersister},
tasks::{
task_builder::{TaskBuilderImpl, TasksBuilder},
task_builder::TaskBuilderImpl,
task_strategist::{
StrategyExecutionMode, TaskStrategist, TransactionStrategy,
},
Expand Down Expand Up @@ -443,10 +443,9 @@ where
)
.await?;

let finalized_stage = finalize_executor.done(finalize_signature);
Ok(ExecutionOutput::TwoStage {
commit_signature: finalized_stage.commit_signature,
finalize_signature: finalized_stage.finalize_signature,
commit_signature,
finalize_signature,
})
}

Expand Down Expand Up @@ -654,7 +653,6 @@ where
/// alias its signature. Such intents must carry a per-intent uniqueness noop.
fn requires_uniqueness_nonce(commit_tasks: &[BaseTaskImpl]) -> bool {
commit_tasks.iter().any(|task| match task {
BaseTaskImpl::Commit(task) => task.commit_id <= 1,
BaseTaskImpl::CommitFinalize(task) => task.commit_id <= 1,
_ => false,
})
Expand All @@ -666,10 +664,10 @@ mod tests {
use solana_account::Account;

use super::*;
use crate::tasks::{utils::create_commit_task, FinalizeTask};
use crate::tasks::{utils::create_commit_finalize_task, UndelegateTask};

fn commit_task(commit_id: u64) -> BaseTaskImpl {
create_commit_task(
create_commit_finalize_task(
commit_id,
false,
CommittedAccount {
Expand All @@ -684,14 +682,17 @@ mod tests {

#[test]
fn test_requires_uniqueness_nonce_on_first_commit_only() {
let finalize = BaseTaskImpl::Finalize(FinalizeTask {
let undelegate = BaseTaskImpl::Undelegate(UndelegateTask {
delegated_account: Pubkey::new_unique(),
owner_program: Pubkey::new_unique(),
rent_reimbursement: Pubkey::new_unique(),
include_undelegation_request: false,
});

assert!(requires_uniqueness_nonce(&[commit_task(1)]));
assert!(requires_uniqueness_nonce(&[commit_task(5), commit_task(1)]));
assert!(!requires_uniqueness_nonce(&[commit_task(2)]));
assert!(!requires_uniqueness_nonce(&[finalize]));
assert!(!requires_uniqueness_nonce(&[undelegate]));
assert!(!requires_uniqueness_nonce(&[]));
}
}
Loading
Loading