Conversation
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
1902b12 to
c690458
Compare
266c486 to
ccbe21d
Compare
076b715 to
b45cdf8
Compare
064ef18 to
8cc286d
Compare
496473d to
d2d94dd
Compare
067ddca to
ef9cedd
Compare
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Warning
|
| Layer / File(s) | Summary |
|---|---|
Runtime contracts and SVM execution Cargo.toml, processor/Cargo.toml, processor/src/lib.rs, processor/src/error.rs, processor/src/metrics.rs, processor/src/callback.rs, processor/src/svm.rs |
Adds the workspace crate, processor message and error types, Prometheus metrics, keeper-backed account callbacks, and block-aware SVM transaction execution. |
Executor worker lifecycle processor/src/executor.rs |
Adds worker-thread executors that process transaction batches and block transitions, report completion, and persist execution results with replay-mode handling. |
Scheduling, locks, and block coordination processor/src/sequencer/* |
Adds executor-pool management, account-lock acquisition and release, blocked transaction retry, barriers, draining, shutdown, transaction hash chaining, and block finalization. Unit tests cover lock and dispatch behavior. |
Isolated transaction simulation processor/src/simulator.rs, processor/README.md |
Adds a dedicated simulator worker for non-persistent transaction execution and documents scheduling, barriers, finalization, and simulation behavior. |
End-to-end processor validation processor/src/tests.rs |
Adds tests for execution versus simulation, block hashes, CPI loading, sysvar transitions, replay mode, failures, and high-contention workloads. |
Estimated code review effort: 5 (Critical) | ~90 minutes
Sequence Diagram(s)
sequenceDiagram
participant Client
participant Sequencer
participant Executor
participant SvmContext
participant Keeper
Client->>Sequencer: submit resolved transactions
Sequencer->>Executor: dispatch conflict-free batch
Executor->>SvmContext: execute transaction batch
SvmContext->>Keeper: load and persist account state
Executor->>Sequencer: report completed batch
Sequencer->>Client: acknowledge barrier or finalized block
Possibly related PRs
- magicblock-labs/magicblock-engine#19: Provides customized transaction-context and SVM execution infrastructure used by this processor.
- magicblock-labs/magicblock-engine#31: Provides upstream Solana SVM and runtime crates used by transaction execution and account loading.
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
| Check name | Status | Explanation |
|---|---|---|
| Title check | ✅ Passed | The title clearly identifies the main change: implementation of the transaction processor crate. |
| Description check | ✅ Passed | The description explains the processor crate, conflict-aware scheduling, parallel execution, simulation, and its intended integration. |
| Linked Issues check | ✅ Passed | The changes implement scheduling, sequencing, parallel executor dispatch, conflict handling, ordering, and keeper-backed state retrieval and persistence required by issue [#11]. |
| Out of Scope Changes check | ✅ Passed | The changes remain within the linked issue scope and include supporting metrics, documentation, and tests for the transaction processor. |
| Docstring Coverage | ✅ Passed | Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. |
✨ Finishing Touches
📝 Generate docstrings
- Create stacked PR
- Commit on current branch
🧪 Generate unit tests (beta)
- Create PR with unit tests
- Commit unit tests in branch
processor
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 help to get the list of available commands.
|
@CodeRabbit review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
processor/src/tests.rs (1)
51-66: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocument the channel-capacity coupling of the prefill tests.
unspawnedlets tests fill the execution channel before the sequencer runs. The channel capacity is 1024 (processor/src/sequencer/mod.rsLine 90). The prefill tests send 128 and 512 transactions. If the capacity is lowered below a test's transaction count,Harness::executeblocks forever and the test hangs instead of failing. Add a note here so the constraint stays visible.📝 Proposed doc note
/// This lets tests fill the execution channel before the sequencer can /// consume from it, forcing contention resolution to happen from a backlog. + /// + /// Callers must keep their prefill count below the sequencer's execution + /// channel capacity. A larger prefill blocks `execute` and hangs the test. async fn unspawned(replay: bool) -> (Self, Sequencer) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@processor/src/tests.rs` around lines 51 - 66, Add a documentation note to Harness::unspawned stating that prefill tests must remain at or below the execution channel capacity of 1024, because exceeding it causes Harness::execute to block while the sequencer is unspawned. Mention that the existing 128- and 512-transaction tests depend on this constraint.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@processor/src/sequencer/locks.rs`:
- Around line 77-78: Update the rustdoc for the `release` method to state that
it releases every account lock read from `executor.locks`, replacing the stale
`held` reference while preserving the documented behavior.
- Around line 60-73: Remove each zero-count account from executor.locks during
the rollback loop in LockTable’s release path, while preserving lock.contend and
unlock behavior for remaining counts; update processor/src/sequencer/tests.rs
lines 138-138 to assert blocked.locks.get(&a) is None. Also make
AccountLock::unlock clear WRITE_BIT only when that executor actually owns it.
In `@processor/src/simulator.rs`:
- Around line 51-65: Update the worker loop in run so self.rx.recv() explicitly
exits when the channel returns None, preserving message handling for Some(msg)
and the documented behavior that channel closure ends the worker.
---
Nitpick comments:
In `@processor/src/tests.rs`:
- Around line 51-66: Add a documentation note to Harness::unspawned stating that
prefill tests must remain at or below the execution channel capacity of 1024,
because exceeding it causes Harness::execute to block while the sequencer is
unspawned. Mention that the existing 128- and 512-transaction tests depend on
this constraint.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 7fd7928a-706f-4d11-bafe-0a0c5ca26191
📒 Files selected for processing (15)
Cargo.tomlprocessor/Cargo.tomlprocessor/README.mdprocessor/src/callback.rsprocessor/src/error.rsprocessor/src/executor.rsprocessor/src/lib.rsprocessor/src/metrics.rsprocessor/src/sequencer/locks.rsprocessor/src/sequencer/mod.rsprocessor/src/sequencer/pool.rsprocessor/src/sequencer/tests.rsprocessor/src/simulator.rsprocessor/src/svm.rsprocessor/src/tests.rs
| for acc in txn.static_account_keys().iter().take(locked) { | ||
| let Some(count) = executor.locks.get_mut(acc) else { | ||
| continue; | ||
| }; | ||
| *count -= 1; | ||
| let Some(lock) = self.get_mut(acc) else { | ||
| continue; | ||
| }; | ||
| // Retry runs on `blocker`; reserve its acquired prefix against other work. | ||
| lock.contend(blocker); | ||
| if *count == 0 { | ||
| lock.unlock(id); | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Rollback leaves zero-count lock entries, which later strip another executor's write bit.
The rollback loop decrements *count but never removes the entry from executor.locks. An account whose count reaches 0 stays in the map. When that executor later finishes a batch, LockTable::release drains the map and calls AccountLock::unlock(id) for an account it does not hold. unlock clears WRITE_BIT unconditionally (Line 138), so the exclusive hold of the real writer degrades to a shared hold. A later read by a third executor then succeeds while the writer is still in flight, and two conflicting transactions can execute in parallel.
processor/src/sequencer/locks.rs#L60-L73: remove the entry fromexecutor.lockswhen its count reaches0.processor/src/sequencer/tests.rs#L138-L138: change the assertion toassert_eq!(blocked.locks.get(&a), None);.
🐛 Proposed fix in `LockTable::acquire`
for acc in txn.static_account_keys().iter().take(locked) {
- let Some(count) = executor.locks.get_mut(acc) else {
+ let Some(count) = executor.locks.get_mut(acc) else {
continue;
};
*count -= 1;
+ let drained = *count == 0;
+ if drained {
+ executor.locks.remove(acc);
+ }
let Some(lock) = self.get_mut(acc) else {
continue;
};
// Retry runs on `blocker`; reserve its acquired prefix against other work.
lock.contend(blocker);
- if *count == 0 {
+ if drained {
lock.unlock(id);
}
}Consider also making unlock clear WRITE_BIT only when the executor actually holds the bit. That removes the whole class of spurious write-bit loss.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for acc in txn.static_account_keys().iter().take(locked) { | |
| let Some(count) = executor.locks.get_mut(acc) else { | |
| continue; | |
| }; | |
| *count -= 1; | |
| let Some(lock) = self.get_mut(acc) else { | |
| continue; | |
| }; | |
| // Retry runs on `blocker`; reserve its acquired prefix against other work. | |
| lock.contend(blocker); | |
| if *count == 0 { | |
| lock.unlock(id); | |
| } | |
| } | |
| for acc in txn.static_account_keys().iter().take(locked) { | |
| let Some(count) = executor.locks.get_mut(acc) else { | |
| continue; | |
| }; | |
| *count -= 1; | |
| let drained = *count == 0; | |
| if drained { | |
| executor.locks.remove(acc); | |
| } | |
| let Some(lock) = self.get_mut(acc) else { | |
| continue; | |
| }; | |
| // Retry runs on `blocker`; reserve its acquired prefix against other work. | |
| lock.contend(blocker); | |
| if drained { | |
| lock.unlock(id); | |
| } | |
| } |
📍 Affects 2 files
processor/src/sequencer/locks.rs#L60-L73(this comment)processor/src/sequencer/tests.rs#L138-L138
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@processor/src/sequencer/locks.rs` around lines 60 - 73, Remove each
zero-count account from executor.locks during the rollback loop in LockTable’s
release path, while preserving lock.contend and unlock behavior for remaining
counts; update processor/src/sequencer/tests.rs lines 138-138 to assert
blocked.locks.get(&a) is None. Also make AccountLock::unlock clear WRITE_BIT
only when that executor actually owns it.
| /// Releases every account lock recorded in `held` for `executor`. | ||
| pub(super) fn release(&mut self, executor: &mut ExecutorHandle) { |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix the stale parameter name in the doc comment.
The doc refers to held, but the function takes only executor. The locks are read from executor.locks.
📝 Proposed doc fix
- /// Releases every account lock recorded in `held` for `executor`.
+ /// Releases every account lock recorded in `executor.locks`.
pub(super) fn release(&mut self, executor: &mut ExecutorHandle) {As per path instructions: "Check docs and rustdoc for factual consistency with the code."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /// Releases every account lock recorded in `held` for `executor`. | |
| pub(super) fn release(&mut self, executor: &mut ExecutorHandle) { | |
| /// Releases every account lock recorded in `executor.locks`. | |
| pub(super) fn release(&mut self, executor: &mut ExecutorHandle) { |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@processor/src/sequencer/locks.rs` around lines 77 - 78, Update the rustdoc
for the `release` method to state that it releases every account lock read from
`executor.locks`, replacing the stale `held` reference while preserving the
documented behavior.
Source: Path instructions
| /// Worker loop: simulates requests and applies block transitions until the | ||
| /// channel closes, then reports cooperative shutdown. | ||
| async fn run(mut self) { | ||
| // Mirror the executor: simulated MagicRoot calls authorize against the | ||
| // same authority published on this simulator thread. | ||
| AUTHORITY.set(self.state.authority()); | ||
| loop { | ||
| tokio::select! { | ||
| biased; | ||
| _ = self.shutdown.signalled() => { | ||
| break; | ||
| } | ||
| Some(msg) = self.rx.recv() => { | ||
| self.handle_message(msg); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Make the channel-close behavior match the rustdoc.
Lines 51-53 state that channel closure ends the worker. When self.rx.recv() returns None, the Some(msg) branch is disabled and the loop waits for a shutdown signal instead.
Break on None, or change the rustdoc to state that shutdown is required after all senders drop.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@processor/src/simulator.rs` around lines 51 - 65, Update the worker loop in
run so self.rx.recv() explicitly exits when the channel returns None, preserving
message handling for Some(msg) and the documented behavior that channel closure
ends the worker.
1 participant
What changed
Added the
magicblock-processorcrate, which schedules and executes transactionsover the keeper.
Why
Inbound transactions need conflict-aware scheduling that runs disjoint account
sets concurrently while reading and writing accounts through
keeper/accountsdb.Closes #11.
Impact
serializes the rest.
Pubkeywith a write bit plus a per-executoroccupancy bitset (
MAX_EXECUTORS).and returns the execution record without committing.
Reviewer notes
A quiescence barrier drains in-flight work for consistent snapshots at superblock
seals and during replay — the main concurrency-correctness surface.
Follow-up
enginewires this sequencer to durable state upstack.