Feat: multi threaded scheduler #589
Conversation
taco-paco
left a comment
taco-paco
left a comment
There was a problem hiding this comment.
We need tests simulating various load scenarios and seeing that our Scheduler copes with them
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
Cargo.toml(1 hunks)magicblock-processor/Cargo.toml(1 hunks)magicblock-processor/src/executor/mod.rs(4 hunks)magicblock-processor/src/scheduler/coordinator.rs(1 hunks)magicblock-processor/src/scheduler/locks.rs(1 hunks)magicblock-processor/src/scheduler/mod.rs(1 hunks)magicblock-processor/src/scheduler/tests.rs(1 hunks)
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 589
File: magicblock-processor/src/scheduler/locks.rs:110-122
Timestamp: 2025-11-04T10:53:50.896Z
Learning: In magicblock-processor, the TransactionScheduler runs in a single, dedicated thread and will always remain single-threaded. The `next_transaction_id()` function in scheduler/locks.rs uses `unsafe static mut` which is safe given this architectural guarantee.
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 578
File: magicblock-accounts-db/src/lib.rs:63-72
Timestamp: 2025-10-21T10:34:59.140Z
Learning: In magicblock-validator, the AccountsDb "stop-the-world" synchronizer is managed at the processor/executor level, not at the AccountsDb API level. Transaction executors in magicblock-processor hold a read lock (sync.read()) for the duration of each slot and release it only at slot boundaries, ensuring all account writes happen under the read lock. Snapshot operations acquire a write lock, blocking until all executors release their read locks. This pattern ensures mutual exclusion between writes and snapshots without requiring read guards in AccountsDb write APIs.
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-04T10:53:50.896Z
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 589
File: magicblock-processor/src/scheduler/locks.rs:110-122
Timestamp: 2025-11-04T10:53:50.896Z
Learning: In magicblock-processor, the TransactionScheduler runs in a single, dedicated thread and will always remain single-threaded. The `next_transaction_id()` function in scheduler/locks.rs uses `unsafe static mut` which is safe given this architectural guarantee.
Applied to files:
magicblock-processor/src/scheduler/mod.rsmagicblock-processor/src/scheduler/tests.rsmagicblock-processor/src/executor/mod.rsmagicblock-processor/src/scheduler/locks.rsmagicblock-processor/src/scheduler/coordinator.rs
📚 Learning: 2025-11-04T10:48:00.047Z
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 589
File: magicblock-processor/src/scheduler/mod.rs:217-219
Timestamp: 2025-11-04T10:48:00.047Z
Learning: In magicblock-validator, the codebase uses a pattern where types containing non-Send/non-Sync fields (like Rc<RefCell<...>>) are marked with unsafe impl Send when they are guaranteed to be confined to a single thread through careful API design and thread spawning patterns.
Applied to files:
magicblock-processor/src/scheduler/mod.rsmagicblock-processor/src/scheduler/locks.rs
📚 Learning: 2025-10-21T10:34:59.140Z
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 578
File: magicblock-accounts-db/src/lib.rs:63-72
Timestamp: 2025-10-21T10:34:59.140Z
Learning: In magicblock-validator, the AccountsDb "stop-the-world" synchronizer is managed at the processor/executor level, not at the AccountsDb API level. Transaction executors in magicblock-processor hold a read lock (sync.read()) for the duration of each slot and release it only at slot boundaries, ensuring all account writes happen under the read lock. Snapshot operations acquire a write lock, blocking until all executors release their read locks. This pattern ensures mutual exclusion between writes and snapshots without requiring read guards in AccountsDb write APIs.
Applied to files:
magicblock-processor/src/scheduler/mod.rsmagicblock-processor/src/executor/mod.rsmagicblock-processor/src/scheduler/locks.rsmagicblock-processor/src/scheduler/coordinator.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-processor/src/scheduler/mod.rsmagicblock-processor/src/scheduler/locks.rsmagicblock-processor/src/scheduler/coordinator.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-processor/src/scheduler/mod.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:
magicblock-processor/src/scheduler/tests.rs
🧬 Code graph analysis (3)
magicblock-processor/src/scheduler/mod.rs (4)
magicblock-core/src/link.rs (1)
link(56-82)magicblock-processor/src/executor/mod.rs (4)
new(66-107)spawn(131-142)run(156-195)transition_to_new_slot(199-204)magicblock-processor/src/scheduler/coordinator.rs (3)
new(32-37)new(57-65)is_ready(106-108)magicblock-processor/src/scheduler.rs (2)
TransactionScheduler(40-148)TransactionScheduler(26-38)
magicblock-processor/src/scheduler/tests.rs (1)
magicblock-processor/src/scheduler/coordinator.rs (5)
new(32-37)new(57-65)try_acquire_locks(142-181)get_ready_executor(111-113)next_blocked_transaction(130-135)
magicblock-processor/src/scheduler/coordinator.rs (2)
magicblock-processor/src/scheduler/locks.rs (1)
next_transaction_id(111-122)magicblock-processor/src/scheduler/mod.rs (1)
new(58-91)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: run_make_ci_lint
🔇 Additional comments (2)
magicblock-processor/Cargo.toml (1)
42-42: Workspace dependencies appropriately added for multi-threaded scheduler.The additions of
rustc-hashandsolana-keypairare consistent with the new lock-based scheduler design.rustc-hashis a fast hasher for fixed-size account keys used in contention tracking, andsolana-keypairprovides test utilities for the executor suite.Also applies to: 46-46
Cargo.toml (1)
148-148:rustc-hashversion pinned appropriately for workspace.Version 2.1 is a stable release of the Rust compiler's hash implementation, ideal for the fixed-size account key hashing in the new lock contention tracking subsystem introduced by the multi-threaded scheduler.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
magicblock-aperture/src/requests/http/mocked.rs (1)
197-201: Type mismatch remains unresolved:commitmentshould be Vec, not array.The previously flagged issue still exists.
RpcBlockCommitment { commitment: Option<Vec<_>> }expects a Vec, but an array[0; 32]is provided.Apply this diff to fix:
- let response = RpcBlockCommitment { - commitment: Some([0; 32]), - total_stake: 0, - }; + let response = RpcBlockCommitment { + commitment: Some(vec![0; 32]), + total_stake: 0, + };magicblock-aperture/src/requests/http/get_blocks_with_limit.rs (1)
22-24: Address overflow risks in end_slot calculation.The addition of
+ 1toblock_height()on line 24 partially addresses the past review comment's off-by-one concern, but critical overflow issues remain unresolved:
- Line 22:
start_slot + limitcan overflow when both values are large (e.g., high slot number + 500,000 limit).- Line 24:
block_height() + 1can overflow ifblock_height()returnsu64::MAX.Both operations should use
saturating_addto prevent panic in release builds or wrap-around behavior.Apply this diff to fix the overflow issues:
- let end_slot = start_slot + limit; + let end_slot = start_slot.saturating_add(limit); // Calculate the end slot, ensuring it does not exceed the latest block height. - let end_slot = (end_slot).min(self.blocks.block_height() + 1); + let end_slot = end_slot.min(self.blocks.block_height().saturating_add(1));magicblock-processor/src/scheduler/mod.rs (1)
172-190: Restore reserved executors when a reschedule attempt fails.
reschedule_blocked_transactionspulls the next ready executor before callingschedule_transaction. If lock acquisition still fails, we break out of the loop without putting that executor back intoready_executors, so one contention cycle silently shrinks the pool. After a few such misses every worker gets leaked,is_ready()staysfalse, and the scheduler stops dispatching even though threads are idle. Please defer taking the next executor until after a successful schedule (or explicitly release it on failure).- while let Some(exec) = executor { + while let Some(exec) = executor { let txn = self.coordinator.next_blocked_transaction(blocker); let scheduled = if let Some(txn) = txn { - executor = self.coordinator.get_ready_executor(); - self.schedule_transaction(exec, txn, true).await + let scheduled = + self.schedule_transaction(exec, txn, true).await; + if scheduled { + executor = self.coordinator.get_ready_executor(); + } else { + executor = None; + } + scheduled } else { self.coordinator.release_executor(exec); break; }; if !scheduled { break; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
Cargo.toml(2 hunks)magicblock-aperture/Cargo.toml(1 hunks)magicblock-aperture/src/requests/http/get_blocks_with_limit.rs(1 hunks)magicblock-aperture/src/requests/http/get_token_account_balance.rs(2 hunks)magicblock-aperture/src/requests/http/mocked.rs(3 hunks)magicblock-aperture/src/requests/http/request_airdrop.rs(1 hunks)magicblock-processor/src/scheduler/mod.rs(1 hunks)magicblock-table-mania/src/manager.rs(1 hunks)
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 589
File: magicblock-processor/src/scheduler/locks.rs:110-122
Timestamp: 2025-11-04T10:53:50.896Z
Learning: In magicblock-processor, the TransactionScheduler runs in a single, dedicated thread and will always remain single-threaded. The `next_transaction_id()` function in scheduler/locks.rs uses `unsafe static mut` which is safe given this architectural guarantee.
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 578
File: magicblock-accounts-db/src/lib.rs:63-72
Timestamp: 2025-10-21T10:34:59.140Z
Learning: In magicblock-validator, the AccountsDb "stop-the-world" synchronizer is managed at the processor/executor level, not at the AccountsDb API level. Transaction executors in magicblock-processor hold a read lock (sync.read()) for the duration of each slot and release it only at slot boundaries, ensuring all account writes happen under the read lock. Snapshot operations acquire a write lock, blocking until all executors release their read locks. This pattern ensures mutual exclusion between writes and snapshots without requiring read guards in AccountsDb write APIs.
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-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-aperture/src/requests/http/request_airdrop.rsmagicblock-aperture/src/requests/http/get_token_account_balance.rsmagicblock-aperture/src/requests/http/mocked.rsmagicblock-processor/src/scheduler/mod.rsmagicblock-aperture/src/requests/http/get_blocks_with_limit.rs
📚 Learning: 2025-10-21T11:00:18.396Z
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 578
File: magicblock-aperture/src/encoder.rs:176-187
Timestamp: 2025-10-21T11:00:18.396Z
Learning: In the magicblock validator, the current slot is always the root slot. The SlotEncoder in magicblock-aperture/src/encoder.rs correctly sets `root: slot` because there is no lag between current and root slots in this architecture.
Applied to files:
magicblock-aperture/src/requests/http/get_token_account_balance.rsmagicblock-aperture/src/requests/http/mocked.rsmagicblock-aperture/src/requests/http/get_blocks_with_limit.rs
📚 Learning: 2025-11-04T10:53:50.896Z
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 589
File: magicblock-processor/src/scheduler/locks.rs:110-122
Timestamp: 2025-11-04T10:53:50.896Z
Learning: In magicblock-processor, the TransactionScheduler runs in a single, dedicated thread and will always remain single-threaded. The `next_transaction_id()` function in scheduler/locks.rs uses `unsafe static mut` which is safe given this architectural guarantee.
Applied to files:
magicblock-processor/src/scheduler/mod.rs
📚 Learning: 2025-11-04T10:48:00.047Z
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 589
File: magicblock-processor/src/scheduler/mod.rs:217-219
Timestamp: 2025-11-04T10:48:00.047Z
Learning: In magicblock-validator, the codebase uses a pattern where types containing non-Send/non-Sync fields (like Rc<RefCell<...>>) are marked with unsafe impl Send when they are guaranteed to be confined to a single thread through careful API design and thread spawning patterns.
Applied to files:
magicblock-processor/src/scheduler/mod.rs
📚 Learning: 2025-10-21T10:34:59.140Z
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 578
File: magicblock-accounts-db/src/lib.rs:63-72
Timestamp: 2025-10-21T10:34:59.140Z
Learning: In magicblock-validator, the AccountsDb "stop-the-world" synchronizer is managed at the processor/executor level, not at the AccountsDb API level. Transaction executors in magicblock-processor hold a read lock (sync.read()) for the duration of each slot and release it only at slot boundaries, ensuring all account writes happen under the read lock. Snapshot operations acquire a write lock, blocking until all executors release their read locks. This pattern ensures mutual exclusion between writes and snapshots without requiring read guards in AccountsDb write APIs.
Applied to files:
magicblock-processor/src/scheduler/mod.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-processor/src/scheduler/mod.rs
🧬 Code graph analysis (3)
magicblock-aperture/src/requests/http/request_airdrop.rs (2)
magicblock-aperture/src/error.rs (2)
invalid_request(99-104)invalid_params(78-83)magicblock-aperture/src/requests/mod.rs (1)
params(22-26)
magicblock-table-mania/src/manager.rs (1)
magicblock-committor-service/src/intent_execution_manager/intent_execution_engine.rs (1)
rand(587-587)
magicblock-processor/src/scheduler/mod.rs (4)
magicblock-core/src/link.rs (1)
link(56-82)magicblock-processor/src/scheduler/coordinator.rs (3)
new(32-37)new(57-65)is_ready(106-108)magicblock-processor/src/executor/mod.rs (4)
new(66-107)spawn(131-142)run(156-195)transition_to_new_slot(199-204)magicblock-processor/src/scheduler.rs (2)
TransactionScheduler(40-148)TransactionScheduler(26-38)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: run_make_ci_lint
- GitHub Check: run_make_ci_test
- GitHub Check: Build Project
🔇 Additional comments (12)
magicblock-aperture/src/requests/http/mocked.rs (3)
84-89: LGTM! Slot context standardized.This change addresses the previous review comment by standardizing on
block_height()for consistency with other handlers.
103-108: LGTM! Slot context standardized.Consistent with the standardization applied to other handlers.
120-125: LGTM! Slot context standardized.This completes the standardization across all three handlers flagged in the previous review.
magicblock-aperture/src/requests/http/get_token_account_balance.rs (2)
4-7: LGTM: Necessary imports for canonical token amount formatter.The added imports enable the use of Solana's
token_amount_to_ui_amount_v3function, which provides standardized UI amount formatting for SPL tokens.
64-71: Excellent refactor: adopts Solana's canonical UI amount formatter.This change addresses the previous concern about
ui_amount_stringformatting by delegating totoken_amount_to_ui_amount_v3, which handles edge cases like scientific notation and precision trimming correctly.The
Nonevalues forinterest_bearing_configandscaled_ui_amount_configare appropriate for standard SPL tokens. If Token-2022 extended tokens (interest-bearing, scaled UI amounts) are in scope, additional parsing of mint extensions would be required—acceptable technical debt for now.magicblock-aperture/src/requests/http/request_airdrop.rs (3)
18-20: Improved error message for disabled faucet.The error message has been updated to clearly indicate that the faucet is disabled, which is more informative than the previous generic "method is not supported" message.
27-29: Good addition of lamports validation.The zero-amount validation correctly prevents no-op airdrop requests and provides a clear error message.
25-26: Verify: Previous review suggested explicit Pubkey type cast.The earlier review comment on these lines recommended adding an explicit type annotation to cast
pubkeytoPubkeytype (i.e.,let pubkey: Pubkey = some_or_err!(pubkey);), stating this matched the project's existing pattern. While the lamports validation has been added, the type cast was not implemented.Please confirm whether the explicit cast is necessary for type consistency across the codebase or if the current implicit conversion is sufficient.
magicblock-aperture/Cargo.toml (1)
69-69: LGTM! Workspace reference applied.This change addresses the previous review comment by using a workspace reference for
rand, ensuring centralized version management and consistency with other dependencies.Cargo.toml (2)
94-94: Verify the intentional removal of log level filtering.The
release_max_level_infofeature has been removed, which means log statements will no longer be filtered at compile time in release builds. This could impact performance and binary size.Is this intentional for improved production debugging, or should the feature be retained for performance?
148-148: LGTM! rustc-hash addition is appropriate.The addition of
rustc-hashfor lock contention tracking with fixed 32-byte keys is well-justified and faster than siphash for this use case, as discussed in previous comments.magicblock-table-mania/src/manager.rs (1)
380-381: API migration is correct.The code properly uses the rand 0.9 API:
rand::rng()replacesrand::thread_rng(), andRng::random_range(a..b)replacesgen_range(a, b)with support for inclusive ranges. The range0..=u64::MAXis valid syntax for the new API.
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)
test-kit/src/lib.rs (1)
86-115: Update the constructor docs to match the new behavior.Bullet 3 still claims we “spawn a TransactionScheduler with one worker thread,” but
new_with_confignow accepts an executor count and an option to defer startup. Please revise the doc comment so it explains the configurable worker pool and optional deferred spawn.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (5)
magicblock-core/src/link/transactions.rs(2 hunks)magicblock-processor/src/scheduler/mod.rs(1 hunks)magicblock-processor/tests/fees.rs(1 hunks)magicblock-processor/tests/scheduling.rs(1 hunks)test-kit/src/lib.rs(6 hunks)
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 589
File: magicblock-processor/src/scheduler/locks.rs:110-122
Timestamp: 2025-11-04T10:53:50.922Z
Learning: In magicblock-processor, the TransactionScheduler runs in a single, dedicated thread and will always remain single-threaded. The `next_transaction_id()` function in scheduler/locks.rs uses `unsafe static mut` which is safe given this architectural guarantee.
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.
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 578
File: magicblock-accounts-db/src/lib.rs:63-72
Timestamp: 2025-10-21T10:34:59.140Z
Learning: In magicblock-validator, the AccountsDb "stop-the-world" synchronizer is managed at the processor/executor level, not at the AccountsDb API level. Transaction executors in magicblock-processor hold a read lock (sync.read()) for the duration of each slot and release it only at slot boundaries, ensuring all account writes happen under the read lock. Snapshot operations acquire a write lock, blocking until all executors release their read locks. This pattern ensures mutual exclusion between writes and snapshots without requiring read guards in AccountsDb write APIs.
📚 Learning: 2025-11-04T10:53:50.922Z
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 589
File: magicblock-processor/src/scheduler/locks.rs:110-122
Timestamp: 2025-11-04T10:53:50.922Z
Learning: In magicblock-processor, the TransactionScheduler runs in a single, dedicated thread and will always remain single-threaded. The `next_transaction_id()` function in scheduler/locks.rs uses `unsafe static mut` which is safe given this architectural guarantee.
Applied to files:
magicblock-processor/src/scheduler/mod.rstest-kit/src/lib.rsmagicblock-processor/tests/scheduling.rs
📚 Learning: 2025-11-04T10:48:00.070Z
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 589
File: magicblock-processor/src/scheduler/mod.rs:217-219
Timestamp: 2025-11-04T10:48:00.070Z
Learning: In magicblock-validator, the codebase uses a pattern where types containing non-Send/non-Sync fields (like Rc<RefCell<...>>) are marked with unsafe impl Send when they are guaranteed to be confined to a single thread through careful API design and thread spawning patterns.
Applied to files:
magicblock-processor/src/scheduler/mod.rs
📚 Learning: 2025-10-21T10:34:59.140Z
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 578
File: magicblock-accounts-db/src/lib.rs:63-72
Timestamp: 2025-10-21T10:34:59.140Z
Learning: In magicblock-validator, the AccountsDb "stop-the-world" synchronizer is managed at the processor/executor level, not at the AccountsDb API level. Transaction executors in magicblock-processor hold a read lock (sync.read()) for the duration of each slot and release it only at slot boundaries, ensuring all account writes happen under the read lock. Snapshot operations acquire a write lock, blocking until all executors release their read locks. This pattern ensures mutual exclusion between writes and snapshots without requiring read guards in AccountsDb write APIs.
Applied to files:
magicblock-processor/src/scheduler/mod.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-processor/src/scheduler/mod.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-processor/src/scheduler/mod.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:
magicblock-processor/tests/scheduling.rs
🧬 Code graph analysis (4)
magicblock-processor/src/scheduler/mod.rs (4)
magicblock-core/src/link.rs (1)
link(56-82)magicblock-processor/src/scheduler/coordinator.rs (3)
new(32-37)new(57-65)is_ready(106-108)magicblock-processor/src/executor/mod.rs (4)
new(66-107)spawn(131-142)run(156-195)transition_to_new_slot(199-204)magicblock-processor/src/scheduler.rs (2)
TransactionScheduler(40-148)TransactionScheduler(26-38)
magicblock-processor/tests/fees.rs (1)
test-kit/src/lib.rs (1)
new_with_config(94-152)
test-kit/src/lib.rs (2)
magicblock-accounts/src/scheduled_commits_processor.rs (2)
new(66-90)new(418-433)magicblock-aperture/tests/setup.rs (1)
new(83-138)
magicblock-processor/tests/scheduling.rs (2)
test-kit/src/lib.rs (4)
new_with_config(94-152)new(82-84)get_transaction(200-208)get_account(297-307)magicblock-processor/src/scheduler/mod.rs (1)
new(58-91)
🪛 GitHub Actions: Run CI - Format
magicblock-processor/tests/scheduling.rs
[error] 1-7: cargo fmt --check failed. Detected unstaged formatting changes in tests/scheduling.rs. Run 'cargo fmt' (or update formatting) to fix code style issues.
🪛 GitHub Actions: Run CI - Lint
magicblock-processor/tests/scheduling.rs
[error] 562-562: the loop variable i is only used to index accounts (clippy: needless_range_loop). The lint suggests using an iterator, e.g., for in accounts.iter().take(num_txs).skip(1) { ... }
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build Project
- GitHub Check: run_make_ci_test
🔇 Additional comments (3)
magicblock-core/src/link/transactions.rs (2)
54-54: LGTM: Debug derives improve observability.The Debug trait additions on
TransactionStatusandTransactionExecutionResultare beneficial for debugging and logging, especially valuable in the new multi-threaded scheduler context.Also applies to: 80-80
84-84: All construction sites have been properly updated with thelogsfield.Verification confirms that the single construction site in
magicblock-processor/src/executor/processing.rs:202-210correctly initializes all three struct fields, including the newlogsfield (logs: meta.log_messages.clone(),). No other construction sites or alternative instantiation patterns exist in the codebase.magicblock-processor/tests/fees.rs (1)
270-308: Zero-fee setup still behaves as before.Using
new_with_config(0, 1, false)keeps the slot in gasless mode while spawning the scheduler eagerly, so the test’s intent is preserved.
There was a problem hiding this comment.
Actionable comments posted: 8
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (14)
magicblock-aperture/src/lib.rs(1 hunks)magicblock-aperture/src/tests.rs(1 hunks)magicblock-aperture/tests/mocked.rs(2 hunks)magicblock-aperture/tests/node.rs(1 hunks)magicblock-aperture/tests/setup.rs(1 hunks)magicblock-api/src/magic_validator.rs(1 hunks)magicblock-processor/src/executor/mod.rs(5 hunks)magicblock-processor/src/scheduler/coordinator.rs(1 hunks)magicblock-processor/src/scheduler/locks.rs(1 hunks)magicblock-processor/src/scheduler/mod.rs(1 hunks)magicblock-processor/src/scheduler/tests.rs(1 hunks)magicblock-processor/tests/fees.rs(2 hunks)magicblock-processor/tests/scheduling.rs(1 hunks)test-kit/src/lib.rs(10 hunks)
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 589
File: magicblock-processor/src/scheduler/locks.rs:110-122
Timestamp: 2025-11-04T10:53:50.922Z
Learning: In magicblock-processor, the TransactionScheduler runs in a single, dedicated thread and will always remain single-threaded. The `next_transaction_id()` function in scheduler/locks.rs uses `unsafe static mut` which is safe given this architectural guarantee.
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.
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 578
File: magicblock-accounts-db/src/lib.rs:63-72
Timestamp: 2025-10-21T10:34:59.140Z
Learning: In magicblock-validator, the AccountsDb "stop-the-world" synchronizer is managed at the processor/executor level, not at the AccountsDb API level. Transaction executors in magicblock-processor hold a read lock (sync.read()) for the duration of each slot and release it only at slot boundaries, ensuring all account writes happen under the read lock. Snapshot operations acquire a write lock, blocking until all executors release their read locks. This pattern ensures mutual exclusion between writes and snapshots without requiring read guards in AccountsDb write APIs.
📚 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:
magicblock-aperture/src/tests.rsmagicblock-processor/tests/scheduling.rsmagicblock-processor/src/scheduler/tests.rsmagicblock-processor/tests/fees.rs
📚 Learning: 2025-11-04T10:53:50.922Z
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 589
File: magicblock-processor/src/scheduler/locks.rs:110-122
Timestamp: 2025-11-04T10:53:50.922Z
Learning: In magicblock-processor, the TransactionScheduler runs in a single, dedicated thread and will always remain single-threaded. The `next_transaction_id()` function in scheduler/locks.rs uses `unsafe static mut` which is safe given this architectural guarantee.
Applied to files:
magicblock-api/src/magic_validator.rsmagicblock-processor/src/executor/mod.rsmagicblock-processor/tests/scheduling.rstest-kit/src/lib.rsmagicblock-processor/src/scheduler/mod.rsmagicblock-processor/src/scheduler/locks.rsmagicblock-processor/src/scheduler/coordinator.rsmagicblock-processor/src/scheduler/tests.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-api/src/magic_validator.rsmagicblock-processor/src/scheduler/mod.rsmagicblock-processor/src/scheduler/locks.rsmagicblock-processor/src/scheduler/coordinator.rs
📚 Learning: 2025-10-21T10:34:59.140Z
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 578
File: magicblock-accounts-db/src/lib.rs:63-72
Timestamp: 2025-10-21T10:34:59.140Z
Learning: In magicblock-validator, the AccountsDb "stop-the-world" synchronizer is managed at the processor/executor level, not at the AccountsDb API level. Transaction executors in magicblock-processor hold a read lock (sync.read()) for the duration of each slot and release it only at slot boundaries, ensuring all account writes happen under the read lock. Snapshot operations acquire a write lock, blocking until all executors release their read locks. This pattern ensures mutual exclusion between writes and snapshots without requiring read guards in AccountsDb write APIs.
Applied to files:
magicblock-processor/src/executor/mod.rsmagicblock-processor/src/scheduler/mod.rsmagicblock-processor/src/scheduler/locks.rsmagicblock-processor/src/scheduler/coordinator.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-processor/tests/scheduling.rsmagicblock-processor/src/scheduler/mod.rs
📚 Learning: 2025-11-04T10:48:00.070Z
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 589
File: magicblock-processor/src/scheduler/mod.rs:217-219
Timestamp: 2025-11-04T10:48:00.070Z
Learning: In magicblock-validator, the codebase uses a pattern where types containing non-Send/non-Sync fields (like Rc<RefCell<...>>) are marked with unsafe impl Send when they are guaranteed to be confined to a single thread through careful API design and thread spawning patterns.
Applied to files:
test-kit/src/lib.rsmagicblock-processor/src/scheduler/mod.rsmagicblock-processor/src/scheduler/locks.rs
🧬 Code graph analysis (11)
magicblock-aperture/src/lib.rs (1)
magicblock-aperture/src/processor.rs (1)
start(73-83)
magicblock-api/src/magic_validator.rs (1)
magicblock-processor/src/scheduler/mod.rs (1)
new(58-91)
magicblock-processor/src/executor/mod.rs (1)
magicblock-committor-service/src/service.rs (1)
try_send(327-340)
magicblock-aperture/tests/node.rs (1)
test-kit/src/lib.rs (1)
get_payer(321-327)
magicblock-processor/tests/scheduling.rs (1)
test-kit/src/lib.rs (4)
new_with_config(99-160)new(87-89)get_transaction(208-216)get_account(309-319)
test-kit/src/lib.rs (2)
magicblock-aperture/tests/setup.rs (1)
new(83-138)magicblock-processor/src/scheduler/mod.rs (1)
new(58-91)
magicblock-aperture/tests/mocked.rs (1)
test-kit/src/lib.rs (1)
get_payer(321-327)
magicblock-processor/src/scheduler/mod.rs (3)
magicblock-core/src/link.rs (1)
link(56-82)magicblock-processor/src/executor/mod.rs (4)
new(66-107)spawn(131-142)run(156-196)transition_to_new_slot(200-205)magicblock-processor/src/scheduler/coordinator.rs (3)
new(32-37)new(59-68)is_ready(109-111)
magicblock-processor/src/scheduler/coordinator.rs (2)
magicblock-processor/src/scheduler/locks.rs (1)
next_transaction_id(98-109)magicblock-processor/src/scheduler/mod.rs (1)
new(58-91)
magicblock-processor/src/scheduler/tests.rs (2)
magicblock-processor/src/scheduler/coordinator.rs (5)
new(32-37)new(59-68)try_acquire_locks(147-213)get_ready_executor(115-117)next_blocked_transaction(135-140)magicblock-processor/src/scheduler/mod.rs (1)
new(58-91)
magicblock-processor/tests/fees.rs (1)
test-kit/src/lib.rs (2)
get_payer(321-327)new_with_config(99-160)
🪛 GitHub Actions: Run CI - Format
magicblock-processor/src/scheduler/tests.rs
[error] 10-12: Rustfmt: code formatting check failed. Run 'cargo +nightly fmt --check -- --config-path rustfmt-nightly.toml' to fix formatting.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: run_make_ci_lint
🔇 Additional comments (34)
magicblock-aperture/src/tests.rs (1)
60-60: LGTM!The change correctly adopts the new
get_payer()accessor pattern introduced in test-kit, replacing direct field access with the method-based approach.magicblock-aperture/tests/setup.rs (1)
96-96: LGTM!The change correctly migrates to the new
get_payer()accessor. Note the API shift frompayer.pubkey()(method) toget_payer().pubkey(field access on returnedCommitableAccount).magicblock-aperture/tests/node.rs (1)
37-37: LGTM!The test assertion correctly adopts the new accessor pattern, maintaining the same test logic while using the updated API.
magicblock-aperture/tests/mocked.rs (2)
19-19: LGTM!The test assertion correctly adopts the new
get_payer()accessor pattern.
195-195: LGTM!The string conversion correctly uses the new accessor pattern, maintaining the same test logic.
magicblock-api/src/magic_validator.rs (1)
305-313: LGTM! Dynamic executor sizing is well-designed.The calculation correctly handles edge cases (1-2 CPUs result in 1 executor, scaling appropriately with higher CPU counts). The formula reserves half the available CPUs for execution while accounting for the scheduler thread itself.
magicblock-processor/tests/fees.rs (2)
174-174: LGTM! Payer access updated correctly.The change from direct field access to the getter method aligns with the refactored test-kit API.
270-270: LGTM! Test configuration correctly updated.The new API call properly configures the environment for gasless mode testing with appropriate parameters (fee=0, single executor, no deferred startup).
magicblock-processor/tests/scheduling.rs (2)
101-198: LGTM! Assertion helpers are well-designed.The timeout and polling logic correctly handles both unordered and strict-order validation scenarios. The 100ms poll interval is appropriate for test scenarios.
230-579: Excellent test coverage for concurrent scheduling behavior.The scenarios comprehensively test:
- Parallel non-conflicting execution
- Serialization on write conflicts
- Read-only parallelism
- Mixed workloads
- Order preservation guarantees
Particularly valuable are scenarios 6 and 7, which validate strict ordering for dependent transactions.
magicblock-processor/src/executor/mod.rs (1)
24-24: LGTM! ExecutorId type migration is consistent.The rename from WorkerId to ExecutorId is applied correctly throughout the executor implementation, maintaining type safety across the module.
Also applies to: 34-34, 54-54, 67-67, 70-70
magicblock-processor/src/scheduler/tests.rs (4)
52-96: LGTM: Comprehensive basic test coverage.The basic tests properly verify non-conflicting transactions and concurrent read locks. The test design is clean and the assertions are appropriate.
100-213: LGTM: Contention scenarios are well covered.The contention tests thoroughly verify write-write, write-read, and read-write blocking behavior with appropriate assertions on the blocker IDs.
215-293: LGTM: Advanced scenarios demonstrate complex scheduling flows.The dependency chain and full executor pool tests verify important edge cases like transaction queuing, rescheduling, and executor pool exhaustion. These scenarios are critical for correctness.
295-475: LGTM: Edge cases provide excellent coverage.The edge case tests cover important scenarios like transactions with no accounts, rapid lock cycles, contention on multiple accounts, and blocked transaction chains. This comprehensive coverage will help catch regressions.
test-kit/src/lib.rs (4)
50-68: LGTM: Fields appropriately support multi-executor testing.The addition of
payer_index,payerspool, and optionalschedulerenables proper multi-executor test scenarios with deferred startup control.
99-160: LGTM: Constructor properly initializes multi-payer environment.The
new_with_configconstructor correctly creates multiple payers, allows deferred scheduler startup, and funds all payers. The conditional spawning logic is sound.
162-166: LGTM: Deferred scheduler startup helper is clean.The
run_schedulermethod appropriately spawns a previously deferred scheduler when called.
250-261: Verify AtomicUsize ordering is sufficient for payer selection.The
fetch_add(Ordering::Relaxed)inbuild_transactionis appropriate for round-robin payer selection since there's no cross-thread synchronization requirement. However,get_payerusesload(Ordering::Relaxed)which may return a stale index if called immediately after another thread'sbuild_transaction.If tests rely on
get_payerreturning the payer of the most recently built transaction across threads, consider usingOrdering::SeqCstor documenting thatget_payermay return any payer from the pool.magicblock-processor/src/scheduler/coordinator.rs (5)
30-38: LGTM: TransactionWithId wrapper is clean.The wrapper correctly assigns unique IDs via
next_transaction_id()for tracking transactions through the scheduling pipeline.Based on learnings.
57-68: LGTM: Coordinator initialization is sound.The constructor properly initializes per-executor queues and locks, and pre-populates the ready executor pool.
75-105: LGTM: Transaction queuing with blocker resolution is correct.The
queue_transactionmethod correctly resolves transaction ID blockers to executor IDs and maintains sorted order via binary search. The panic on unknown transaction ID is appropriate given the architectural invariant.
194-200: Clarify write-only contention tracking rationale.The comment states "We only set contention for write locks, in order to prevent writer starvation" (line 196). This design choice means read-only transactions don't register contention, which could lead to reader starvation if many write transactions queue up.
Is this intentional? Consider documenting the trade-off more explicitly, or confirm that the system prioritizes writer throughput over reader fairness.
146-213: Rollback on partial lock acquisition failure is complete and sound.Verification confirms:
No side effects beyond
unlock(): Thewrite()andread()methods inAccountLockonly perform bitwise operations on therwfield;unlock()correctly reverses these via bitwise AND.Vector tracking is exhaustive: The
acquired_locksvector contains only locks that were successfully acquired (pushed after successfulwrite()/read()calls). On failure, all tracked locks are drained and unlocked, ensuring no locks leak.The implementation correctly handles partial failures: lock state is fully reversed via
unlock(), and contention markers are properly set on failure and cleared on success. The code is sound.magicblock-processor/src/scheduler/mod.rs (6)
58-91: LGTM: Scheduler initialization is well-structured.The constructor properly clamps executor count, sets up channels, spawns executor threads, and initializes the coordinator. The sequential setup ensures all components are ready before the scheduler starts.
98-107: LGTM: Spawn pattern follows established architecture.The
spawnmethod correctly creates a dedicated OS thread with a single-threaded Tokio runtime for the scheduler, consistent with the architectural guarantee that enables the unsafe Send impl.Based on learnings.
119-144: LGTM: Event loop priorities are appropriate.The
biasedselect ensures ready executors are processed before new transactions, which maximizes throughput by keeping executors busy. The readiness guard on transaction receipt prevents queueing when no executors are available.
208-225: LGTM: Transaction scheduling with proper error handling.The
schedule_transactionmethod correctly attempts lock acquisition, queues on failure, and sends to the executor on success. The try_send is safe given the capacity guarantee.
234-236: unsafe Send impl is appropriate given architectural guarantees.The unsafe Send impl is justified because the scheduler runs in a single dedicated thread and the Rc<RefCell<...>> never escapes. This follows the established pattern in the codebase.
Based on learnings.
165-169: Program cache thread-safety is verified and correct.The architecture uses separate OS threads for the scheduler and each executor, all sharing a single
Arc<RwLock<ProgramCache>>. Both the scheduler (transition_to_new_slotat lines 165-169) and executors write to the cache, and access is properly serialized by the RwLock:
- Multiple executors run concurrently in parallel threads (spawned at line 79 in scheduler/mod.rs)
- The scheduler runs in its own dedicated thread (confirmed from learnings)
- RwLock ensures mutual exclusion: write operations serialize all access (both readers and writers must wait)
- This is the correct and expected behavior—RwLock trades some concurrency for thread safety
The implementation is sound. The concerns about concurrent reads being blocked during writes are not issues but rather the intended synchronization guarantees of RwLock. No changes are needed.
magicblock-processor/src/scheduler/locks.rs (4)
56-69: LGTM: Write lock implementation is correct.The write lock correctly checks for any existing locks (line 60), returns the conflicting executor via
trailing_zeros()(line 64), and sets both the write bit and executor bit atomically (line 67).
73-86: LGTM: Read lock implementation is correct.The read lock properly checks for write locks (line 78), identifies the write lock holder (line 81), and sets the executor's read bit (line 84). Multiple readers can coexist since they only set their individual bits.
90-95: LGTM: Unlock clears both write and read bits.The unlock operation correctly clears both the write flag and the executor's bit using a single bitwise AND operation. This works for both write locks (which have both bits set) and read locks (which only have the executor bit set).
97-109: LGTM: Transaction ID generation is safe given architectural guarantees.The use of
unsafe static mutis acceptable given the documented guarantee that the scheduler runs in a single dedicated thread. The wrapping add with max ensures IDs stay aboveMAX_SVM_EXECUTORSto distinguish transaction IDs from executor IDs.Based on learnings.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (14)
magicblock-aperture/src/lib.rs(1 hunks)magicblock-aperture/src/tests.rs(1 hunks)magicblock-aperture/tests/mocked.rs(2 hunks)magicblock-aperture/tests/node.rs(1 hunks)magicblock-aperture/tests/setup.rs(1 hunks)magicblock-api/src/magic_validator.rs(1 hunks)magicblock-processor/src/executor/mod.rs(5 hunks)magicblock-processor/src/scheduler/coordinator.rs(1 hunks)magicblock-processor/src/scheduler/locks.rs(1 hunks)magicblock-processor/src/scheduler/mod.rs(1 hunks)magicblock-processor/src/scheduler/tests.rs(1 hunks)magicblock-processor/tests/fees.rs(2 hunks)magicblock-processor/tests/scheduling.rs(1 hunks)test-kit/src/lib.rs(10 hunks)
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 589
File: magicblock-processor/src/scheduler/locks.rs:110-122
Timestamp: 2025-11-04T10:53:50.922Z
Learning: In magicblock-processor, the TransactionScheduler runs in a single, dedicated thread and will always remain single-threaded. The `next_transaction_id()` function in scheduler/locks.rs uses `unsafe static mut` which is safe given this architectural guarantee.
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.
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 578
File: magicblock-accounts-db/src/lib.rs:63-72
Timestamp: 2025-10-21T10:34:59.140Z
Learning: In magicblock-validator, the AccountsDb "stop-the-world" synchronizer is managed at the processor/executor level, not at the AccountsDb API level. Transaction executors in magicblock-processor hold a read lock (sync.read()) for the duration of each slot and release it only at slot boundaries, ensuring all account writes happen under the read lock. Snapshot operations acquire a write lock, blocking until all executors release their read locks. This pattern ensures mutual exclusion between writes and snapshots without requiring read guards in AccountsDb write APIs.
📚 Learning: 2025-11-04T10:53:50.922Z
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 589
File: magicblock-processor/src/scheduler/locks.rs:110-122
Timestamp: 2025-11-04T10:53:50.922Z
Learning: In magicblock-processor, the TransactionScheduler runs in a single, dedicated thread and will always remain single-threaded. The `next_transaction_id()` function in scheduler/locks.rs uses `unsafe static mut` which is safe given this architectural guarantee.
Applied to files:
magicblock-processor/src/scheduler/tests.rsmagicblock-processor/src/executor/mod.rsmagicblock-processor/src/scheduler/mod.rsmagicblock-processor/tests/scheduling.rsmagicblock-api/src/magic_validator.rstest-kit/src/lib.rsmagicblock-processor/src/scheduler/coordinator.rsmagicblock-processor/src/scheduler/locks.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:
magicblock-processor/src/scheduler/tests.rsmagicblock-processor/tests/scheduling.rsmagicblock-processor/tests/fees.rs
📚 Learning: 2025-10-21T10:34:59.140Z
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 578
File: magicblock-accounts-db/src/lib.rs:63-72
Timestamp: 2025-10-21T10:34:59.140Z
Learning: In magicblock-validator, the AccountsDb "stop-the-world" synchronizer is managed at the processor/executor level, not at the AccountsDb API level. Transaction executors in magicblock-processor hold a read lock (sync.read()) for the duration of each slot and release it only at slot boundaries, ensuring all account writes happen under the read lock. Snapshot operations acquire a write lock, blocking until all executors release their read locks. This pattern ensures mutual exclusion between writes and snapshots without requiring read guards in AccountsDb write APIs.
Applied to files:
magicblock-processor/src/executor/mod.rsmagicblock-processor/src/scheduler/mod.rsmagicblock-processor/src/scheduler/coordinator.rsmagicblock-processor/src/scheduler/locks.rs
📚 Learning: 2025-11-04T10:48:00.070Z
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 589
File: magicblock-processor/src/scheduler/mod.rs:217-219
Timestamp: 2025-11-04T10:48:00.070Z
Learning: In magicblock-validator, the codebase uses a pattern where types containing non-Send/non-Sync fields (like Rc<RefCell<...>>) are marked with unsafe impl Send when they are guaranteed to be confined to a single thread through careful API design and thread spawning patterns.
Applied to files:
magicblock-processor/src/scheduler/mod.rstest-kit/src/lib.rsmagicblock-processor/src/scheduler/locks.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-processor/src/scheduler/mod.rsmagicblock-api/src/magic_validator.rsmagicblock-processor/src/scheduler/coordinator.rsmagicblock-processor/src/scheduler/locks.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-processor/src/scheduler/mod.rsmagicblock-processor/tests/scheduling.rs
🧬 Code graph analysis (11)
magicblock-processor/src/scheduler/tests.rs (1)
magicblock-processor/src/scheduler/coordinator.rs (5)
new(32-37)new(59-68)try_acquire_locks(147-213)get_ready_executor(115-117)next_blocked_transaction(135-140)
magicblock-aperture/tests/node.rs (1)
test-kit/src/lib.rs (1)
get_payer(321-327)
magicblock-aperture/src/lib.rs (1)
magicblock-aperture/src/processor.rs (1)
start(73-83)
magicblock-processor/src/executor/mod.rs (1)
magicblock-committor-service/src/service.rs (1)
try_send(327-340)
magicblock-processor/src/scheduler/mod.rs (3)
magicblock-core/src/link.rs (1)
link(56-82)magicblock-processor/src/executor/mod.rs (4)
new(66-107)spawn(131-142)run(156-196)transition_to_new_slot(200-205)magicblock-processor/src/scheduler/coordinator.rs (3)
new(32-37)new(59-68)is_ready(109-111)
magicblock-aperture/tests/mocked.rs (1)
test-kit/src/lib.rs (1)
get_payer(321-327)
magicblock-processor/tests/scheduling.rs (2)
test-kit/src/lib.rs (4)
new_with_config(99-160)new(87-89)get_transaction(208-216)get_account(309-319)magicblock-processor/src/scheduler/mod.rs (1)
new(58-91)
magicblock-api/src/magic_validator.rs (2)
magicblock-processor/src/scheduler/coordinator.rs (2)
new(32-37)new(59-68)magicblock-processor/src/scheduler/mod.rs (1)
new(58-91)
test-kit/src/lib.rs (2)
magicblock-aperture/tests/setup.rs (1)
new(83-138)magicblock-processor/src/scheduler/mod.rs (1)
new(58-91)
magicblock-processor/tests/fees.rs (1)
test-kit/src/lib.rs (2)
get_payer(321-327)new_with_config(99-160)
magicblock-processor/src/scheduler/coordinator.rs (3)
magicblock-processor/src/scheduler/locks.rs (1)
next_transaction_id(98-109)magicblock-processor/src/executor/mod.rs (1)
new(66-107)magicblock-processor/src/scheduler/mod.rs (1)
new(58-91)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: Integration Tests - task-scheduler
- GitHub Check: Integration Tests - cloning
- GitHub Check: Integration Tests - schedule_intents
- GitHub Check: Integration Tests - restore_ledger
- GitHub Check: Integration Tests - table_mania
- GitHub Check: Integration Tests - magicblock_api
- GitHub Check: Integration Tests - config
- GitHub Check: Integration Tests - committor
- GitHub Check: Integration Tests - schedulecommit
🔇 Additional comments (8)
magicblock-aperture/src/lib.rs (1)
35-37: LGTM! EventProcessor scaling aligns with multi-executor architecture.The increase from 1 to 2 EventProcessor instances is consistent with the PR's goal of improving throughput via parallel execution. The comment correctly reflects the new startup count.
magicblock-api/src/magic_validator.rs (1)
305-313: LGTM! Dynamic executor count computed correctly.The formula
(num_cpus::get() / 2).saturating_sub(1).max(1)properly accounts for:
- Dedicating half the CPU resources to execution
- Reserving one thread for the scheduler (per learnings)
- Ensuring at least 1 executor via
.max(1)- Preventing underflow via
.saturating_sub(1)Note that on systems with 2-4 cores, this yields only 1 executor, effectively matching the previous behavior. The logging helps operators understand the configured parallelism level.
Based on learnings
test-kit/src/lib.rs (3)
99-166: LGTM! Multi-payer and deferred startup design is sound.The implementation correctly:
- Creates one payer per executor for parallel transaction submission
- Uses deferred startup to allow tests to configure state before the scheduler runs
- Funds all payers during initialization
- Ensures
run_scheduler()can only spawn once viaOption::take()The new
new_with_configAPI provides good flexibility for tests with varying executor counts and startup requirements.
250-260: Verify payer indexing consistency with multi-executor tests.There's a potential indexing mismatch between
build_transactionandget_payer:
build_transactionusesfetch_add(1, ...)which returns the old index before incrementingget_payerusesload(...)which returns the current (already incremented) indexThis means after calling
build_transaction,get_payer()returns the next payer to be used, not the one that was just used. For tests that verify the fee payer's balance after execution, this could cause failures.Currently, all tests use
executors=1, sopayers[i % 1]always returnspayers[0], masking this issue. Withexecutors > 1, tests expectingget_payer()to return the last-used payer will break.Consider either:
- Documenting that
get_payer()returns the next payer (if intentional), or- Adjusting the indexing so
get_payer()returns the last-used payer, e.g.:pub fn get_payer(&self) -> CommitableAccount { let payer = { let index = self.payer_index.load(Ordering::Relaxed); let last_used = if index == 0 { 0 } else { index - 1 }; &self.payers[last_used % self.payers.len()] }; self.get_account(payer.pubkey()) }Please verify this behavior with tests using
executors > 1.Also applies to: 322-326
275-280: LGTM! schedule_transaction helper is straightforward.The helper appropriately uses
unwrap()for test environments where panicking on failure is acceptable behavior.magicblock-processor/tests/fees.rs (1)
174-174: LGTM! Test API updates align with test-kit changes.Both changes correctly migrate to the new test-kit API:
- Line 174: Uses
get_payer()accessor instead of direct field access- Line 270: Migrates to
new_with_config(fee, executors, defer_startup)with explicit parametersThe tests continue to use
executors=1, maintaining existing behavior while adopting the new API surface.Also applies to: 270-270
magicblock-aperture/tests/setup.rs (1)
96-96: LGTM! Test setup migrates to new get_payer() accessor.The change correctly adopts the new test-kit API for accessing the payer's public key.
magicblock-processor/tests/scheduling.rs (1)
585-733: Comprehensive scenario matrix appreciated.Great coverage across executor counts and contention patterns—this suite should surface scheduler regressions quickly.
taco-paco
left a comment
taco-paco
left a comment
There was a problem hiding this comment.
Couldn't find any critical bugs this run. Tho there're some performance related comments that we should address.
Overall, I think it is ready to be merged after addressing comments. Good job
As a side note for PR:
- There're still cases when rescheduling isn't optimal, added case in one of the comments
- We lack stealing mechanism that we should add(later)
- Code complexity is very high, especially after
accout_contentionaddition
| // NOTE: the channel will always have enough capacity, since the executor was | ||
| // marked ready, which means that its transaction queue is currently empty. | ||
| let _ = self.executors[executor as usize].try_send(txn.txn).inspect_err(|e| { | ||
| error!("Executor {executor} has shutdown or crashed, should not be possible: {e}") |
There was a problem hiding this comment.
In case TrySendError::Full we need to ask @lucacillario to setup an alert
There was a problem hiding this comment.
I think it's going a bit overboard, the Full case will be caught in tests (via failing them due transaction not being executed, as it was dropped), since that can only happen if some logic error has crept in.
There was a problem hiding this comment.
only happen if some logic error has crept in
Yeap, the idea is to know about this right away as this would mean a critical logic error that we missed and need to investigate
There was a problem hiding this comment.
Well I guess we can alert on any ERROR log then, since those are meant to be catastrophic.
There was a problem hiding this comment.
Not in our codebase at the moment :) But that is another topic already, we can take things slow
There was a problem hiding this comment.
I meant just for any log that has error log level (which is easy to detect), we can raise an alert, no changes to codebase are necessary.
| fn contend_account(&mut self, acc: Pubkey, txn: TransactionId) { | ||
| let contenders = self.account_contention.entry(acc).or_default(); | ||
| if let Err(index) = contenders.binary_search(&txn) { | ||
| contenders.insert(index, txn); |
There was a problem hiding this comment.
This takes O(log'n) for searching and then O(n) for insertion on each el-t.
VecDeque isn't suitable for the job.
We can wether use BinaryTree, or simply use HashMap
There was a problem hiding this comment.
The HashMap is not suitable, since it is an unordered DS. an efficient BinaryTree uses Vec under the hood anyway, BTreeSet is a bit slower than VecDeque due to the following reasons:
- The size of the queue is usually very small (less than 10 entries)
- The common insertion cases are to the end or start of the queue (those are O(1)), since transactions are ordered by their IDs which only ever goes up. And the cases when transaction hop queues and inserted in the middle are rare, even then VecDeque beats the BTreeSet due to small size of array.
- BTreeSet keeps large nodes (which are arrays essentially) and any insertion to them is still O(log N) + O(N), since the number is small, the Tree will have only one node usually.
The benchmarks (admittedly artificial) confirm this
Workload 1: Pop Smallest, Insert Largest (N=10)/BTreeSet
time: [12.146 ns 12.149 ns 12.152 ns]
Workload 1: Pop Smallest, Insert Largest (N=10)/VecDeque (binary_search)
time: [9.2427 ns 9.3330 ns 9.4225 ns]
There was a problem hiding this comment.
We mixing data structures with our implementation.
Insertion/removal in VecDeque is O(n)
Insertion/removal in BTree is O(log_n)
Hope that not up to debate
The HashMap is not suitable, since it is an unordered DS.
Not sure why its not suitable if we search by TransactionId. We don't necessarily require sorting in our case, you use it for searching more efficiently.
an efficient BinaryTree uses Vec under the hood
is still O(log N) + O(N)
it would be O(logn) + O(m), where m - is constant and small, and as we know O(c) - O(1). Hope we won't argue that with large Ns, insertions in BTree are cheaper than in vecdeque, with random access.
Now with regards to our implementation, while as I pointed here for clear_account_contention we indeed pop only from front, for contend_accounts its not obvious that we pop_back all the time. And from analysis of contend_accounts one would clearly see O(n) complexity.
We should:
- modify
clear_account_contentionto pop_front or not to pop at all, as its always the case. - Have a gauge metric for executors load and blocked messages, metric for number of idle executors, would be useful anyway.
There was a problem hiding this comment.
The ordering is needed, since we need to ensure the priority access to transactions which first registered contention, which is why HashMap is unacceptable. The N is always small (but I agree that metric wouldn't hurt), and the access is not random, since the elements are using incrementing counter (which is quite bad for BTree). In any case I don't see any significant performance gains by using BTree or other DS, VecDeque has an advantage of cache locality and is faster than more complex structures for small Ns. Let's not turn this into an unproductive argument. If you think that we can speed things up significantly for common case and provide some benchmarks, I'm happy to rewrite the implementation.
| return; | ||
| }; | ||
| if let Ok(index) = contenders.binary_search(&txn) { | ||
| contenders.remove(index); |
There was a problem hiding this comment.
This makes a function O(n) performance.
There was a problem hiding this comment.
It's not, since in almost every case the contender is removed from front of the queue which is O(1), the binary search is just future proofing the logic, so we don't accidentally remove wrong transaction from queue. So the whole operation is O(log N), where N is usually very small.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
magicblock-processor/src/scheduler/mod.rs (1)
11-21: Fail fast when an executor channel closesLine [226] logs and continues after
try_sendfails, but the locks acquired intry_schedulestay recorded under that executor. If a worker panics or its channel closes, the transaction is dropped, the locks are never released, and every contending transaction will block forever—effectively wedging the scheduler. Please release the locks and treat the condition as a hard failure instead of soldiering on.-use log::{error, info}; +use log::info; use magicblock_core::link::transactions::{ ProcessableTransaction, TransactionToProcessRx, }; @@ -use tokio::{ - runtime::Builder, - sync::mpsc::{channel, Receiver, Sender}, -}; +use tokio::{ + runtime::Builder, + sync::mpsc::{channel, error::TrySendError, Receiver, Sender}, +}; @@ - let txn = match self.coordinator.try_schedule(executor, txn) { + let txn = match self.coordinator.try_schedule(executor, txn) { Ok(txn) => txn, Err(blocker) => { return Some(blocker); } }; - // It's safe to ignore the result of the send operation. If the send fails, - // it means the executor's channel is closed, which only happens on shutdown. - // NOTE: the channel will always have enough capacity, since the executor was - // marked ready, which means that its transaction queue is currently empty. - let _ = self.executors[executor as usize].try_send(txn).inspect_err(|e| { - error!("Executor {executor} has shutdown or crashed, should not be possible: {e}") - }); - None + match self.executors[executor as usize].try_send(txn) { + Ok(()) => None, + Err(TrySendError::Full(_)) => unreachable!( + "executor {executor} queue should be empty when scheduling" + ), + Err(TrySendError::Closed(_)) => { + self.coordinator.unlock_accounts(executor); + self.coordinator.release_executor(executor); + panic!( + "executor {executor} channel closed while scheduling; locks released to prevent deadlock" + ); + } + }Also applies to: 222-228
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (6)
magicblock-processor/src/scheduler/coordinator.rs(1 hunks)magicblock-processor/src/scheduler/locks.rs(1 hunks)magicblock-processor/src/scheduler/mod.rs(1 hunks)magicblock-processor/src/scheduler/tests.rs(1 hunks)magicblock-processor/tests/scheduling.rs(1 hunks)test-kit/src/lib.rs(9 hunks)
🧰 Additional context used
🧠 Learnings (11)
📓 Common learnings
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 589
File: magicblock-processor/src/scheduler/locks.rs:110-122
Timestamp: 2025-11-04T10:53:50.922Z
Learning: In magicblock-processor, the TransactionScheduler runs in a single, dedicated thread and will always remain single-threaded. The `next_transaction_id()` function in scheduler/locks.rs uses `unsafe static mut` which is safe given this architectural guarantee.
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 578
File: magicblock-accounts-db/src/lib.rs:63-72
Timestamp: 2025-10-21T10:34:59.140Z
Learning: In magicblock-validator, the AccountsDb "stop-the-world" synchronizer is managed at the processor/executor level, not at the AccountsDb API level. Transaction executors in magicblock-processor hold a read lock (sync.read()) for the duration of each slot and release it only at slot boundaries, ensuring all account writes happen under the read lock. Snapshot operations acquire a write lock, blocking until all executors release their read locks. This pattern ensures mutual exclusion between writes and snapshots without requiring read guards in AccountsDb write APIs.
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.
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.
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.
📚 Learning: 2025-11-07T13:20:13.793Z
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.
Applied to files:
magicblock-processor/src/scheduler/coordinator.rsmagicblock-processor/tests/scheduling.rsmagicblock-processor/src/scheduler/tests.rstest-kit/src/lib.rsmagicblock-processor/src/scheduler/locks.rsmagicblock-processor/src/scheduler/mod.rs
📚 Learning: 2025-11-04T10:53:50.922Z
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 589
File: magicblock-processor/src/scheduler/locks.rs:110-122
Timestamp: 2025-11-04T10:53:50.922Z
Learning: In magicblock-processor, the TransactionScheduler runs in a single, dedicated thread and will always remain single-threaded. The `next_transaction_id()` function in scheduler/locks.rs uses `unsafe static mut` which is safe given this architectural guarantee.
Applied to files:
magicblock-processor/src/scheduler/coordinator.rsmagicblock-processor/tests/scheduling.rsmagicblock-processor/src/scheduler/tests.rstest-kit/src/lib.rsmagicblock-processor/src/scheduler/locks.rsmagicblock-processor/src/scheduler/mod.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-processor/src/scheduler/coordinator.rsmagicblock-processor/tests/scheduling.rsmagicblock-processor/src/scheduler/tests.rstest-kit/src/lib.rsmagicblock-processor/src/scheduler/locks.rsmagicblock-processor/src/scheduler/mod.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-processor/src/scheduler/coordinator.rstest-kit/src/lib.rsmagicblock-processor/src/scheduler/locks.rsmagicblock-processor/src/scheduler/mod.rs
📚 Learning: 2025-10-21T10:34:59.140Z
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 578
File: magicblock-accounts-db/src/lib.rs:63-72
Timestamp: 2025-10-21T10:34:59.140Z
Learning: In magicblock-validator, the AccountsDb "stop-the-world" synchronizer is managed at the processor/executor level, not at the AccountsDb API level. Transaction executors in magicblock-processor hold a read lock (sync.read()) for the duration of each slot and release it only at slot boundaries, ensuring all account writes happen under the read lock. Snapshot operations acquire a write lock, blocking until all executors release their read locks. This pattern ensures mutual exclusion between writes and snapshots without requiring read guards in AccountsDb write APIs.
Applied to files:
magicblock-processor/src/scheduler/coordinator.rsmagicblock-processor/src/scheduler/locks.rsmagicblock-processor/src/scheduler/mod.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-processor/src/scheduler/coordinator.rsmagicblock-processor/tests/scheduling.rsmagicblock-processor/src/scheduler/mod.rs
📚 Learning: 2025-11-12T09:46:27.533Z
Learnt from: Dodecahedr0x
Repo: magicblock-labs/magicblock-validator PR: 614
File: magicblock-task-scheduler/src/db.rs:26-0
Timestamp: 2025-11-12T09:46:27.533Z
Learning: In magicblock-task-scheduler, task parameter validation (including ensuring iterations > 0 and enforcing minimum execution intervals) is performed in the Magic program (on-chain) before ScheduleTaskRequest instances reach the scheduler service. The From<&ScheduleTaskRequest> conversion in db.rs does not need additional validation because inputs are already validated at the program level.
Applied to files:
magicblock-processor/tests/scheduling.rsmagicblock-processor/src/scheduler/tests.rstest-kit/src/lib.rsmagicblock-processor/src/scheduler/locks.rsmagicblock-processor/src/scheduler/mod.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:
magicblock-processor/tests/scheduling.rsmagicblock-processor/src/scheduler/tests.rs
📚 Learning: 2025-11-04T10:48:00.070Z
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 589
File: magicblock-processor/src/scheduler/mod.rs:217-219
Timestamp: 2025-11-04T10:48:00.070Z
Learning: In magicblock-validator, the codebase uses a pattern where types containing non-Send/non-Sync fields (like Rc<RefCell<...>>) are marked with unsafe impl Send when they are guaranteed to be confined to a single thread through careful API design and thread spawning patterns.
Applied to files:
test-kit/src/lib.rsmagicblock-processor/src/scheduler/locks.rsmagicblock-processor/src/scheduler/mod.rs
📚 Learning: 2025-11-07T14:20:31.457Z
Learnt from: thlorenz
Repo: magicblock-labs/magicblock-validator PR: 621
File: magicblock-chainlink/src/remote_account_provider/chain_pubsub_actor.rs:457-495
Timestamp: 2025-11-07T14:20:31.457Z
Learning: In magicblock-chainlink/src/remote_account_provider/chain_pubsub_client.rs, the unsubscribe closure returned by PubSubConnection::account_subscribe(...) resolves to () (unit), not a Result. Downstream code should not attempt to inspect an unsubscribe result and can optionally wrap it in a timeout to guard against hangs.
Applied to files:
magicblock-processor/src/scheduler/mod.rs
🧬 Code graph analysis (5)
magicblock-processor/src/scheduler/coordinator.rs (2)
magicblock-processor/src/scheduler/locks.rs (1)
next_transaction_id(106-117)magicblock-processor/src/scheduler/mod.rs (1)
new(58-91)
magicblock-processor/tests/scheduling.rs (1)
test-kit/src/lib.rs (4)
new_with_config(99-160)new(87-89)get_transaction(208-216)get_account(309-319)
magicblock-processor/src/scheduler/tests.rs (2)
magicblock-processor/src/scheduler/coordinator.rs (5)
new(32-37)new(59-68)try_acquire_locks(145-218)get_ready_executor(113-115)next_blocked_transaction(133-138)magicblock-processor/src/scheduler/mod.rs (1)
new(58-91)
test-kit/src/lib.rs (2)
magicblock-processor/src/scheduler/mod.rs (1)
new(58-91)magicblock-aperture/tests/setup.rs (1)
new(83-138)
magicblock-processor/src/scheduler/mod.rs (3)
magicblock-core/src/link.rs (1)
link(56-82)magicblock-processor/src/scheduler/coordinator.rs (3)
new(32-37)new(59-68)is_ready(107-109)magicblock-processor/src/executor/mod.rs (4)
new(66-107)spawn(131-142)run(156-196)transition_to_new_slot(200-205)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build Project
- GitHub Check: run_make_ci_test
- GitHub Check: run_make_ci_lint
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
magicblock-processor/src/scheduler/mod.rs (1)
222-228: Fail fast when an executor channel closesLogging and continuing on
try_sendfailure silently drops the transaction while leaving all of its locks recorded against the executor. If a worker panics and its channel closes, the coordinator now believes the executor still holds those locks, so every contender will block forever and the scheduler wedges.Treat this as a fatal invariant breach and unwind only after releasing the locks and putting the executor back into the ready pool:
- let _ = self.executors[executor as usize].try_send(txn).inspect_err(|e| { - error!("Executor {executor} has shutdown or crashed, should not be possible: {e}") - }); + self.executors[executor as usize] + .try_send(txn) + .unwrap_or_else(|e| { + error!( + "executor {executor} channel closed while scheduling; cannot recover: {e}" + ); + self.coordinator.unlock_accounts(executor); + self.coordinator.release_executor(executor); + panic!( + "executor {executor} channel unexpectedly closed during scheduling" + ); + });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (6)
magicblock-processor/src/scheduler/coordinator.rs(1 hunks)magicblock-processor/src/scheduler/locks.rs(1 hunks)magicblock-processor/src/scheduler/mod.rs(1 hunks)magicblock-processor/src/scheduler/tests.rs(1 hunks)magicblock-processor/tests/scheduling.rs(1 hunks)test-kit/src/lib.rs(9 hunks)
🧰 Additional context used
🧠 Learnings (11)
📓 Common learnings
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 589
File: magicblock-processor/src/scheduler/locks.rs:110-122
Timestamp: 2025-11-04T10:53:50.922Z
Learning: In magicblock-processor, the TransactionScheduler runs in a single, dedicated thread and will always remain single-threaded. The `next_transaction_id()` function in scheduler/locks.rs uses `unsafe static mut` which is safe given this architectural guarantee.
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.
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 578
File: magicblock-accounts-db/src/lib.rs:63-72
Timestamp: 2025-10-21T10:34:59.140Z
Learning: In magicblock-validator, the AccountsDb "stop-the-world" synchronizer is managed at the processor/executor level, not at the AccountsDb API level. Transaction executors in magicblock-processor hold a read lock (sync.read()) for the duration of each slot and release it only at slot boundaries, ensuring all account writes happen under the read lock. Snapshot operations acquire a write lock, blocking until all executors release their read locks. This pattern ensures mutual exclusion between writes and snapshots without requiring read guards in AccountsDb write APIs.
📚 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-processor/tests/scheduling.rsmagicblock-processor/src/scheduler/tests.rsmagicblock-processor/src/scheduler/coordinator.rstest-kit/src/lib.rsmagicblock-processor/src/scheduler/locks.rsmagicblock-processor/src/scheduler/mod.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:
magicblock-processor/tests/scheduling.rsmagicblock-processor/src/scheduler/tests.rs
📚 Learning: 2025-11-04T10:53:50.922Z
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 589
File: magicblock-processor/src/scheduler/locks.rs:110-122
Timestamp: 2025-11-04T10:53:50.922Z
Learning: In magicblock-processor, the TransactionScheduler runs in a single, dedicated thread and will always remain single-threaded. The `next_transaction_id()` function in scheduler/locks.rs uses `unsafe static mut` which is safe given this architectural guarantee.
Applied to files:
magicblock-processor/tests/scheduling.rsmagicblock-processor/src/scheduler/tests.rsmagicblock-processor/src/scheduler/coordinator.rstest-kit/src/lib.rsmagicblock-processor/src/scheduler/locks.rsmagicblock-processor/src/scheduler/mod.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-processor/tests/scheduling.rsmagicblock-processor/src/scheduler/coordinator.rsmagicblock-processor/src/scheduler/mod.rs
📚 Learning: 2025-11-07T13:20:13.793Z
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.
Applied to files:
magicblock-processor/tests/scheduling.rsmagicblock-processor/src/scheduler/tests.rsmagicblock-processor/src/scheduler/coordinator.rstest-kit/src/lib.rsmagicblock-processor/src/scheduler/locks.rsmagicblock-processor/src/scheduler/mod.rs
📚 Learning: 2025-11-12T09:46:27.533Z
Learnt from: Dodecahedr0x
Repo: magicblock-labs/magicblock-validator PR: 614
File: magicblock-task-scheduler/src/db.rs:26-0
Timestamp: 2025-11-12T09:46:27.533Z
Learning: In magicblock-task-scheduler, task parameter validation (including ensuring iterations > 0 and enforcing minimum execution intervals) is performed in the Magic program (on-chain) before ScheduleTaskRequest instances reach the scheduler service. The From<&ScheduleTaskRequest> conversion in db.rs does not need additional validation because inputs are already validated at the program level.
Applied to files:
magicblock-processor/tests/scheduling.rsmagicblock-processor/src/scheduler/tests.rstest-kit/src/lib.rsmagicblock-processor/src/scheduler/locks.rsmagicblock-processor/src/scheduler/mod.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-processor/src/scheduler/coordinator.rstest-kit/src/lib.rsmagicblock-processor/src/scheduler/locks.rsmagicblock-processor/src/scheduler/mod.rs
📚 Learning: 2025-10-21T10:34:59.140Z
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 578
File: magicblock-accounts-db/src/lib.rs:63-72
Timestamp: 2025-10-21T10:34:59.140Z
Learning: In magicblock-validator, the AccountsDb "stop-the-world" synchronizer is managed at the processor/executor level, not at the AccountsDb API level. Transaction executors in magicblock-processor hold a read lock (sync.read()) for the duration of each slot and release it only at slot boundaries, ensuring all account writes happen under the read lock. Snapshot operations acquire a write lock, blocking until all executors release their read locks. This pattern ensures mutual exclusion between writes and snapshots without requiring read guards in AccountsDb write APIs.
Applied to files:
magicblock-processor/src/scheduler/coordinator.rsmagicblock-processor/src/scheduler/locks.rsmagicblock-processor/src/scheduler/mod.rs
📚 Learning: 2025-11-04T10:48:00.070Z
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 589
File: magicblock-processor/src/scheduler/mod.rs:217-219
Timestamp: 2025-11-04T10:48:00.070Z
Learning: In magicblock-validator, the codebase uses a pattern where types containing non-Send/non-Sync fields (like Rc<RefCell<...>>) are marked with unsafe impl Send when they are guaranteed to be confined to a single thread through careful API design and thread spawning patterns.
Applied to files:
test-kit/src/lib.rsmagicblock-processor/src/scheduler/locks.rsmagicblock-processor/src/scheduler/mod.rs
📚 Learning: 2025-11-07T14:20:31.457Z
Learnt from: thlorenz
Repo: magicblock-labs/magicblock-validator PR: 621
File: magicblock-chainlink/src/remote_account_provider/chain_pubsub_actor.rs:457-495
Timestamp: 2025-11-07T14:20:31.457Z
Learning: In magicblock-chainlink/src/remote_account_provider/chain_pubsub_client.rs, the unsubscribe closure returned by PubSubConnection::account_subscribe(...) resolves to () (unit), not a Result. Downstream code should not attempt to inspect an unsubscribe result and can optionally wrap it in a timeout to guard against hangs.
Applied to files:
magicblock-processor/src/scheduler/mod.rs
🧬 Code graph analysis (5)
magicblock-processor/tests/scheduling.rs (1)
test-kit/src/lib.rs (4)
new_with_config(99-160)new(87-89)get_transaction(208-216)get_account(309-319)
magicblock-processor/src/scheduler/tests.rs (1)
magicblock-processor/src/scheduler/coordinator.rs (5)
new(32-37)new(59-68)try_acquire_locks(145-218)get_ready_executor(113-115)next_blocked_transaction(133-138)
magicblock-processor/src/scheduler/coordinator.rs (2)
magicblock-processor/src/scheduler/locks.rs (1)
next_transaction_id(106-117)magicblock-processor/src/scheduler/mod.rs (1)
new(58-91)
test-kit/src/lib.rs (2)
magicblock-processor/src/scheduler/mod.rs (1)
new(58-91)magicblock-aperture/tests/setup.rs (1)
new(83-138)
magicblock-processor/src/scheduler/mod.rs (3)
magicblock-core/src/link.rs (1)
link(56-82)magicblock-processor/src/scheduler/coordinator.rs (3)
new(32-37)new(59-68)is_ready(107-109)magicblock-processor/src/executor/mod.rs (4)
new(66-107)spawn(131-142)run(156-196)transition_to_new_slot(200-205)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: run_make_ci_lint
- GitHub Check: Build Project
- GitHub Check: run_make_ci_test
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
magicblock-metrics/src/metrics/mod.rs(2 hunks)magicblock-processor/src/scheduler/coordinator.rs(1 hunks)
🧰 Additional context used
🧠 Learnings (8)
📓 Common learnings
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 589
File: magicblock-processor/src/scheduler/locks.rs:110-122
Timestamp: 2025-11-04T10:53:50.922Z
Learning: In magicblock-processor, the TransactionScheduler runs in a single, dedicated thread and will always remain single-threaded. The `next_transaction_id()` function in scheduler/locks.rs uses `unsafe static mut` which is safe given this architectural guarantee.
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.788Z
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.
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 578
File: magicblock-accounts-db/src/lib.rs:63-72
Timestamp: 2025-10-21T10:34:59.140Z
Learning: In magicblock-validator, the AccountsDb "stop-the-world" synchronizer is managed at the processor/executor level, not at the AccountsDb API level. Transaction executors in magicblock-processor hold a read lock (sync.read()) for the duration of each slot and release it only at slot boundaries, ensuring all account writes happen under the read lock. Snapshot operations acquire a write lock, blocking until all executors release their read locks. This pattern ensures mutual exclusion between writes and snapshots without requiring read guards in AccountsDb write APIs.
📚 Learning: 2025-11-07T13:20:13.793Z
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.
Applied to files:
magicblock-metrics/src/metrics/mod.rsmagicblock-processor/src/scheduler/coordinator.rs
📚 Learning: 2025-11-13T09:38:43.788Z
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.788Z
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.
Applied to files:
magicblock-processor/src/scheduler/coordinator.rs
📚 Learning: 2025-11-04T10:53:50.922Z
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 589
File: magicblock-processor/src/scheduler/locks.rs:110-122
Timestamp: 2025-11-04T10:53:50.922Z
Learning: In magicblock-processor, the TransactionScheduler runs in a single, dedicated thread and will always remain single-threaded. The `next_transaction_id()` function in scheduler/locks.rs uses `unsafe static mut` which is safe given this architectural guarantee.
Applied to files:
magicblock-processor/src/scheduler/coordinator.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-processor/src/scheduler/coordinator.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-processor/src/scheduler/coordinator.rs
📚 Learning: 2025-10-21T10:34:59.140Z
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 578
File: magicblock-accounts-db/src/lib.rs:63-72
Timestamp: 2025-10-21T10:34:59.140Z
Learning: In magicblock-validator, the AccountsDb "stop-the-world" synchronizer is managed at the processor/executor level, not at the AccountsDb API level. Transaction executors in magicblock-processor hold a read lock (sync.read()) for the duration of each slot and release it only at slot boundaries, ensuring all account writes happen under the read lock. Snapshot operations acquire a write lock, blocking until all executors release their read locks. This pattern ensures mutual exclusion between writes and snapshots without requiring read guards in AccountsDb write APIs.
Applied to files:
magicblock-processor/src/scheduler/coordinator.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-processor/src/scheduler/coordinator.rs
🧬 Code graph analysis (1)
magicblock-processor/src/scheduler/coordinator.rs (2)
magicblock-processor/src/scheduler/locks.rs (1)
next_transaction_id(106-117)magicblock-processor/src/scheduler/mod.rs (1)
new(58-91)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: run_make_ci_test
- GitHub Check: Build Project
- GitHub Check: run_make_ci_lint
🔇 Additional comments (6)
magicblock-metrics/src/metrics/mod.rs (1)
262-262: LGTM!The metric registration follows the correct pattern and is properly placed in the registration block.
magicblock-processor/src/scheduler/coordinator.rs (5)
25-39: LGTM: Clean wrapper with centralized ID generation.The
TransactionWithIdstruct provides a simple, clear wrapper that pairs transactions with unique IDs. The use ofnext_transaction_id()is appropriate and safe given the single-threaded scheduler architecture.
58-69: LGTM: Proper initialization of coordinator state.The constructor correctly initializes all data structures for the given executor count, starting with all executors in the ready pool. The initialization is clean and comprehensive.
71-106: LGTM: Robust transaction queuing with proper ordering.The method correctly resolves blockers and maintains sorted transaction queues. The use of
.expect()on line 94 is appropriate as this indicates a hard logic error that should fail fast. The binary search insertion prevents duplicates and maintains ordering by transaction ID, which is essential for fairness. Metrics are properly updated.
223-238: LGTM: Clean coordination wrapper.The method properly orchestrates lock acquisition, handles failures by releasing the executor and queueing the transaction, and returns appropriate results for both success and failure cases.
240-263: LGTM: Proper contention tracking with cleanup.The contention management methods correctly maintain sorted transaction ordering using binary search. The cleanup in
clear_account_contentionremoves empty entries to prevent unbounded growth while maintaining slack capacity for performance, as per the established design decision.Based on learnings.
| pub static ref MAX_LOCK_CONTENTION_QUEUE_SIZE: IntGauge = IntGauge::new( | ||
| "max_lock_contention_queue_size", | ||
| "Maximum observed queue size for an account lock contention" | ||
| ).unwrap(); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider adding a helper function for atomic max-tracking.
The metric is correctly declared, but since it tracks a "maximum observed" value, consider adding a helper function to encapsulate the compare-and-set logic. This would ensure consistent usage across the codebase and follow the pattern of other IntGauge metrics in this file.
Add a helper function at the end of the file:
pub fn update_max_lock_contention_queue_size(size: i64) {
let current = MAX_LOCK_CONTENTION_QUEUE_SIZE.get();
if size > current {
MAX_LOCK_CONTENTION_QUEUE_SIZE.set(size);
}
}🤖 Prompt for AI Agents
In magicblock-metrics/src/metrics/mod.rs around lines 176 to 179, add a helper
function to atomically update the "maximum observed" gauge so callers don't
duplicate compare-and-set logic; implement a public function (e.g.,
update_max_lock_contention_queue_size) that reads
MAX_LOCK_CONTENTION_QUEUE_SIZE.get(), compares the provided size, and only calls
MAX_LOCK_CONTENTION_QUEUE_SIZE.set(size) when the new size is greater than the
current value, placing the function near the other IntGauge helpers at the end
of the file for consistent usage.
|
|
||
|
|
||
|
|
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Remove excessive blank lines.
Three consecutive blank lines are inconsistent with the spacing pattern elsewhere in this file. Most sections use 0-1 blank lines between declarations.
-
-
-
+📝 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.
🤖 Prompt for AI Agents
In magicblock-metrics/src/metrics/mod.rs around lines 180 to 182 there are three
consecutive blank lines; remove the excess blank lines and leave at most one
blank line to match the file's spacing convention (or run rustfmt to enforce
consistent formatting), ensuring declarations above and below remain separated
by a single blank line.
| let message = transaction.txn.transaction.message(); | ||
| let accounts_to_lock = message.account_keys().iter().enumerate(); | ||
| let acquired_locks = &mut self.acquired_locks[executor as usize]; | ||
|
|
||
| for (i, &acc) in accounts_to_lock.clone() { | ||
| // Get or create the lock for the account. | ||
| let lock = self.locks.entry(acc).or_default().clone(); | ||
| // See whether there's a contention for the given account | ||
| // if there's one, then we need to follow the breadcrumbs | ||
| // (txns->executor) to find out where our transaction | ||
| // needs to be queued. | ||
| let mut result = | ||
| if let Some(contenders) = self.account_contention.get(&acc) { | ||
| match contenders.binary_search(&transaction.id) { | ||
| // If we are the first contender, then we can proceed | ||
| // and try to acquire the needed account locks | ||
| // 1. Ok(0) => the transaction has already contended this account, | ||
| // and now it is its turn to be scheduled | ||
| // 2. Err(0) => the transaction didn't contend the account, but it | ||
| // has smaller ID (was received earlier), so we don't | ||
| // block it and let it proceed with lock acquisition | ||
| Ok(0) | Err(0) => Ok(()), | ||
| // If we are not, then we need to get queued after | ||
| // the transaction contending right in front of us | ||
| Ok(index) | Err(index) => { | ||
| Err(BlockerId::Transaction(contenders[index - 1])) | ||
| } | ||
| } | ||
| } else { | ||
| Ok(()) | ||
| }; | ||
|
|
||
| if result.is_ok() { | ||
| // Attempt to acquire a write or read lock. | ||
| result = if message.is_writable(i) { | ||
| lock.borrow_mut().write(executor) | ||
| } else { | ||
| lock.borrow_mut().read(executor) | ||
| } | ||
| .map_err(BlockerId::Executor); | ||
| } | ||
|
|
||
| // We couldn't lock all of the accounts, so we are bailing, but | ||
| // first we need to set contention, and unlock successful locks | ||
| if let Err(e) = result { | ||
| for lock in acquired_locks.drain(..) { | ||
| let mut lock = lock.borrow_mut(); | ||
| lock.unlock(executor); | ||
| } | ||
| for (i, &acc) in accounts_to_lock { | ||
| // We only set contention for write locks, | ||
| // in order to prevent writer starvation | ||
| if message.is_writable(i) { | ||
| self.contend_account(acc, transaction.id); | ||
| } | ||
| } | ||
| return Err(e); | ||
| } | ||
|
|
||
| acquired_locks.push(lock); | ||
| } | ||
|
|
||
| // On success, the transaction is no longer blocking anything. | ||
| self.transaction_contention.remove(&transaction.id); | ||
| for (_, acc) in accounts_to_lock { | ||
| self.clear_account_contention(acc, transaction.id); | ||
| } | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider refactoring the multi-pass iteration pattern for clarity.
The method clones the accounts_to_lock iterator on line 157 to enable reuse on lines 202 and 217. While this works correctly (since Enumerate<Iter<&Pubkey>> implements Clone), the pattern is unusual and may reduce maintainability.
Consider collecting the accounts once and iterating over the collection:
let message = transaction.txn.transaction.message();
- let accounts_to_lock = message.account_keys().iter().enumerate();
+ let accounts_to_lock: Vec<_> = message.account_keys().iter().enumerate().collect();
let acquired_locks = &mut self.acquired_locks[executor as usize];
- for (i, &acc) in accounts_to_lock.clone() {
+ for &(i, acc) in &accounts_to_lock {
// ... (rest remains the same)
- for lock in acquired_locks.drain(..) {
- let mut lock = lock.borrow_mut();
- lock.unlock(executor);
- }
- for (i, &acc) in accounts_to_lock {
+ // Cleanup code...
+ for &(i, acc) in &accounts_to_lock {
// We only set contention for write locks,
// in order to prevent writer starvation
if message.is_writable(i) {
@@ -214,7 +214,7 @@
// On success, the transaction is no longer blocking anything.
self.transaction_contention.remove(&transaction.id);
- for (_, acc) in accounts_to_lock {
+ for (_, acc) in &accounts_to_lock {
self.clear_account_contention(acc, transaction.id);
}This makes the multi-pass iteration explicit and more idiomatic. The performance impact is negligible since account lists are typically small.
📝 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.
| let message = transaction.txn.transaction.message(); | |
| let accounts_to_lock = message.account_keys().iter().enumerate(); | |
| let acquired_locks = &mut self.acquired_locks[executor as usize]; | |
| for (i, &acc) in accounts_to_lock.clone() { | |
| // Get or create the lock for the account. | |
| let lock = self.locks.entry(acc).or_default().clone(); | |
| // See whether there's a contention for the given account | |
| // if there's one, then we need to follow the breadcrumbs | |
| // (txns->executor) to find out where our transaction | |
| // needs to be queued. | |
| let mut result = | |
| if let Some(contenders) = self.account_contention.get(&acc) { | |
| match contenders.binary_search(&transaction.id) { | |
| // If we are the first contender, then we can proceed | |
| // and try to acquire the needed account locks | |
| // 1. Ok(0) => the transaction has already contended this account, | |
| // and now it is its turn to be scheduled | |
| // 2. Err(0) => the transaction didn't contend the account, but it | |
| // has smaller ID (was received earlier), so we don't | |
| // block it and let it proceed with lock acquisition | |
| Ok(0) | Err(0) => Ok(()), | |
| // If we are not, then we need to get queued after | |
| // the transaction contending right in front of us | |
| Ok(index) | Err(index) => { | |
| Err(BlockerId::Transaction(contenders[index - 1])) | |
| } | |
| } | |
| } else { | |
| Ok(()) | |
| }; | |
| if result.is_ok() { | |
| // Attempt to acquire a write or read lock. | |
| result = if message.is_writable(i) { | |
| lock.borrow_mut().write(executor) | |
| } else { | |
| lock.borrow_mut().read(executor) | |
| } | |
| .map_err(BlockerId::Executor); | |
| } | |
| // We couldn't lock all of the accounts, so we are bailing, but | |
| // first we need to set contention, and unlock successful locks | |
| if let Err(e) = result { | |
| for lock in acquired_locks.drain(..) { | |
| let mut lock = lock.borrow_mut(); | |
| lock.unlock(executor); | |
| } | |
| for (i, &acc) in accounts_to_lock { | |
| // We only set contention for write locks, | |
| // in order to prevent writer starvation | |
| if message.is_writable(i) { | |
| self.contend_account(acc, transaction.id); | |
| } | |
| } | |
| return Err(e); | |
| } | |
| acquired_locks.push(lock); | |
| } | |
| // On success, the transaction is no longer blocking anything. | |
| self.transaction_contention.remove(&transaction.id); | |
| for (_, acc) in accounts_to_lock { | |
| self.clear_account_contention(acc, transaction.id); | |
| } | |
| Ok(()) | |
| } | |
| let message = transaction.txn.transaction.message(); | |
| let accounts_to_lock: Vec<_> = message.account_keys().iter().enumerate().collect(); | |
| let acquired_locks = &mut self.acquired_locks[executor as usize]; | |
| for &(i, acc) in &accounts_to_lock { | |
| // Get or create the lock for the account. | |
| let lock = self.locks.entry(acc).or_default().clone(); | |
| // See whether there's a contention for the given account | |
| // if there's one, then we need to follow the breadcrumbs | |
| // (txns->executor) to find out where our transaction | |
| // needs to be queued. | |
| let mut result = | |
| if let Some(contenders) = self.account_contention.get(&acc) { | |
| match contenders.binary_search(&transaction.id) { | |
| // If we are the first contender, then we can proceed | |
| // and try to acquire the needed account locks | |
| // 1. Ok(0) => the transaction has already contended this account, | |
| // and now it is its turn to be scheduled | |
| // 2. Err(0) => the transaction didn't contend the account, but it | |
| // has smaller ID (was received earlier), so we don't | |
| // block it and let it proceed with lock acquisition | |
| Ok(0) | Err(0) => Ok(()), | |
| // If we are not, then we need to get queued after | |
| // the transaction contending right in front of us | |
| Ok(index) | Err(index) => { | |
| Err(BlockerId::Transaction(contenders[index - 1])) | |
| } | |
| } | |
| } else { | |
| Ok(()) | |
| }; | |
| if result.is_ok() { | |
| // Attempt to acquire a write or read lock. | |
| result = if message.is_writable(i) { | |
| lock.borrow_mut().write(executor) | |
| } else { | |
| lock.borrow_mut().read(executor) | |
| } | |
| .map_err(BlockerId::Executor); | |
| } | |
| // We couldn't lock all of the accounts, so we are bailing, but | |
| // first we need to set contention, and unlock successful locks | |
| if let Err(e) = result { | |
| for lock in acquired_locks.drain(..) { | |
| let mut lock = lock.borrow_mut(); | |
| lock.unlock(executor); | |
| } | |
| for &(i, acc) in &accounts_to_lock { | |
| // We only set contention for write locks, | |
| // in order to prevent writer starvation | |
| if message.is_writable(i) { | |
| self.contend_account(acc, transaction.id); | |
| } | |
| } | |
| return Err(e); | |
| } | |
| acquired_locks.push(lock); | |
| } | |
| // On success, the transaction is no longer blocking anything. | |
| self.transaction_contention.remove(&transaction.id); | |
| for (_, acc) in &accounts_to_lock { | |
| self.clear_account_contention(acc, transaction.id); | |
| } | |
| Ok(()) |
🤖 Prompt for AI Agents
In magicblock-processor/src/scheduler/coordinator.rs around lines 153 to 221,
the code relies on cloning the Enumerate iterator (accounts_to_lock.clone()) to
iterate multiple times, which is unusual; collect the enumerated account keys
into a small Vec once (preserving the (index, &Pubkey) pairs), then iterate over
that Vec in the three places currently using accounts_to_lock (the main
acquisition loop, the loop that sets contention on failure, and the final loop
that clears contention), ensuring you keep the index for message.is_writable(i)
checks and adjust types accordingly.
This introduces a new multi-threaded transaction scheduler to the
magicblock-processor, replacing the previous single-threaded implementation. This new design significantly improves performance by processing non-conflicting transactions in parallel.The core of this feature is a central scheduler that dispatches transactions to a pool of worker threads. A lock-free coordination mechanism based on bitmasks is used to manage account access, ensuring that only transactions with non-overlapping write sets are executed concurrently.
Changes in this PR include:
Summary by CodeRabbit
Performance Improvements
Behavior Changes
Tests
Chores