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
|
Warning Review limit reached
Next review available in: 59 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe new ChangesTransaction processor
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 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->>Executor: release locks and dispatch blocked work
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
|
@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
| /// 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
|
@CodeRabbit review |
✅ Action performedReview finished.
|
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.