Recover from unfinalized accout #731
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Manual Deploy AvailableYou can trigger a manual deploy of this PR branch to testnet: Alternative: Comment
Comment updated automatically when the PR is synchronized. |
Contributor
📝 WalkthroughWalkthroughUpdated the Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
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
Contributor
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
Contributor
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
Contributor
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
Comment on lines
+146
to
+147
| #[error("Unfinalized account error: {0}, {:?}", .1)] | ||
| UnfinalizedAccountError(#[source] TransactionError, Option<Signature>), |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
New UnfinalizedAccountError variant is well-integrated; consider a dedicated metrics label.
The new UnfinalizedAccountError(TransactionError, Option<Signature>) variant and its inclusion in signature() look consistent with the existing error variants. However, in impl metrics::LabelValue for TransactionStrategyExecutionError (Lines 307-316), this variant currently falls into the generic "failed" bucket. Given this error is central to the new recovery path, adding an explicit arm such as:
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",
_ => "failed",
}
}
}would make it much easier to monitor unfinalized-account incidents and validate the effectiveness of the new recovery logic.
Also applies to: 199-200, 307-316
Comment on lines
+212
to
+223
| // Commit Nonce order error | ||
| const NONCE_OUT_OF_ORDER: u32 = | ||
| dlp::error::DlpError::NonceOutOfOrder as u32; | ||
| // Errors when commit state already exists | ||
| const COMMIT_STATE_INVALID_ACCOUNT_OWNER: u32 = | ||
| dlp::error::DlpError::CommitStateInvalidAccountOwner as u32; | ||
| const COMMIT_STATE_ALREADY_INITIALIZED: u32 = | ||
| dlp::error::DlpError::CommitStateAlreadyInitialized as u32; | ||
| const COMMIT_RECORD_INVALID_ACCOUNT_OWNER: u32 = | ||
| dlp::error::DlpError::CommitRecordInvalidAccountOwner as u32; | ||
| const COMMIT_RECORD_ALREADY_INITIALIZED: u32 = | ||
| dlp::error::DlpError::CommitRecordAlreadyInitialized as u32; |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Clarify why these specific DlpError codes are treated as UnfinalizedAccountError.
The commit-branch mapping that classifies
COMMIT_STATE_INVALID_ACCOUNT_OWNERCOMMIT_STATE_ALREADY_INITIALIZEDCOMMIT_RECORD_INVALID_ACCOUNT_OWNERCOMMIT_RECORD_ALREADY_INITIALIZED
as UnfinalizedAccountError for TaskType::Commit is aligned with the PR goal (treating “commit state/record already exists” as recoverable via finalize). The structure of the nested match and reuse of tx_err_helper and signature looks sound.
To aid future readers, consider adding a brief comment above these constants or the corresponding match arm explaining that these DlpError codes are specifically emitted when a commit is attempted against an already-committed/unfinalized account, and thus are intentionally classified as UnfinalizedAccountError. That will make it clearer that this is a deliberate recovery path rather than a generic reclassification of commit failures.
Also applies to: 250-279
🤖 Prompt for AI Agents
magicblock-committor-service/src/intent_executor/error.rs lines 212-223 (and
similarly 250-279): add a short explanatory comment above the listed DlpError
constant definitions (or above the match arm that maps them to
UnfinalizedAccountError) stating that these specific DlpError codes are emitted
when a commit is attempted against an already-committed or unfinalized account
and therefore are intentionally classified as UnfinalizedAccountError to allow
recovery via finalization; keep the comment concise (1-2 lines) and reference
TaskType::Commit and the rationale that this is a deliberate recovery path.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Resolves #683
Summary by CodeRabbit
New Features
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.