Report Intent's patched errors #667
Conversation
feat: add metrics for task preparation & alt preparation
# Conflicts: # test-integration/test-committor-service/tests/test_intent_executor.rs
feat: trigger SentCommit on intent failure and failed retry
… feat/base-layer-ix/improve-executors # Conflicts: # magicblock-accounts/src/scheduled_commits_processor.rs # test-integration/programs/flexi-counter/src/processor.rs
feat: SentCommit fails if underlying intent failed to execute on Base fix: compilation
feat: trigger SentCommit on intent failure and failed retry
… feat/base-layer-ix/improve-executors # Conflicts: # magicblock-accounts/src/scheduled_commits_processor.rs # programs/magicblock/src/schedule_transactions/process_scheduled_commit_sent.rs # test-integration/test-committor-service/tests/test_intent_executor.rs
📝 WalkthroughWalkthroughThis PR refactors the intent execution result handling and error propagation pipeline. The changes introduce Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
test-integration/test-committor-service/tests/test_intent_executor.rs (2)
329-395: Use ofIntentExecutionResultandpatched_errorsin recovery tests is mostly solid; watch ordering assumptionsThe recovery tests (
test_commit_id_error_recovery,test_undelegation_error_recovery,test_action_error_recovery,test_commit_id_and_action_errors_recovery) all:
Call
intent_executor.execute(...)and destructureIntentExecutionResultas:let IntentExecutionResult { inner: res, patched_errors } = res;so
resbecomes the innerResult<ExecutionOutput, TransactionStrategyExecutionError>, andpatched_errorsis the collected patch trace.Assert:
res.is_ok()andmatches!(res.unwrap(), ExecutionOutput::SingleStage(_))(i.e., execution recovered to a single‑stage success).patched_errors.len()matches the expected number of patches.- The patched error variants are what you expect (CommitIDError, UndelegationError, ActionsError).
Call
intent_executor.cleanup().awaitand then verify on‑chain state and TableMania ref‑counts viaverify/verify_committed_accounts_state/verify_table_mania_released.This is a good pattern and uses the new
IntentExecutionResultsurface correctly.One thing to consider: in
test_commit_id_and_action_errors_recovery, you additionally assume a specific ordering of patches:let mut iter = patched_errors.into_iter(); let commit_id_error = iter.next().unwrap(); let actions_error = iter.next().unwrap();If patch order is not guaranteed as part of the public contract, you may want to relax this by e.g. collecting variants into a multiset and asserting on membership/counts rather than sequence, to avoid brittle tests when recovery logic is refactored but remains semantically equivalent.
If patch ordering is intended to be stable API, consider documenting that guarantee near the
patched_errorsdefinition; otherwise, loosening these tests will better tolerate future internal changes.Also applies to: 396-449, 451-515, 517-596
598-683: Direct access tointent_executor.patched_errorsis intentional but tightly couples tests to executor internalsIn the two‑stage/CPI‑limit recovery tests:
test_cpi_limits_error_recoveryusessingle_stage_execution_flowand only checks the returnedExecutionOutput::TwoStage { .. }plus post‑cleanup state.
test_commit_id_actions_cpi_limit_errors_recoveryadditionally inspects the executor’s internalpatched_errorsvia:let res = intent_executor .single_stage_execution_flow(...) .await; let patched_errors = mem::take(&mut intent_executor.patched_errors); assert!(res.is_ok()); assert_eq!(patched_errors.len(), 3, "Only 3 patches expected"); // then match CommitIDError, CpiLimitError, ActionsErrorThis makes sense given the current design, where
single_stage_execution_flowitself doesn’t return anIntentExecutionResultbut the executor tracks patched errors as state. Usingmem::taketo consume and reset that state is a clean way to avoid leaking patches between runs.However, it does mean:
- Tests rely on
IntentExecutorImpl.patched_errorsbeing a public field, similar to the documented pattern forSingleStageExecutor/TwoStageExecutorpatched_errors. If that visibility ever changes, these tests will break. A small improvement would be to expose a method likedrain_patched_errors()on the executor instead of touching the field directly. Based on learnings.- As with the single‑stage tests, you assert a specific ordering of the three patches (CommitID, CpiLimit, then Actions). If ordering is not part of the external contract, consider asserting only on the set of variants and the length.
Functionally, the cleanup and verification steps (
verify,verify_buffers_cleaned_upwith adjustedinvalidated_keys, and TableMania checks) are coherent and match the intent of ensuring both commit‑related buffers and lookup tables are cleaned up after recovery.If you’d like, I can sketch a
drain_patched_errors(&mut self) -> Vec<TransactionStrategyExecutionError>helper onIntentExecutorImpland update the tests to use it, to decouple them from the field layout.Also applies to: 685-825
magicblock-committor-service/src/stubs/changeset_committor_stub.rs (1)
18-26: Stub result construction matches new broadcast typeThe stub now constructs
BroadcastedIntentExecutionResultwith the right shape (id,trigger_type,inner: Ok(ExecutionOutput::TwoStage { .. }), andpatched_errors), so consumers can transparently consume it like the real engine output. If you want to tighten style a bit,patched_errors: Arc::default()(orArc::new(Vec::new())) would avoid an explicitvec![], but current code is functionally correct.Also applies to: 190-208
magicblock-committor-service/src/intent_execution_manager/intent_execution_engine.rs (1)
268-317: execute() now integrates IntentExecutionResult, broadcasting, and async cleanup correctlyThe updated
executewrapper awaitsexecutor.execute(...)into anIntentExecutionResult, logs failures viaresult.inner.inspect_err, records metrics against&result.inner, wraps everything intoBroadcastedIntentExecutionResult::new(...), and then spawns a background task that movesexecutorand awaitsexecutor.cleanup(), logging anyBufferExecutionError. This ensures callers always receive a fully‑populated broadcast (including patched_errors) while decoupling potentially heavy cleanup from the hot path, and still unblocking the scheduler viacomplete(&intent.inner)and dropping the semaphore permit at the end.Also applies to: 322-357
magicblock-committor-service/src/intent_executor/two_stage_executor.rs (1)
143-178: Commit/finalize patching rules and new undelegation handling are consistent with task layout
patch_commit_strategyonly patchesCommitIDErrorand treatsActionsError/UndelegationErroras unexpected in the commit phase, which matches the expectation that non‑commit tasks (actions/undelegations) should not appear in the commit‑only strategy produced byhandle_cpi_limit_error.patch_finalize_strategycorrectly mirrors the single‑stage semantics by:
- treating
ActionsErroras patchable viahandle_actions_error,- adding
UndelegationErroras patchable viahandle_undelegation_error,- and breaking for
CommitIDError,CpiLimitError, and internal errors.- One small nit: the log message for
CommitIDErrorinpatch_finalize_strategystill says “two stage commit flow” even though this is the finalize stage; adjusting the wording would make logs clearer when debugging finalize vs commit issues.Also applies to: 261-295
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (25)
magicblock-accounts/src/scheduled_commits_processor.rs(6 hunks)magicblock-committor-service/src/intent_execution_manager.rs(1 hunks)magicblock-committor-service/src/intent_execution_manager/intent_execution_engine.rs(16 hunks)magicblock-committor-service/src/intent_executor/error.rs(6 hunks)magicblock-committor-service/src/intent_executor/mod.rs(8 hunks)magicblock-committor-service/src/intent_executor/single_stage_executor.rs(4 hunks)magicblock-committor-service/src/intent_executor/task_info_fetcher.rs(2 hunks)magicblock-committor-service/src/intent_executor/two_stage_executor.rs(6 hunks)magicblock-committor-service/src/service_ext.rs(1 hunks)magicblock-committor-service/src/stubs/changeset_committor_stub.rs(2 hunks)magicblock-committor-service/src/tasks/args_task.rs(2 hunks)magicblock-committor-service/src/tasks/buffer_task.rs(2 hunks)magicblock-committor-service/src/tasks/mod.rs(2 hunks)magicblock-committor-service/src/tasks/task_builder.rs(2 hunks)magicblock-committor-service/src/transaction_preparator/delivery_preparator.rs(5 hunks)magicblock-committor-service/src/transaction_preparator/error.rs(2 hunks)magicblock-committor-service/src/transaction_preparator/mod.rs(3 hunks)magicblock-metrics/src/metrics/mod.rs(5 hunks)programs/magicblock/src/schedule_transactions/process_scheduled_commit_sent.rs(6 hunks)test-integration/programs/flexi-counter/src/processor.rs(2 hunks)test-integration/programs/flexi-counter/src/state.rs(1 hunks)test-integration/test-committor-service/tests/test_intent_executor.rs(22 hunks)test-integration/test-committor-service/tests/test_ix_commit_local.rs(2 hunks)test-integration/test-committor-service/tests/utils/instructions.rs(1 hunks)test-integration/test-committor-service/tests/utils/transactions.rs(2 hunks)
🧰 Additional context used
🧠 Learnings (13)
📓 Common learnings
Learnt from: taco-paco
Repo: magicblock-labs/magicblock-validator PR: 661
File: magicblock-committor-service/src/intent_executor/single_stage_executor.rs:20-28
Timestamp: 2025-11-21T10:22:07.492Z
Learning: In magicblock-committor-service's SingleStageExecutor and TwoStageExecutor (single_stage_executor.rs and two_stage_executor.rs), the fields transaction_strategy, junk, and patched_errors are intentionally public because these executors are designed to be used independently outside of the IntentExecutor scope, and callers need access to these execution reports for cleanup and error handling.
📚 Learning: 2025-11-19T09:34:37.917Z
Learnt from: thlorenz
Repo: magicblock-labs/magicblock-validator PR: 621
File: test-integration/test-chainlink/tests/ix_remote_account_provider.rs:62-63
Timestamp: 2025-11-19T09:34:37.917Z
Learning: In test-integration/test-chainlink/tests/ix_remote_account_provider.rs and similar test files, the `_fwd_rx` receiver returned by `init_remote_account_provider()` is intentionally kept alive (but unused) to prevent "receiver dropped" errors on the sender side. The pattern `let (remote_account_provider, _fwd_rx) = init_remote_account_provider().await;` should NOT be changed to `let (remote_account_provider, _) = ...` because dropping the receiver would cause send() operations to fail.
Applied to files:
test-integration/test-committor-service/tests/utils/transactions.rstest-integration/test-committor-service/tests/utils/instructions.rstest-integration/test-committor-service/tests/test_ix_commit_local.rstest-integration/test-committor-service/tests/test_intent_executor.rsmagicblock-committor-service/src/intent_execution_manager/intent_execution_engine.rs
📚 Learning: 2025-10-14T09:56:14.047Z
Learnt from: taco-paco
Repo: magicblock-labs/magicblock-validator PR: 564
File: test-integration/programs/flexi-counter/src/processor/call_handler.rs:122-125
Timestamp: 2025-10-14T09:56:14.047Z
Learning: The file test-integration/programs/flexi-counter/src/processor/call_handler.rs contains a test smart contract used for integration testing, not production code.
Applied to files:
test-integration/test-committor-service/tests/utils/transactions.rsmagicblock-committor-service/src/tasks/args_task.rstest-integration/test-committor-service/tests/test_ix_commit_local.rstest-integration/programs/flexi-counter/src/processor.rstest-integration/programs/flexi-counter/src/state.rstest-integration/test-committor-service/tests/test_intent_executor.rs
📚 Learning: 2025-10-21T14:00:54.642Z
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 578
File: magicblock-aperture/src/requests/websocket/account_subscribe.rs:18-27
Timestamp: 2025-10-21T14:00:54.642Z
Learning: In magicblock-aperture account_subscribe handler (src/requests/websocket/account_subscribe.rs), the RpcAccountInfoConfig fields data_slice, commitment, and min_context_slot are currently ignored—only encoding is applied. This is tracked as technical debt in issue #579: https://github.com/magicblock-labs/magicblock-validator/issues/579
Applied to files:
test-integration/test-committor-service/tests/utils/transactions.rstest-integration/test-committor-service/tests/test_ix_commit_local.rs
📚 Learning: 2025-11-21T10:22:07.492Z
Learnt from: taco-paco
Repo: magicblock-labs/magicblock-validator PR: 661
File: magicblock-committor-service/src/intent_executor/single_stage_executor.rs:20-28
Timestamp: 2025-11-21T10:22:07.492Z
Learning: In magicblock-committor-service's SingleStageExecutor and TwoStageExecutor (single_stage_executor.rs and two_stage_executor.rs), the fields transaction_strategy, junk, and patched_errors are intentionally public because these executors are designed to be used independently outside of the IntentExecutor scope, and callers need access to these execution reports for cleanup and error handling.
Applied to files:
magicblock-committor-service/src/intent_executor/task_info_fetcher.rsmagicblock-committor-service/src/intent_execution_manager.rsmagicblock-committor-service/src/transaction_preparator/mod.rsmagicblock-committor-service/src/service_ext.rsprograms/magicblock/src/schedule_transactions/process_scheduled_commit_sent.rsmagicblock-committor-service/src/tasks/buffer_task.rsmagicblock-committor-service/src/stubs/changeset_committor_stub.rsmagicblock-committor-service/src/transaction_preparator/delivery_preparator.rsmagicblock-accounts/src/scheduled_commits_processor.rstest-integration/test-committor-service/tests/test_intent_executor.rsmagicblock-committor-service/src/intent_executor/error.rsmagicblock-committor-service/src/intent_execution_manager/intent_execution_engine.rsmagicblock-committor-service/src/intent_executor/two_stage_executor.rsmagicblock-committor-service/src/intent_executor/single_stage_executor.rsmagicblock-metrics/src/metrics/mod.rsmagicblock-committor-service/src/intent_executor/mod.rs
📚 Learning: 2025-11-04T10:48:00.070Z
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 589
File: magicblock-processor/src/scheduler/mod.rs:217-219
Timestamp: 2025-11-04T10:48:00.070Z
Learning: In magicblock-validator, the codebase uses a pattern where types containing non-Send/non-Sync fields (like Rc<RefCell<...>>) are marked with unsafe impl Send when they are guaranteed to be confined to a single thread through careful API design and thread spawning patterns.
Applied to files:
magicblock-committor-service/src/intent_execution_manager.rs
📚 Learning: 2025-11-07T14:20:31.457Z
Learnt from: thlorenz
Repo: magicblock-labs/magicblock-validator PR: 621
File: magicblock-chainlink/src/remote_account_provider/chain_pubsub_actor.rs:457-495
Timestamp: 2025-11-07T14:20:31.457Z
Learning: In magicblock-chainlink/src/remote_account_provider/chain_pubsub_client.rs, the unsubscribe closure returned by PubSubConnection::account_subscribe(...) resolves to () (unit), not a Result. Downstream code should not attempt to inspect an unsubscribe result and can optionally wrap it in a timeout to guard against hangs.
Applied to files:
magicblock-committor-service/src/service_ext.rs
📚 Learning: 2025-11-18T08:47:39.702Z
Learnt from: Dodecahedr0x
Repo: magicblock-labs/magicblock-validator PR: 639
File: magicblock-chainlink/tests/04_redeleg_other_separate_slots.rs:158-165
Timestamp: 2025-11-18T08:47:39.702Z
Learning: In magicblock-chainlink tests involving compressed accounts, `set_remote_slot()` sets the slot of the `AccountSharedData`, while `compressed_account_shared_with_owner_and_slot()` sets the slot of the delegation record. These are two different fields and both calls are necessary.
Applied to files:
test-integration/test-committor-service/tests/test_ix_commit_local.rs
📚 Learning: 2025-11-07T13:20:13.793Z
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 589
File: magicblock-processor/src/scheduler/coordinator.rs:227-238
Timestamp: 2025-11-07T13:20:13.793Z
Learning: In magicblock-processor's ExecutionCoordinator (scheduler/coordinator.rs), the `account_contention` HashMap intentionally does not call `shrink_to_fit()`. Maintaining slack capacity is beneficial for performance by avoiding frequent reallocations during high transaction throughput. As long as empty entries are removed from the map (which `clear_account_contention` does), the capacity overhead is acceptable.
Applied to files:
magicblock-committor-service/src/transaction_preparator/delivery_preparator.rs
📚 Learning: 2025-11-07T13:09:52.253Z
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 589
File: test-kit/src/lib.rs:275-0
Timestamp: 2025-11-07T13:09:52.253Z
Learning: In test-kit, the transaction scheduler in ExecutionTestEnv is not expected to shut down during tests. Therefore, using `.unwrap()` in test helper methods like `schedule_transaction` is acceptable and will not cause issues in the test environment.
Applied to files:
magicblock-accounts/src/scheduled_commits_processor.rstest-integration/test-committor-service/tests/test_intent_executor.rsmagicblock-committor-service/src/intent_execution_manager/intent_execution_engine.rsmagicblock-committor-service/src/intent_executor/single_stage_executor.rs
📚 Learning: 2025-11-04T10:53:50.922Z
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 589
File: magicblock-processor/src/scheduler/locks.rs:110-122
Timestamp: 2025-11-04T10:53:50.922Z
Learning: In magicblock-processor, the TransactionScheduler runs in a single, dedicated thread and will always remain single-threaded. The `next_transaction_id()` function in scheduler/locks.rs uses `unsafe static mut` which is safe given this architectural guarantee.
Applied to files:
magicblock-accounts/src/scheduled_commits_processor.rsmagicblock-committor-service/src/intent_execution_manager/intent_execution_engine.rs
📚 Learning: 2025-11-12T09:46:27.553Z
Learnt from: Dodecahedr0x
Repo: magicblock-labs/magicblock-validator PR: 614
File: magicblock-task-scheduler/src/db.rs:26-0
Timestamp: 2025-11-12T09:46:27.553Z
Learning: In magicblock-task-scheduler, task parameter validation (including ensuring iterations > 0 and enforcing minimum execution intervals) is performed in the Magic program (on-chain) before ScheduleTaskRequest instances reach the scheduler service. The From<&ScheduleTaskRequest> conversion in db.rs does not need additional validation because inputs are already validated at the program level.
Applied to files:
magicblock-committor-service/src/intent_executor/single_stage_executor.rsmagicblock-committor-service/src/intent_executor/mod.rs
📚 Learning: 2025-11-13T09:38:43.804Z
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 589
File: magicblock-processor/src/scheduler/locks.rs:64-102
Timestamp: 2025-11-13T09:38:43.804Z
Learning: In magicblock-processor's TransactionScheduler (scheduler/mod.rs line 59), the executor count is clamped to MAX_SVM_EXECUTORS (63) at initialization time, and executor IDs are assigned sequentially from 0 to count-1. This architectural guarantee ensures that executor IDs used in the bitmask-based AccountLock (scheduler/locks.rs) will always be within valid bounds for bit shifting operations, making runtime bounds checks unnecessary.
Applied to files:
magicblock-committor-service/src/intent_executor/mod.rs
🧬 Code graph analysis (18)
test-integration/test-committor-service/tests/utils/transactions.rs (1)
test-integration/test-committor-service/tests/utils/instructions.rs (1)
init_account_and_delegate_ixs(20-51)
magicblock-committor-service/src/intent_executor/task_info_fetcher.rs (5)
magicblock-committor-service/src/intent_executor/error.rs (2)
signature(29-34)signature(156-164)magicblock-committor-service/src/tasks/task_builder.rs (1)
signature(211-216)magicblock-committor-service/src/transaction_preparator/delivery_preparator.rs (2)
signature(551-556)signature(568-573)magicblock-committor-service/src/transaction_preparator/error.rs (1)
signature(20-25)magicblock-rpc-client/src/lib.rs (1)
signature(82-90)
magicblock-committor-service/src/tasks/args_task.rs (3)
magicblock-committor-service/src/intent_executor/mod.rs (1)
value(75-83)magicblock-committor-service/src/intent_executor/error.rs (2)
value(115-127)value(242-250)magicblock-committor-service/src/tasks/buffer_task.rs (1)
value(146-150)
magicblock-committor-service/src/tasks/task_builder.rs (2)
magicblock-committor-service/src/intent_executor/task_info_fetcher.rs (1)
signature(279-285)magicblock-rpc-client/src/lib.rs (1)
signature(82-90)
programs/magicblock/src/schedule_transactions/process_scheduled_commit_sent.rs (1)
magicblock-committor-service/src/intent_executor/two_stage_executor.rs (1)
commit(69-134)
magicblock-committor-service/src/tasks/buffer_task.rs (3)
magicblock-committor-service/src/intent_executor/mod.rs (1)
value(75-83)magicblock-committor-service/src/intent_executor/error.rs (2)
value(115-127)value(242-250)magicblock-committor-service/src/tasks/args_task.rs (1)
value(171-178)
test-integration/test-committor-service/tests/test_ix_commit_local.rs (1)
test-integration/test-committor-service/tests/utils/transactions.rs (1)
init_and_delegate_account_on_chain(127-224)
magicblock-committor-service/src/stubs/changeset_committor_stub.rs (1)
magicblock-committor-service/src/intent_execution_manager/intent_execution_engine.rs (3)
new(51-64)new(105-122)new(701-707)
magicblock-committor-service/src/transaction_preparator/delivery_preparator.rs (3)
magicblock-metrics/src/metrics/mod.rs (2)
observe_committor_intent_task_preparation_time(434-442)observe_committor_intent_alt_preparation_time(444-446)magicblock-committor-service/src/transaction_preparator/error.rs (1)
signature(20-25)magicblock-rpc-client/src/lib.rs (2)
signature(82-90)error(193-195)
magicblock-accounts/src/scheduled_commits_processor.rs (3)
magicblock-committor-service/src/intent_executor/mod.rs (1)
value(75-83)magicblock-committor-service/src/intent_executor/error.rs (4)
value(115-127)value(242-250)signature(29-34)signature(156-164)magicblock-committor-service/src/intent_executor/two_stage_executor.rs (2)
commit(69-134)finalize(188-252)
magicblock-committor-service/src/transaction_preparator/error.rs (1)
magicblock-committor-service/src/transaction_preparator/delivery_preparator.rs (2)
signature(551-556)signature(568-573)
test-integration/test-committor-service/tests/test_intent_executor.rs (5)
magicblock-committor-service/src/intent_execution_manager/intent_execution_engine.rs (3)
new(51-64)new(105-122)new(701-707)magicblock-committor-service/src/intent_executor/mod.rs (1)
new(124-138)test-integration/programs/flexi-counter/src/state.rs (2)
pda(34-37)new(16-22)test-integration/test-tools/src/conversions.rs (1)
err(8-8)test-integration/test-committor-service/tests/utils/transactions.rs (1)
init_and_delegate_account_on_chain(127-224)
magicblock-committor-service/src/intent_executor/error.rs (4)
magicblock-rpc-client/src/lib.rs (2)
error(193-195)signature(82-90)magicblock-committor-service/src/tasks/args_task.rs (2)
value(171-178)task_type(147-154)magicblock-committor-service/src/tasks/buffer_task.rs (2)
value(146-150)task_type(123-127)magicblock-committor-service/src/tasks/mod.rs (1)
task_type(90-90)
magicblock-committor-service/src/intent_execution_manager/intent_execution_engine.rs (4)
magicblock-rpc-client/src/lib.rs (1)
error(193-195)magicblock-committor-service/src/intent_executor/mod.rs (5)
new(124-138)None(904-904)None(920-920)cleanup(104-104)cleanup(839-849)magicblock-committor-service/src/transaction_preparator/delivery_preparator.rs (4)
new(50-60)signature(551-556)signature(568-573)cleanup(438-500)magicblock-committor-service/src/intent_executor/error.rs (2)
signature(29-34)signature(156-164)
magicblock-committor-service/src/intent_executor/two_stage_executor.rs (2)
magicblock-committor-service/src/intent_executor/error.rs (4)
signature(29-34)signature(156-164)from_commit_execution_error(68-76)from_finalize_execution_error(78-88)magicblock-committor-service/src/intent_executor/mod.rs (3)
new(124-138)cleanup(104-104)cleanup(839-849)
magicblock-committor-service/src/intent_executor/single_stage_executor.rs (3)
magicblock-committor-service/src/intent_executor/mod.rs (5)
new(124-138)execute(97-101)execute(802-836)cleanup(104-104)cleanup(839-849)magicblock-committor-service/src/intent_executor/two_stage_executor.rs (1)
new(55-67)magicblock-committor-service/src/intent_executor/error.rs (1)
from_finalize_execution_error(78-88)
magicblock-metrics/src/metrics/mod.rs (3)
magicblock-committor-service/src/tasks/args_task.rs (2)
new(37-42)task_type(147-154)magicblock-committor-service/src/tasks/buffer_task.rs (2)
new(38-46)task_type(123-127)magicblock-committor-service/src/tasks/mod.rs (1)
task_type(90-90)
magicblock-committor-service/src/intent_executor/mod.rs (4)
magicblock-committor-service/src/intent_execution_manager/intent_execution_engine.rs (7)
execute(267-320)execute(778-818)cleanup(820-822)new(51-64)new(105-122)new(701-707)spawn(125-130)magicblock-committor-service/src/intent_executor/single_stage_executor.rs (2)
execute(40-103)new(30-38)magicblock-committor-service/src/transaction_preparator/delivery_preparator.rs (2)
cleanup(438-500)new(50-60)magicblock-committor-service/src/intent_executor/two_stage_executor.rs (1)
new(55-67)
🔇 Additional comments (37)
magicblock-committor-service/src/intent_executor/task_info_fetcher.rs (2)
13-13: LGTM!The import is necessary for the new
signature()method and is correctly placed.
278-286: LGTM! Consistent with project-wide pattern.The implementation correctly follows the established pattern for extracting signatures from error types across the codebase. All three error variants are properly handled, with delegation to the inner
MagicBlockRpcClientError::signature()method when appropriate.test-integration/programs/flexi-counter/src/state.rs (1)
12-13: LGTM! Test constants are well-defined.These constants provide a clear mechanism for injecting undelegation failures in integration tests. The naming is descriptive and the values are appropriate for test scenarios.
Based on learnings
test-integration/programs/flexi-counter/src/processor.rs (1)
40-40: LGTM! Import additions are correct.The new constants are properly imported from the state module to support the conditional undelegation failure logic.
programs/magicblock/src/schedule_transactions/process_scheduled_commit_sent.rs (5)
32-33: Good addition for error tracking.The new fields appropriately capture execution diagnostics. The
Option<String>for a terminal error andVec<String>for intermediate patched errors provide clear semantics for consumers.
48-49: LGTM!The printable struct correctly mirrors the new diagnostic fields from
SentCommit.
77-78: LGTM!The conversion correctly propagates the new diagnostic fields without unnecessary transformations.
222-229: LGTM!The patched errors logging follows the same pattern as the chain signatures logging and provides clear diagnostics.
231-241: LGTM!The error handling correctly returns a failure when an error message is present, otherwise succeeds. The ownership semantics are correct since
commitis fully owned at this point.magicblock-committor-service/src/tasks/task_builder.rs (1)
10-10: Expose TaskBuilderError.signature for upstream consumersForwarding
signature()to the innerTaskInfoFetcherErrorin both variants is clean and keeps error reporting consistent with other components that already expose signatures. No issues with the new import or method.Also applies to: 210-217
magicblock-metrics/src/metrics/mod.rs (1)
195-198: Committor intent metrics and helpers are wired consistentlyThe new counters/histograms and their helpers:
- Use clear, stable metric names and reasonable bucket choices.
- Are correctly registered in
register().- Leverage
LabelValuefortask_typelabeling, aligning with other committor metrics.This should integrate cleanly with existing observability without behavior regressions.
Also applies to: 227-247, 281-288, 395-402, 434-446
magicblock-committor-service/src/tasks/buffer_task.rs (1)
3-3: BufferTask label integrates with task metricsImplementing
LabelValueforBufferTaskand returning"buffer_commit"for theCommitvariant matches the existing naming scheme and allows Buffer tasks to be used directly with the new preparation-time histograms. No issues.Also applies to: 145-151
magicblock-committor-service/src/service_ext.rs (1)
94-104: Dispatcher now keyed directly by result idUsing
execution_result.idas the key forpending_messages.removealigns the dispatcher with theScheduledBaseIntentWrapper.inner.idused on insertion and the newBroadcastedIntentExecutionResultshape. Listener lookup and delivery semantics remain the same.magicblock-committor-service/src/tasks/args_task.rs (1)
2-2: ArgsTask LabelValue mapping matches task strategy taxonomyThe
LabelValueimpl mapping:
- Commit →
"args_commit"- BaseAction →
"args_action"- Finalize →
"args_finalize"- Undelegate →
"args_undelegate"is consistent with BufferTask’s
"buffer_commit"and gives you fine-grained metric labels by task strategy and kind. Implementation is straightforward and side‑effect free.Also applies to: 170-179
magicblock-committor-service/src/intent_execution_manager.rs (1)
7-7: Public API now centered on BroadcastedIntentExecutionResultRestricting the public re-export to
BroadcastedIntentExecutionResultmatches the new execution flow and avoids leakingExecutionOutputWrapper. Please ensure no external crates rely on importingExecutionOutputWrappervia this module; the compiler will surface any such usages.magicblock-committor-service/src/tasks/mod.rs (1)
13-13: BaseTask now requires LabelValue for metric labelingExtending
BaseTasktoSend + Sync + DynClone + LabelValueis a clean way to ensure every task can provide a stable label string (e.g., fortask_typehistograms) without extra plumbing. Just confirm that any otherBaseTaskimplementors also implementLabelValue; compile errors will highlight any misses.Also applies to: 54-54
test-integration/test-committor-service/tests/test_ix_commit_local.rs (1)
113-119:init_and_delegate_account_on_chaincall sites correctly updated withNonelabelBoth helpers now pass
Noneas the third argument, which keeps behavior identical to the old 2‑arg version (default label path) while aligning with the new API. No issues here.Also applies to: 393-403
magicblock-committor-service/src/transaction_preparator/error.rs (1)
1-26: Error plumbing andsignature()helper are consistentWrapping
DeliveryPreparatorErrorinTransactionPreparatorError::DeliveryPreparationErrorand exposingfn signature(&self) -> Option<Signature>that delegates to the inner error keeps the error surface coherent and makes signature extraction straightforward. TheFrom<TaskStrategistError>mapping intoFailedToFitErrorandSignerErroralso looks correct.No functional issues spotted here.
Also applies to: 28-35
test-integration/test-committor-service/tests/test_intent_executor.rs (2)
1-4: Label‑aware counter setup and new imports are wired correctly
- Importing
mem,FlexiCounterInstruction,FlexiCounter, andFAIL_UNDELEGATION_LABELis justified by later usage (patched‑error inspection, failing undelegation actions, and CPI stress tests).setup_counter(counter_bytes, label)cleanly forwards the optional label intoinit_and_delegate_account_on_chain(&counter_auth, counter_bytes, label)and then sets the owner toprogram_flexi_counter::id(), which aligns with how other helpers use that account.This all looks consistent with the rest of the test helpers and the new label‑driven failure paths.
Also applies to: 16-22, 40-42, 917-924
112-161: New parsing tests correctly exercise labeled counters and updated setup
test_commit_id_error_parsing,test_undelegation_error_parsing,test_action_error_parsing, andtest_cpi_limits_error_parsingall use the updatedsetup_counter(COUNTER_SIZE, ...)signature, passingNonefor the default path andSome(FAIL_UNDELEGATION_LABEL.to_string())when an undelegation failure is desired.- Each test:
- Ensures
prepare_and_execute_strategysucceeds (outer preparation ok).- Asserts that the inner result is
Errwith the expectedTransactionStrategyExecutionErrorvariant.- Checks that the formatted error string contains a stable substring (
EXPECTED_ERR_MSG), which is a good balance between precision and robustness.The control flow and assertions are consistent across variants; no logical issues found.
Also applies to: 163-210, 224-225, 284-287
test-integration/test-committor-service/tests/utils/transactions.rs (1)
126-147: Label threading verified across all call sitesAll 7 call sites of
init_and_delegate_account_on_chainhave been correctly updated to the new 3-argument signature:
- 5 sites pass
None(preserving default label behavior)- 1 site passes
Some(FAIL_UNDELEGATION_LABEL.to_string())(custom label)- 1 site passes the
labelvariable (custom label)The implementation and threading are correct. No issues found.
magicblock-committor-service/src/transaction_preparator/mod.rs (1)
13-17: Error handling verified; call site correctly usestry_join_allwithBufferExecutionErrorThe trait and impl both correctly define
cleanup_for_strategyto returnDeliveryPreparatorResult<(), BufferExecutionError>(which isResult<(), BufferExecutionError>). The impl forwards todelivery_preparator.cleanup()which also returns the same type. At the call site inintent_executor/mod.rs:839, the cleanup method properly collects futures yieldingResult<(), BufferExecutionError>and usestry_join_all()to handle them, which propagates the error type correctly. The error handling is type-safe and consistent throughout.magicblock-committor-service/src/transaction_preparator/delivery_preparator.rs (3)
69-103: Task/ALT preparation metrics are correctly scopedThe new timers around
prepare_task_handling_errorsandprepare_lookup_tablesare wrapped tightly around the awaited calls and stopped in all paths via localresbindings, so they should give accurate per-task and per-ALT timing without leaking. No behavioral change beyond metrics.
237-247: Double‑check safety of parallel buffer realloc transactions
initialize_buffer_accountnow sends the first chunk sequentially, then fires all remainingpreparation_instructionsin parallel viatry_join_all. This is great for latency, but it assumes that the individual realloc chunks are independent enough that concurrent transactions (likely touching the same buffer PDA) won’t race on size/ownership or cause non‑deterministic failures at the runtime level. Please confirm thatchunk_realloc_ixsguarantees such independence (or that later chunks are idempotent with respect to earlier ones); if not, this should remain sequential.
443-500: Error typing and signature propagation align with executor/cleanup flowChanging
cleanupto returnDeliveryPreparatorResult<(), BufferExecutionError>and introducingDeliveryPreparatorErrorplusInternalError::signature()cleanly separate buffer‑level execution failures from higher‑level preparation failures, while still allowing callers and metrics to extract aSignaturewhen available. This lines up with theIntentExecutor::cleanupcontract and withTransactionPreparatorError::signature(), and keeps the error surface consistent across preparation and cleanup.Also applies to: 550-577
magicblock-accounts/src/scheduled_commits_processor.rs (1)
210-243: SentCommit now correctly captures both primary and patched intent errorsThe new flow that passes
BroadcastedIntentExecutionResultthroughprocess_intent_resultintobuild_sent_commitlooks sound: you extractintent_id/trigger_typeonce in the loop, useresult.as_ref().err()for an optionalerror_message, derivechain_signaturesfrom eitherExecutionOutput(success) orIntentExecutorError::signatures()(failure), and then log and stringify eachresult.patched_errorsentry into thepatched_errorsfield. This gives downstream consumers a complete picture of both the final error and all patched intermediate errors without dropping any data.Also applies to: 272-321
magicblock-committor-service/src/intent_executor/error.rs (3)
48-65: Composite IntentExecutorError variants and signatures() are consistent with consumersThe new
FailedToCommitError/FailedToFinalizeErrorvariants plus thefrom_*_execution_errorhelpers cleanly wrapTransactionStrategyExecutionErrorwhile capturing commit/finalize signatures viaerror.signature(). Thesignatures()method then exposes a uniform(commit, finalize?)view across execution, preparation, and builder errors, which is exactly whatScheduledCommitsProcessor::build_sent_commitexpects when reconstructingchain_signatures.Also applies to: 67-112
241-251: Metrics labels now reflect semantic execution error classesThe
metrics::LabelValueimpl forTransactionStrategyExecutionErrorprovides stable, semantically meaningful labels (actions_failed,commit_nonce_failed,cpi_limit_failed,undelegation_failed), and theIntentExecutorErrorlabel impl correctly delegates to the nestedTransactionStrategyExecutionErrorwhere present, falling back to"failed"otherwise. This should give much clearer error distribution metrics without changing behavior.
130-143: Document and validate the task-instruction layout coupling intry_from_transaction_errorThe review correctly identifies a fragile coupling in
try_from_transaction_error(lines 169–238). The function assumes exactly two budget instructions precede task instructions and that thetasksarray order matches instruction order. While the code is defensive and won't silently drop errors, it can map errors to the wrong variant if this invariant breaks.Verified findings:
budget_instructions()returns[Instruction; 2](tasks/utils.rs:135)- Instructions are concatenated as
[budget_ixs, task_ixs](tasks/utils.rs:98)- OFFSET=2 is correct and well-established
- No tests validate that the
tasksparameter corresponds to the instruction array- This mapping is used in production (intent_executor/mod.rs:721, delivery_preparator.rs:370)
Action required: Add a comment documenting the contract (that
tasksmust correspond to the per-task instructions in order), consider adding a test validating this invariant, or refactor to eliminate the implicit coupling (e.g., pass task indices alongside errors).magicblock-committor-service/src/intent_execution_manager/intent_execution_engine.rs (2)
1-5: BroadcastedIntentExecutionResult cleanly exposes execution outcome and patched errorsDefining
PatchedErrors = Vec<TransactionStrategyExecutionError>and wrappingIntentExecutionResultintoBroadcastedIntentExecutionResult { inner: Result<ExecutionOutput, Arc<IntentExecutorError>>, id, trigger_type, patched_errors: Arc<PatchedErrors> }gives subscribers direct access to both the final outcome and the full list of patched errors. TheDerefimpl back toResult<ExecutionOutput, Arc<IntentExecutorError>>preserves existing ergonomics (is_ok,is_err,as_ref, etc.), so the public surface evolves without breaking callers.Also applies to: 18-34, 40-73
328-345: Tests validate new broadcast shape and patched error reportingThe tests now assert that
BroadcastedIntentExecutionResultcarries the correctid,trigger_type, error type, andpatched_errorscontent (including the exact error string), and that success paths under contention/high throughput still behave as expected. The failingMockIntentExecutorbuilds anIntentExecutionResultwith a nestedFailedToCommitErrorand a non‑emptypatched_errorsvector, which exercises both the error and patched‑error plumbing end‑to‑end.Also applies to: 372-397, 440-497, 587-623
magicblock-committor-service/src/intent_executor/single_stage_executor.rs (1)
20-23: Single‑stage executor state, recursion, and new undelegation patching look coherent
- Using
&mut selfplus a storedtransaction_strategyand crate‑localinnernicely encodes the stateful loop without exposing internals beyondtransaction_strategy(which matches the earlier intent to allow external callers to inspect/drive the strategy).- The recursion‑capped loop correctly distinguishes:
- success →
ExecutionOutput::SingleStage,- unpatchable errors / missing committed pubkeys / CPI‑limit / internal errors → immediate break,
- patchable errors →
inner.junk.push(cleanup)+inner.patched_errors.push(execution_err)on each retry.- The new
UndelegationErrorarm inpatch_strategycleanly reusesIntentExecutorImpl::handle_undelegation_error, in line with howActionsErrorandCommitIDErrorare already handled, and ensures the patched error is recorded once per retry.- Early return when
committed_pubkeysisNonepreserves the previous behavior for intents that don’t commit accounts (standalone actions), which seems consistent with the existing assumptions around reset/patch flows.Also applies to: 30-38, 40-103, 112-159
magicblock-committor-service/src/intent_executor/mod.rs (3)
86-91: IntentExecutionResult and executor state tracking align with the new broadcasting flow
- Introducing
IntentExecutionResult { inner, patched_errors }and making it the return type ofIntentExecutor::executecleanly separates user‑visible outcome from the internal patch trace while keeping type signatures straightforward.- Keeping
junkandpatched_errorsaspubonIntentExecutorImplpreserves the ability to run the low‑level executors independently and inspect their state, whilemem::take(&mut self.patched_errors)inexecuteguarantees per‑intent isolation of the patched error list.- The new
cleanup(self)on the trait, backed byIntentExecutorImpl::cleanup, matches howintent_execution_enginenow awaits cleanup asynchronously after broadcasting results and lets tests explicitly await cleanup when needed.Also applies to: 93-105, 107-117, 124-137
273-323: Single‑stage → two‑stage fallback on CPI limit is handled correctly and records patches
- The
single_stage_execution_flowlogic that:
- runs the
SingleStageExecutor,- special‑cases
FailedToFinalizeError { err: CpiLimitError(..), .. }with committed pubkeys,- then splits via
handle_cpi_limit_error, pushes the cleanup strategy intojunk, records the CPI‑limit error intopatched_errors, and continues intwo_stage_execution_flow,
ensures that:- you don’t persist or expose the transient single‑stage failure as the final result,
- all cleanup and the fact that a CPI‑limit patch occurred are captured in the executor state and surfaced through
IntentExecutionResult.- For all non‑CPI‑limit outcomes (including success), pushing the final
transaction_strategyintojunkbefore returning preserves the invariant that every strategy used for an intent eventually gets cleaned up.
838-849: Cleanup implementation matches how junk is accumulated
cleanup(mut self)iterates overself.junkand callscleanup_for_strategyfor each accumulatedTransactionStrategy, then usestry_join_allto await them and collapse toResult<(), BufferExecutionError>.- This is consistent with how
SingleStageExecutorandTwoStageExecutorboth push per‑patch cleanup strategies and final strategies intojunk, so all resources scheduled during patches and normal execution are eventually released.magicblock-committor-service/src/intent_executor/two_stage_executor.rs (2)
22-47: Stateful TwoStageExecutor design and integration with executor state look solid
- Splitting the two‑stage flow into
Initialized,Committed, andFinalizedstates and threading them throughTwoStageExecutor<'a, T, F, S: Sealed>gives a clear, type‑level encoding of the valid progression (new → commit → finalize).commitandfinalizeboth:
- operate on
self.state.*_strategy,- push per‑patch cleanup strategies into
inner.junk,- on each successful patch, record the
TransactionStrategyExecutionErrorintoinner.patched_errors,- and, after the loop, always push the final commit/finalize strategy into
inner.junkbefore mapping the last execution result into anIntentExecutorError.- This mirrors the single‑stage behavior while keeping the API safe and explicit for callers of the two‑stage path.
Also applies to: 48-67, 69-134
298-304: Sealed trait correctly restricts TwoStageExecutor state parameter
- The private
sealedmodule with apub trait Sealedand impls only forInitialized,Committed, andFinalizedensures external code cannot introduce additional state types forTwoStageExecutor, while still allowing them to name and use the three provided states.- This is an idiomatic Rust sealing pattern and fits well with the stateful design here.
programs/magicblock/src/schedule_transactions/process_scheduled_commit_sent.rs
Outdated
Show resolved
Hide resolved
programs/magicblock/src/schedule_transactions/process_scheduled_commit_sent.rs
Show resolved
Hide resolved
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_ix_commit_local.rs (1)
519-530: Add assertions for patched_errors to improve test quality.The PR description states "Patched errors trace is now checked in tests to improve test quality," but this test file doesn't validate
patched_errors. Since line 530 accesses.inner.unwrap(), the outer wrapper likely contains patched error information that should be verified.Consider adding assertions before unwrapping to validate the expected state of patched errors:
for (execution_output, base_intent) in execution_outputs.into_iter().zip(base_intents.into_iter()) { + // Verify no errors were patched in these test scenarios + assert!( + execution_output.patched_errors.is_empty(), + "Unexpected patched errors: {:?}", + execution_output.patched_errors + ); + let execution_output = execution_output.inner.unwrap();This validates a key feature introduced in this PR and prevents regressions where errors are silently patched in scenarios that should execute cleanly.
♻️ Duplicate comments (1)
magicblock-committor-service/src/intent_executor/mod.rs (1)
471-498: Fix handle_undelegation_error docstring and double-check no-undelegate pathThe doc comment still says “Handles actions error, stripping away actions”, but this helper is specifically handling undelegation by draining tasks from the first
TaskType::Undelegateonward and returning those drained tasks plus the old ALTs as a cleanupTransactionStrategy. It would be clearer to update the comment to describe the undelegation semantics explicitly and avoid conflation withhandle_actions_error.Also, when no
TaskType::Undelegateis found, the function returns an entirely empty cleanup strategy (no tasks, no lookup table keys). That’s fine if all call sites guarantee at least one undelegation task when this helper is invoked; otherwise, it would silently skip ALT cleanup. Please sanity‑check that assumption and, if it’s not guaranteed, consider returning appropriate lookup table keys in this branch as well.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
magicblock-committor-service/src/intent_executor/mod.rs(8 hunks)test-integration/test-committor-service/tests/test_ix_commit_local.rs(6 hunks)
🧰 Additional context used
🧠 Learnings (8)
📓 Common learnings
Learnt from: taco-paco
Repo: magicblock-labs/magicblock-validator PR: 661
File: magicblock-committor-service/src/intent_executor/single_stage_executor.rs:20-28
Timestamp: 2025-11-21T10:22:07.520Z
Learning: In magicblock-committor-service's SingleStageExecutor and TwoStageExecutor (single_stage_executor.rs and two_stage_executor.rs), the fields transaction_strategy, junk, and patched_errors are intentionally public because these executors are designed to be used independently outside of the IntentExecutor scope, and callers need access to these execution reports for cleanup and error handling.
📚 Learning: 2025-10-14T09:56:14.047Z
Learnt from: taco-paco
Repo: magicblock-labs/magicblock-validator PR: 564
File: test-integration/programs/flexi-counter/src/processor/call_handler.rs:122-125
Timestamp: 2025-10-14T09:56:14.047Z
Learning: The file test-integration/programs/flexi-counter/src/processor/call_handler.rs contains a test smart contract used for integration testing, not production code.
Applied to files:
test-integration/test-committor-service/tests/test_ix_commit_local.rs
📚 Learning: 2025-11-19T09:34:37.917Z
Learnt from: thlorenz
Repo: magicblock-labs/magicblock-validator PR: 621
File: test-integration/test-chainlink/tests/ix_remote_account_provider.rs:62-63
Timestamp: 2025-11-19T09:34:37.917Z
Learning: In test-integration/test-chainlink/tests/ix_remote_account_provider.rs and similar test files, the `_fwd_rx` receiver returned by `init_remote_account_provider()` is intentionally kept alive (but unused) to prevent "receiver dropped" errors on the sender side. The pattern `let (remote_account_provider, _fwd_rx) = init_remote_account_provider().await;` should NOT be changed to `let (remote_account_provider, _) = ...` because dropping the receiver would cause send() operations to fail.
Applied to files:
test-integration/test-committor-service/tests/test_ix_commit_local.rs
📚 Learning: 2025-10-21T14:00:54.642Z
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 578
File: magicblock-aperture/src/requests/websocket/account_subscribe.rs:18-27
Timestamp: 2025-10-21T14:00:54.642Z
Learning: In magicblock-aperture account_subscribe handler (src/requests/websocket/account_subscribe.rs), the RpcAccountInfoConfig fields data_slice, commitment, and min_context_slot are currently ignored—only encoding is applied. This is tracked as technical debt in issue #579: https://github.com/magicblock-labs/magicblock-validator/issues/579
Applied to files:
test-integration/test-committor-service/tests/test_ix_commit_local.rs
📚 Learning: 2025-11-04T10:48:00.070Z
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 589
File: magicblock-processor/src/scheduler/mod.rs:217-219
Timestamp: 2025-11-04T10:48:00.070Z
Learning: In magicblock-validator, the codebase uses a pattern where types containing non-Send/non-Sync fields (like Rc<RefCell<...>>) are marked with unsafe impl Send when they are guaranteed to be confined to a single thread through careful API design and thread spawning patterns.
Applied to files:
test-integration/test-committor-service/tests/test_ix_commit_local.rs
📚 Learning: 2025-11-21T10:22:07.520Z
Learnt from: taco-paco
Repo: magicblock-labs/magicblock-validator PR: 661
File: magicblock-committor-service/src/intent_executor/single_stage_executor.rs:20-28
Timestamp: 2025-11-21T10:22:07.520Z
Learning: In magicblock-committor-service's SingleStageExecutor and TwoStageExecutor (single_stage_executor.rs and two_stage_executor.rs), the fields transaction_strategy, junk, and patched_errors are intentionally public because these executors are designed to be used independently outside of the IntentExecutor scope, and callers need access to these execution reports for cleanup and error handling.
Applied to files:
test-integration/test-committor-service/tests/test_ix_commit_local.rsmagicblock-committor-service/src/intent_executor/mod.rs
📚 Learning: 2025-11-13T09:38:43.804Z
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 589
File: magicblock-processor/src/scheduler/locks.rs:64-102
Timestamp: 2025-11-13T09:38:43.804Z
Learning: In magicblock-processor's TransactionScheduler (scheduler/mod.rs line 59), the executor count is clamped to MAX_SVM_EXECUTORS (63) at initialization time, and executor IDs are assigned sequentially from 0 to count-1. This architectural guarantee ensures that executor IDs used in the bitmask-based AccountLock (scheduler/locks.rs) will always be within valid bounds for bit shifting operations, making runtime bounds checks unnecessary.
Applied to files:
magicblock-committor-service/src/intent_executor/mod.rs
📚 Learning: 2025-11-12T09:46:27.553Z
Learnt from: Dodecahedr0x
Repo: magicblock-labs/magicblock-validator PR: 614
File: magicblock-task-scheduler/src/db.rs:26-0
Timestamp: 2025-11-12T09:46:27.553Z
Learning: In magicblock-task-scheduler, task parameter validation (including ensuring iterations > 0 and enforcing minimum execution intervals) is performed in the Magic program (on-chain) before ScheduleTaskRequest instances reach the scheduler service. The From<&ScheduleTaskRequest> conversion in db.rs does not need additional validation because inputs are already validated at the program level.
Applied to files:
magicblock-committor-service/src/intent_executor/mod.rs
🧬 Code graph analysis (2)
test-integration/test-committor-service/tests/test_ix_commit_local.rs (1)
test-integration/test-committor-service/tests/utils/transactions.rs (1)
init_and_delegate_account_on_chain(127-224)
magicblock-committor-service/src/intent_executor/mod.rs (4)
magicblock-committor-service/src/intent_executor/single_stage_executor.rs (2)
execute(40-103)new(30-38)magicblock-committor-service/src/intent_execution_manager/intent_execution_engine.rs (7)
execute(267-320)execute(778-818)cleanup(820-822)new(51-64)new(105-122)new(701-707)spawn(125-130)magicblock-committor-service/src/transaction_preparator/delivery_preparator.rs (2)
cleanup(438-500)new(50-60)magicblock-committor-service/src/intent_executor/two_stage_executor.rs (1)
new(55-67)
🔇 Additional comments (8)
test-integration/test-committor-service/tests/test_ix_commit_local.rs (2)
6-6: LGTM!The new imports support the FlexiCounter serialization functionality added to improve test realism.
Also applies to: 30-30
116-117: LGTM!The calls to
init_and_delegate_account_on_chaincorrectly match the updated signature with the newlabelparameter set toNone.Also applies to: 406-411
magicblock-committor-service/src/intent_executor/mod.rs (6)
7-7: Imports updated consistently with new executor behaviorAdding
mem,ControlFlow,Duration, narrowing thelogimport totrace/warn, and pulling inBufferExecutionErrorall line up with the new patched‑errors surface, retry flow, and cleanup API. No issues here.Also applies to: 11-11, 54-54
86-105: IntentExecutionResult wrapper and trait surface look coherentWrapping the inner
IntentExecutorResult<ExecutionOutput>pluspatched_errorsinIntentExecutionResult, switchingexecuteto&mut self, and introducingasync fn cleanup(self)give a clean, stateful lifecycle for executors (execute → inspect result/patched_errors → cleanup). The API shape is internally consistent and matches how the type is later consumed.
113-117: Public junk/patched_errors on IntentExecutorImpl are consistent with executor reportingExposing
pub junk: Vec<TransactionStrategy>andpub patched_errors: Vec<TransactionStrategyExecutionError>, and initializing both to empty vectors innew(), aligns with the pattern whereSingleStageExecutor/TwoStageExecutorpublicly expose their execution reports so callers/tests can inspect and drive cleanup. This keeps the reporting surface uniform across the executor stack.Based on learnings
Also applies to: 135-137
174-178: Stateful single-/two-stage flows and CPI-limit fallback look correctMoving
execute_inner,single_stage_execution_flow, andtwo_stage_execution_flowto&mut selfand threadingSingleStageExecutor::new(self, transaction_strategy)/TwoStageExecutor::new(self, ...)correctly centralizes mutation ofjunkandpatched_errorsinsideIntentExecutorImpl. The CPI‑limit branch insingle_stage_execution_flow:
- Special‑cases
FailedToFinalizeError::CpiLimitErroronly whencommitted_pubkeys.is_some(),- Splits the original strategy via
handle_cpi_limit_error,- Pushes the cleanup strategy and patched error onto executor state,
- Then continues via
two_stage_execution_flow,which matches the intended “retry as two‑stage with recorded cleanup + patched error trace” behavior. I don’t see obvious edge‑case gaps here.
Also applies to: 273-343
799-832: Executor execute wrapper cleanly exposes patched errors and offloads metricsThe updated
executeimplementation:
- Resets the
TaskInfoFetcheronly when the result is an error or the intent is an undelegation, which matches the comment and avoids unnecessary resets.- Persists the inner
IntentExecutorResult<ExecutionOutput>as before.- Uses
Result::inspectto spawnintent_metricson success, keeping RPC CU lookups off the hot path.- Constructs
IntentExecutionResultwhile drainingself.patched_errorswithmem::take, ensuring each call returns only the patched‑error trace for that intent and doesn’t leak state across executions.This wiring looks correct and matches how
BroadcastedIntentExecutionResult::newlater consumes the result.
834-845: cleanup(self) correctly centralizes junk cleanupConsuming
selfincleanup, iterating overself.junk, and runningtransaction_preparator.cleanup_for_strategyfor each strategy viatry_join_allis a good fit for the new lifecycle: callers can either await cleanup explicitly or offload it to a background task, and all outstanding cleanup strategies are handled in one place. Behavior here looks sound.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
test-integration/test-committor-service/tests/test_ix_commit_local.rs (2)
117-128: Verify serialized size and consider more descriptive label.The past review comment about potential truncation if serialized FlexiCounter exceeds
bytesstill applies here. Additionally, the hardcoded label"Counter"could be more descriptive for debugging purposes.Consider making the label more descriptive:
let counter = FlexiCounter { - label: "Counter".to_string(), + label: format!("Counter_{}B", bytes), updates: 0, count: 101, };
414-415: Consider using serialized FlexiCounter data for consistency.As noted in the previous review comment,
create_bundlesstill uses dummy zero data whilecommit_single_account(lines 121-128) now properly initializes with serializedFlexiCounter. This inconsistency should be addressed for improved test quality.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
test-integration/test-committor-service/tests/test_ix_commit_local.rs(5 hunks)
🧰 Additional context used
🧠 Learnings (8)
📓 Common learnings
Learnt from: taco-paco
Repo: magicblock-labs/magicblock-validator PR: 661
File: magicblock-committor-service/src/intent_executor/single_stage_executor.rs:20-28
Timestamp: 2025-11-21T10:22:07.520Z
Learning: In magicblock-committor-service's SingleStageExecutor and TwoStageExecutor (single_stage_executor.rs and two_stage_executor.rs), the fields transaction_strategy, junk, and patched_errors are intentionally public because these executors are designed to be used independently outside of the IntentExecutor scope, and callers need access to these execution reports for cleanup and error handling.
📚 Learning: 2025-10-14T09:56:14.047Z
Learnt from: taco-paco
Repo: magicblock-labs/magicblock-validator PR: 564
File: test-integration/programs/flexi-counter/src/processor/call_handler.rs:122-125
Timestamp: 2025-10-14T09:56:14.047Z
Learning: The file test-integration/programs/flexi-counter/src/processor/call_handler.rs contains a test smart contract used for integration testing, not production code.
Applied to files:
test-integration/test-committor-service/tests/test_ix_commit_local.rs
📚 Learning: 2025-11-19T09:34:37.917Z
Learnt from: thlorenz
Repo: magicblock-labs/magicblock-validator PR: 621
File: test-integration/test-chainlink/tests/ix_remote_account_provider.rs:62-63
Timestamp: 2025-11-19T09:34:37.917Z
Learning: In test-integration/test-chainlink/tests/ix_remote_account_provider.rs and similar test files, the `_fwd_rx` receiver returned by `init_remote_account_provider()` is intentionally kept alive (but unused) to prevent "receiver dropped" errors on the sender side. The pattern `let (remote_account_provider, _fwd_rx) = init_remote_account_provider().await;` should NOT be changed to `let (remote_account_provider, _) = ...` because dropping the receiver would cause send() operations to fail.
Applied to files:
test-integration/test-committor-service/tests/test_ix_commit_local.rs
📚 Learning: 2025-10-21T14:00:54.642Z
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 578
File: magicblock-aperture/src/requests/websocket/account_subscribe.rs:18-27
Timestamp: 2025-10-21T14:00:54.642Z
Learning: In magicblock-aperture account_subscribe handler (src/requests/websocket/account_subscribe.rs), the RpcAccountInfoConfig fields data_slice, commitment, and min_context_slot are currently ignored—only encoding is applied. This is tracked as technical debt in issue #579: https://github.com/magicblock-labs/magicblock-validator/issues/579
Applied to files:
test-integration/test-committor-service/tests/test_ix_commit_local.rs
📚 Learning: 2025-11-07T13:20:13.793Z
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 589
File: magicblock-processor/src/scheduler/coordinator.rs:227-238
Timestamp: 2025-11-07T13:20:13.793Z
Learning: In magicblock-processor's ExecutionCoordinator (scheduler/coordinator.rs), the `account_contention` HashMap intentionally does not call `shrink_to_fit()`. Maintaining slack capacity is beneficial for performance by avoiding frequent reallocations during high transaction throughput. As long as empty entries are removed from the map (which `clear_account_contention` does), the capacity overhead is acceptable.
Applied to files:
test-integration/test-committor-service/tests/test_ix_commit_local.rs
📚 Learning: 2025-11-18T08:47:39.702Z
Learnt from: Dodecahedr0x
Repo: magicblock-labs/magicblock-validator PR: 639
File: magicblock-chainlink/tests/04_redeleg_other_separate_slots.rs:158-165
Timestamp: 2025-11-18T08:47:39.702Z
Learning: In magicblock-chainlink tests involving compressed accounts, `set_remote_slot()` sets the slot of the `AccountSharedData`, while `compressed_account_shared_with_owner_and_slot()` sets the slot of the delegation record. These are two different fields and both calls are necessary.
Applied to files:
test-integration/test-committor-service/tests/test_ix_commit_local.rs
📚 Learning: 2025-10-21T13:06:38.900Z
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 578
File: magicblock-aperture/src/requests/http/get_program_accounts.rs:17-25
Timestamp: 2025-10-21T13:06:38.900Z
Learning: The magicblock validator does not support ledger forking, so commitment-based state queries (processed/confirmed/finalized) are not applicable. RPC methods can safely ignore commitment and minContextSlot parameters from Solana RPC config objects.
Applied to files:
test-integration/test-committor-service/tests/test_ix_commit_local.rs
📚 Learning: 2025-11-21T10:22:07.520Z
Learnt from: taco-paco
Repo: magicblock-labs/magicblock-validator PR: 661
File: magicblock-committor-service/src/intent_executor/single_stage_executor.rs:20-28
Timestamp: 2025-11-21T10:22:07.520Z
Learning: In magicblock-committor-service's SingleStageExecutor and TwoStageExecutor (single_stage_executor.rs and two_stage_executor.rs), the fields transaction_strategy, junk, and patched_errors are intentionally public because these executors are designed to be used independently outside of the IntentExecutor scope, and callers need access to these execution reports for cleanup and error handling.
Applied to files:
test-integration/test-committor-service/tests/test_ix_commit_local.rs
🧬 Code graph analysis (1)
test-integration/test-committor-service/tests/test_ix_commit_local.rs (2)
test-integration/test-committor-service/tests/utils/transactions.rs (1)
init_and_delegate_account_on_chain(127-224)test-integration/programs/flexi-counter/src/state.rs (2)
pda(34-37)new(16-22)
🔇 Additional comments (2)
test-integration/test-committor-service/tests/test_ix_commit_local.rs (2)
407-412: LGTM!The addition of the
Noneargument for the label parameter is consistent with the function signature change ininit_and_delegate_account_on_chain.
520-520: LGTM!The change from collecting
Result<Vec<_>, _>toVec<_>correctly reflects the API refactoring whereschedule_base_intents_waitingnow returnsResult<Vec<BroadcastedIntentExecutionResult>>instead of individual Results.
# Conflicts: # magicblock-committor-service/src/intent_executor/error.rs # magicblock-committor-service/src/intent_executor/mod.rs # magicblock-committor-service/src/intent_executor/single_stage_executor.rs # magicblock-metrics/src/metrics/mod.rs # test-integration/test-committor-service/tests/test_intent_executor.rs # test-integration/test-committor-service/tests/test_ix_commit_local.rs
Manual Deploy AvailableYou can trigger a manual deploy of this PR branch to testnet: Alternative: Comment
Comment updated automatically when the PR is synchronized. |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
test-integration/test-committor-service/tests/test_ix_commit_local.rs (2)
531-531: Consider usingexpectwith a descriptive message.As noted in previous reviews, the
unwrap()here will panic with a generic message ifexecution_result.innerisNone. A descriptiveexpectmessage would help debug test failures more quickly.
540-544: Assertion message could be more informative.As noted in previous reviews, the assertion message would be more helpful if it included the actual count and contents of
patched_errorswhen the assertion fails. This would aid debugging by showing what errors were unexpectedly patched.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
magicblock-accounts/src/scheduled_commits_processor.rs(7 hunks)magicblock-committor-service/src/intent_executor/task_info_fetcher.rs(2 hunks)magicblock-metrics/src/metrics/mod.rs(5 hunks)test-integration/test-committor-service/tests/test_ix_commit_local.rs(1 hunks)
🧰 Additional context used
🧠 Learnings (9)
📓 Common learnings
Learnt from: taco-paco
Repo: magicblock-labs/magicblock-validator PR: 661
File: magicblock-committor-service/src/intent_executor/single_stage_executor.rs:20-28
Timestamp: 2025-11-21T10:22:07.520Z
Learning: In magicblock-committor-service's SingleStageExecutor and TwoStageExecutor (single_stage_executor.rs and two_stage_executor.rs), the fields transaction_strategy, junk, and patched_errors are intentionally public because these executors are designed to be used independently outside of the IntentExecutor scope, and callers need access to these execution reports for cleanup and error handling.
📚 Learning: 2025-11-21T10:22:07.520Z
Learnt from: taco-paco
Repo: magicblock-labs/magicblock-validator PR: 661
File: magicblock-committor-service/src/intent_executor/single_stage_executor.rs:20-28
Timestamp: 2025-11-21T10:22:07.520Z
Learning: In magicblock-committor-service's SingleStageExecutor and TwoStageExecutor (single_stage_executor.rs and two_stage_executor.rs), the fields transaction_strategy, junk, and patched_errors are intentionally public because these executors are designed to be used independently outside of the IntentExecutor scope, and callers need access to these execution reports for cleanup and error handling.
Applied to files:
magicblock-committor-service/src/intent_executor/task_info_fetcher.rstest-integration/test-committor-service/tests/test_ix_commit_local.rsmagicblock-accounts/src/scheduled_commits_processor.rsmagicblock-metrics/src/metrics/mod.rs
📚 Learning: 2025-10-14T09:56:14.047Z
Learnt from: taco-paco
Repo: magicblock-labs/magicblock-validator PR: 564
File: test-integration/programs/flexi-counter/src/processor/call_handler.rs:122-125
Timestamp: 2025-10-14T09:56:14.047Z
Learning: The file test-integration/programs/flexi-counter/src/processor/call_handler.rs contains a test smart contract used for integration testing, not production code.
Applied to files:
test-integration/test-committor-service/tests/test_ix_commit_local.rs
📚 Learning: 2025-10-21T14:00:54.642Z
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 578
File: magicblock-aperture/src/requests/websocket/account_subscribe.rs:18-27
Timestamp: 2025-10-21T14:00:54.642Z
Learning: In magicblock-aperture account_subscribe handler (src/requests/websocket/account_subscribe.rs), the RpcAccountInfoConfig fields data_slice, commitment, and min_context_slot are currently ignored—only encoding is applied. This is tracked as technical debt in issue #579: https://github.com/magicblock-labs/magicblock-validator/issues/579
Applied to files:
test-integration/test-committor-service/tests/test_ix_commit_local.rs
📚 Learning: 2025-11-19T09:34:37.917Z
Learnt from: thlorenz
Repo: magicblock-labs/magicblock-validator PR: 621
File: test-integration/test-chainlink/tests/ix_remote_account_provider.rs:62-63
Timestamp: 2025-11-19T09:34:37.917Z
Learning: In test-integration/test-chainlink/tests/ix_remote_account_provider.rs and similar test files, the `_fwd_rx` receiver returned by `init_remote_account_provider()` is intentionally kept alive (but unused) to prevent "receiver dropped" errors on the sender side. The pattern `let (remote_account_provider, _fwd_rx) = init_remote_account_provider().await;` should NOT be changed to `let (remote_account_provider, _) = ...` because dropping the receiver would cause send() operations to fail.
Applied to files:
test-integration/test-committor-service/tests/test_ix_commit_local.rs
📚 Learning: 2025-11-07T13:20:13.793Z
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 589
File: magicblock-processor/src/scheduler/coordinator.rs:227-238
Timestamp: 2025-11-07T13:20:13.793Z
Learning: In magicblock-processor's ExecutionCoordinator (scheduler/coordinator.rs), the `account_contention` HashMap intentionally does not call `shrink_to_fit()`. Maintaining slack capacity is beneficial for performance by avoiding frequent reallocations during high transaction throughput. As long as empty entries are removed from the map (which `clear_account_contention` does), the capacity overhead is acceptable.
Applied to files:
test-integration/test-committor-service/tests/test_ix_commit_local.rs
📚 Learning: 2025-11-18T08:47:39.702Z
Learnt from: Dodecahedr0x
Repo: magicblock-labs/magicblock-validator PR: 639
File: magicblock-chainlink/tests/04_redeleg_other_separate_slots.rs:158-165
Timestamp: 2025-11-18T08:47:39.702Z
Learning: In magicblock-chainlink tests involving compressed accounts, `set_remote_slot()` sets the slot of the `AccountSharedData`, while `compressed_account_shared_with_owner_and_slot()` sets the slot of the delegation record. These are two different fields and both calls are necessary.
Applied to files:
test-integration/test-committor-service/tests/test_ix_commit_local.rs
📚 Learning: 2025-11-07T13:09:52.253Z
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 589
File: test-kit/src/lib.rs:275-0
Timestamp: 2025-11-07T13:09:52.253Z
Learning: In test-kit, the transaction scheduler in ExecutionTestEnv is not expected to shut down during tests. Therefore, using `.unwrap()` in test helper methods like `schedule_transaction` is acceptable and will not cause issues in the test environment.
Applied to files:
test-integration/test-committor-service/tests/test_ix_commit_local.rsmagicblock-accounts/src/scheduled_commits_processor.rs
📚 Learning: 2025-11-04T10:53:50.922Z
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 589
File: magicblock-processor/src/scheduler/locks.rs:110-122
Timestamp: 2025-11-04T10:53:50.922Z
Learning: In magicblock-processor, the TransactionScheduler runs in a single, dedicated thread and will always remain single-threaded. The `next_transaction_id()` function in scheduler/locks.rs uses `unsafe static mut` which is safe given this architectural guarantee.
Applied to files:
magicblock-accounts/src/scheduled_commits_processor.rs
🧬 Code graph analysis (2)
magicblock-committor-service/src/intent_executor/task_info_fetcher.rs (3)
magicblock-committor-service/src/intent_executor/error.rs (2)
signature(29-34)signature(156-164)magicblock-committor-service/src/transaction_preparator/error.rs (1)
signature(20-25)magicblock-rpc-client/src/lib.rs (1)
signature(82-90)
magicblock-accounts/src/scheduled_commits_processor.rs (3)
magicblock-committor-service/src/intent_executor/error.rs (4)
value(115-127)value(242-250)signature(29-34)signature(156-164)magicblock-committor-service/src/intent_executor/mod.rs (1)
value(75-83)magicblock-committor-service/src/intent_executor/two_stage_executor.rs (2)
commit(69-134)finalize(188-252)
🔇 Additional comments (14)
magicblock-committor-service/src/intent_executor/task_info_fetcher.rs (2)
14-14: LGTM: Import added for signature extraction.The import is necessary for the new
signature()method return type and is correctly placed.
280-288: LGTM: Signature extraction method follows established pattern.The implementation correctly:
- Returns
Nonefor domain-specific errors without transaction signatures- Delegates to the underlying RPC error's
signature()method- Matches all enum variants exhaustively
- Aligns with the signature extraction pattern used in other error types across the codebase
magicblock-metrics/src/metrics/mod.rs (5)
252-254: LGTM!The metric definition follows the established pattern for IntCounter metrics in this file.
319-319: Verify the 17.0 bucket value is intentional.The bucket includes 17.0, which is oddly specific compared to the round numbers around it (15.0, 20.0). If this is intentional based on observed data distribution, consider adding a comment explaining the choice.
354-354: LGTM!All three new metrics are properly registered using the macro pattern consistent with the rest of the file.
Also applies to: 360-361
477-483: LGTM!Both counter increment functions follow the established patterns in this file and are correctly implemented.
516-528: LGTM!Both histogram observation functions are correctly implemented:
- Proper use of generic bounds for label values
- Correct pattern for starting timers that can be stopped by the caller
- Appropriate distinction between HistogramVec (with labels) and Histogram (without labels)
test-integration/test-committor-service/tests/test_ix_commit_local.rs (1)
515-544: Test correctly adapted to new execution result structure.The changes properly adapt the test to work with the new
BroadcastedIntentExecutionResultstructure. The addition ofpatched_errorsvalidation is a good improvement to test quality, as noted in the PR objectives.magicblock-accounts/src/scheduled_commits_processor.rs (6)
7-7: LGTM! Import changes align with refactoring.The addition of
infoto log imports and the switch fromExecutionOutputWrappertoBroadcastedIntentExecutionResultare consistent with the PR's goal to improve execution result handling and error reporting.Also applies to: 19-19
204-237: LGTM! Cleaner result processing.The direct field access on
execution_resultand passing the complete result object toprocess_intent_resultsimplifies the code flow and aligns with the new result-driven execution model.
241-250: LGTM! Function signature update aligns with refactoring.The updated signature to accept
BroadcastedIntentExecutionResultand the simplified call site are consistent with the PR's execution result refactoring.
273-294: LGTM! Error handling logic is correct.The signature extraction logic properly handles both success and partial failure cases. When execution fails, it correctly attempts to extract any available signatures (e.g., commit signature even if finalize failed) using
err.signatures().The nested unwrapping in lines 288-290 is somewhat complex but handles the optional finalize signature correctly.
313-314: LGTM! New fields properly added to SentCommit.The addition of
error_messageandpatched_errorsfields to theSentCommitstruct aligns perfectly with the PR objective to report Intent's patched errors. The computed values are correctly passed to the struct construction.
271-272: Theas_ref()API usage is correct—review comment is inaccurate.The code
result.as_ref().err()on line 272 is valid and idiomatic.BroadcastedIntentExecutionResultimplementsDereftargetingResult<ExecutionOutput, Arc<IntentExecutorError>>. Rust's method resolution automatically applies deref coercion, allowingas_ref()(aResultmethod) to be called directly. This pattern avoids explicitly accessing.innerand is the recommended Rust style.No changes needed.
# Conflicts: # magicblock-committor-service/src/intent_executor/mod.rs
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
magicblock-committor-service/src/transaction_preparator/delivery_preparator.rs(3 hunks)magicblock-committor-service/src/transaction_preparator/mod.rs(3 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: taco-paco
Repo: magicblock-labs/magicblock-validator PR: 661
File: magicblock-committor-service/src/intent_executor/single_stage_executor.rs:20-28
Timestamp: 2025-11-21T10:22:07.520Z
Learning: In magicblock-committor-service's SingleStageExecutor and TwoStageExecutor (single_stage_executor.rs and two_stage_executor.rs), the fields transaction_strategy, junk, and patched_errors are intentionally public because these executors are designed to be used independently outside of the IntentExecutor scope, and callers need access to these execution reports for cleanup and error handling.
📚 Learning: 2025-11-21T10:22:07.520Z
Learnt from: taco-paco
Repo: magicblock-labs/magicblock-validator PR: 661
File: magicblock-committor-service/src/intent_executor/single_stage_executor.rs:20-28
Timestamp: 2025-11-21T10:22:07.520Z
Learning: In magicblock-committor-service's SingleStageExecutor and TwoStageExecutor (single_stage_executor.rs and two_stage_executor.rs), the fields transaction_strategy, junk, and patched_errors are intentionally public because these executors are designed to be used independently outside of the IntentExecutor scope, and callers need access to these execution reports for cleanup and error handling.
Applied to files:
magicblock-committor-service/src/transaction_preparator/mod.rsmagicblock-committor-service/src/transaction_preparator/delivery_preparator.rs
🧬 Code graph analysis (1)
magicblock-committor-service/src/transaction_preparator/delivery_preparator.rs (3)
magicblock-committor-service/src/intent_executor/error.rs (2)
signature(29-34)signature(156-164)magicblock-committor-service/src/transaction_preparator/error.rs (1)
signature(20-25)magicblock-rpc-client/src/lib.rs (2)
signature(82-90)error(193-195)
🔇 Additional comments (4)
magicblock-committor-service/src/transaction_preparator/mod.rs (1)
14-14: LGTM!The import and return type updates for
cleanup_for_strategyare consistent between the trait definition and implementation. UsingBufferExecutionErroras the specific error type for cleanup operations is semantically appropriate since these operations deal with buffer account lifecycle.Also applies to: 41-41
magicblock-committor-service/src/transaction_preparator/delivery_preparator.rs (3)
94-101: LGTM!The error mapping correctly distinguishes between buffer account preparation failures and ALT creation failures, wrapping the
InternalErrorin semantically appropriateDeliveryPreparatorErrorvariants.
437-442: LGTM!The return type change to
DeliveryPreparatorResult<(), BufferExecutionError>is consistent withsend_ixs_with_retryand the trait definition.
558-576: LGTM!The
DeliveryPreparatorErrorenum structure and itssignature()method correctly delegate to the innerInternalError. The pattern matches the existing signature extraction approach inerror.rs. The result type alias update is appropriate.
magicblock-committor-service/src/transaction_preparator/delivery_preparator.rs
Show resolved
Hide resolved
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)
magicblock-accounts/src/scheduled_commits_processor.rs (1)
266-316: LGTM: Result-driven flow centralizes error and signature handling.The refactored
build_sent_commitcorrectly extractserror_message,chain_signatures, andpatched_errorsfrom the unifiedBroadcastedIntentExecutionResult. The signature extraction logic (lines 273-294) properly handles both success cases (SingleStagevsTwoStage) and error cases where partial signatures may exist.The patched error logging at line 299 is the centralized location for this audit trail, as confirmed in previous review.
Optional: Consider extracting signature logic to a helper function.
The nested pattern matching in lines 273-294 handles multiple execution strategies and partial-failure scenarios correctly, but the complexity could be reduced by extracting to a helper method like
extract_chain_signatures(result: &Result<ExecutionOutput, E>) -> Vec<Signature>.This would improve readability without changing behavior:
fn extract_chain_signatures( result: &Result<ExecutionOutput, impl TransactionStrategyExecutionError> ) -> Vec<Signature> { match result { Ok(output) => match output { ExecutionOutput::SingleStage(sig) => vec![*sig], ExecutionOutput::TwoStage { commit_signature, finalize_signature } => { vec![*commit_signature, *finalize_signature] } }, Err(err) => { err.signatures() .map(|(commit, finalize)| { finalize .map(|f| vec![commit, f]) .unwrap_or_else(|| vec![commit]) }) .unwrap_or_default() } } }Then call it as:
let chain_signatures = extract_chain_signatures(&result.inner);
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
magicblock-accounts/src/scheduled_commits_processor.rs(7 hunks)magicblock-committor-service/src/intent_executor/two_stage_executor.rs(6 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: taco-paco
Repo: magicblock-labs/magicblock-validator PR: 661
File: magicblock-committor-service/src/intent_executor/single_stage_executor.rs:20-28
Timestamp: 2025-11-21T10:22:07.520Z
Learning: In magicblock-committor-service's SingleStageExecutor and TwoStageExecutor (single_stage_executor.rs and two_stage_executor.rs), the fields transaction_strategy, junk, and patched_errors are intentionally public because these executors are designed to be used independently outside of the IntentExecutor scope, and callers need access to these execution reports for cleanup and error handling.
📚 Learning: 2025-11-21T10:22:07.520Z
Learnt from: taco-paco
Repo: magicblock-labs/magicblock-validator PR: 661
File: magicblock-committor-service/src/intent_executor/single_stage_executor.rs:20-28
Timestamp: 2025-11-21T10:22:07.520Z
Learning: In magicblock-committor-service's SingleStageExecutor and TwoStageExecutor (single_stage_executor.rs and two_stage_executor.rs), the fields transaction_strategy, junk, and patched_errors are intentionally public because these executors are designed to be used independently outside of the IntentExecutor scope, and callers need access to these execution reports for cleanup and error handling.
Applied to files:
magicblock-accounts/src/scheduled_commits_processor.rsmagicblock-committor-service/src/intent_executor/two_stage_executor.rs
📚 Learning: 2025-11-07T13:09:52.253Z
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 589
File: test-kit/src/lib.rs:275-0
Timestamp: 2025-11-07T13:09:52.253Z
Learning: In test-kit, the transaction scheduler in ExecutionTestEnv is not expected to shut down during tests. Therefore, using `.unwrap()` in test helper methods like `schedule_transaction` is acceptable and will not cause issues in the test environment.
Applied to files:
magicblock-accounts/src/scheduled_commits_processor.rs
📚 Learning: 2025-11-04T10:53:50.922Z
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 589
File: magicblock-processor/src/scheduler/locks.rs:110-122
Timestamp: 2025-11-04T10:53:50.922Z
Learning: In magicblock-processor, the TransactionScheduler runs in a single, dedicated thread and will always remain single-threaded. The `next_transaction_id()` function in scheduler/locks.rs uses `unsafe static mut` which is safe given this architectural guarantee.
Applied to files:
magicblock-accounts/src/scheduled_commits_processor.rs
🔇 Additional comments (9)
magicblock-committor-service/src/intent_executor/two_stage_executor.rs (5)
1-20: LGTM!The imports are correctly structured and all are utilized in the state machine implementation.
22-41: LGTM!The state structs clearly represent the progression of the two-stage execution flow (Initialized → Committed → Finalized), with appropriate fields at each stage. The public fields align with the documented design for independent usage outside IntentExecutor.
43-46: LGTM!The generic structure correctly implements the type-state pattern with the
Sealedtrait boundary ensuring only the three defined states can be used, preventing misuse.
48-176: LGTM!The Initialized implementation correctly handles the commit stage with:
- Retry logic with recursion ceiling protection
- Proper error patching and recording of patched errors
- Cleanup tracking in the junk collection
- Correct state transition to Committed
295-301: LGTM!The sealed trait pattern is correctly implemented, ensuring type safety by restricting the state parameter to only the three defined states (Initialized, Committed, Finalized).
magicblock-accounts/src/scheduled_commits_processor.rs (4)
7-7: LGTM: Import updates align with refactoring.The import changes support the new
BroadcastedIntentExecutionResult-based flow and the added logging for patched errors.Also applies to: 19-19, 31-31
204-205: LGTM: Simplified field access.Direct field access on
BroadcastedIntentExecutionResultis cleaner than the previous nested extraction.
231-237: LGTM: Call site updated for new signature.The updated call correctly passes the unified
execution_resultto match the refactored function signature.
241-264: LGTM: Simplified function signature.Accepting a unified
BroadcastedIntentExecutionResultimproves the API over separate parameters and aligns with the refactoring goals.
magicblock-committor-service/src/intent_executor/two_stage_executor.rs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
magicblock-committor-service/src/intent_executor/two_stage_executor.rs (1)
258-292: Tighten patch_finalize_strategy: sync function + clearer comment
patch_finalize_strategyis declaredasyncbut does not perform anyawaits; it only calls synchronous helpers (handle_actions_error,handle_undelegation_error). Unless you expect to add async work here soon, making it synchronous would avoid the extra async state machine and the “async fn with no await” lint, and letsfinalizecall it without.await. Also, the comment above theCommitIDErrorarm still mentions “Two Stage commit” even though this is finalize‑stage patching.You could address both points with changes like:
- pub async fn patch_finalize_strategy( + pub fn patch_finalize_strategy( inner: &IntentExecutorImpl<T, F>, err: &TransactionStrategyExecutionError, finalize_strategy: &mut TransactionStrategy, ) -> IntentExecutorResult<ControlFlow<(), TransactionStrategy>> { match err { TransactionStrategyExecutionError::CommitIDError(_, _) => { - // Unexpected error in Two Stage commit - error!("Unexpected error in two stage finalize flow: {}", err); + // Unexpected CommitIDError during finalize; indicates a bug in the two-stage flow wiring. + error!("Unexpected error in two stage finalize flow: {}", err); }and in
finalize:- let flow = Self::patch_finalize_strategy( - self.inner, - &execution_err, - &mut self.state.finalize_strategy, - ) - .await?; + let flow = Self::patch_finalize_strategy( + self.inner, + &execution_err, + &mut self.state.finalize_strategy, + )?;If you prefer the symmetry of both patch methods being async for future flexibility, leaving it as‑is is also acceptable; in that case, I’d still recommend updating the comment to avoid the “commit” wording.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
magicblock-committor-service/src/intent_executor/two_stage_executor.rs(6 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: taco-paco
Repo: magicblock-labs/magicblock-validator PR: 661
File: magicblock-committor-service/src/intent_executor/single_stage_executor.rs:20-28
Timestamp: 2025-11-21T10:22:07.520Z
Learning: In magicblock-committor-service's SingleStageExecutor and TwoStageExecutor (single_stage_executor.rs and two_stage_executor.rs), the fields transaction_strategy, junk, and patched_errors are intentionally public because these executors are designed to be used independently outside of the IntentExecutor scope, and callers need access to these execution reports for cleanup and error handling.
📚 Learning: 2025-11-21T10:22:07.520Z
Learnt from: taco-paco
Repo: magicblock-labs/magicblock-validator PR: 661
File: magicblock-committor-service/src/intent_executor/single_stage_executor.rs:20-28
Timestamp: 2025-11-21T10:22:07.520Z
Learning: In magicblock-committor-service's SingleStageExecutor and TwoStageExecutor (single_stage_executor.rs and two_stage_executor.rs), the fields transaction_strategy, junk, and patched_errors are intentionally public because these executors are designed to be used independently outside of the IntentExecutor scope, and callers need access to these execution reports for cleanup and error handling.
Applied to files:
magicblock-committor-service/src/intent_executor/two_stage_executor.rs
🔇 Additional comments (4)
magicblock-committor-service/src/intent_executor/two_stage_executor.rs (4)
22-46: State structs and TwoStageExecutor abstraction look solidUsing explicit
Initialized/Committed/Finalizedstate structs plus a genericTwoStageExecutor<'_, T, F, S: Sealed>cleanly enforces the stage ordering and exposes just enough public state (strategies and signatures) for external inspection, while keepinginnerconstrained tocrate::intent_executor. This matches the intent of making executor state observable to callers without over‑exposing internals.Based on learnings, this preserves the pattern of exposing execution reports (strategies, patched errors, junk) for external cleanup/error handling.
55-131: Commit loop correctly records patched errors and cleanup; verify preparation‑error behaviorThe commit loop’s retry logic looks consistent: only
TransactionStrategyExecutionErrors that are actually patched and retried are recorded intoinner.patched_errors, and each retry’s cleanupTransactionStrategyis pushed intoinner.junk, with the finalcommit_strategyalso eventually moved intojunkon success or patched failure. One subtlety: ifprepare_and_execute_strategyfails with a preparation error (mapped toFailedCommitPreparationError), the?short‑circuits before the final “dump into junk” push, so those failures never record junk or patched errors. If that “prepare failed ⇒ nothing to clean up” assumption is intentional, this is fine; otherwise you may want to mirror the junk/patched_errors behavior for that path as well.Can you confirm that
FailedCommitPreparationErrorindeed represents a state where no cleanup or patched error tracking is needed?
140-175: Commit‑stage patching logic and logging match the error taxonomy
patch_commit_strategyonly treatsCommitIDErroras recoverable (viahandle_commit_id_errorandControlFlow::Continue), while treatingActionsError,UndelegationError,CpiLimitError, andInternalErroras non‑recoverable and logging them appropriately for the commit stage. This is a clear and consistent partition of recoverable vs non‑recoverable commit errors, and the logging context now correctly references the two‑stage commit flow.
295-301: Sealed trait pattern correctly constrains executor statesThe private
sealedmodule withpub trait Sealedimplemented only forInitialized,Committed, andFinalizedis a clean typestate pattern: it prevents external code from inventing newSstates while still allowingTwoStageExecutor<'_, T, F, S: Sealed>to be generic over the three internal stages. This nicely constrains the API surface without extra boilerplate.
# Conflicts: # magicblock-accounts/src/scheduled_commits_processor.rs # magicblock-committor-service/src/transaction_preparator/delivery_preparator.rs # programs/magicblock/src/schedule_transactions/process_scheduled_commit_sent.rs
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
magicblock-committor-service/src/intent_executor/two_stage_executor.rs (1)
282-285: Confirm intended log severity forCpiLimitErrorin finalize vs commit.In commit‑stage patching,
CpiLimitErroris logged witherror!, while in finalize‑stage patching it’s logged withwarn!:
- Commit path:
error!("Commit tasks exceeded CpiLimitError: {}", err);- Finalize path:
warn!("Finalization tasks exceeded CpiLimitError: {}", err);If both represent similarly severe resource exhaustion, you may want to align these levels (or add a brief comment explaining why finalization is downgraded to a warning) so operators can interpret logs consistently.
For example, if this was unintentional:
- warn!("Finalization tasks exceeded CpiLimitError: {}", err); + error!("Finalization tasks exceeded CpiLimitError: {}", err);
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
magicblock-committor-service/src/intent_executor/two_stage_executor.rs(6 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: taco-paco
Repo: magicblock-labs/magicblock-validator PR: 661
File: magicblock-committor-service/src/intent_executor/single_stage_executor.rs:20-28
Timestamp: 2025-11-21T10:22:07.520Z
Learning: In magicblock-committor-service's SingleStageExecutor and TwoStageExecutor (single_stage_executor.rs and two_stage_executor.rs), the fields transaction_strategy, junk, and patched_errors are intentionally public because these executors are designed to be used independently outside of the IntentExecutor scope, and callers need access to these execution reports for cleanup and error handling.
📚 Learning: 2025-11-21T10:22:07.520Z
Learnt from: taco-paco
Repo: magicblock-labs/magicblock-validator PR: 661
File: magicblock-committor-service/src/intent_executor/single_stage_executor.rs:20-28
Timestamp: 2025-11-21T10:22:07.520Z
Learning: In magicblock-committor-service's SingleStageExecutor and TwoStageExecutor (single_stage_executor.rs and two_stage_executor.rs), the fields transaction_strategy, junk, and patched_errors are intentionally public because these executors are designed to be used independently outside of the IntentExecutor scope, and callers need access to these execution reports for cleanup and error handling.
Applied to files:
magicblock-committor-service/src/intent_executor/two_stage_executor.rs
📚 Learning: 2025-11-21T11:03:26.756Z
Learnt from: thlorenz
Repo: magicblock-labs/magicblock-validator PR: 664
File: magicblock-chainlink/src/testing/mod.rs:342-370
Timestamp: 2025-11-21T11:03:26.756Z
Learning: In the magicblock-validator codebase, avoid leaving review comments that merely acknowledge code is correct or well-structured when there is no actionable suggestion, improvement, or issue to flag. Only comment when there is something specific to recommend, fix, or clarify.
Applied to files:
magicblock-committor-service/src/intent_executor/two_stage_executor.rs
📚 Learning: 2025-11-20T08:57:07.217Z
Learnt from: thlorenz
Repo: magicblock-labs/magicblock-validator PR: 650
File: magicblock-chainlink/src/submux/subscription_task.rs:13-99
Timestamp: 2025-11-20T08:57:07.217Z
Learning: In the magicblock-validator repository, avoid posting review comments that merely confirm code is correct or matches intended behavior without providing actionable feedback, suggestions for improvement, or identifying potential issues. Such confirmatory comments are considered unhelpful noise by the maintainers.
Applied to files:
magicblock-committor-service/src/intent_executor/two_stage_executor.rs
| TransactionStrategyExecutionError::CommitIDError(_, _) => { | ||
| // Unexpected error in Two Stage commit | ||
| error!("Unexpected error in Two stage commit flow: {}", err); | ||
| error!("Unexpected error in two stage finalize flow: {}", err); | ||
| Ok(ControlFlow::Break(())) |
There was a problem hiding this comment.
Clarify finalize‑stage comment for unexpected CommitIDError.
In the patch_finalize_strategy branch for TransactionStrategyExecutionError::CommitIDError, the inline comment still says “Unexpected error in Two Stage commit”, but this function is finalize‑specific and you already log "two stage finalize flow".
To avoid commit/finalize confusion when skimming, consider updating the comment to explicitly mention finalize and why a commit‑ID error here is unexpected:
- TransactionStrategyExecutionError::CommitIDError(_, _) => {
- // Unexpected error in Two Stage commit
- error!("Unexpected error in two stage finalize flow: {}", err);
+ TransactionStrategyExecutionError::CommitIDError(_, _) => {
+ // CommitIDError should only occur during commit; seeing it in finalize is unexpected
+ error!("Unexpected error in two stage finalize flow: {}", err);📝 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.
| TransactionStrategyExecutionError::CommitIDError(_, _) => { | |
| // Unexpected error in Two Stage commit | |
| error!("Unexpected error in Two stage commit flow: {}", err); | |
| error!("Unexpected error in two stage finalize flow: {}", err); | |
| Ok(ControlFlow::Break(())) | |
| TransactionStrategyExecutionError::CommitIDError(_, _) => { | |
| // CommitIDError should only occur during commit; seeing it in finalize is unexpected | |
| error!("Unexpected error in two stage finalize flow: {}", err); | |
| Ok(ControlFlow::Break(())) |
🤖 Prompt for AI Agents
In magicblock-committor-service/src/intent_executor/two_stage_executor.rs around
lines 264 to 267, update the inline comment for the
TransactionStrategyExecutionError::CommitIDError arm in the
patch_finalize_strategy branch to explicitly reference the finalize stage (not
commit) and clarify why a commit-ID error is unexpected during finalize; change
the comment to something like "Unexpected CommitIDError during Two Stage
finalize flow — commit ID errors should not occur in finalize" so readers don't
confuse finalize with commit when scanning the code.
| let intent_id = execution_result.id; | ||
| let trigger_type = execution_result.trigger_type; | ||
| // Here we handle on OnChain triggered intent | ||
| // TODO: should be removed once crank supported |
There was a problem hiding this comment.
Should we open an issue to clean this up, since the cranking is supported now?
|
|
||
| impl<'a, T, F> Deref for TwoStageExecutor<'a, T, F> { | ||
| type Target = IntentExecutorImpl<T, F>; | ||
| mod sealed { |
There was a problem hiding this comment.
What's the point of sealing?
|
|
||
| /// Error code returned when an intent execution failed. | ||
| /// This indicates the intent could not be successfully executed despite patching attempts. | ||
| const INTENT_FAILED_CODE: u32 = 0x7461636F; |
There was a problem hiding this comment.
Probably this custom instruction code should be documented somewhere in the public docs later
Added patched errors report in SentCommit
Refactor
SingleStageExecutor & TwoStageExecutors to iterate and modify their own state to record erros the patched.
IntentExecutor now has method cleanup that can be explicitly awaited on(useful for tests) or offloaded on another task(useful for prod).
Patched errors trace also now checked in tests, which improves tests quality
Depends
#635 #643
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.