feat: invoke CommitFinalize by default instead of Commit+Finalize#1174
feat: invoke CommitFinalize by default instead of Commit+Finalize#1174
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a mode-aware commit flow with a new public enum Suggested reviewers
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Manual Deploy AvailableYou can trigger a manual deploy of this PR branch to testnet: Alternative: Comment
Comment updated automatically when the PR is synchronized. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
magicblock-committor-service/src/tasks/task_builder.rs (1)
184-209: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winReturn early when there are no committed accounts.
execute_inner(...)now routes standalone-action intents through this method, but this block still callsfetch_next_commit_noncesandget_base_accountswith an empty key list. That adds avoidable async work and makes action-only execution depend on everyTaskInfoFetcheraccepting empty inputs.Suggested change
let committed_accounts = intent_bundle.get_all_committed_accounts(); + if committed_accounts.is_empty() { + return Ok(tasks); + } // Get commit nonces and base accounts let min_context_slot = committed_accounts🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@magicblock-committor-service/src/tasks/task_builder.rs` around lines 184 - 209, This block should return early when there are no committed accounts to avoid calling fetch_commit_nonces/fetch_diffable_accounts with empty inputs: after computing committed_accounts (from intent_bundle.get_all_committed_accounts()) check committed_accounts.is_empty() and short-circuit the function (or return the appropriate action-only result) instead of executing tokio::join; otherwise, when non-empty proceed to compute min_context_slot and call Self::fetch_commit_nonces and Self::fetch_diffable_accounts as before. Ensure any downstream variables used later (commit_ids, base_accounts) are set to sensible defaults or not referenced if you short-circuit, and reference symbols: execute_inner, intent_bundle, committed_accounts, fetch_commit_nonces, fetch_diffable_accounts, task_info_fetcher, and min_context_slot to locate the change.magicblock-committor-service/src/intent_executor/mod.rs (1)
382-406:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftRebuild the CPI-limit fallback instead of reusing
CommitFinalizetasks.With
CommitFinalizenow being the default task shape,handle_cpi_limit_error(...)can hand this branch acommit_strategythat still consists entirely ofBaseTaskImpl::CommitFinalizetasks and an emptyfinalize_strategy. In that case this path just retries the same CPI-heavy instructions and then skips finalize entirely, so a single-stageCpiLimitErroron a commit-only bundle no longer has a real recovery path.Please rebuild the fallback in
SeparateCommitAndFinalizemode here, or explicitly splitCommitFinalizetasks intoCommit+Finalizebefore entering two-stage execution. Otherwise large commit bundles will still fail after the retry.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@magicblock-committor-service/src/intent_executor/mod.rs` around lines 382 - 406, The current flow may pass a commit_strategy composed of BaseTaskImpl::CommitFinalize tasks with an empty finalize_strategy into TwoStageExecutor, causing retries to re-run CPI-heavy combined tasks; change the setup before calling TwoStageExecutor::new / execute_with_timeout so the executor runs in SeparateCommitAndFinalize mode: either rebuild the CPI-limit fallback by converting CommitFinalize tasks into explicit Commit tasks and corresponding Finalize tasks (splitting each CommitFinalize into two tasks and populating finalize_strategy), or detect the CommitFinalize-only case and construct a SeparateCommitAndFinalize commit_strategy/finalize_strategy pair (so CommitStage executes lightweight commit-only tasks and the finalize stage runs the finalize tasks) before executing CommitStage/TwoStageExecutor; reference functions/types: CommitFinalize, TwoStageExecutor::new, CommitStage, handle_cpi_limit_error, SeparateCommitAndFinalize, commit_strategy, finalize_strategy.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@magicblock-committor-service/src/intent_executor/mod.rs`:
- Around line 382-406: The current flow may pass a commit_strategy composed of
BaseTaskImpl::CommitFinalize tasks with an empty finalize_strategy into
TwoStageExecutor, causing retries to re-run CPI-heavy combined tasks; change the
setup before calling TwoStageExecutor::new / execute_with_timeout so the
executor runs in SeparateCommitAndFinalize mode: either rebuild the CPI-limit
fallback by converting CommitFinalize tasks into explicit Commit tasks and
corresponding Finalize tasks (splitting each CommitFinalize into two tasks and
populating finalize_strategy), or detect the CommitFinalize-only case and
construct a SeparateCommitAndFinalize commit_strategy/finalize_strategy pair (so
CommitStage executes lightweight commit-only tasks and the finalize stage runs
the finalize tasks) before executing CommitStage/TwoStageExecutor; reference
functions/types: CommitFinalize, TwoStageExecutor::new, CommitStage,
handle_cpi_limit_error, SeparateCommitAndFinalize, commit_strategy,
finalize_strategy.
In `@magicblock-committor-service/src/tasks/task_builder.rs`:
- Around line 184-209: This block should return early when there are no
committed accounts to avoid calling fetch_commit_nonces/fetch_diffable_accounts
with empty inputs: after computing committed_accounts (from
intent_bundle.get_all_committed_accounts()) check committed_accounts.is_empty()
and short-circuit the function (or return the appropriate action-only result)
instead of executing tokio::join; otherwise, when non-empty proceed to compute
min_context_slot and call Self::fetch_commit_nonces and
Self::fetch_diffable_accounts as before. Ensure any downstream variables used
later (commit_ids, base_accounts) are set to sensible defaults or not referenced
if you short-circuit, and reference symbols: execute_inner, intent_bundle,
committed_accounts, fetch_commit_nonces, fetch_diffable_accounts,
task_info_fetcher, and min_context_slot to locate the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: d8b9a6d9-6469-480b-a0c6-43455a6a9f44
📒 Files selected for processing (7)
magicblock-committor-service/src/intent_executor/error.rsmagicblock-committor-service/src/intent_executor/mod.rsmagicblock-committor-service/src/intent_executor/single_stage_executor.rsmagicblock-committor-service/src/intent_executor/two_stage_executor.rsmagicblock-committor-service/src/intent_executor/utils.rsmagicblock-committor-service/src/tasks/task_builder.rsmagicblock-committor-service/src/tasks/task_strategist.rs
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@magicblock-committor-service/src/tasks/task_builder.rs`:
- Around line 225-283: The two inner functions create_commit_finalize_task and
create_commit_task duplicate the commit_id lookup and base_account extraction
logic; refactor by extracting that common logic into a helper (e.g.,
prepare_commit_data(intent_id, commit_ids, base_accounts, account) -> (u64,
Option<Account)) or accept a closure that builds the final task, then have
create_commit_finalize_task and create_commit_task call the helper and only
differ in invoking TaskBuilderImpl::create_commit_finalize_task versus
TaskBuilderImpl::create_commit_task with the returned commit_id and
base_account; ensure you reference the same symbols (commit_ids, base_accounts,
intent_id, CommittedAccount, and TaskBuilderImpl::create_commit*_task) and
preserve the existing error logging behavior when commit_id is absent.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: a9a861da-ada6-4d9f-ab1f-1c75d1456269
📒 Files selected for processing (1)
magicblock-committor-service/src/tasks/task_builder.rs
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@magicblock-committor-service/src/tasks/task_builder.rs`:
- Around line 221-232: The code currently treats a missing commit id as nonce 0
(e.g., places where commit_ids.get(pubkey).copied().unwrap_or(0) is used) and
also silently skips persisting absent entries in the loop that calls
persister.set_commit_id(intent_bundle.id, pubkey, *commit_id); change this so
that any missing commit id causes task construction to fail immediately: replace
any unwrap_or(0) usage with explicit presence checks on commit_ids (e.g.,
commit_ids.get(pubkey)), return an Err (with contextual message including
intent_bundle.id and pubkey) if absent, and ensure the persister loop
returns/panics on persister.set_commit_id failures rather than silently
continuing; apply the same change to the other two blocks noted (the sections
around the current unwrap_or(0) sites at the other ranges).
- Around line 618-745: Add assertions in the four tests to verify the routing of
WithBaseActions/bundled base-actions: for the combined flow tests that call
TaskBuilderImpl::commit_tasks and TaskBuilderImpl::finalize_tasks assert that
the CommitFinalize variant (BaseTaskImpl::CommitFinalize) contains the
WithBaseActions bundle/work (or the field that holds base actions) and that
finalize_tasks is empty or only contains Undelegate as before; for the separate
mode tests that call commit_tasks_with_mode and finalize_tasks_with_mode assert
that the Commit variant does NOT hold the WithBaseActions and instead the
Finalize variant (BaseTaskImpl::Finalize) carries the WithBaseActions bundle,
and in the undelegate case assert the finalize_tasks slice contains Finalize
then Undelegate with the Finalize carrying base-actions; reference
TaskBuilderImpl::commit_tasks, TaskBuilderImpl::finalize_tasks,
TaskBuilderImpl::commit_tasks_with_mode,
TaskBuilderImpl::finalize_tasks_with_mode and the BaseTaskImpl enum variants
(CommitFinalize, Commit, Finalize, Undelegate, WithBaseActions) when adding
these assertions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: c27f6a93-b166-43b3-9047-9e3ec408e8de
📒 Files selected for processing (2)
magicblock-committor-service/src/intent_executor/mod.rsmagicblock-committor-service/src/tasks/task_builder.rs
| for (pubkey, commit_id) in &commit_ids { | ||
| if let Err(err) = | ||
| persister.set_commit_id(intent_bundle.id, pubkey, *commit_id) | ||
| { | ||
| error!( | ||
| intent_id = intent_bundle.id, | ||
| pubkey = %pubkey, | ||
| error = ?err, | ||
| "Failed to persist commit id" | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
Don't synthesize a missing commit nonce as 0.
Line 242 and Line 272 currently fall back to 0 when commit_ids lacks a pubkey. 0 is a valid nonce in this file's own mock fetcher (Lines 586-590), so this can silently turn an incomplete fetch into a real commit instead of failing/retrying. The loop at Lines 221-232 also skips persisting missing entries, leaving task execution and persisted state out of sync. Please fail task construction when a commit id is absent rather than defaulting it.
Also applies to: 241-252, 271-282
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@magicblock-committor-service/src/tasks/task_builder.rs` around lines 221 -
232, The code currently treats a missing commit id as nonce 0 (e.g., places
where commit_ids.get(pubkey).copied().unwrap_or(0) is used) and also silently
skips persisting absent entries in the loop that calls
persister.set_commit_id(intent_bundle.id, pubkey, *commit_id); change this so
that any missing commit id causes task construction to fail immediately: replace
any unwrap_or(0) usage with explicit presence checks on commit_ids (e.g.,
commit_ids.get(pubkey)), return an Err (with contextual message including
intent_bundle.id and pubkey) if absent, and ensure the persister loop
returns/panics on persister.set_commit_id failures rather than silently
continuing; apply the same change to the other two blocks noted (the sections
around the current unwrap_or(0) sites at the other ranges).
snawaz
force-pushed
the
snawaz/commit-finalize
branch
from
0486bf9 to
4a27adf
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test-integration/test-committor-service/tests/test_intent_executor.rs (1)
860-1017: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winMake
patched_errorsassertions order-independent (test robustness).This test asserts the order of
patched_errorsviaiter.next()(Line 964-968), which can easily become flaky if the executor’s patching iteration order changes (e.g., due to mode changes likeCommitFinalizeand any internal reordering). Since you already assertpatched_errors.len() == 3(Line 950-955), it’s better to assert presence of each expected error variant regardless of order.♻️ Proposed diff
- let mut iter = patched_errors.iter(); - let commit_id_error = iter.next().unwrap(); - let cpi_limit_err = iter.next().unwrap(); - let action_error = iter.next().unwrap(); - - assert!(matches!( - commit_id_error, - TransactionStrategyExecutionError::CommitIDError(_, _) - )); - assert!(matches!( - cpi_limit_err, - TransactionStrategyExecutionError::CpiLimitError(_, _) - )); - assert!(matches!( - action_error, - TransactionStrategyExecutionError::ActionsError(_, _) - )); + assert!( + patched_errors + .iter() + .any(|e| matches!(e, TransactionStrategyExecutionError::CommitIDError(_, _))), + "expected CommitIDError in patched_errors: {patched_errors:#?}" + ); + assert!( + patched_errors + .iter() + .any(|e| matches!(e, TransactionStrategyExecutionError::CpiLimitError(_, _))), + "expected CpiLimitError in patched_errors: {patched_errors:#?}" + ); + assert!( + patched_errors + .iter() + .any(|e| matches!(e, TransactionStrategyExecutionError::ActionsError(_, _))), + "expected ActionsError in patched_errors: {patched_errors:#?}" + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test-integration/test-committor-service/tests/test_intent_executor.rs` around lines 860 - 1017, The test currently assumes a fixed order by pulling patched_errors via iter.next(); instead make the assertions order-independent by checking presence of each variant in patched_errors (use patched_errors.iter().any(|e| matches!(e, TransactionStrategyExecutionError::CommitIDError(_, _))) and similarly for CpiLimitError and ActionsError) and remove the iter.next()/ordered binds (commit_id_error, cpi_limit_err, action_error); keep the length check and the TwoStage match as-is so the test verifies three patches exist and that each expected error variant is present regardless of order.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test-integration/test-committor-service/tests/test_ix_commit_local.rs`:
- Around line 1001-1015: The test disables commit-flow checks by hard-setting
has_commit_flow = false, making the two-stage Commit+Finalize assertions
unreachable; remove the assignment that overwrites has_commit_flow and keep the
workaround that sets has_commit_finalize_flow = has_commit_finalize_flow ||
has_commit_flow so both flags reflect the runtime behavior (i.e., delete the
line "let has_commit_flow = false" or the reassignment shadowing
has_commit_flow) and leave the subsequent assertions using has_commit_flow and
has_commit_finalize_flow intact.
In `@test-integration/test-runner/bin/run_tests.rs`:
- Around line 287-297: The change hardcodes RunTestConfig to only run
"test_ix_commit_local" for package "schedulecommit-committor-service", which
narrows CI coverage; revert or modify the RunTestConfig in run_tests.rs so it
does not restrict test_file (e.g., set test_file: None or remove the field and
use RunTestConfig::default() / the previous behavior) so all committor
integration tests in schedulecommit-committor-service run by default; ensure any
intentional selective runs are gated behind an explicit CLI flag or env var
instead of being the default in the RunTestConfig construction.
---
Outside diff comments:
In `@test-integration/test-committor-service/tests/test_intent_executor.rs`:
- Around line 860-1017: The test currently assumes a fixed order by pulling
patched_errors via iter.next(); instead make the assertions order-independent by
checking presence of each variant in patched_errors (use
patched_errors.iter().any(|e| matches!(e,
TransactionStrategyExecutionError::CommitIDError(_, _))) and similarly for
CpiLimitError and ActionsError) and remove the iter.next()/ordered binds
(commit_id_error, cpi_limit_err, action_error); keep the length check and the
TwoStage match as-is so the test verifies three patches exist and that each
expected error variant is present regardless of order.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7a7731ad-e602-4145-80bc-be375091a87f
📒 Files selected for processing (4)
test-integration/test-committor-service/tests/test_intent_executor.rstest-integration/test-committor-service/tests/test_ix_commit_local.rstest-integration/test-committor-service/tests/utils/transactions.rstest-integration/test-runner/bin/run_tests.rs
snawaz
force-pushed
the
snawaz/commit-finalize
branch
from
4a27adf to
2cbbc7d
Compare
snawaz
force-pushed
the
snawaz/commit-finalize
branch
from
2cbbc7d to
4efacdb
Compare
There was a problem hiding this comment.
Where could I read on task that this PR closes?
From the title it would seem that changes shall come from magic-program and not by setting mode for building tasks.
Users shall be able to choose the flow: standart, commit+finalize. Now if they choose standard(old) flow we shouldn't implicitly convert it for them.
The change that I see:
process_schedule_commitpractically redirects intoprocess_schedule_commit_finalize. We communicate this to users- Users still have an option to create standart commit flow via
MagicIntentBundleBuilder
Both are supported
| } | ||
| _ => { | ||
| error!( | ||
| task_index = ?task_index, |
There was a problem hiding this comment.
This is practically duplicates else block below.
This can be avoided simply by
let optimized_tasks =
self.transaction_strategy.optimized_tasks.as_slice();
if let Some(delegate_account) = err
.task_index()
.and_then(|index| optimized_tasks.get(index as usize))
.and_then(|el| {
match el {
BaseTaskImpl::Commit(task) => {
Some(task.committed_account.pubkey)
}
BaseTaskImpl::CommitFinalize(task) => {
Some(task.committed_account.pubkey)
}
_=> None
}
})
{
self.handle_unfinalized_account_error(
signature, delegate_account, transaction_preparator
)
.await
} else {
error!(
task_index = err.task_index(),
optimized_tasks_len = optimized_tasks.len(),
error = ?err,
"RPC returned unexpected task index"
);
Ok(ControlFlow::Break(()))
}| task.committed_account.pubkey | ||
| } | ||
| _ => { | ||
| error!( |
There was a problem hiding this comment.
This is practically duplicates else block below.
This can be avoided simply by
let optimized_tasks =
self.transaction_strategy.optimized_tasks.as_slice();
if let Some(delegate_account) = err
.task_index()
.and_then(|index| optimized_tasks.get(index as usize))
.and_then(|el| {
match el {
BaseTaskImpl::Commit(task) => {
Some(task.committed_account.pubkey)
}
BaseTaskImpl::CommitFinalize(task) => {
Some(task.committed_account.pubkey)
}
_=> None
}
})
{
self.handle_unfinalized_account_error(
signature, delegate_account, transaction_preparator
)
.await
} else {
error!(
task_index = err.task_index(),
optimized_tasks_len = optimized_tasks.len(),
error = ?err,
"RPC returned unexpected task index"
);
Ok(ControlFlow::Break(()))
}| ) -> TaskStrategistResult<StrategyExecutionMode> { | ||
| const MAX_UNITED_TASKS_LEN: usize = 22; | ||
|
|
||
| if commit_tasks.is_empty() || finalize_tasks.is_empty() { |
There was a problem hiding this comment.
What do empty commit_tasks mean? From what i know this shouldn't be possible and if it is should be an error
2 participants
This PR changes (non-breaking) scheduled commits to use the DLP
CommitFinalizeinstruction instead of executing separateCommitandFinalizeinstructions.Naming problem
MagicBlockInstruction::ScheduleCommitwas already a misleading name because it never meant "commit only"; it always scheduled a commit followed by finalize. But now, with this PR, the codebase feels even more misleading because the underlying DLP operation is now explicitlyCommitFinalizewhile we also haveCommit(which also executesFinalizeimplicitly).Possible fix
Rename the existing
ScheduleCommittoScheduleCommitFinalizesince that is what it actually does (even before this PR). Rename the currently unusedScheduleCommitFinalizeenum slot toScheduleCommitand leave it unimplemented for now because commit-only scheduling semantics are not yet clear and we never supported that. This keeps the enum shape stable while making the active API naming accurate.Summary by CodeRabbit
New Features
Bug Fixes
Tests