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
067ddca to
ef9cedd
Compare
|
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe new ChangesTransaction processor
Estimated code review effort: 5 (Critical) | ~90 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant Sequencer
participant TransactionExecutor
participant SvmContext
participant Keeper
Client->>Sequencer: submit resolved transactions
Sequencer->>TransactionExecutor: dispatch conflict-free batch
TransactionExecutor->>SvmContext: execute transaction batch
SvmContext->>Keeper: load account state
TransactionExecutor->>Keeper: persist execution records
TransactionExecutor->>Sequencer: report completed batch
Sequencer->>TransactionExecutor: release locks and dispatch blocked work
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
|
@CodeRabbit review |
✅ Action performedReview finished.
|
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
|
@CodeRabbit review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/mod.rs`:
- Around line 251-276: Update finalize so its documentation accurately describes
the block boundary sequencing, and preferably append the block via
state.blocks().append before notifying executor handles through
ExecutorMessage::Block. Preserve error propagation and ensure hash advancement
and slot updates remain consistent with the chosen ordering.
- Around line 134-140: Track whether the sequencer loop in the error-handling
branch exits because of a failure, setting a `failed` indicator before breaking
after logging the error. At the shutdown reason selection near `ShutdownReason`,
use that indicator to report the failure-specific reason instead of
`ShutdownReason::Signalled`, while preserving the signalled reason for normal
shutdown requests.
- Around line 65-107: Ensure Sequencer::new never creates an executor pool with
zero workers: clamp the requested executors value to at least 1 before
calculating count and initializing the channel. Preserve the existing
MAX_EXECUTORS limit so schedule always has a valid executor.
🪄 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: 89521626-0b3c-4d5d-937f-1e22e22327f4
📒 Files selected for processing (3)
Cargo.tomlprocessor/src/executor.rsprocessor/src/sequencer/mod.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- Cargo.toml
- processor/src/executor.rs

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.