feat(committor): enforce per-account commit limit via MagicSys syscall#1024
Merged
feat(committor): enforce per-account commit limit via MagicSys syscall#1024
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR enforces a commit-limit by introducing nonce-based checks and related plumbing: a global MagicSys trait and adapter, moved CommittedAccount into magicblock-core, added APIs to fetch current commit nonces via the committor service and task-info-fetcher, wired check_commit_limits into scheduling paths, replaced persister-based account-mod persistence with magic_sys calls, updated many tests and test utilities, and added integration tests for commit-limit behavior. Assessment against linked issues
Out-of-scope changes
Suggested reviewers
✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test-integration/test-tools/src/toml_to_args.rs (1)
100-111: 🧹 Nitpick | 🔵 TrivialInclude the
read_direrror detail for faster CI triage.The fallback message on Line 109-Line 110 is helpful, but adding the underlying error makes debugging more direct.
Proposed tweak
- if let Ok(entries) = fs::read_dir(parent) { - for entry in entries { - if let Ok(entry) = entry { - eprintln!(" - {:?}", entry.file_name()); - } - } - } else { - eprintln!(" (Unable to read directory)"); - } + match fs::read_dir(parent) { + Ok(entries) => { + for entry in entries { + if let Ok(entry) = entry { + eprintln!(" - {:?}", entry.file_name()); + } + } + } + Err(err) => { + eprintln!(" (Unable to read directory: {err})"); + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test-integration/test-tools/src/toml_to_args.rs` around lines 100 - 111, The directory-read fallback currently prints "Unable to read directory" without the underlying error; update the fs::read_dir(parent) handling around full_path_to_resolve.parent() so the Err branch captures the error (e.g., match or if let Err(e)) and include e in the eprintln to show the underlying io::Error; keep the existing context message ("Directory contents of ...") and print the error alongside it to aid CI debugging.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@magicblock-api/src/magic_sys_adapter.rs`:
- Around line 17-19: Add brief doc comments above the constants RECV_ERR,
FETCH_ERR, and NO_COMMITTOR_ERR in magic_sys_adapter.rs explaining their
semantics and why their high-bit pattern (0xE0xx_xxxx) is used to avoid
collisions with other error codes; for each constant state when it is returned
(e.g., receive failure, fetch failure, no committer available) and recommend
including these codes in logs/error reports to aid debugging. Ensure the
comments reference the constant names (RECV_ERR, FETCH_ERR, NO_COMMITTOR_ERR)
and the numeric values so anyone reading the code or logs can map the code to
the meaning quickly.
In `@magicblock-committor-service/src/intent_executor/task_info_fetcher.rs`:
- Line 109: Replace the panicking .expect() on mutex locks (the
self.inner.lock().expect(MUTEX_POISONED_MSG) sites) with explicit non-panicking
handling: map the PoisonError to a Result-returning error type (or a custom
error variant) and propagate it from the enclosing function, or catch it, log
the poisoning with context and attempt a safe recovery strategy; update the
methods that call self.inner.lock() (the three occurrences flagged) to return
Result or handle the error instead of panicking so mutex poisoning is not
allowed to crash the committor service.
In `@programs/magicblock/src/magic_sys.rs`:
- Around line 27-32: The code currently uses expect(...) on the MAGIC_SYS RwLock
(e.g., in init_magic_sys and the other accessors that reference MAGIC_SYS and
MAGIC_SYS_POISONED_MSG), which will panic on lock poisoning; change these APIs
to return Result instead of panicking: replace
.write().expect(MAGIC_SYS_POISONED_MSG) and .read().expect(...) with
.write().map_err(|_| /* create a descriptive error type/value */)? and
.read().map_err(|_| /* error */)?, update function signatures (e.g.,
init_magic_sys, any get_/with_/take_ accessors) to return Result<(), YourError>
or Result<T, YourError> as appropriate, and propagate errors instead of calling
process-wide panics; ensure the error type includes context (lock poisoned for
MAGIC_SYS) and update call sites to handle the Result.
In `@programs/magicblock/src/schedule_transactions/process_schedule_commit.rs`:
- Around line 236-246: Fix the typo in the comment above the commit-limit check:
change "progibiting" to "prohibiting" in the comment block that precedes the
conditional which uses opts.request_undelegation and calls
check_commit_limits(&committed_accounts, invoke_context) so the comment reads
correctly while leaving the logic unchanged.
---
Outside diff comments:
In `@test-integration/test-tools/src/toml_to_args.rs`:
- Around line 100-111: The directory-read fallback currently prints "Unable to
read directory" without the underlying error; update the fs::read_dir(parent)
handling around full_path_to_resolve.parent() so the Err branch captures the
error (e.g., match or if let Err(e)) and include e in the eprintln to show the
underlying io::Error; keep the existing context message ("Directory contents of
...") and print the error alongside it to aid CI debugging.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: b9e8036e-05a2-40f1-b2d9-084f93811d05
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.locktest-integration/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (49)
magicblock-accounts/Cargo.tomlmagicblock-accounts/src/scheduled_commits_processor.rsmagicblock-api/Cargo.tomlmagicblock-api/src/lib.rsmagicblock-api/src/magic_sys_adapter.rsmagicblock-api/src/magic_validator.rsmagicblock-committor-service/Cargo.tomlmagicblock-committor-service/src/committor_processor.rsmagicblock-committor-service/src/error.rsmagicblock-committor-service/src/intent_execution_manager.rsmagicblock-committor-service/src/intent_execution_manager/intent_scheduler.rsmagicblock-committor-service/src/intent_executor/intent_executor_factory.rsmagicblock-committor-service/src/intent_executor/mod.rsmagicblock-committor-service/src/intent_executor/task_info_fetcher.rsmagicblock-committor-service/src/persist/commit_persister.rsmagicblock-committor-service/src/service.rsmagicblock-committor-service/src/service_ext.rsmagicblock-committor-service/src/stubs/changeset_committor_stub.rsmagicblock-committor-service/src/tasks/mod.rsmagicblock-committor-service/src/tasks/task_builder.rsmagicblock-committor-service/src/tasks/task_strategist.rsmagicblock-core/Cargo.tomlmagicblock-core/src/intent.rsmagicblock-core/src/lib.rsmagicblock-core/src/traits.rsmagicblock-ledger/src/store/data_mod_persister.rsmagicblock-ledger/src/store/mod.rsprograms/magicblock/src/lib.rsprograms/magicblock/src/magic_scheduled_base_intent.rsprograms/magicblock/src/magic_sys.rsprograms/magicblock/src/mutate_accounts/account_mod_data.rsprograms/magicblock/src/mutate_accounts/mod.rsprograms/magicblock/src/mutate_accounts/process_mutate_accounts.rsprograms/magicblock/src/schedule_transactions/mod.rsprograms/magicblock/src/schedule_transactions/process_schedule_commit.rsprograms/magicblock/src/schedule_transactions/process_schedule_commit_tests.rsprograms/magicblock/src/schedule_transactions/process_schedule_intent_bundle.rsprograms/magicblock/src/schedule_transactions/process_scheduled_commit_sent.rsprograms/magicblock/src/test_utils/mod.rstest-integration/schedulecommit/test-scenarios/Cargo.tomltest-integration/schedulecommit/test-scenarios/tests/03_commit_limit.rstest-integration/schedulecommit/test-scenarios/tests/utils/mod.rstest-integration/test-committor-service/Cargo.tomltest-integration/test-committor-service/tests/common.rstest-integration/test-committor-service/tests/test_intent_executor.rstest-integration/test-committor-service/tests/test_ix_commit_local.rstest-integration/test-runner/bin/run_tests.rstest-integration/test-tools/src/toml_to_args.rstest-integration/test-tools/src/validator.rs
💤 Files with no reviewable changes (3)
- magicblock-ledger/src/store/mod.rs
- programs/magicblock/src/mutate_accounts/mod.rs
- magicblock-ledger/src/store/data_mod_persister.rs
magicblock-committor-service/src/intent_executor/task_info_fetcher.rs
Outdated
Show resolved
Hide resolved
programs/magicblock/src/schedule_transactions/process_schedule_commit.rs
Show resolved
Hide resolved
# Conflicts: # test-integration/Cargo.lock # test-integration/test-runner/bin/run_tests.rs # test-integration/test-tools/src/toml_to_args.rs # test-integration/test-tools/src/validator.rs
Manual Deploy AvailableYou can trigger a manual deploy of this PR branch to testnet: Alternative: Comment
Comment updated automatically when the PR is synchronized. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
magicblock-committor-service/src/intent_executor/task_info_fetcher.rs (1)
345-345:⚠️ Potential issue | 🟠 MajorPanicking
.expect()on mutex locks can crash the committor.These production lock sites panic on poisoning. Please switch to explicit poison handling (recover/log or propagate) instead of aborting the flow.
Suggested non-panicking pattern
- let mut inner = self.inner.lock().expect(MUTEX_POISONED_MSG); + let mut inner = match self.inner.lock() { + Ok(guard) => guard, + Err(poisoned) => { + error!("CacheTaskInfoFetcher mutex poisoned while dropping CacheInnerGuard"); + poisoned.into_inner() + } + };Apply the same pattern at Line 389, Line 409, and Line 435.
As per coding guidelines
Treat any usage of .unwrap() or .expect() in production Rust code as a MAJOR issue. These should not be categorized as trivial or nit-level concerns. Request proper error handling or explicit justification with invariants.Also applies to: 389-389, 409-409, 435-435
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@magicblock-committor-service/src/intent_executor/task_info_fetcher.rs` at line 345, The code currently uses self.inner.lock().expect(MUTEX_POISONED_MSG) which will abort on a poisoned mutex; replace each expect/unwrap on Mutex::lock (the occurrences calling self.inner.lock()) with explicit poison handling: match on self.inner.lock() -> Ok(inner) => use inner, Err(poisoned) => either recover via let inner = poisoned.into_inner() and log the poison with tracing::error!/log::error! (or return a propagated error from the surrounding function), or return a clear Result::Err so callers can handle it; apply the same replacement for every place where self.inner.lock().expect(...) is used (the four occurrences) so you do not panic on mutex poisoning and either recover or propagate the error consistently.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@magicblock-api/src/magic_sys_adapter.rs`:
- Around line 54-63: In fetch_current_commit_nonces, short-circuit when
commits.is_empty() by returning Ok(HashMap::new()) before trying to access
self.committor_service; currently the code fetches committor_service (and errors
if missing) even for empty input — change the logic in
fetch_current_commit_nonces to check commits.is_empty() at the top and return an
empty map immediately, otherwise proceed to obtain committor_service (the
committor_service binding) and the existing flow.
In `@magicblock-committor-service/src/intent_executor/task_info_fetcher.rs`:
- Line 28: The constant NUM_FETCH_RETRIES currently calls
NonZeroUsize::new(5).unwrap(), which is disallowed; replace the unwrap with an
explicit invariant handling: call NonZeroUsize::new(5) and match on the result,
returning the Some(value) arm to initialize NUM_FETCH_RETRIES and the None arm
should call unreachable!("NUM_FETCH_RETRIES must be non-zero") (or an equivalent
explicit panic message). Update the binding for NUM_FETCH_RETRIES to use this
match on NonZeroUsize::new so there is no .unwrap()/.expect() in production
code.
In `@programs/magicblock/src/schedule_transactions/process_schedule_commit.rs`:
- Around line 241-243: Tighten the fallback comment in
process_schedule_commit.rs: replace the current text with clearer grammar, e.g.
"If the account is broken the user should contact us. Fallback actions: 1) the
user tops up the account; 2) we trigger a manual undelegation." Update the
comment near the existing fallback block in process_schedule_commit.rs (where
the two bullet points are present) so it reads as a single clear sentence plus
the two numbered actions.
---
Duplicate comments:
In `@magicblock-committor-service/src/intent_executor/task_info_fetcher.rs`:
- Line 345: The code currently uses self.inner.lock().expect(MUTEX_POISONED_MSG)
which will abort on a poisoned mutex; replace each expect/unwrap on Mutex::lock
(the occurrences calling self.inner.lock()) with explicit poison handling: match
on self.inner.lock() -> Ok(inner) => use inner, Err(poisoned) => either recover
via let inner = poisoned.into_inner() and log the poison with
tracing::error!/log::error! (or return a propagated error from the surrounding
function), or return a clear Result::Err so callers can handle it; apply the
same replacement for every place where self.inner.lock().expect(...) is used
(the four occurrences) so you do not panic on mutex poisoning and either recover
or propagate the error consistently.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2151a5e4-db25-43b8-8a53-da5497d22deb
📒 Files selected for processing (3)
magicblock-api/src/magic_sys_adapter.rsmagicblock-committor-service/src/intent_executor/task_info_fetcher.rsprograms/magicblock/src/schedule_transactions/process_schedule_commit.rs
programs/magicblock/src/schedule_transactions/process_schedule_commit.rs
Outdated
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
magicblock-api/src/magic_sys_adapter.rs (1)
76-90:⚠️ Potential issue | 🔴 CriticalBound the wait on nonce fetch to avoid worker-thread stalls.
At Line 82,
futures::executor::block_on(receiver)is still unbounded. If the service loop wedges, this can block a tx execution worker indefinitely. Please enforce a timeout and map it to a distinctInstructionError::Custom(...).💡 Proposed change
-use std::{collections::HashMap, error::Error, sync::Arc}; +use std::{collections::HashMap, error::Error, sync::Arc, time::Duration}; impl MagicSysAdapter { @@ /// Returned when no committor service is configured. const NO_COMMITTOR_ERR: u32 = 0xE002_0000; + /// Returned when waiting for nonce response times out. + const TIMEOUT_ERR: u32 = 0xE003_0000; @@ - futures::executor::block_on(receiver) + let received = futures::executor::block_on(async { + tokio::time::timeout(Duration::from_secs(2), receiver).await + }) + .inspect_err(|err| { + error!(error = ?err, "Timed out waiting for nonces from CommittorService") + }) + .map_err(|_| InstructionError::Custom(Self::TIMEOUT_ERR))?; + + received .inspect_err(|err| { error!(error = ?err, "Failed to receive nonces from CommittorService") }) .map_err(|_| InstructionError::Custom(Self::RECV_ERR))? .inspect_err(|err| { error!(error = ?err, "Failed to fetch current commit nonces") }) .map_err(|_| InstructionError::Custom(Self::FETCH_ERR))#!/bin/bash set -euo pipefail echo "1) Verify unbounded block_on in adapter and whether timeout guard exists:" rg -n -C3 --type=rust '\bblock_on\s*\(\s*receiver\s*\)|tokio::time::timeout|TIMEOUT_ERR' magicblock-api/src/magic_sys_adapter.rs echo echo "2) Verify committor API shape (oneshot receiver returned to adapter):" rg -n -C3 --type=rust 'fn fetch_current_commit_nonces|oneshot::Receiver' magicblock-committor-service/src/service.rs echo echo "3) Verify execution path reaches MagicSys fetch in tx flow:" rg -n -C3 --type=rust 'MAGIC_SYS|fetch_current_commit_nonces\(' programs/magicblock/src/magic_sys.rs🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@magicblock-api/src/magic_sys_adapter.rs` around lines 76 - 90, The current use of futures::executor::block_on(receiver) in the fetch_current_commit_nonces path can block forever; wrap the receiver wait in a bounded timeout and map timeout failures to a new distinct InstructionError::Custom variant (add e.g. TIMEOUT_ERR) instead of RECV_ERR/FETCH_ERR, so the tx worker won't stall; specifically, update the call site that invokes fetch_current_commit_nonces/receiver and replace the unbounded block_on with a timed wait (use a timeout helper compatible with futures::executor::block_on or convert to a futures::select/timeout future), log the timeout with context (include error and which pubkeys/min_context_slot), and ensure you return InstructionError::Custom(Self::TIMEOUT_ERR) on timeout while preserving existing error mappings for receive/fetch failures.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@magicblock-api/src/magic_sys_adapter.rs`:
- Around line 76-90: The current use of futures::executor::block_on(receiver) in
the fetch_current_commit_nonces path can block forever; wrap the receiver wait
in a bounded timeout and map timeout failures to a new distinct
InstructionError::Custom variant (add e.g. TIMEOUT_ERR) instead of
RECV_ERR/FETCH_ERR, so the tx worker won't stall; specifically, update the call
site that invokes fetch_current_commit_nonces/receiver and replace the unbounded
block_on with a timed wait (use a timeout helper compatible with
futures::executor::block_on or convert to a futures::select/timeout future), log
the timeout with context (include error and which pubkeys/min_context_slot), and
ensure you return InstructionError::Custom(Self::TIMEOUT_ERR) on timeout while
preserving existing error mappings for receive/fetch failures.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 96cf9611-5991-4c74-8a7f-ab8acc88cce5
📒 Files selected for processing (2)
magicblock-api/src/magic_sys_adapter.rsprograms/magicblock/src/schedule_transactions/process_schedule_commit.rs
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 977f430840
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
thlorenz
approved these changes
Mar 6, 2026
Collaborator
thlorenz
left a comment
thlorenz
left a comment
There was a problem hiding this comment.
LGTM overall. Some nits and questions that should be addressed before merging.
magicblock-committor-service/src/intent_executor/task_info_fetcher.rs
Outdated
Show resolved
Hide resolved
programs/magicblock/src/schedule_transactions/process_schedule_commit_tests.rs
Outdated
Show resolved
Hide resolved
Collaborator
GabrielePicco
left a comment
GabrielePicco
left a comment
There was a problem hiding this comment.
Overall LGTM,
but the current implementation may have few problems that needs to be double-checked before merging (or addressed with follow-up PR to get this in quickly):
- commit limits can be bypassed by burst-scheduling before execution advances nonce (would be better to increment the nonce during scheduling)
- commit_and_undelegate can degrade to commit-only after undelegation failure, in which case the limit as far as I can see are not checked
Contributor
Author
That is valid concern, will address in followup PR.
This isn't a problem as account marked as undelegated and further commits aren't possible since commits require account to be delegated |
thlorenz
added a commit
that referenced
this pull request
Mar 10, 2026
* master: Fix: explorer rendering for failed transactions (#1032) feat: forward original bincoded transaction (#991) Improve commit limit exceeded error message (#1029) feat: allow for accountsdb checksum computation (#990) feat: modify account cloning to use transactions (#974) feat: backfill-aware subscriptions and throttled alerts (#1025) feat(committor): enforce per-account commit limit via MagicSys syscall (#1024) refactor: replace BaseTask trait objects with BaseTaskImpl enum (#973) fix: serialize nan for explorer compatibility (#1021)
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
Summary
MagicSysto enable syscalls outside of SVM.fetch_current_commit_noncestoMagicSysandMagicSysAdapter;fetch_current_commit_noncesin magic-program to limit commits without undelegationCacheTaskInfoFetcherto prevent race conditions and duplicate concurrent requests: replace flat nonce cache with per-accountNonceLock(Arc<TMutex<u64>>), addretiringmap for in-flight evicted locks, addedfetch_current_commit_nonces(no-increment)Also since intents are used both in magic-program and validator it is proposed to move it into
magicblock-coreas it was done withCommittedAccount, as it was needed for MagicSysCompatibility
Testing
process_schedule_commit_tests.rs— commit limit exceeded returnsCOMMIT_LIMIT_ERR; commit+undelegate at limit succeeds03_commit_limit.rs— end-to-end validation against a running validatorChecklist
Summary by CodeRabbit
New Features
Tests
Chores