Recover from unfinalized accout #731
Conversation
Manual Deploy AvailableYou can trigger a manual deploy of this PR branch to testnet: Alternative: Comment
Comment updated automatically when the PR is synchronized. |
📝 WalkthroughWalkthroughThis PR implements recovery handling for unfinalized account errors that occur during the commit stage. A new error variant Assessment against linked issues
Out-of-scope changesNo out-of-scope changes detected. All code modifications directly support the implementation of unfinalized account error recovery during the commit stage as described in the linked issue. Pre-merge checks and finishing touches✅ 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: 2
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (3)
Cargo.lockis excluded by!**/*.locktest-integration/Cargo.lockis excluded by!**/*.locktest-integration/schedulecommit/elfs/dlp.sois excluded by!**/*.so
📒 Files selected for processing (6)
Cargo.toml(1 hunks)magicblock-committor-service/src/intent_executor/error.rs(5 hunks)magicblock-committor-service/src/intent_executor/single_stage_executor.rs(4 hunks)magicblock-committor-service/src/intent_executor/two_stage_executor.rs(4 hunks)test-integration/Cargo.toml(1 hunks)test-integration/test-committor-service/tests/test_intent_executor.rs(3 hunks)
🧰 Additional context used
🧠 Learnings (11)
📓 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.
Learnt from: Dodecahedr0x
Repo: magicblock-labs/magicblock-validator PR: 639
File: test-integration/test-committor-service/tests/test_ix_commit_local.rs:867-881
Timestamp: 2025-12-03T09:33:48.707Z
Learning: Repo: magicblock-labs/magicblock-validator PR: 639
Context: test-integration/test-committor-service/tests/test_ix_commit_local.rs (ix_commit_local)
Learning: The PhotonIndexer used for compressed account fetches (get_compressed_account) has built‑in retry logic (defaults to ~10 attempts), so tests should not add separate retry loops around compressed fetches unless there’s a specific need.
Learnt from: Dodecahedr0x
Repo: magicblock-labs/magicblock-validator PR: 639
File: magicblock-chainlink/src/remote_account_provider/mod.rs:1350-1353
Timestamp: 2025-12-03T09:36:01.527Z
Learning: Repo: magicblock-labs/magicblock-validator
File: magicblock-chainlink/src/remote_account_provider/mod.rs
Context: consolidate_fetched_remote_accounts
Learning: For unexpected result counts (>2), the project prefers logging an error and returning an empty Vec over panicking; acceptable during development per maintainer (Dodecahedr0x).
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.
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
📚 Learning: 2025-11-24T14:21:00.996Z
Learnt from: Dodecahedr0x
Repo: magicblock-labs/magicblock-validator PR: 639
File: Cargo.toml:58-58
Timestamp: 2025-11-24T14:21:00.996Z
Learning: In the magicblock-validator codebase, magicblock-api/Cargo.toml intentionally uses borsh = "1.5.3" (instead of the workspace version 0.10.4) because it needs to deserialize types from the magic-domain-program external dependency, which requires borsh 1.5.x compatibility. This is an intentional exception for interoperability with the magic domain program.
Applied to files:
Cargo.tomltest-integration/Cargo.toml
📚 Learning: 2025-10-26T16:54:39.084Z
Learnt from: thlorenz
Repo: magicblock-labs/magicblock-validator PR: 587
File: test-manual/Cargo.toml:0-0
Timestamp: 2025-10-26T16:54:39.084Z
Learning: In the magicblock-validator repository, use git branch references (not commit hashes or tags) for the helius-laserstream dependency to allow automatic updates when the branch is pushed to.
Applied to files:
Cargo.tomltest-integration/Cargo.toml
📚 Learning: 2025-12-03T09:33:48.707Z
Learnt from: Dodecahedr0x
Repo: magicblock-labs/magicblock-validator PR: 639
File: test-integration/test-committor-service/tests/test_ix_commit_local.rs:867-881
Timestamp: 2025-12-03T09:33:48.707Z
Learning: Repo: magicblock-labs/magicblock-validator PR: 639
Context: test-integration/test-committor-service/tests/test_ix_commit_local.rs (ix_commit_local)
Learning: The PhotonIndexer used for compressed account fetches (get_compressed_account) has built‑in retry logic (defaults to ~10 attempts), so tests should not add separate retry loops around compressed fetches unless there’s a specific need.
Applied to files:
test-integration/test-committor-service/tests/test_intent_executor.rsmagicblock-committor-service/src/intent_executor/error.rsmagicblock-committor-service/src/intent_executor/single_stage_executor.rsmagicblock-committor-service/src/intent_executor/two_stage_executor.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_intent_executor.rsmagicblock-committor-service/src/intent_executor/error.rsmagicblock-committor-service/src/intent_executor/single_stage_executor.rsmagicblock-committor-service/src/intent_executor/two_stage_executor.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_intent_executor.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_intent_executor.rs
📚 Learning: 2025-12-03T09:36:01.527Z
Learnt from: Dodecahedr0x
Repo: magicblock-labs/magicblock-validator PR: 639
File: magicblock-chainlink/src/remote_account_provider/mod.rs:1350-1353
Timestamp: 2025-12-03T09:36:01.527Z
Learning: Repo: magicblock-labs/magicblock-validator
File: magicblock-chainlink/src/remote_account_provider/mod.rs
Context: consolidate_fetched_remote_accounts
Learning: For unexpected result counts (>2), the project prefers logging an error and returning an empty Vec over panicking; acceptable during development per maintainer (Dodecahedr0x).
Applied to files:
test-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/test_intent_executor.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_intent_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:
test-integration/test-committor-service/tests/test_intent_executor.rsmagicblock-committor-service/src/intent_executor/single_stage_executor.rsmagicblock-committor-service/src/intent_executor/two_stage_executor.rs
🧬 Code graph analysis (4)
test-integration/test-committor-service/tests/test_intent_executor.rs (3)
test-integration/programs/flexi-counter/src/state.rs (1)
pda(34-37)magicblock-processor/tests/fees.rs (1)
ephemeral_balance_pda_from_payer(21-27)magicblock-rpc-client/src/lib.rs (1)
ensure_committed(155-167)
magicblock-committor-service/src/intent_executor/error.rs (3)
magicblock-rpc-client/src/lib.rs (2)
error(201-203)signature(90-98)magicblock-committor-service/src/error.rs (1)
signature(77-84)magicblock-committor-service/src/transaction_preparator/error.rs (1)
signature(26-31)
magicblock-committor-service/src/intent_executor/single_stage_executor.rs (3)
magicblock-committor-service/src/intent_executor/two_stage_executor.rs (3)
handle_unfinalized_account_error(205-240)None(227-227)new(60-72)magicblock-committor-service/src/intent_execution_manager/intent_execution_engine.rs (4)
None(417-417)new(51-64)new(105-122)new(703-709)magicblock-committor-service/src/tasks/args_task.rs (1)
new(37-42)
magicblock-committor-service/src/intent_executor/two_stage_executor.rs (3)
magicblock-committor-service/src/intent_executor/single_stage_executor.rs (3)
handle_unfinalized_account_error(190-226)None(212-212)new(36-44)magicblock-committor-service/src/tasks/buffer_task.rs (1)
new(38-46)magicblock-committor-service/src/tasks/args_task.rs (1)
new(37-42)
🔇 Additional comments (17)
test-integration/Cargo.toml (2)
60-62: Consistency check: Both Cargo.toml files updated identically.The dependency revision update is consistently applied to both test-integration/Cargo.toml (line 60) and root Cargo.toml (line 110), maintaining workspace coherence.
60-62: Clarify which Cargo.toml files are being updated and verify the old revision.Both test-integration/Cargo.toml and root Cargo.toml currently reference
magicblock-delegation-programat the same revisiona56deac(full hash: a56deaca2fac5fb422565fdf6369baf893266d19). The comment claims both files update fromaa1de56d90ctoa56deac, but the old revisionaa1de56d90cdoes not appear in the current codebase. Clarify whether: (1) both files were already ata56deacbefore this PR, (2) only one file was updated, or (3) the old revision reference is incorrect. Additionally, confirm the delegation-program revision is valid and supports the UnfinalizedAccountError handling logic referenced in the PR.Likely an incorrect or invalid review comment.
Cargo.toml (1)
110-112: Confirm the delegation-program revision is appropriate for this PR's scope.The root Cargo.toml already references revision a56deac, which matches test-integration/Cargo.toml. The revision includes new features such as diff algorithms (compute_diff, apply_diff_*), CommitDiff instruction, and enhanced error codes for require_uninitialized validation. However, verify that these additions are actually necessary for the intended PR objectives, as the current codebase does not directly use CommitDiff or diff module APIs. The UnfinalizedAccountError recovery mechanism in the committor service uses a Finalize task pattern rather than the new diff-based features introduced in this delegation-program revision.
Likely an incorrect or invalid review comment.
magicblock-committor-service/src/intent_executor/single_stage_executor.rs (2)
5-24: LGTM! Imports support the new unfinalized account recovery path.The new imports are appropriate for constructing and executing the finalize task during recovery.
148-168: LGTM! UnfinalizedAccountError handling integrates well with existing recovery patterns.The approach correctly extracts the failed task using
task_index()and delegates to the recovery helper. The fallback toControlFlow::Break(())when the task cannot be located is appropriate.magicblock-committor-service/src/intent_executor/error.rs (5)
147-148: LGTM! New error variant for unfinalized account recovery.The
UnfinalizedAccountErrorvariant follows the same pattern as existing variants with source error and optional signature.
160-191: LGTM! Task index extraction is robust.Using
checked_subsafely handles the offset calculation, and the method correctly covers all error variants containing instruction errors.
193-202: LGTM! Signature propagation extended correctly.
250-279: LGTM! Error classification for Commit tasks is well-structured.The nested match correctly routes:
- Nonce ordering issues →
CommitIDError(recoverable via ID refresh)- Pre-existing state/record →
UnfinalizedAccountError(recoverable via finalize)- Other errors → passthrough (unhandled)
212-223: Adddlpcrate as a dependency to fix compilation error.The code references
dlp::error::DlpErrorvariants but thedlpcrate is not declared as a dependency inmagicblock-committor-service/Cargo.toml. This will cause a compilation failure. Add the appropriatedlpdependency with the version compatible with your DLP library.⛔ Skipped due to learnings
Learnt from: Dodecahedr0x Repo: magicblock-labs/magicblock-validator PR: 639 File: magicblock-chainlink/src/remote_account_provider/mod.rs:1350-1353 Timestamp: 2025-12-03T09:36:01.527Z Learning: Repo: magicblock-labs/magicblock-validator File: magicblock-chainlink/src/remote_account_provider/mod.rs Context: consolidate_fetched_remote_accounts Learning: For unexpected result counts (>2), the project prefers logging an error and returning an empty Vec over panicking; acceptable during development per maintainer (Dodecahedr0x).Learnt from: Dodecahedr0x Repo: magicblock-labs/magicblock-validator PR: 639 File: test-integration/test-committor-service/tests/test_ix_commit_local.rs:867-881 Timestamp: 2025-12-03T09:33:48.707Z Learning: Repo: magicblock-labs/magicblock-validator PR: 639 Context: test-integration/test-committor-service/tests/test_ix_commit_local.rs (ix_commit_local) Learning: The PhotonIndexer used for compressed account fetches (get_compressed_account) has built‑in retry logic (defaults to ~10 attempts), so tests should not add separate retry loops around compressed fetches unless there’s a specific need.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/579Learnt from: Dodecahedr0x Repo: magicblock-labs/magicblock-validator PR: 639 File: Cargo.toml:58-58 Timestamp: 2025-11-24T14:21:00.996Z Learning: In the magicblock-validator codebase, magicblock-api/Cargo.toml intentionally uses borsh = "1.5.3" (instead of the workspace version 0.10.4) because it needs to deserialize types from the magic-domain-program external dependency, which requires borsh 1.5.x compatibility. This is an intentional exception for interoperability with the magic domain program.test-integration/test-committor-service/tests/test_intent_executor.rs (3)
13-13: LGTM! New imports support unfinalized account test setup.Also applies to: 38-38
828-903: LGTM! Test validates single-stage unfinalized account recovery.The test correctly:
- Simulates an unfinalized account by committing state without finalization
- Executes a new intent and verifies recovery succeeds
- Asserts the expected error sequence (
UnfinalizedAccountError→CommitIDError)The
CommitIDErrorafterUnfinalizedAccountErroris expected because finalizing the stale commit advances the nonce, invalidating the cached commit ID.
905-993: Test validates two-stage unfinalized account recovery path.The test correctly simulates recovery for multi-account intents that trigger the two-stage flow. The error sequence assertion (
UnfinalizedAccountError→CommitIDError) aligns with the single-stage test.magicblock-committor-service/src/intent_executor/two_stage_executor.rs (4)
17-23: LGTM! Imports support the unfinalized account recovery path.
158-178: LGTM! UnfinalizedAccountError handling in commit strategy.The implementation correctly integrates with the existing error handling pattern and delegates to the recovery helper.
205-240: Verify error variant semantics for two-stage recovery context.The error mapping uses
FailedCommitPreparationErrorandFailedToCommitErrorsince this recovery occurs during the commit phase. However, the operation being performed is a finalization of a stale account, which is semantically a finalize operation.Consider whether:
- Using commit-phase errors is correct because we're in the commit execution loop
- Or finalize-phase errors would be more accurate since we're executing a finalize task
The current approach is defensible (we're recovering during commit), but worth documenting the rationale.
329-337: LGTM! UnfinalizedAccountError is correctly treated as unexpected in finalize phase.During finalization, the commit stage has already succeeded, so encountering an unfinalized account error would indicate an unexpected state. Logging and breaking execution is the appropriate response.
test-integration/test-committor-service/tests/test_intent_executor.rs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
test-integration/test-committor-service/tests/test_intent_executor.rs(3 hunks)
🧰 Additional context used
🧠 Learnings (11)
📓 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.
Learnt from: Dodecahedr0x
Repo: magicblock-labs/magicblock-validator PR: 639
File: test-integration/test-committor-service/tests/test_ix_commit_local.rs:867-881
Timestamp: 2025-12-03T09:33:48.707Z
Learning: Repo: magicblock-labs/magicblock-validator PR: 639
Context: test-integration/test-committor-service/tests/test_ix_commit_local.rs (ix_commit_local)
Learning: The PhotonIndexer used for compressed account fetches (get_compressed_account) has built‑in retry logic (defaults to ~10 attempts), so tests should not add separate retry loops around compressed fetches unless there’s a specific need.
Learnt from: Dodecahedr0x
Repo: magicblock-labs/magicblock-validator PR: 639
File: magicblock-chainlink/src/remote_account_provider/mod.rs:1350-1353
Timestamp: 2025-12-03T09:36:01.527Z
Learning: Repo: magicblock-labs/magicblock-validator
File: magicblock-chainlink/src/remote_account_provider/mod.rs
Context: consolidate_fetched_remote_accounts
Learning: For unexpected result counts (>2), the project prefers logging an error and returning an empty Vec over panicking; acceptable during development per maintainer (Dodecahedr0x).
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
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.
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.
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.
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 596
File: magicblock-processor/src/scheduler.rs:1-1
Timestamp: 2025-10-28T13:15:42.706Z
Learning: In magicblock-processor, transaction indexes were always set to 0 even before the changes in PR #596. The proper transaction indexing within slots will be addressed during the planned ledger rewrite.
📚 Learning: 2025-12-03T09:33:48.707Z
Learnt from: Dodecahedr0x
Repo: magicblock-labs/magicblock-validator PR: 639
File: test-integration/test-committor-service/tests/test_ix_commit_local.rs:867-881
Timestamp: 2025-12-03T09:33:48.707Z
Learning: Repo: magicblock-labs/magicblock-validator PR: 639
Context: test-integration/test-committor-service/tests/test_ix_commit_local.rs (ix_commit_local)
Learning: The PhotonIndexer used for compressed account fetches (get_compressed_account) has built‑in retry logic (defaults to ~10 attempts), so tests should not add separate retry loops around compressed fetches unless there’s a specific need.
Applied to files:
test-integration/test-committor-service/tests/test_intent_executor.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_intent_executor.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_intent_executor.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_intent_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:
test-integration/test-committor-service/tests/test_intent_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:
test-integration/test-committor-service/tests/test_intent_executor.rs
📚 Learning: 2025-12-03T09:36:01.527Z
Learnt from: Dodecahedr0x
Repo: magicblock-labs/magicblock-validator PR: 639
File: magicblock-chainlink/src/remote_account_provider/mod.rs:1350-1353
Timestamp: 2025-12-03T09:36:01.527Z
Learning: Repo: magicblock-labs/magicblock-validator
File: magicblock-chainlink/src/remote_account_provider/mod.rs
Context: consolidate_fetched_remote_accounts
Learning: For unexpected result counts (>2), the project prefers logging an error and returning an empty Vec over panicking; acceptable during development per maintainer (Dodecahedr0x).
Applied to files:
test-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/test_intent_executor.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_intent_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:
test-integration/test-committor-service/tests/test_intent_executor.rs
# Conflicts: # Cargo.lock # test-integration/Cargo.lock
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-committor-service/src/intent_executor/error.rs (1)
306-316: Add a specific metric label forUnfinalizedAccountError.The
LabelValueimplementation doesn't include a specific case forUnfinalizedAccountError, causing it to fall through to the generic"failed"label. Adding a specific label would improve observability for this recovery scenario.impl metrics::LabelValue for TransactionStrategyExecutionError { fn value(&self) -> &str { match self { Self::ActionsError(_, _) => "actions_failed", Self::CpiLimitError(_, _) => "cpi_limit_failed", Self::CommitIDError(_, _) => "commit_nonce_failed", Self::UndelegationError(_, _) => "undelegation_failed", + Self::UnfinalizedAccountError(_, _) => "unfinalized_account", _ => "failed", } } }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.locktest-integration/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
Cargo.toml(1 hunks)magicblock-committor-service/src/intent_executor/error.rs(5 hunks)magicblock-committor-service/src/intent_executor/single_stage_executor.rs(4 hunks)magicblock-committor-service/src/intent_executor/two_stage_executor.rs(4 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.
Learnt from: Dodecahedr0x
Repo: magicblock-labs/magicblock-validator PR: 639
File: test-integration/test-committor-service/tests/test_ix_commit_local.rs:867-881
Timestamp: 2025-12-03T09:33:48.707Z
Learning: Repo: magicblock-labs/magicblock-validator PR: 639
Context: test-integration/test-committor-service/tests/test_ix_commit_local.rs (ix_commit_local)
Learning: The PhotonIndexer used for compressed account fetches (get_compressed_account) has built‑in retry logic (defaults to ~10 attempts), so tests should not add separate retry loops around compressed fetches unless there’s a specific need.
Learnt from: Dodecahedr0x
Repo: magicblock-labs/magicblock-validator PR: 639
File: magicblock-chainlink/src/remote_account_provider/mod.rs:1350-1353
Timestamp: 2025-12-03T09:36:01.527Z
Learning: Repo: magicblock-labs/magicblock-validator
File: magicblock-chainlink/src/remote_account_provider/mod.rs
Context: consolidate_fetched_remote_accounts
Learning: For unexpected result counts (>2), the project prefers logging an error and returning an empty Vec over panicking; acceptable during development per maintainer (Dodecahedr0x).
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
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.
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.
📚 Learning: 2025-11-24T14:21:00.996Z
Learnt from: Dodecahedr0x
Repo: magicblock-labs/magicblock-validator PR: 639
File: Cargo.toml:58-58
Timestamp: 2025-11-24T14:21:00.996Z
Learning: In the magicblock-validator codebase, magicblock-api/Cargo.toml intentionally uses borsh = "1.5.3" (instead of the workspace version 0.10.4) because it needs to deserialize types from the magic-domain-program external dependency, which requires borsh 1.5.x compatibility. This is an intentional exception for interoperability with the magic domain program.
Applied to files:
Cargo.toml
📚 Learning: 2025-10-26T16:54:39.084Z
Learnt from: thlorenz
Repo: magicblock-labs/magicblock-validator PR: 587
File: test-manual/Cargo.toml:0-0
Timestamp: 2025-10-26T16:54:39.084Z
Learning: In the magicblock-validator repository, use git branch references (not commit hashes or tags) for the helius-laserstream dependency to allow automatic updates when the branch is pushed to.
Applied to files:
Cargo.toml
📚 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/error.rsmagicblock-committor-service/src/intent_executor/two_stage_executor.rsmagicblock-committor-service/src/intent_executor/single_stage_executor.rs
📚 Learning: 2025-12-03T09:33:48.707Z
Learnt from: Dodecahedr0x
Repo: magicblock-labs/magicblock-validator PR: 639
File: test-integration/test-committor-service/tests/test_ix_commit_local.rs:867-881
Timestamp: 2025-12-03T09:33:48.707Z
Learning: Repo: magicblock-labs/magicblock-validator PR: 639
Context: test-integration/test-committor-service/tests/test_ix_commit_local.rs (ix_commit_local)
Learning: The PhotonIndexer used for compressed account fetches (get_compressed_account) has built‑in retry logic (defaults to ~10 attempts), so tests should not add separate retry loops around compressed fetches unless there’s a specific need.
Applied to files:
magicblock-committor-service/src/intent_executor/error.rsmagicblock-committor-service/src/intent_executor/two_stage_executor.rsmagicblock-committor-service/src/intent_executor/single_stage_executor.rs
📚 Learning: 2025-12-03T09:36:01.527Z
Learnt from: Dodecahedr0x
Repo: magicblock-labs/magicblock-validator PR: 639
File: magicblock-chainlink/src/remote_account_provider/mod.rs:1350-1353
Timestamp: 2025-12-03T09:36:01.527Z
Learning: Repo: magicblock-labs/magicblock-validator
File: magicblock-chainlink/src/remote_account_provider/mod.rs
Context: consolidate_fetched_remote_accounts
Learning: For unexpected result counts (>2), the project prefers logging an error and returning an empty Vec over panicking; acceptable during development per maintainer (Dodecahedr0x).
Applied to files:
magicblock-committor-service/src/intent_executor/error.rsmagicblock-committor-service/src/intent_executor/single_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-committor-service/src/intent_executor/two_stage_executor.rsmagicblock-committor-service/src/intent_executor/single_stage_executor.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:
magicblock-committor-service/src/intent_executor/single_stage_executor.rs
🧬 Code graph analysis (2)
magicblock-committor-service/src/intent_executor/two_stage_executor.rs (2)
magicblock-committor-service/src/tasks/buffer_task.rs (1)
new(38-46)magicblock-committor-service/src/tasks/args_task.rs (1)
new(37-42)
magicblock-committor-service/src/intent_executor/single_stage_executor.rs (5)
magicblock-committor-service/src/intent_executor/error.rs (2)
signature(34-39)signature(192-201)magicblock-committor-service/src/intent_executor/task_info_fetcher.rs (2)
signature(285-291)new(52-59)magicblock-committor-service/src/intent_executor/two_stage_executor.rs (3)
handle_unfinalized_account_error(205-240)None(227-227)new(60-72)magicblock-committor-service/src/intent_executor/mod.rs (1)
new(124-138)magicblock-committor-service/src/tasks/args_task.rs (1)
new(37-42)
🔇 Additional comments (9)
magicblock-committor-service/src/intent_executor/single_stage_executor.rs (2)
1-24: LGTM - Imports for unfinalized account recovery.The new imports are well-organized and necessary for the
handle_unfinalized_account_errorimplementation.
148-168: LGTM - UnfinalizedAccountError handling path.The logic correctly:
- Extracts the task index from the error
- Retrieves the corresponding task from the strategy
- Delegates to the recovery handler or breaks if task lookup fails
magicblock-committor-service/src/intent_executor/error.rs (2)
146-147: LGTM - New error variant for unfinalized account recovery.The
UnfinalizedAccountErrorvariant follows the established pattern with#[source]annotation and optional signature.
211-278: LGTM - Error classification for unfinalized account detection.The nested match structure cleanly separates:
NONCE_OUT_OF_ORDER→CommitIDError(existing behavior)COMMIT_STATE_*/COMMIT_RECORD_*errors →UnfinalizedAccountError(new recovery path)This correctly identifies accounts that had a previous commit succeed but finalize fail, enabling the recovery flow.
magicblock-committor-service/src/intent_executor/two_stage_executor.rs (4)
17-23: LGTM - Imports for unfinalized account recovery.The imports align with the single-stage executor and support the new recovery functionality.
158-178: LGTM - Commit phase recovery for unfinalized accounts.The handling correctly identifies the failing task and delegates to the recovery handler, consistent with the single-stage implementation.
205-240: LGTM - Recovery handler with commit-phase error semantics.The implementation correctly uses
FailedCommitPreparationErrorandFailedToCommitErrorfor the commit phase context, distinguishing it from the single-stage executor's finalize-phase error mapping.
329-337: LGTM - UnfinalizedAccountError correctly treated as unexpected in finalize phase.During the finalize phase, encountering
UnfinalizedAccountErrorwould indicate a logic error (the commit phase should have already resolved this). Breaking execution with an error log is the correct response.Cargo.toml (1)
98-100: LGTM - Dependency update for unfinalized account recovery.The delegation-program revision update aligns with the new error handling for
UnfinalizedAccountErrorthat relies on DLP error codes (CommitStateAlreadyInitialized,CommitRecordAlreadyInitialized, etc.). The commita56deacexists in the delegation-program repository on thefeat/custom-codesbranch.
magicblock-committor-service/src/intent_executor/single_stage_executor.rs
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
test-integration/schedulecommit/elfs/dlp.sois excluded by!**/*.so
📒 Files selected for processing (1)
magicblock-committor-service/src/intent_executor/error.rs(5 hunks)
🧰 Additional context used
🧠 Learnings (6)
📓 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.
Learnt from: Dodecahedr0x
Repo: magicblock-labs/magicblock-validator PR: 639
File: test-integration/test-committor-service/tests/test_ix_commit_local.rs:867-881
Timestamp: 2025-12-03T09:33:48.707Z
Learning: Repo: magicblock-labs/magicblock-validator PR: 639
Context: test-integration/test-committor-service/tests/test_ix_commit_local.rs (ix_commit_local)
Learning: The PhotonIndexer used for compressed account fetches (get_compressed_account) has built‑in retry logic (defaults to ~10 attempts), so tests should not add separate retry loops around compressed fetches unless there’s a specific need.
Learnt from: Dodecahedr0x
Repo: magicblock-labs/magicblock-validator PR: 639
File: magicblock-chainlink/src/remote_account_provider/mod.rs:1350-1353
Timestamp: 2025-12-03T09:36:01.527Z
Learning: Repo: magicblock-labs/magicblock-validator
File: magicblock-chainlink/src/remote_account_provider/mod.rs
Context: consolidate_fetched_remote_accounts
Learning: For unexpected result counts (>2), the project prefers logging an error and returning an empty Vec over panicking; acceptable during development per maintainer (Dodecahedr0x).
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.
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
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.
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 596
File: magicblock-processor/src/scheduler.rs:1-1
Timestamp: 2025-10-28T13:15:42.706Z
Learning: In magicblock-processor, transaction indexes were always set to 0 even before the changes in PR #596. The proper transaction indexing within slots will be addressed during the planned ledger rewrite.
📚 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/error.rs
📚 Learning: 2025-12-03T09:36:01.527Z
Learnt from: Dodecahedr0x
Repo: magicblock-labs/magicblock-validator PR: 639
File: magicblock-chainlink/src/remote_account_provider/mod.rs:1350-1353
Timestamp: 2025-12-03T09:36:01.527Z
Learning: Repo: magicblock-labs/magicblock-validator
File: magicblock-chainlink/src/remote_account_provider/mod.rs
Context: consolidate_fetched_remote_accounts
Learning: For unexpected result counts (>2), the project prefers logging an error and returning an empty Vec over panicking; acceptable during development per maintainer (Dodecahedr0x).
Applied to files:
magicblock-committor-service/src/intent_executor/error.rs
📚 Learning: 2025-12-03T09:33:48.707Z
Learnt from: Dodecahedr0x
Repo: magicblock-labs/magicblock-validator PR: 639
File: test-integration/test-committor-service/tests/test_ix_commit_local.rs:867-881
Timestamp: 2025-12-03T09:33:48.707Z
Learning: Repo: magicblock-labs/magicblock-validator PR: 639
Context: test-integration/test-committor-service/tests/test_ix_commit_local.rs (ix_commit_local)
Learning: The PhotonIndexer used for compressed account fetches (get_compressed_account) has built‑in retry logic (defaults to ~10 attempts), so tests should not add separate retry loops around compressed fetches unless there’s a specific need.
Applied to files:
magicblock-committor-service/src/intent_executor/error.rs
📚 Learning: 2025-10-28T13:15:42.706Z
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 596
File: magicblock-processor/src/scheduler.rs:1-1
Timestamp: 2025-10-28T13:15:42.706Z
Learning: In magicblock-processor, transaction indexes were always set to 0 even before the changes in PR #596. The proper transaction indexing within slots will be addressed during the planned ledger rewrite.
Applied to files:
magicblock-committor-service/src/intent_executor/error.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:
magicblock-committor-service/src/intent_executor/error.rs
🧬 Code graph analysis (1)
magicblock-committor-service/src/intent_executor/error.rs (2)
magicblock-committor-service/src/error.rs (1)
signature(72-79)magicblock-committor-service/src/transaction_preparator/delivery_preparator.rs (4)
signature(505-510)signature(525-530)signature(566-572)signature(584-589)
🔇 Additional comments (1)
magicblock-committor-service/src/intent_executor/error.rs (1)
158-161: CentralizingTASK_OFFSETand reusing it intask_index/try_from_transaction_errorlooks correct.The new
TASK_OFFSETassociated const (with doc comment) plus the use ofchecked_sub(Self::TASK_OFFSET)both intask_index()and intry_from_transaction_errorcleanly encode the “two compute-budget instructions prelude” invariant and avoid underflow. The additional pattern arm forUnfinalizedAccountErrorintask_index()keeps the index mapping consistent across all per-task error variants. This resolves the earlier OFFSET documentation nit and improves maintainability.Also applies to: 167-191, 239-241
taco-paco
force-pushed
the
feat/base-layer-ix/recover-finalize
branch
from
aaeacb1 to
04d775c
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
magicblock-committor-service/src/intent_executor/error.rs (2)
212-279: Add a short rationale for the “commit-state/record exists” → UnfinalizedAccountError mapping.A 1–2 line comment explaining that these DLP errors indicate a prior successful commit with a failed/missing finalize (hence recoverable by running finalize) would prevent future “why is this not a hard commit failure?” confusion.
308-316: Expose UnfinalizedAccountError as a dedicated metrics label.Right now it falls into the generic
"failed"bucket, which makes it harder to validate how effective the recovery path is in production.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (3)
Cargo.lockis excluded by!**/*.locktest-integration/Cargo.lockis excluded by!**/*.locktest-integration/schedulecommit/elfs/dlp.sois excluded by!**/*.so
📒 Files selected for processing (5)
Cargo.toml(1 hunks)magicblock-committor-service/src/intent_executor/error.rs(5 hunks)magicblock-committor-service/src/intent_executor/single_stage_executor.rs(4 hunks)magicblock-committor-service/src/intent_executor/two_stage_executor.rs(4 hunks)test-integration/Cargo.toml(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.
Learnt from: Dodecahedr0x
Repo: magicblock-labs/magicblock-validator PR: 639
File: test-integration/test-committor-service/tests/test_ix_commit_local.rs:867-881
Timestamp: 2025-12-03T09:33:48.707Z
Learning: Repo: magicblock-labs/magicblock-validator PR: 639
Context: test-integration/test-committor-service/tests/test_ix_commit_local.rs (ix_commit_local)
Learning: The PhotonIndexer used for compressed account fetches (get_compressed_account) has built‑in retry logic (defaults to ~10 attempts), so tests should not add separate retry loops around compressed fetches unless there’s a specific need.
Learnt from: Dodecahedr0x
Repo: magicblock-labs/magicblock-validator PR: 639
File: magicblock-chainlink/src/remote_account_provider/mod.rs:1350-1353
Timestamp: 2025-12-03T09:36:01.527Z
Learning: Repo: magicblock-labs/magicblock-validator
File: magicblock-chainlink/src/remote_account_provider/mod.rs
Context: consolidate_fetched_remote_accounts
Learning: For unexpected result counts (>2), the project prefers logging an error and returning an empty Vec over panicking; acceptable during development per maintainer (Dodecahedr0x).
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.
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
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.
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 596
File: magicblock-processor/src/scheduler.rs:1-1
Timestamp: 2025-10-28T13:15:42.706Z
Learning: In magicblock-processor, transaction indexes were always set to 0 even before the changes in PR #596. The proper transaction indexing within slots will be addressed during the planned ledger rewrite.
📚 Learning: 2025-11-24T14:21:00.996Z
Learnt from: Dodecahedr0x
Repo: magicblock-labs/magicblock-validator PR: 639
File: Cargo.toml:58-58
Timestamp: 2025-11-24T14:21:00.996Z
Learning: In the magicblock-validator codebase, magicblock-api/Cargo.toml intentionally uses borsh = "1.5.3" (instead of the workspace version 0.10.4) because it needs to deserialize types from the magic-domain-program external dependency, which requires borsh 1.5.x compatibility. This is an intentional exception for interoperability with the magic domain program.
Applied to files:
test-integration/Cargo.tomlCargo.toml
📚 Learning: 2025-10-26T16:54:39.084Z
Learnt from: thlorenz
Repo: magicblock-labs/magicblock-validator PR: 587
File: test-manual/Cargo.toml:0-0
Timestamp: 2025-10-26T16:54:39.084Z
Learning: In the magicblock-validator repository, use git branch references (not commit hashes or tags) for the helius-laserstream dependency to allow automatic updates when the branch is pushed to.
Applied to files:
test-integration/Cargo.tomlCargo.toml
📚 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/error.rsmagicblock-committor-service/src/intent_executor/single_stage_executor.rsmagicblock-committor-service/src/intent_executor/two_stage_executor.rs
📚 Learning: 2025-12-03T09:36:01.527Z
Learnt from: Dodecahedr0x
Repo: magicblock-labs/magicblock-validator PR: 639
File: magicblock-chainlink/src/remote_account_provider/mod.rs:1350-1353
Timestamp: 2025-12-03T09:36:01.527Z
Learning: Repo: magicblock-labs/magicblock-validator
File: magicblock-chainlink/src/remote_account_provider/mod.rs
Context: consolidate_fetched_remote_accounts
Learning: For unexpected result counts (>2), the project prefers logging an error and returning an empty Vec over panicking; acceptable during development per maintainer (Dodecahedr0x).
Applied to files:
magicblock-committor-service/src/intent_executor/error.rs
📚 Learning: 2025-12-03T09:33:48.707Z
Learnt from: Dodecahedr0x
Repo: magicblock-labs/magicblock-validator PR: 639
File: test-integration/test-committor-service/tests/test_ix_commit_local.rs:867-881
Timestamp: 2025-12-03T09:33:48.707Z
Learning: Repo: magicblock-labs/magicblock-validator PR: 639
Context: test-integration/test-committor-service/tests/test_ix_commit_local.rs (ix_commit_local)
Learning: The PhotonIndexer used for compressed account fetches (get_compressed_account) has built‑in retry logic (defaults to ~10 attempts), so tests should not add separate retry loops around compressed fetches unless there’s a specific need.
Applied to files:
magicblock-committor-service/src/intent_executor/error.rsmagicblock-committor-service/src/intent_executor/single_stage_executor.rsmagicblock-committor-service/src/intent_executor/two_stage_executor.rs
📚 Learning: 2025-10-28T13:15:42.706Z
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 596
File: magicblock-processor/src/scheduler.rs:1-1
Timestamp: 2025-10-28T13:15:42.706Z
Learning: In magicblock-processor, transaction indexes were always set to 0 even before the changes in PR #596. The proper transaction indexing within slots will be addressed during the planned ledger rewrite.
Applied to files:
magicblock-committor-service/src/intent_executor/error.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:
magicblock-committor-service/src/intent_executor/error.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-committor-service/src/intent_executor/single_stage_executor.rsmagicblock-committor-service/src/intent_executor/two_stage_executor.rs
🧬 Code graph analysis (3)
magicblock-committor-service/src/intent_executor/error.rs (2)
magicblock-committor-service/src/error.rs (1)
signature(72-79)magicblock-committor-service/src/transaction_preparator/delivery_preparator.rs (4)
signature(505-510)signature(525-530)signature(566-572)signature(584-589)
magicblock-committor-service/src/intent_executor/single_stage_executor.rs (3)
magicblock-committor-service/src/intent_executor/error.rs (2)
signature(34-39)signature(193-202)magicblock-committor-service/src/intent_executor/two_stage_executor.rs (3)
handle_unfinalized_account_error(205-240)None(227-227)new(60-72)magicblock-committor-service/src/tasks/args_task.rs (1)
new(37-42)
magicblock-committor-service/src/intent_executor/two_stage_executor.rs (2)
magicblock-committor-service/src/intent_executor/single_stage_executor.rs (3)
handle_unfinalized_account_error(190-226)None(212-212)new(36-44)magicblock-committor-service/src/tasks/args_task.rs (1)
new(37-42)
🔇 Additional comments (7)
test-integration/Cargo.toml (1)
60-62: Dependency rev bump looks fine; verify the target commit is intentional and reachable.Since this is a git
revpin, please double-checka56deaccontains the expected delegation-program changes required by the new unfinalized-account recovery tests, and that it matches the root workspace’s pin to avoid integration drift.Cargo.toml (1)
98-100: Workspace dependency rev bump is consistent with test-integration.Please confirm the pinned
a56deacis the intended delegation-program commit for this PR (and that the commit is kept in sync anywhere else this dependency is declared).magicblock-committor-service/src/intent_executor/single_stage_executor.rs (2)
5-23: Imports for recovery flow are appropriate and consistent with two-stage executor.
148-168: UnfinalizedAccountError task lookup looks correct, but depends on TASK_OFFSET invariants.
err.task_index()must stay aligned with the transaction “prelude” size (compute budget ix count). If that prelude ever changes, this recovery will silently stop finding the right task (and justBreak(())).magicblock-committor-service/src/intent_executor/error.rs (1)
146-150: New error variant + TASK_OFFSET/task_index() are wired correctly and safely.Please re-verify the “2 compute budget instructions prepended” assumption remains true across all transaction construction paths; otherwise
task_index()will misattribute tasks.Also applies to: 159-191
magicblock-committor-service/src/intent_executor/two_stage_executor.rs (2)
17-24: Recovery-related imports are coherent and consistent with single-stage.
329-337: Treating UnfinalizedAccountError as unrecoverable during finalize stage is reasonable.
| /// Handles unfinalized account error | ||
| /// Sends a separate tx to finalize account and then continues execution | ||
| async fn handle_unfinalized_account_error( | ||
| inner: &IntentExecutorImpl<T, F>, | ||
| failed_signature: &Option<Signature>, | ||
| task: &dyn BaseTask, | ||
| ) -> IntentExecutorResult<ControlFlow<(), TransactionStrategy>> { | ||
| let mut visitor = TaskVisitorUtils::GetCommitMeta(None); | ||
| task.visit(&mut visitor); | ||
|
|
||
| let TaskVisitorUtils::GetCommitMeta(Some(commit_meta)) = visitor else { | ||
| // Can't recover - break execution | ||
| return Ok(ControlFlow::Break(())); | ||
| }; | ||
| let finalize_task = | ||
| Box::new(ArgsTask::new(ArgsTaskType::Finalize(FinalizeTask { | ||
| delegated_account: commit_meta.committed_pubkey, | ||
| }))) as Box<dyn BaseTask>; | ||
| inner | ||
| .prepare_and_execute_strategy( | ||
| &mut TransactionStrategy { | ||
| optimized_tasks: vec![finalize_task], | ||
| lookup_tables_keys: vec![], | ||
| }, | ||
| &None::<IntentPersisterImpl>, | ||
| ) | ||
| .await | ||
| .map_err(IntentExecutorError::FailedFinalizePreparationError)? | ||
| .map_err(|err| IntentExecutorError::FailedToFinalizeError { | ||
| err, | ||
| commit_signature: None, | ||
| finalize_signature: *failed_signature, | ||
| })?; | ||
|
|
||
| Ok(ControlFlow::Continue(TransactionStrategy { | ||
| optimized_tasks: vec![], | ||
| lookup_tables_keys: vec![], | ||
| })) | ||
| } |
There was a problem hiding this comment.
Potentially misleading signature surfaced on recovery-finalize failure.
On failure, you map the recovery error into IntentExecutorError::FailedToFinalizeError but set finalize_signature: *failed_signature (i.e., the signature from the original UnfinalizedAccountError) rather than the signature of the recovery finalize tx (which is likely inside err). If the intent is to keep the original signature for reporting, please document that explicitly; otherwise consider capturing the recovery signature (or both) so callers can debug the recovery failure accurately.
🤖 Prompt for AI Agents
magicblock-committor-service/src/intent_executor/single_stage_executor.rs lines
188-226: The recovery-finalize error mapping currently sets finalize_signature
to *failed_signature (the original UnfinalizedAccountError signature) which is
misleading; change the mapping to capture and pass the actual finalize
transaction signature returned by the recovery attempt (or include both
signatures: keep the original as commit_signature and set finalize_signature to
the recovery's signature). Update the Err mapping to extract the recovery
signature from err and populate IntentExecutorError::FailedToFinalizeError
accordingly (or document explicitly if you intentionally want to keep the
original signature).
There was a problem hiding this comment.
I agree with this comment. At the very least add a clarifying comment that you intentionally pick the original sig.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.locktest-integration/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (2)
Cargo.toml(1 hunks)test-integration/Cargo.toml(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 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.
Learnt from: Dodecahedr0x
Repo: magicblock-labs/magicblock-validator PR: 639
File: test-integration/test-committor-service/tests/test_ix_commit_local.rs:867-881
Timestamp: 2025-12-03T09:33:48.707Z
Learning: Repo: magicblock-labs/magicblock-validator PR: 639
Context: test-integration/test-committor-service/tests/test_ix_commit_local.rs (ix_commit_local)
Learning: The PhotonIndexer used for compressed account fetches (get_compressed_account) has built‑in retry logic (defaults to ~10 attempts), so tests should not add separate retry loops around compressed fetches unless there’s a specific need.
Learnt from: Dodecahedr0x
Repo: magicblock-labs/magicblock-validator PR: 639
File: magicblock-chainlink/src/remote_account_provider/mod.rs:1350-1353
Timestamp: 2025-12-03T09:36:01.527Z
Learning: Repo: magicblock-labs/magicblock-validator
File: magicblock-chainlink/src/remote_account_provider/mod.rs
Context: consolidate_fetched_remote_accounts
Learning: For unexpected result counts (>2), the project prefers logging an error and returning an empty Vec over panicking; acceptable during development per maintainer (Dodecahedr0x).
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.
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
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.
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 596
File: magicblock-processor/src/scheduler.rs:1-1
Timestamp: 2025-10-28T13:15:42.706Z
Learning: In magicblock-processor, transaction indexes were always set to 0 even before the changes in PR #596. The proper transaction indexing within slots will be addressed during the planned ledger rewrite.
📚 Learning: 2025-11-24T14:21:00.996Z
Learnt from: Dodecahedr0x
Repo: magicblock-labs/magicblock-validator PR: 639
File: Cargo.toml:58-58
Timestamp: 2025-11-24T14:21:00.996Z
Learning: In the magicblock-validator codebase, magicblock-api/Cargo.toml intentionally uses borsh = "1.5.3" (instead of the workspace version 0.10.4) because it needs to deserialize types from the magic-domain-program external dependency, which requires borsh 1.5.x compatibility. This is an intentional exception for interoperability with the magic domain program.
Applied to files:
Cargo.tomltest-integration/Cargo.toml
📚 Learning: 2025-10-26T16:54:39.084Z
Learnt from: thlorenz
Repo: magicblock-labs/magicblock-validator PR: 587
File: test-manual/Cargo.toml:0-0
Timestamp: 2025-10-26T16:54:39.084Z
Learning: In the magicblock-validator repository, use git branch references (not commit hashes or tags) for the helius-laserstream dependency to allow automatic updates when the branch is pushed to.
Applied to files:
Cargo.tomltest-integration/Cargo.toml
🔇 Additional comments (1)
Cargo.toml (1)
98-100: No changes needed. The dependency is already properly configured with short SHA pinning consistent with all other git dependencies in the project (magic-domain-program,solana-account,solana-svm,rocksdb), and is already aligned withtest-integration/Cargo.toml.Likely an incorrect or invalid review comment.
bmuddha
left a comment
bmuddha
left a comment
There was a problem hiding this comment.
LGTM, with minor clarifications required.
| .map_err(IntentExecutorError::FailedCommitPreparationError)? | ||
| .map_err(|err| IntentExecutorError::FailedToCommitError { | ||
| err, | ||
| signature: *failed_signature, |
There was a problem hiding this comment.
Is it a correct behavior to report the original failed signature here?
There was a problem hiding this comment.
The idea is that we report the signature of failed commit, instead of failed recovery
There was a problem hiding this comment.
Same issue raised here as in the other duplicated code .. so you'd have to explain this in two places (to me that's a smell).
Ideally we'd remove the duplication if possible like I mentioned.
| ) | ||
| .await | ||
| } else { | ||
| Ok(ControlFlow::Break(())) |
There was a problem hiding this comment.
Is it a common case that task_index() might return None, that we don't even log anything here?
There was a problem hiding this comment.
That should never happen really, that's why I don't even log anything
There was a problem hiding this comment.
Yeah but from the code logic it looks like it could theoretically get there.
Do you feel comfortable putting an unreachable! there which would panic if it did?
If not please log an error!("BUG: ... ") so we have a chance to catch it.
There was a problem hiding this comment.
I don't expect bug from my side, but rather an invalid index from RPC. Added log
# Conflicts: # test-integration/Cargo.lock
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
magicblock-committor-service/src/intent_executor/error.rs (1)
212-223: Consider enhancing the comment to explain the recovery rationale.The constants are well-defined and the comment "Errors when commit state already exists" provides basic context. However, as noted in a previous review, explicitly stating that these errors indicate an already-committed/unfinalized account and are intentionally classified as
UnfinalizedAccountErrorto enable recovery via finalization would make the recovery path clearer to future maintainers.This is optional and the current implementation is correct.
Example enhancement
- // Errors when commit state already exists + // Errors when commit state already exists (indicates successful commit but failed finalize). + // These are classified as UnfinalizedAccountError to enable recovery via finalization. const COMMIT_STATE_INVALID_ACCOUNT_OWNER: u32 =
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
magicblock-committor-service/src/intent_executor/error.rs
🧰 Additional context used
🧠 Learnings (6)
📓 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.
Learnt from: Dodecahedr0x
Repo: magicblock-labs/magicblock-validator PR: 639
File: test-integration/test-committor-service/tests/test_ix_commit_local.rs:867-881
Timestamp: 2025-12-03T09:33:48.707Z
Learning: Repo: magicblock-labs/magicblock-validator PR: 639
Context: test-integration/test-committor-service/tests/test_ix_commit_local.rs (ix_commit_local)
Learning: The PhotonIndexer used for compressed account fetches (get_compressed_account) has built‑in retry logic (defaults to ~10 attempts), so tests should not add separate retry loops around compressed fetches unless there’s a specific need.
Learnt from: Dodecahedr0x
Repo: magicblock-labs/magicblock-validator PR: 639
File: magicblock-chainlink/src/remote_account_provider/mod.rs:1350-1353
Timestamp: 2025-12-03T09:36:01.527Z
Learning: Repo: magicblock-labs/magicblock-validator
File: magicblock-chainlink/src/remote_account_provider/mod.rs
Context: consolidate_fetched_remote_accounts
Learning: For unexpected result counts (>2), the project prefers logging an error and returning an empty Vec over panicking; acceptable during development per maintainer (Dodecahedr0x).
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.
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
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.
📚 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/error.rs
📚 Learning: 2025-12-03T09:36:01.527Z
Learnt from: Dodecahedr0x
Repo: magicblock-labs/magicblock-validator PR: 639
File: magicblock-chainlink/src/remote_account_provider/mod.rs:1350-1353
Timestamp: 2025-12-03T09:36:01.527Z
Learning: Repo: magicblock-labs/magicblock-validator
File: magicblock-chainlink/src/remote_account_provider/mod.rs
Context: consolidate_fetched_remote_accounts
Learning: For unexpected result counts (>2), the project prefers logging an error and returning an empty Vec over panicking; acceptable during development per maintainer (Dodecahedr0x).
Applied to files:
magicblock-committor-service/src/intent_executor/error.rs
📚 Learning: 2025-12-03T09:33:48.707Z
Learnt from: Dodecahedr0x
Repo: magicblock-labs/magicblock-validator PR: 639
File: test-integration/test-committor-service/tests/test_ix_commit_local.rs:867-881
Timestamp: 2025-12-03T09:33:48.707Z
Learning: Repo: magicblock-labs/magicblock-validator PR: 639
Context: test-integration/test-committor-service/tests/test_ix_commit_local.rs (ix_commit_local)
Learning: The PhotonIndexer used for compressed account fetches (get_compressed_account) has built‑in retry logic (defaults to ~10 attempts), so tests should not add separate retry loops around compressed fetches unless there’s a specific need.
Applied to files:
magicblock-committor-service/src/intent_executor/error.rs
📚 Learning: 2025-10-28T13:15:42.706Z
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 596
File: magicblock-processor/src/scheduler.rs:1-1
Timestamp: 2025-10-28T13:15:42.706Z
Learning: In magicblock-processor, transaction indexes were always set to 0 even before the changes in PR #596. The proper transaction indexing within slots will be addressed during the planned ledger rewrite.
Applied to files:
magicblock-committor-service/src/intent_executor/error.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:
magicblock-committor-service/src/intent_executor/error.rs
🧬 Code graph analysis (1)
magicblock-committor-service/src/intent_executor/error.rs (2)
magicblock-committor-service/src/error.rs (1)
signature(72-79)magicblock-committor-service/src/transaction_preparator/delivery_preparator.rs (4)
signature(505-510)signature(525-530)signature(566-572)signature(584-589)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build Project
- GitHub Check: run_make_ci_test
- GitHub Check: run_make_ci_lint
🔇 Additional comments (7)
magicblock-committor-service/src/intent_executor/error.rs (7)
146-147: LGTM! Error variant is well-designed.The
UnfinalizedAccountErrorvariant follows the existing pattern and carries the necessary context (TransactionError and optional Signature) for the recovery path described in the PR objectives.
159-161: LGTM! Constant is well-documented.The
TASK_OFFSETconstant is clearly documented, explaining the invariant that transactions are prepended with 2 compute budget instructions. This was correctly addressed from previous review feedback.
167-191: LGTM! Correct use of checked arithmetic.The
task_index()method correctly extracts task indices from instruction errors and properly handles all relevant error variants. The use ofchecked_subprevents underflow, and returningNonefor non-instruction errors is appropriate.
193-202: LGTM! Signature propagation is correct.The
UnfinalizedAccountErroris correctly included in thesignature()method, maintaining consistency with other error variants.
239-240: LGTM! Good use of the defined constant.Using
Self::TASK_OFFSETinstead of a local offset improves consistency and maintainability.
252-280: LGTM! Error classification logic is sound.The nested match correctly classifies Commit task errors:
NONCE_OUT_OF_ORDER→CommitIDError(commit ordering issue)- Four
COMMIT_STATE/COMMIT_RECORDerrors →UnfinalizedAccountError(indicates successful commit but unfinalized state, enabling recovery via finalization)- Other errors → propagated
The pattern matching is clean and the logic aligns with the PR objectives for handling unfinalized accounts.
308-319: LGTM! Metrics label correctly added.The
UnfinalizedAccountErroris properly mapped to"unfinalized_account_failed"in the metrics label implementation, making it easier to monitor and validate the effectiveness of the recovery logic. This was correctly addressed from previous review feedback.
# Conflicts: # Cargo.lock # Cargo.toml # test-integration/Cargo.lock # test-integration/Cargo.toml # test-integration/schedulecommit/elfs/dlp.so
| /// Handles unfinalized account error | ||
| /// Sends a separate tx to finalize account and then continues execution | ||
| async fn handle_unfinalized_account_error( | ||
| inner: &IntentExecutorImpl<T, F>, | ||
| failed_signature: &Option<Signature>, | ||
| task: &dyn BaseTask, | ||
| ) -> IntentExecutorResult<ControlFlow<(), TransactionStrategy>> { | ||
| let mut visitor = TaskVisitorUtils::GetCommitMeta(None); | ||
| task.visit(&mut visitor); | ||
|
|
||
| let TaskVisitorUtils::GetCommitMeta(Some(commit_meta)) = visitor else { | ||
| // Can't recover - break execution | ||
| return Ok(ControlFlow::Break(())); | ||
| }; | ||
| let finalize_task = | ||
| Box::new(ArgsTask::new(ArgsTaskType::Finalize(FinalizeTask { | ||
| delegated_account: commit_meta.committed_pubkey, | ||
| }))) as Box<dyn BaseTask>; | ||
| inner | ||
| .prepare_and_execute_strategy( | ||
| &mut TransactionStrategy { | ||
| optimized_tasks: vec![finalize_task], | ||
| lookup_tables_keys: vec![], | ||
| }, | ||
| &None::<IntentPersisterImpl>, | ||
| ) | ||
| .await | ||
| .map_err(IntentExecutorError::FailedFinalizePreparationError)? | ||
| .map_err(|err| IntentExecutorError::FailedToFinalizeError { | ||
| err, | ||
| commit_signature: None, | ||
| finalize_signature: *failed_signature, | ||
| })?; | ||
|
|
||
| Ok(ControlFlow::Continue(TransactionStrategy { | ||
| optimized_tasks: vec![], | ||
| lookup_tables_keys: vec![], | ||
| })) | ||
| } |
There was a problem hiding this comment.
I agree with this comment. At the very least add a clarifying comment that you intentionally pick the original sig.
magicblock-committor-service/src/intent_executor/single_stage_executor.rs
Show resolved
Hide resolved
| ) | ||
| .await | ||
| } else { | ||
| Ok(ControlFlow::Break(())) |
There was a problem hiding this comment.
Yeah but from the code logic it looks like it could theoretically get there.
Do you feel comfortable putting an unreachable! there which would panic if it did?
If not please log an error!("BUG: ... ") so we have a chance to catch it.
| // Can't recover - break execution | ||
| return Ok(ControlFlow::Break(())); | ||
| }; | ||
| let finalize_task = |
There was a problem hiding this comment.
Definitely lots of duplication, just those verbose lines are exactly the same as from above:
let finalize_task =
Box::new(ArgsTask::new(ArgsTaskType::Finalize(FinalizeTask {
delegated_account: commit_meta.committed_pubkey,
}))) as Box<dyn BaseTask>;If it's not possible to factor out the whole method then maybe some smaller helper methods could minimize the verbose repeated code like this.
There was a problem hiding this comment.
Simplified to the point where any further abstractions/simplifications feel like it would decrease readability and simplicity rather than improve them.
see 86e4897
| .map_err(IntentExecutorError::FailedCommitPreparationError)? | ||
| .map_err(|err| IntentExecutorError::FailedToCommitError { | ||
| err, | ||
| signature: *failed_signature, |
There was a problem hiding this comment.
Same issue raised here as in the other duplicated code .. so you'd have to explain this in two places (to me that's a smell).
Ideally we'd remove the duplication if possible like I mentioned.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
magicblock-committor-service/src/intent_executor/two_stage_executor.rs (1)
214-217: Same suggestion applies: consider logging whencommit_metaisNone.Parallel to the single-stage executor, a brief log here would help diagnose the unexpected scenario where an
UnfinalizedAccountErroroccurs for a non-commit task.
🧹 Nitpick comments (1)
magicblock-committor-service/src/intent_executor/single_stage_executor.rs (1)
199-202: Consider adding a log whencommit_metaisNone.If
TaskVisitorUtils::commit_metareturnsNonefor a task that triggered anUnfinalizedAccountError, this would indicate a bug (the error should only occur for commit-type tasks). A brief log would help diagnose such cases without requiring deep investigation.🔎 Proposed enhancement
let Some(commit_meta) = TaskVisitorUtils::commit_meta(task) else { // Can't recover - break execution + error!("BUG: UnfinalizedAccountError on a task without commit metadata"); return Ok(ControlFlow::Break(())); };
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
magicblock-committor-service/src/intent_executor/single_stage_executor.rsmagicblock-committor-service/src/intent_executor/two_stage_executor.rsmagicblock-committor-service/src/tasks/task_visitors/utility_visitor.rs
🧰 Additional context used
🧠 Learnings (12)
📚 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.rsmagicblock-committor-service/src/intent_executor/single_stage_executor.rs
📚 Learning: 2025-12-03T09:33:48.707Z
Learnt from: Dodecahedr0x
Repo: magicblock-labs/magicblock-validator PR: 639
File: test-integration/test-committor-service/tests/test_ix_commit_local.rs:867-881
Timestamp: 2025-12-03T09:33:48.707Z
Learning: Repo: magicblock-labs/magicblock-validator PR: 639
Context: test-integration/test-committor-service/tests/test_ix_commit_local.rs (ix_commit_local)
Learning: The PhotonIndexer used for compressed account fetches (get_compressed_account) has built‑in retry logic (defaults to ~10 attempts), so tests should not add separate retry loops around compressed fetches unless there’s a specific need.
Applied to files:
magicblock-committor-service/src/intent_executor/two_stage_executor.rsmagicblock-committor-service/src/intent_executor/single_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.rsmagicblock-committor-service/src/intent_executor/single_stage_executor.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:
magicblock-committor-service/src/intent_executor/two_stage_executor.rsmagicblock-committor-service/src/intent_executor/single_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.rsmagicblock-committor-service/src/intent_executor/single_stage_executor.rs
📚 Learning: 2025-10-28T13:15:42.706Z
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 596
File: magicblock-processor/src/scheduler.rs:1-1
Timestamp: 2025-10-28T13:15:42.706Z
Learning: In magicblock-processor, transaction indexes were always set to 0 even before the changes in PR #596. The proper transaction indexing within slots will be addressed during the planned ledger rewrite.
Applied to files:
magicblock-committor-service/src/intent_executor/two_stage_executor.rsmagicblock-committor-service/src/intent_executor/single_stage_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:
magicblock-committor-service/src/intent_executor/two_stage_executor.rs
📚 Learning: 2025-10-26T16:54:39.084Z
Learnt from: thlorenz
Repo: magicblock-labs/magicblock-validator PR: 587
File: test-manual/Cargo.toml:0-0
Timestamp: 2025-10-26T16:54:39.084Z
Learning: In the magicblock-validator repository, use git branch references (not commit hashes or tags) for the helius-laserstream dependency to allow automatic updates when the branch is pushed to.
Applied to files:
magicblock-committor-service/src/intent_executor/two_stage_executor.rsmagicblock-committor-service/src/intent_executor/single_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-committor-service/src/intent_executor/two_stage_executor.rsmagicblock-committor-service/src/intent_executor/single_stage_executor.rs
📚 Learning: 2025-10-27T09:55:17.808Z
Learnt from: thlorenz
Repo: magicblock-labs/magicblock-validator PR: 587
File: magicblock-chainlink/src/remote_account_provider/mod.rs:296-298
Timestamp: 2025-10-27T09:55:17.808Z
Learning: In the magicblock-validator codebase, it is acceptable to log API keys in debug/info statements because logs are private and the team considers API keys not that sensitive in their security model.
Applied to files:
magicblock-committor-service/src/intent_executor/single_stage_executor.rs
📚 Learning: 2025-11-19T12:55:48.931Z
Learnt from: Dodecahedr0x
Repo: magicblock-labs/magicblock-validator PR: 639
File: test-integration/test-tools/src/validator.rs:193-197
Timestamp: 2025-11-19T12:55:48.931Z
Learning: In the magicblock-validator codebase, when constructing arguments for the light CLI test validator in start_light_validator_with_config (test-integration/test-tools/src/validator.rs), the shlex::split approach is necessary because the light CLI does not handle whitespaces properly in validator arguments. The string concatenation + shlex parsing pattern should not be refactored to direct argument construction in this specific case.
Applied to files:
magicblock-committor-service/src/intent_executor/single_stage_executor.rs
📚 Learning: 2025-12-03T09:36:01.527Z
Learnt from: Dodecahedr0x
Repo: magicblock-labs/magicblock-validator PR: 639
File: magicblock-chainlink/src/remote_account_provider/mod.rs:1350-1353
Timestamp: 2025-12-03T09:36:01.527Z
Learning: Repo: magicblock-labs/magicblock-validator
File: magicblock-chainlink/src/remote_account_provider/mod.rs
Context: consolidate_fetched_remote_accounts
Learning: For unexpected result counts (>2), the project prefers logging an error and returning an empty Vec over panicking; acceptable during development per maintainer (Dodecahedr0x).
Applied to files:
magicblock-committor-service/src/intent_executor/single_stage_executor.rs
🧬 Code graph analysis (2)
magicblock-committor-service/src/intent_executor/two_stage_executor.rs (4)
magicblock-committor-service/src/tasks/task_builder.rs (2)
signature(274-279)finalize_task(175-180)magicblock-committor-service/src/intent_executor/error.rs (2)
signature(34-39)signature(193-202)magicblock-committor-service/src/tasks/task_visitors/utility_visitor.rs (1)
commit_meta(28-35)magicblock-committor-service/src/tasks/args_task.rs (1)
new(42-47)
magicblock-committor-service/src/intent_executor/single_stage_executor.rs (3)
magicblock-committor-service/src/intent_executor/two_stage_executor.rs (3)
handle_unfinalized_account_error(209-239)new(60-72)None(226-226)magicblock-committor-service/src/tasks/task_visitors/utility_visitor.rs (1)
commit_meta(28-35)magicblock-committor-service/src/tasks/args_task.rs (1)
new(42-47)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: run_make_ci_test
- GitHub Check: run_make_ci_lint
- GitHub Check: Build Project
🔇 Additional comments (7)
magicblock-committor-service/src/intent_executor/single_stage_executor.rs (2)
148-172: LGTM — Error routing and logging are correct.The
UnfinalizedAccountErrorhandling properly:
- Extracts the task using the error's task index
- Logs when the index is unexpected/out-of-bounds
- Delegates to the recovery handler or breaks execution appropriately
203-225: Recovery finalize implementation is sound.The approach of creating a standalone
FinalizeTaskfrom commit metadata and executing it without persistence correctly handles the recovery scenario. The empty strategy returned as cleanup metadata is appropriate since the recovery finalize modifies external state, enabling the original commit to succeed on retry.magicblock-committor-service/src/tasks/task_visitors/utility_visitor.rs (2)
15-21: Clean conversion implementation.The
Fromimpl correctly extracts only thecommitted_pubkeyfor theFinalizeTask, ascommit_idis not required for finalization. This provides a clean API for the recovery flow.
27-36: Useful helper encapsulating visitor pattern.The
commit_metahelper provides a clean interface for extracting commit metadata from any task, hiding the visitor machinery. This is used effectively by both executors for recovery.magicblock-committor-service/src/intent_executor/two_stage_executor.rs (3)
158-182: LGTM — Error routing with proper logging.The handling correctly identifies the affected task via
task_index(), delegates to recovery, and logs unexpected index scenarios. This addresses the maintainer's feedback from the past review about logging for edge cases.
228-233: Minor inconsistency: error type differs from single-stage executor.Line 229 uses
FailedCommitPreparationErrorfor the finalize task preparation, while the equivalent insingle_stage_executor.rs(line 214) usesFailedFinalizePreparationError.This may be intentional—reflecting that we're in the commit stage context here versus the combined single-stage flow. If so, no change needed. If the intent is to indicate what operation failed (finalize preparation), consider aligning with
FailedFinalizePreparationErrorfor consistency.
328-336: Correct grouping ofUnfinalizedAccountErroras unexpected in finalize stage.During the finalize phase, encountering an
UnfinalizedAccountErrorwould be a logic error (finalization happens after successful commit). Grouping it withCommitIDErrorand breaking execution is the right approach.
Resolves #683
Summary by CodeRabbit
Release Notes
New Features
Improvements
Tests
✏️ Tip: You can customize this high-level summary in your review settings.