Feat: add support for geyser plugins #707
Conversation
chore: upgrade rust-toolchain to 1.91.1
Manual Deploy AvailableYou can trigger a manual deploy of this PR branch to testnet: Alternative: Comment
Comment updated automatically when the PR is synchronized. |
📝 WalkthroughWalkthroughAdds geyser plugin support and related workspace dependencies (agave-geyser-plugin-interface, libloading, itertools). Introduces a GeyserPluginManager to load/manage dynamic plugins and integrates plugin notifications into the EventProcessor flow. Adds ApertureConfig (listen, event_processors, geyser_plugins) and moves RPC listen configuration under Suggested reviewers
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🧰 Additional context used🧠 Learnings (3)📚 Learning: 2025-11-24T14:21:00.996ZApplied to files:
📚 Learning: 2025-11-24T08:45:11.164ZApplied to files:
📚 Learning: 2025-10-26T16:54:39.084ZApplied to files:
🔇 Additional comments (3)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
magicblock-committor-service/src/compute_budget.rs (1)
202-207: Consider applying the same refactor for consistency.The
ComputeBudget::total_budgetmethod could benefit from the samesaturating_addrefactor for consistency with the change on line 63.Apply this diff:
fn total_budget(&self, committee_count: u32) -> u32 { - self.per_committee() - .checked_mul(committee_count) - .and_then(|product| product.checked_add(self.base_budget())) - .unwrap_or(u32::MAX) + self.per_committee() + .saturating_mul(committee_count) + .saturating_add(self.base_budget()) }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (28)
Cargo.toml(2 hunks)config.example.toml(1 hunks)magicblock-accounts-db/src/index.rs(1 hunks)magicblock-accounts-db/src/lib.rs(1 hunks)magicblock-aperture/Cargo.toml(3 hunks)magicblock-aperture/src/error.rs(2 hunks)magicblock-aperture/src/geyser.rs(1 hunks)magicblock-aperture/src/lib.rs(4 hunks)magicblock-aperture/src/processor.rs(6 hunks)magicblock-aperture/src/tests.rs(2 hunks)magicblock-aperture/tests/setup.rs(3 hunks)magicblock-api/src/errors.rs(1 hunks)magicblock-api/src/magic_validator.rs(2 hunks)magicblock-committor-program/src/state/chunks.rs(1 hunks)magicblock-committor-service/src/compute_budget.rs(1 hunks)magicblock-committor-service/src/intent_execution_manager/intent_execution_engine.rs(1 hunks)magicblock-committor-service/src/intent_executor/task_info_fetcher.rs(2 hunks)magicblock-config/src/config/aperture.rs(1 hunks)magicblock-config/src/config/cli.rs(2 hunks)magicblock-config/src/config/mod.rs(2 hunks)magicblock-config/src/lib.rs(3 hunks)magicblock-config/src/tests.rs(5 hunks)magicblock-config/src/validator.rs(0 hunks)magicblock-ledger/src/database/db.rs(1 hunks)magicblock-ledger/src/database/rocks_db.rs(5 hunks)magicblock-ledger/src/store/api.rs(2 hunks)magicblock-validator/src/main.rs(1 hunks)tools/ledger-stats/src/accounts.rs(1 hunks)
💤 Files with no reviewable changes (1)
- magicblock-config/src/validator.rs
🧰 Additional context used
🧠 Learnings (8)
📚 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-validator/src/main.rsmagicblock-accounts-db/src/lib.rsmagicblock-config/src/lib.rsmagicblock-api/src/magic_validator.rsmagicblock-aperture/tests/setup.rs
📚 Learning: 2025-11-18T08:47:39.702Z
Learnt from: Dodecahedr0x
Repo: magicblock-labs/magicblock-validator PR: 639
File: magicblock-chainlink/tests/04_redeleg_other_separate_slots.rs:158-165
Timestamp: 2025-11-18T08:47:39.702Z
Learning: In magicblock-chainlink tests involving compressed accounts, `set_remote_slot()` sets the slot of the `AccountSharedData`, while `compressed_account_shared_with_owner_and_slot()` sets the slot of the delegation record. These are two different fields and both calls are necessary.
Applied to files:
magicblock-accounts-db/src/lib.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-accounts-db/src/lib.rs
📚 Learning: 2025-11-21T10:22:07.520Z
Learnt from: taco-paco
Repo: magicblock-labs/magicblock-validator PR: 661
File: magicblock-committor-service/src/intent_executor/single_stage_executor.rs:20-28
Timestamp: 2025-11-21T10:22:07.520Z
Learning: In magicblock-committor-service's SingleStageExecutor and TwoStageExecutor (single_stage_executor.rs and two_stage_executor.rs), the fields transaction_strategy, junk, and patched_errors are intentionally public because these executors are designed to be used independently outside of the IntentExecutor scope, and callers need access to these execution reports for cleanup and error handling.
Applied to files:
magicblock-committor-service/src/intent_executor/task_info_fetcher.rsmagicblock-committor-service/src/intent_execution_manager/intent_execution_engine.rs
📚 Learning: 2025-11-24T14:21:00.996Z
Learnt from: Dodecahedr0x
Repo: magicblock-labs/magicblock-validator PR: 639
File: Cargo.toml:58-58
Timestamp: 2025-11-24T14:21:00.996Z
Learning: In the magicblock-validator codebase, magicblock-api/Cargo.toml intentionally uses borsh = "1.5.3" (instead of the workspace version 0.10.4) because it needs to deserialize types from the magic-domain-program external dependency, which requires borsh 1.5.x compatibility. This is an intentional exception for interoperability with the magic domain program.
Applied to files:
Cargo.tomlmagicblock-aperture/Cargo.toml
📚 Learning: 2025-11-19T09:34:37.917Z
Learnt from: thlorenz
Repo: magicblock-labs/magicblock-validator PR: 621
File: test-integration/test-chainlink/tests/ix_remote_account_provider.rs:62-63
Timestamp: 2025-11-19T09:34:37.917Z
Learning: In test-integration/test-chainlink/tests/ix_remote_account_provider.rs and similar test files, the `_fwd_rx` receiver returned by `init_remote_account_provider()` is intentionally kept alive (but unused) to prevent "receiver dropped" errors on the sender side. The pattern `let (remote_account_provider, _fwd_rx) = init_remote_account_provider().await;` should NOT be changed to `let (remote_account_provider, _) = ...` because dropping the receiver would cause send() operations to fail.
Applied to files:
magicblock-aperture/tests/setup.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-aperture/src/processor.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-committor-service/src/intent_execution_manager/intent_execution_engine.rs
🧬 Code graph analysis (11)
magicblock-ledger/src/database/db.rs (1)
magicblock-ledger/src/database/rocks_db.rs (2)
raw_iterator_cf(209-211)batch(213-215)
magicblock-committor-program/src/state/chunks.rs (1)
magicblock-accounts-db/src/storage.rs (1)
offset(184-190)
tools/ledger-stats/src/accounts.rs (1)
magicblock-committor-program/src/state/changeset.rs (1)
owner(119-124)
magicblock-config/src/lib.rs (1)
magicblock-api/src/magic_validator.rs (1)
config(367-374)
magicblock-api/src/magic_validator.rs (1)
magicblock-aperture/src/lib.rs (1)
initialize_aperture(14-25)
magicblock-ledger/src/database/rocks_db.rs (1)
magicblock-ledger/src/database/db.rs (1)
raw_iterator_cf(105-107)
magicblock-aperture/tests/setup.rs (1)
magicblock-aperture/src/lib.rs (1)
initialize_aperture(14-25)
magicblock-api/src/errors.rs (1)
magicblock-aperture/src/error.rs (5)
from(36-38)from(42-44)from(48-50)from(54-56)from(60-62)
magicblock-aperture/src/tests.rs (1)
magicblock-aperture/src/processor.rs (1)
start(91-103)
magicblock-aperture/src/processor.rs (2)
magicblock-core/src/link.rs (1)
link(60-89)magicblock-aperture/src/geyser.rs (2)
notify_slot(102-109)notify_account(73-100)
magicblock-aperture/src/lib.rs (3)
magicblock-aperture/src/processor.rs (2)
start(91-103)new(59-79)magicblock-aperture/tests/setup.rs (1)
new(86-148)magicblock-aperture/src/server/http/mod.rs (1)
new(40-55)
🔇 Additional comments (31)
magicblock-committor-service/src/intent_execution_manager/intent_execution_engine.rs (1)
363-371: HashSet import correctly matches test usageSwitching the import to
collections::HashSetaligns withreceived_idsusage intest_non_blocking_messagesand removes the implicit HashMap dependency. Looks good and keeps the test code clear and minimal.magicblock-committor-service/src/compute_budget.rs (1)
63-63: Excellent improvement—more idiomatic overflow handling.The change from
checked_add(...).unwrap_or(u32::MAX)tosaturating_add(...)is cleaner and more idiomatic while preserving the same overflow behavior.magicblock-committor-service/src/intent_executor/task_info_fetcher.rs (2)
16-16: LGTM! Safe alternative to unsafe code.Replacing
new_uncheckedwithnew().unwrap()is appropriate here since 5 is a compile-time constant and non-zero. The compiler optimizes this to the same code without runtime overhead.
53-53: LGTM! Safe alternative to unsafe code.Similar to line 16, replacing
new_uncheckedwithnew().unwrap()is safe and correct since 1000 is non-zero. This eliminates unnecessary unsafe code without runtime cost.Note: This change appears unrelated to the PR's stated objective (Geyser plugin support). Consider grouping such cleanup changes in a separate PR for clearer change tracking.
magicblock-accounts-db/src/lib.rs (1)
246-274: Modulo guard change is behavior‑preservingThe rewritten condition
if slot % self.snapshot_frequency != 0 {is equivalent to the previous form given theassert_ne!(snapshot_frequency, 0, ...)above, so snapshot triggering semantics are unchanged and remain correct.magicblock-accounts-db/src/index.rs (1)
360-365: Lifetime‑annotated return type is consistent and clearerReturning
AccountsDbResult<AccountOffsetFinder<'_>>aligns withAccountsReader<'db>usingAccountOffsetFinder<'db>and makes the borrow relationship explicit without changing behavior. This looks correct and type‑sound.magicblock-ledger/src/store/api.rs (1)
210-210: LGTM! Explicit lifetime annotations improve clarity.The addition of explicit
'_lifetime annotations toRwLockReadGuardreturn types correctly documents that the returned guards borrow fromself. This change improves code clarity without affecting behavior, as the same lifetimes were previously inferred through elision.Also applies to: 232-232
magicblock-ledger/src/database/db.rs (1)
105-105: LGTM! Lifetime annotations align with underlying RocksDB API.The explicit
'_lifetime annotations onDBRawIteratorandWriteBatchreturn types correctly document that these types borrow fromself. This aligns with the lifetime annotations in the underlyingrocks_db.rsmethods and maintains API consistency.Also applies to: 109-109
magicblock-ledger/src/database/rocks_db.rs (1)
87-87: LGTM! Explicit lifetime annotations improve safety documentation.The addition of explicit
'_lifetime annotations to RocksDB iterator and slice types (DBPinnableSlice,DBIterator,DBRawIterator) correctly documents that these types borrow from the database instance. This makes the borrowing relationships explicit, which:
- Improves code readability and maintainability
- Helps prevent lifetime-related errors
- Aligns with modern Rust best practices
The changes are purely type-level clarifications and do not affect runtime behavior.
Also applies to: 106-106, 176-176, 196-196, 209-209
config.example.toml (2)
89-108: LGTM! Well-structured configuration section with clear documentation.The new
[aperture]section properly consolidates RPC, event processor, and geyser plugin configuration. Documentation is comprehensive with defaults, CLI flags, and environment variable mappings.
109-111: Terminology update noted: "L2 -> L1" changed to "ER -> BASE".This aligns with the project's naming conventions (Ephemeral Runtime → Base chain).
magicblock-config/src/config/mod.rs (1)
2-2: LGTM! Clean module addition following existing conventions.The new
aperturemodule and itsApertureConfigre-export follow the established pattern in this file.Also applies to: 14-14
magicblock-validator/src/main.rs (1)
95-97: LGTM! Configuration access paths updated correctly.The changes align with the new
ApertureConfigstructure wherelistenis nested underaperture.magicblock-api/src/magic_validator.rs (2)
19-21: LGTM! Import updated for new initialization entrypoint.The import change from
JsonRpcServertoinitialize_aperturereflects the architectural shift to a unified aperture initialization function.
261-267: LGTM! Clean refactoring to use the new aperture initialization entrypoint.The change from direct
JsonRpcServer::new()construction toinitialize_aperture()properly encapsulates:
- Event processor task startup
- Geyser plugin loading
- JSON-RPC server creation
This aligns with the
initialize_aperturefunction signature inmagicblock-aperture/src/lib.rs(lines 13-24).magicblock-aperture/src/tests.rs (1)
53-54: LGTM! Test setup updated to use the new configuration-driven initialization.Using
ApertureConfig::default()is appropriate for unit tests - it provides sensible defaults (1 event processor, no geyser plugins) without requiring external configuration. The test now mirrors the production initialization flow throughEventProcessor::start.Also applies to: 76-78
magicblock-api/src/errors.rs (1)
12-13: LGTM! Error variant correctly updated to use the new umbrella error type.The change from
RpcErrortoApertureErrorproperly reflects the expanded scope of the aperture module, which now handles both RPC and Geyser plugin errors. The#[from]attribute ensures seamless error propagation frominitialize_aperturecalls.magicblock-config/src/lib.rs (1)
4-7: LGTM! Import and re-export changes align with the configuration restructuring.The
ApertureConfigimport and removal ofBindAddressfrom re-exports correctly reflect the new configuration hierarchy.Also applies to: 28-28
Cargo.toml (1)
49-49: agave-geyser-plugin-interface version 2.2 is significantly outdated; use 3.1.3 instead.The latest version of
agave-geyser-plugin-interfaceis 3.1.3, not 2.2. Using an outdated version introduces security risks, misses bug fixes, and may cause compatibility issues. Update toagave-geyser-plugin-interface = { version = "3.1" }or pin to"3.1.3"directly.For
libloading, version 0.8 is acceptable, though the latest 0.8.x patch is 0.8.9.⛔ Skipped due to learnings
Learnt from: Dodecahedr0x Repo: magicblock-labs/magicblock-validator PR: 639 File: Cargo.toml:58-58 Timestamp: 2025-11-24T14:21:00.996Z Learning: In the magicblock-validator codebase, magicblock-api/Cargo.toml intentionally uses borsh = "1.5.3" (instead of the workspace version 0.10.4) because it needs to deserialize types from the magic-domain-program external dependency, which requires borsh 1.5.x compatibility. This is an intentional exception for interoperability with the magic domain program.magicblock-aperture/Cargo.toml (1)
19-19: LGTM! Dependencies align well with Geyser plugin support.The added dependencies are appropriate:
libloadingfor dynamic.soplugin loadingagave-geyser-plugin-interfacefor the official Geyser plugin APIthiserrorfor the new error typesAlso applies to: 42-42, 70-70
magicblock-aperture/tests/setup.rs (2)
96-127: LGTM! Server initialization refactored correctly to use new aperture API.The test setup correctly adapts to the new
initialize_apertureentrypoint andApertureConfigstructure while preserving the port-finding loop for parallel test isolation.
131-137: The WebSocket port convention (RPC port + 1) is correct and follows Solana's established pattern.The pubsub URL binding to
port + 1is verified across the codebase:magicblock-aperture/src/lib.rsexplicitly setsaddr.set_port(addr.port() + 1)before binding the WebSocket listener, and this convention is documented inmagicblock-config/src/types/network.rsas "By solana convention, websocket listens on rpc port + 1". The test code correctly implements this pattern.magicblock-config/src/config/aperture.rs (1)
7-20: Well-structured configuration type for the new aperture functionality.The struct appropriately captures the RPC listen address, parallelism settings, and plugin paths with sensible serde attributes (
deny_unknown_fieldsprevents typos,defaultallows partial configs).magicblock-config/src/tests.rs (1)
69-69: LGTM! Tests properly updated to reflect the new aperture configuration structure.The test coverage correctly validates:
- Default values for
aperture.listenandaperture.event_processors- CLI overlay behavior preserving unset fields
- Environment variable mapping with the
MBV_APERTURE__*prefix- Example config parsing
Also applies to: 175-192, 468-471, 502-504, 551-554
magicblock-aperture/src/error.rs (1)
15-21: LGTM! Clean umbrella error type for the aperture crate.The
ApertureErrorenum correctly aggregates RPC and Geyser plugin errors with automaticFromconversions via#[from].magicblock-config/src/config/cli.rs (1)
62-76: LGTM! Clean CLI config structure for aperture settings.The new
CliApertureConfigstruct follows the established patterns in this file with appropriate use of#[command(flatten)]for CLI integration and consistent serialization attributes.magicblock-aperture/src/geyser.rs (1)
16-19: Good: Field ordering ensures correct drop order.The
pluginsfield is declared before_libs, so plugins are dropped before the libraries are unloaded. This prevents use-after-free when plugin destructors run.magicblock-aperture/src/lib.rs (2)
14-25: LGTM! Clean initialization entrypoint.The
initialize_aperturefunction provides a well-structured public API that properly sequences EventProcessor startup before server creation, with appropriate error propagation.
33-54: Good encapsulation by makingnewprivate.Making
JsonRpcServer::newprivate ensures callers useinitialize_aperture, which properly initializes the EventProcessor first. This prevents misuse where a server could be created without the event processing infrastructure.magicblock-aperture/src/processor.rs (2)
116-136: Reasonable error handling for geyser notifications.Logging errors at
warnlevel and continuing is appropriate since geyser plugin failures shouldn't crash the validator. Consider adding rate limiting for error logs if plugins fail continuously, but this is fine for initial implementation.
59-79: Good safety documentation for the FFI boundary.The SAFETY comment clearly explains the risks and responsibilities around loading Geyser plugins. Based on learnings, this pattern of documenting unsafe code rationale is consistent with the codebase conventions.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
magicblock-committor-program/src/state/chunks.rs (1)
102-102: Inconsistent offset validation remains unresolved.This line continues to use
> 0while the identical validation inis_offset_delivered(line 117) uses!= 0. A previous review already requested standardizing both checks to!= 0for consistency and idiomatic Rust style, but the issue remains unaddressed.🔎 Recommended standardization
- if offset % self.chunk_size as usize > 0 { + if offset % self.chunk_size as usize != 0 { Err(ChunksError::InvalidOffsetError(offset, self.chunk_size))magicblock-aperture/src/geyser.rs (3)
76-77: Missing null check beforeBox::from_raw— potential undefined behavior.If
create_plugin()returns a null pointer (e.g., due to plugin initialization failure),Box::from_raw(plugin_raw)will create a box from null, leading to undefined behavior when dereferenced.🔎 Proposed fix
let plugin_raw: *mut dyn GeyserPlugin = create_plugin(); + if plugin_raw.is_null() { + return Err(GeyserPluginError::ConfigFileReadError { + msg: "Plugin factory returned null pointer".into(), + }); + } let mut plugin: Box<dyn GeyserPlugin> = Box::from_raw(plugin_raw);
115-122: Edge case: slot 0 would have itself as parent.
slot.saturating_sub(1)returns 0 whenslot == 0, making slot 0 its own parent. Useslot.checked_sub(1)to returnNonefor slot 0.🔎 Proposed fix
pub fn notify_slot(&self, slot: u64) -> Result<(), GeyserPluginError> { check_if_enabled!(self); let status = &SlotStatus::Rooted; - let parent = Some(slot.saturating_sub(1)); + let parent = slot.checked_sub(1); for plugin in &self.plugins { plugin.update_slot_status(slot, parent, status)?; } Ok(()) }
153-167:strict_sub(1)will panic ifblock.meta.slotis 0.Unlike
notify_slotwhich usessaturating_sub, this usesstrict_subwhich panics on underflow. If a block with slot 0 is ever processed, this will crash the validator.🔎 Proposed fix
let block = ReplicaBlockInfoV4 { slot: block.meta.slot, - parent_slot: block.meta.slot.strict_sub(1), + parent_slot: block.meta.slot.saturating_sub(1), blockhash: &block.hash.to_string(),magicblock-aperture/src/processor.rs (1)
91-103: Each EventProcessor creates its own GeyserPluginManager — plugins loaded multiple times.The
startmethod spawnsconfig.event_processorsinstances, each callingEventProcessor::newwhich loads all plugins independently. This means each plugin is loaded and initialized multiple times, which:
- May cause resource contention or conflicts in plugins expecting singleton behavior
- Wastes memory by duplicating plugin state
- Could cause issues with plugins that open exclusive resources (files, ports, etc.)
Consider creating the
GeyserPluginManageronce and sharing it viaArc.🔎 Proposed fix
pub(crate) fn start( config: &ApertureConfig, state: &SharedState, channels: &DispatchEndpoints, cancel: CancellationToken, ) -> ApertureResult<()> { + // SAFETY: see documentation on GeyserPluginManager::new + let geyser: Arc<GeyserPluginManager> = + unsafe { GeyserPluginManager::new(&config.geyser_plugins) }?.into(); for id in 0..config.event_processors { - let processor = - EventProcessor::new(channels, state, &config.geyser_plugins)?; + let processor = + EventProcessor::new_with_geyser(channels, state, geyser.clone()); tokio::spawn(processor.run(id, cancel.clone())); } Ok(()) }Add a new constructor that accepts an existing
Arc<GeyserPluginManager>:fn new_with_geyser( channels: &DispatchEndpoints, state: &SharedState, geyser: Arc<GeyserPluginManager>, ) -> Self { Self { subscriptions: state.subscriptions.clone(), transactions: state.transactions.clone(), blocks: state.blocks.clone(), account_update_rx: channels.account_update.clone(), transaction_status_rx: channels.transaction_status.clone(), block_update_rx: channels.block_update.clone(), geyser, } }
🧹 Nitpick comments (1)
magicblock-ledger/src/store/api.rs (1)
1703-1710: Minor: redundant clone in test.The expression
&sanitized_uno.clone()creates an unnecessary clone sincesanitized_unois already owned. Consider using&sanitized_unodirectly.🔎 Suggested simplification
.write_transaction( sig_uno, slot_uno, - &sanitized_uno.clone(), + &sanitized_uno, tx_uno.tx_with_meta.get_status_meta().unwrap(), )Apply similar changes to other test call sites (lines 1733, 2369, 2392).
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (25)
Cargo.tomlconfig.example.tomlmagicblock-aperture/Cargo.tomlmagicblock-aperture/src/encoder.rsmagicblock-aperture/src/geyser.rsmagicblock-aperture/src/processor.rsmagicblock-aperture/src/state/subscriptions.rsmagicblock-aperture/src/tests.rsmagicblock-aperture/tests/setup.rsmagicblock-api/src/errors.rsmagicblock-api/src/magic_validator.rsmagicblock-committor-program/src/state/chunks.rsmagicblock-config/src/config/cli.rsmagicblock-config/src/lib.rsmagicblock-config/src/tests.rsmagicblock-config/src/types/mod.rsmagicblock-core/Cargo.tomlmagicblock-core/src/link/transactions.rsmagicblock-ledger/src/store/api.rsmagicblock-ledger/tests/common.rsmagicblock-processor/src/executor/processing.rsmagicblock-processor/tests/execution.rsmagicblock-processor/tests/fees.rsmagicblock-processor/tests/scheduling.rsmagicblock-validator/src/main.rs
🚧 Files skipped from review as they are similar to previous changes (6)
- magicblock-api/src/magic_validator.rs
- magicblock-config/src/lib.rs
- magicblock-config/src/config/cli.rs
- magicblock-aperture/Cargo.toml
- Cargo.toml
- magicblock-aperture/src/tests.rs
🧰 Additional context used
🧠 Learnings (14)
📚 Learning: 2025-11-24T14:21:00.996Z
Learnt from: Dodecahedr0x
Repo: magicblock-labs/magicblock-validator PR: 639
File: Cargo.toml:58-58
Timestamp: 2025-11-24T14:21:00.996Z
Learning: In the magicblock-validator codebase, magicblock-api/Cargo.toml intentionally uses borsh = "1.5.3" (instead of the workspace version 0.10.4) because it needs to deserialize types from the magic-domain-program external dependency, which requires borsh 1.5.x compatibility. This is an intentional exception for interoperability with the magic domain program.
Applied to files:
magicblock-core/Cargo.toml
📚 Learning: 2025-12-03T09:33:48.707Z
Learnt from: Dodecahedr0x
Repo: magicblock-labs/magicblock-validator PR: 639
File: test-integration/test-committor-service/tests/test_ix_commit_local.rs:867-881
Timestamp: 2025-12-03T09:33:48.707Z
Learning: Repo: magicblock-labs/magicblock-validator PR: 639
Context: test-integration/test-committor-service/tests/test_ix_commit_local.rs (ix_commit_local)
Learning: The PhotonIndexer used for compressed account fetches (get_compressed_account) has built‑in retry logic (defaults to ~10 attempts), so tests should not add separate retry loops around compressed fetches unless there’s a specific need.
Applied to files:
magicblock-processor/tests/execution.rsmagicblock-processor/tests/fees.rsmagicblock-aperture/src/state/subscriptions.rs
📚 Learning: 2025-12-03T09:36:01.527Z
Learnt from: Dodecahedr0x
Repo: magicblock-labs/magicblock-validator PR: 639
File: magicblock-chainlink/src/remote_account_provider/mod.rs:1350-1353
Timestamp: 2025-12-03T09:36:01.527Z
Learning: Repo: magicblock-labs/magicblock-validator
File: magicblock-chainlink/src/remote_account_provider/mod.rs
Context: consolidate_fetched_remote_accounts
Learning: For unexpected result counts (>2), the project prefers logging an error and returning an empty Vec over panicking; acceptable during development per maintainer (Dodecahedr0x).
Applied to files:
magicblock-processor/tests/execution.rsmagicblock-config/src/types/mod.rsmagicblock-api/src/errors.rsmagicblock-aperture/src/state/subscriptions.rsmagicblock-aperture/tests/setup.rsmagicblock-aperture/src/processor.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/execution.rsmagicblock-processor/tests/fees.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/tests/execution.rsmagicblock-processor/tests/fees.rsmagicblock-processor/tests/scheduling.rs
📚 Learning: 2025-11-21T10:22:07.520Z
Learnt from: taco-paco
Repo: magicblock-labs/magicblock-validator PR: 661
File: magicblock-committor-service/src/intent_executor/single_stage_executor.rs:20-28
Timestamp: 2025-11-21T10:22:07.520Z
Learning: In magicblock-committor-service's SingleStageExecutor and TwoStageExecutor (single_stage_executor.rs and two_stage_executor.rs), the fields transaction_strategy, junk, and patched_errors are intentionally public because these executors are designed to be used independently outside of the IntentExecutor scope, and callers need access to these execution reports for cleanup and error handling.
Applied to files:
magicblock-aperture/src/encoder.rsmagicblock-aperture/src/state/subscriptions.rsmagicblock-processor/src/executor/processing.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-aperture/src/encoder.rsmagicblock-validator/src/main.rsmagicblock-aperture/src/state/subscriptions.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-aperture/src/state/subscriptions.rsmagicblock-aperture/src/processor.rs
📚 Learning: 2025-12-01T16:02:05.367Z
Learnt from: thlorenz
Repo: magicblock-labs/magicblock-validator PR: 703
File: magicblock-chainlink/src/submux/mod.rs:652-654
Timestamp: 2025-12-01T16:02:05.367Z
Learning: In magicblock-chainlink/src/submux/mod.rs, the subscribe_program method intentionally adds program_id to program_subs before attempting the subscription. This ensures that even if the initial subscription fails or only partially succeeds across clients, the reconnection logic will retry the subscription. This is a deliberate design pattern for resilience in the multi-client architecture and should not be "fixed" to remove entries on failure.
Applied to files:
magicblock-aperture/src/state/subscriptions.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-aperture/src/state/subscriptions.rs
📚 Learning: 2025-11-18T08:47:39.702Z
Learnt from: Dodecahedr0x
Repo: magicblock-labs/magicblock-validator PR: 639
File: magicblock-chainlink/tests/04_redeleg_other_separate_slots.rs:158-165
Timestamp: 2025-11-18T08:47:39.702Z
Learning: In magicblock-chainlink tests involving compressed accounts, `set_remote_slot()` sets the slot of the `AccountSharedData`, while `compressed_account_shared_with_owner_and_slot()` sets the slot of the delegation record. These are two different fields and both calls are necessary.
Applied to files:
magicblock-aperture/src/state/subscriptions.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-aperture/src/state/subscriptions.rsmagicblock-aperture/src/processor.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/geyser.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-ledger/src/store/api.rsmagicblock-processor/src/executor/processing.rs
🧬 Code graph analysis (8)
magicblock-processor/tests/execution.rs (1)
tools/ledger-stats/src/utils.rs (1)
logs(6-16)
magicblock-processor/tests/fees.rs (1)
magicblock-committor-service/src/intent_executor/task_info_fetcher.rs (1)
signature(285-291)
magicblock-committor-program/src/state/chunks.rs (1)
magicblock-accounts-db/src/storage.rs (1)
offset(184-190)
magicblock-validator/src/main.rs (1)
magicblock-api/src/magic_validator.rs (1)
config(373-379)
magicblock-aperture/src/geyser.rs (2)
magicblock-core/src/link.rs (1)
link(60-89)magicblock-aperture/src/processor.rs (1)
new(59-79)
magicblock-aperture/tests/setup.rs (1)
magicblock-aperture/src/lib.rs (1)
initialize_aperture(14-25)
magicblock-aperture/src/processor.rs (1)
magicblock-aperture/src/geyser.rs (5)
new(35-84)notify_slot(115-123)notify_block(148-173)notify_account(86-113)notify_transaction(125-146)
magicblock-processor/src/executor/processing.rs (3)
magicblock-committor-service/src/intent_executor/task_info_fetcher.rs (1)
signature(285-291)test-integration/test-tools/src/integration_test_context.rs (1)
signature(66-68)magicblock-rpc-client/src/lib.rs (1)
error(201-203)
⏰ 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). (4)
- GitHub Check: run_make_ci_format
- GitHub Check: Build Project
- GitHub Check: run_make_ci_lint
- GitHub Check: run_make_ci_test
🔇 Additional comments (42)
magicblock-config/src/types/mod.rs (1)
9-9: LGTM!The expanded re-export of
RemotealongsideBindAddressaligns with the configuration restructuring in this PR, whereRemoteis now used more extensively for networking configuration.magicblock-validator/src/main.rs (1)
78-80: LGTM!The RPC host and port are now correctly sourced from the new
config.aperture.listenpath, consistent with the configuration restructuring in this PR.config.example.toml (2)
94-112: LGTM!The new
[aperture]section is well-documented with clear defaults, CLI arguments, and environment variable mappings. Thegeyser-pluginsfield appropriately omits CLI/Env mappings since complex list configurations are best handled via TOML.
114-116: Terminology update noted.The comment header update from "L2 -> L1" to "ER -> BASE" reflects the project's naming conventions.
magicblock-config/src/tests.rs (6)
68-68: LGTM!The default port assertion correctly uses the new
aperture.listenpath and verifies the expected default of 8899.
174-191: LGTM!Excellent test coverage for the non-destructive overlay behavior. The test properly verifies that:
- CLI-specified fields (
basefee,listen) are updated- TOML-specified fields (
keypair) are preserved- Default fields (
event_processors) remain at their defaults
374-374: LGTM!The assertion correctly validates that the example config's aperture listen port matches the documented value.
429-435: LGTM!Good addition of dedicated aperture section validation in the full coverage test, ensuring the example config properly sets the new configuration fields.
464-466: LGTM!The environment variable guards follow the established naming convention and properly test the new aperture configuration paths.
514-517: LGTM!The assertions correctly validate that the aperture configuration fields are properly populated from environment variables.
magicblock-api/src/errors.rs (1)
12-13: LGTM!The migration from
RpcErrorto the broaderApertureErrorumbrella type properly consolidates RPC and Geyser error handling under a unified variant. All references to the oldApiError::RpcErrorvariant have been successfully updated.magicblock-core/Cargo.toml (1)
24-24: LGTM!The new dependency is correctly added using workspace version and aligns with the
TransactionStatusMetausage inmagicblock-core/src/link/transactions.rsfor richer transaction status handling.magicblock-aperture/src/encoder.rs (2)
150-157: LGTM!The updated data access patterns correctly align with the new
TransactionStatusstructure, properly extracting logs frommeta.log_messagesand account keys fromtxn.message().account_keys().
167-172: LGTM!The signature and error extraction logic correctly adapts to the new
TransactionStatusstructure, properly dereferencing the signature and converting the transaction error to a string representation.magicblock-core/src/link/transactions.rs (2)
12-12: LGTM!Correctly imports
TransactionStatusMetafrom the standard Solana client types crate, ensuring compatibility with the Agave Geyser plugin interface.
59-65: Clean refactor ofTransactionStatusfor Geyser plugin support.The new structure provides the complete data needed by Geyser plugins: the full sanitized transaction (with signature accessible via
txn.signature()), standard metadata, and block-level indexing. This aligns well with Solana's notification patterns.magicblock-ledger/tests/common.rs (1)
34-36: LGTM!Correctly updated to pass the transaction by reference, aligning with the updated
write_transactionAPI signature. The returned index is appropriately ignored in this test helper since it's not needed.magicblock-processor/tests/fees.rs (4)
202-205: LGTM!Test correctly updated to access the transaction status through the new
status.meta.statuspath.
246-249: LGTM!Consistent with the updated access pattern for checking transaction failure status.
290-294: LGTM!Correctly updated to access the signature via
status.txn.signature()with proper dereferencing, and status viastatus.meta.status.
340-344: LGTM!Consistent with the other gasless mode test, correctly accessing signature and status through the new
TransactionStatusstructure.magicblock-processor/tests/scheduling.rs (2)
121-135: LGTM!The updates correctly adapt to the new
TransactionStatusstructure, usingstatus.txn.signature()andstatus.meta.statusinstead of the previous direct fields. The borrow pattern (using&Signaturefor set removal) is appropriate.
173-182: LGTM!The dereferencing of
status.txn.signature()to obtain an ownedSignaturefor the vector is correct. The status check viastatus.meta.statusis consistent with the refactored structure.magicblock-processor/tests/execution.rs (3)
14-14: LGTM!The
SVMMessageimport is correctly added to support callingaccount_keys()on the transaction, which is a method from this trait thatSanitizedTransactionimplements.
94-102: LGTM!The assertions are correctly updated to use the new field paths:
*status.txn.signature()for signature comparisonstatus.meta.log_messagesfor accessing transaction logs
126-143: LGTM!The iteration over transaction account keys is correctly updated to use
status.txn.account_keys().iter(), leveraging theSVMMessagetrait. The skip/take pattern remains unchanged, maintaining the original test logic.magicblock-aperture/src/state/subscriptions.rs (2)
11-14: LGTM!The import is appropriately simplified to bring in
TransactionStatusdirectly, which is now used as the primary type for transaction status updates.
191-202: LGTM!The refactored
send_signature_updatemethod correctly:
- Takes a single
&TransactionStatusparameter instead of separate signature and update objects- Extracts the signature via
update.txn.signature()for the subscription lookup- Sends the notification with
update.meta.status(the transaction result) andupdate.slotThis aligns well with the new
TransactionStatusstructure containingtxn,meta,index, andslotfields.magicblock-aperture/tests/setup.rs (3)
16-24: LGTM!The imports are correctly updated to include
initialize_aperturefrom the aperture crate andApertureConfigfrom the config crate, aligning with the new initialization API.
99-130: LGTM!The test setup correctly adapts to the new aperture initialization flow:
- Creates
ApertureConfigwith the listen address using struct update syntax for defaults- Uses
initialize_aperturewhich handles both event processor and server startup- Preserves the port-retry loop for test isolation in parallel test runs
134-140: WebSocket port offset convention is correct.The test setup properly uses
port + 1for the WebSocket endpoint, matching the server's actual binding behavior inJsonRpcServer::new()whereaddr.set_port(addr.port() + 1)is called before binding the WebSocket listener.magicblock-ledger/src/store/api.rs (2)
861-888: LGTM!The
write_transactionsignature improvements are well-designed:
- Taking
&SanitizedTransactionavoids unnecessary cloning at call sites- Returning
u32(the transaction slot index) enables callers to properly constructTransactionStatuswith the correct indexThe implementation correctly propagates the index from
write_transaction_status.
991-1043: LGTM!The
write_transaction_statusmethod now correctly returns the computedtransaction_slot_index. This enables the caller (write_transaction) to surface this value to higher-level code for inclusion inTransactionStatus.magicblock-aperture/src/geyser.rs (2)
86-113: LGTM!The
notify_accountmethod correctly constructsReplicaAccountInfoV3, checksaccount_data_notifications_enabled()before notifying each plugin, and handles the conversion properly. The TODO comment acknowledges the architectural limitation with transaction syncing.
125-146: LGTM!The
notify_transactionmethod correctly buildsReplicaTransactionInfoV2fromTransactionStatus, checkstransaction_notifications_enabled()per plugin, and properly wraps in versioned types before calling the plugin interface.magicblock-aperture/src/processor.rs (3)
116-126: LGTM!The geyser notifications for slot and block updates are correctly integrated. Notifying the slot before the block is logical ordering. Error handling via
inspect_errwith warning logs appropriately prevents plugin failures from blocking core event processing.
134-137: LGTM!Geyser account notifications are correctly placed after subscription notifications, maintaining the existing notification flow while adding plugin support.
148-158: LGTM!The transaction notification integration correctly forwards the status to geyser plugins before updating the local cache, which ensures plugins receive data promptly. The updated
SignatureResultconstruction usingstatus.meta.statusand*status.txn.signature()aligns with the newTransactionStatusstructure.magicblock-processor/src/executor/processing.rs (4)
6-7: LGTM!Import reorganization correctly reflects the new types used:
TransactionSimulationResultreplaces the previous result type, andTxnSimulationResultTxis retained for simulation channel communication.
223-244: LGTM!The refactored
commit_transactionmethod correctly:
- Handles ledger write errors early with appropriate logging
- Constructs
TransactionStatuswith the new structure (slot,index,txn,meta)- Sends the status only after successful ledger write
The TODO on
meta.clone()is acknowledged and appropriately deferred to the ledger rewrite.
260-268: LGTM!The signature change to pass
&txninstead of consumingtxnis consistent withcommit_transactionand aligns with the updated ledger API.
131-146: LGTM!The
TransactionSimulationResultconstruction correctly extracts all execution details (result, units consumed, logs, return data, inner instructions) for both successful and failed paths, providing the complete data needed forsimulateTransactionRPC responses.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
config.example.toml (2)
103-104: Minor grammar improvement for clarity.Consider changing "Greater number" to "A greater number" for better readability.
🔎 Suggested improvement
-# Greater number might improve the performance of websocket +# A greater number might improve the performance of websocket
94-113: Consider adding a brief explanation of the aperture section.The new
[aperture]section is introduced without explaining what "aperture" represents in the system architecture. Adding a brief comment describing its purpose (e.g., "Main JSON RPC and event processing configuration") would help users understand the role of this configuration section.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.locktest-integration/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
Cargo.tomlconfig.example.tomlmagicblock-aperture/src/tests.rsmagicblock-api/src/magic_validator.rsmagicblock-config/src/tests.rsmagicblock-validator/src/main.rstest-integration/test-tools/src/validator.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- magicblock-validator/src/main.rs
🧰 Additional context used
🧠 Learnings (3)
📚 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-api/src/magic_validator.rstest-integration/test-tools/src/validator.rs
📚 Learning: 2025-12-03T09:36:01.527Z
Learnt from: Dodecahedr0x
Repo: magicblock-labs/magicblock-validator PR: 639
File: magicblock-chainlink/src/remote_account_provider/mod.rs:1350-1353
Timestamp: 2025-12-03T09:36:01.527Z
Learning: Repo: magicblock-labs/magicblock-validator
File: magicblock-chainlink/src/remote_account_provider/mod.rs
Context: consolidate_fetched_remote_accounts
Learning: For unexpected result counts (>2), the project prefers logging an error and returning an empty Vec over panicking; acceptable during development per maintainer (Dodecahedr0x).
Applied to files:
magicblock-api/src/magic_validator.rs
📚 Learning: 2025-11-19T12:55:48.931Z
Learnt from: Dodecahedr0x
Repo: magicblock-labs/magicblock-validator PR: 639
File: test-integration/test-tools/src/validator.rs:193-197
Timestamp: 2025-11-19T12:55:48.931Z
Learning: In the magicblock-validator codebase, when constructing arguments for the light CLI test validator in start_light_validator_with_config (test-integration/test-tools/src/validator.rs), the shlex::split approach is necessary because the light CLI does not handle whitespaces properly in validator arguments. The string concatenation + shlex parsing pattern should not be refactored to direct argument construction in this specific case.
Applied to files:
test-integration/test-tools/src/validator.rs
🧬 Code graph analysis (2)
magicblock-api/src/magic_validator.rs (1)
magicblock-aperture/src/lib.rs (1)
initialize_aperture(14-25)
magicblock-aperture/src/tests.rs (1)
magicblock-aperture/src/processor.rs (1)
start(91-103)
⏰ 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 (11)
magicblock-api/src/magic_validator.rs (2)
20-20: LGTM: Import updated correctly.The import change from
JsonRpcServertoinitialize_aperturecorrectly reflects the new initialization pattern introduced in this PR for Geyser plugin support.
266-272: LGTM: Aperture initialization correctly implemented.The refactoring from direct
JsonRpcServerinstantiation toinitialize_apertureis correctly implemented:
- Async/await pattern is properly used
- Error propagation with
?is appropriate- All parameters (
config.aperture,shared_state,&dispatch,token.clone()) are correctly passed- The change integrates
EventProcessorstartup as part of the initialization flow, supporting the Geyser plugin feature introduced in this PRmagicblock-config/src/tests.rs (5)
68-68: LGTM!The migration from
config.listentoconfig.aperture.listencorrectly reflects the new configuration structure.
174-188: LGTM!The test correctly validates non-destructive overlay behavior by ensuring that:
- CLI arguments (basefee, listen) are applied
- Fields not mentioned in CLI (keypair) are preserved from TOML
374-374: LGTM!Accessor correctly updated to use the new aperture configuration path.
429-432: LGTM!The new test section appropriately verifies the aperture configuration from the example config file, checking both listen address and event_processors count.
468-470: LGTM!Environment variable setup and corresponding assertions correctly test the aperture configuration through environment variables. The naming convention (
MBV_APERTURE__LISTEN,MBV_APERTURE__EVENT_PROCESSORS) follows the established pattern.Also applies to: 519-522
magicblock-aperture/src/tests.rs (1)
55-57: LGTM!The import updates correctly reflect the new configuration-based API. The constant rename to
DEFAULT_LEDGER_BLOCK_TIME_MSimproves clarity.test-integration/test-tools/src/validator.rs (2)
208-209: LGTM!The refactoring correctly updates the field path from
config.listentoconfig.aperture.listen, aligning with the newApertureConfigstructure. The logic is preserved: reading the IP from the config and constructing a newBindAddresswith a randomly assigned RPC port.
250-251: LGTM! Refactoring is consistent and complete.The change at lines 250-251 correctly mirrors the earlier refactoring at lines 208-209, updating
config.aperture.listenidentically in both functions. Both occurrences follow the same pattern, confirming thorough and consistent application of theValidatorParamsrefactoring.The remaining
config.listenreferences in the codebase (intoml_to_args.rsandmagicblock-aperture/src/lib.rs) are in different config types (ConfigandApertureConfig) and do not require updating, as the refactoring applies only toValidatorParams.Cargo.toml (1)
49-49: All three dependencies are secure with no known advisories. The version constraints are intentionally aligned with the Solana 2.2.x ecosystem:agave-geyser-plugin-interface,solana-*, and related packages all target "2.2". The pinned versions in Cargo.lock (agave-geyser-plugin-interface@2.2.1,itertools@0.14.0,libloading@0.8.9) are compatible and verified against crates.io.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test-integration/test-tools/src/toml_to_args.rs (1)
50-55: Consider extracting duplicated port-parsing logic.This listen/port extraction logic is duplicated in
rpc_port_from_config(lines 103-108). Consider extracting a helper function to reduce duplication.🔎 Proposed refactor
+fn extract_port_from_listen(listen: &str) -> &str { + listen.split(':').nth(1).unwrap_or("8899") +} + pub fn config_to_args( config_path: &PathBuf, program_loader: Option<ProgramLoader>, ) -> Vec<String> { let config = parse_config(config_path); let program_loader = program_loader.unwrap_or_default(); let listen = config .aperture .as_ref() .map(|a| a.listen.as_str()) .unwrap_or("127.0.0.1:8899"); - let port = listen.split(':').nth(1).unwrap_or("8899"); + let port = extract_port_from_listen(listen);
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
test-integration/test-tools/src/toml_to_args.rs
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-03T09:36:01.527Z
Learnt from: Dodecahedr0x
Repo: magicblock-labs/magicblock-validator PR: 639
File: magicblock-chainlink/src/remote_account_provider/mod.rs:1350-1353
Timestamp: 2025-12-03T09:36:01.527Z
Learning: Repo: magicblock-labs/magicblock-validator
File: magicblock-chainlink/src/remote_account_provider/mod.rs
Context: consolidate_fetched_remote_accounts
Learning: For unexpected result counts (>2), the project prefers logging an error and returning an empty Vec over panicking; acceptable during development per maintainer (Dodecahedr0x).
Applied to files:
test-integration/test-tools/src/toml_to_args.rs
🪛 GitHub Actions: Run CI - Format
test-integration/test-tools/src/toml_to_args.rs
[error] 90-90: Rust fmt check failed (ci-fmt). The diff indicates formatting changes are required in the file. Run 'cargo +nightly fmt --check -- --config-path ../rustfmt-nightly.toml' to see diffs or 'cargo +nightly fmt -- --config-path ../rustfmt-nightly.toml' to fix.
[warning] 1-1: Note: The failure originated from a formatting check during 'ci-fmt'. Consider enabling automatic formatting on save or pre-commit.
⏰ 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 (3)
test-integration/test-tools/src/toml_to_args.rs (3)
8-16: LGTM!The Config struct changes correctly implement the aperture-based configuration model with appropriate defaults for backward compatibility.
18-21: LGTM!The
ApertureConfigstruct is appropriately minimal for parsing the aperture configuration section.
101-113: LGTM!The port extraction logic correctly handles missing or malformed listen addresses with appropriate defaults.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
magicblock-aperture/src/geyser.rs (1)
49-61: Error messages refer to wrong field name.The code retrieves the
"libpath"field (line 50) but error messages at lines 53 and 59 refer to"path". This inconsistency will confuse operators debugging configuration issues.🔎 Proposed fix
let path = config .get("libpath") .ok_or(GeyserPluginError::ConfigFileReadError { - msg: - "Plugin configuration file doesn't contain `path` field" - .into(), + msg: "Plugin configuration file doesn't contain `libpath` field" + .into(), })? .as_str() .ok_or(GeyserPluginError::ConfigFileReadError { - msg: - "The `path` field in the configuration must be a string" - .into(), + msg: "The `libpath` field in the configuration must be a string" + .into(), })?;
🧹 Nitpick comments (1)
test-integration/test-tools/src/toml_to_args.rs (1)
18-21: Consider adding a default for thelistenfield.If a user provides an
aperturesection in the TOML config but omits thelistenfield, deserialization will fail. Adding#[serde(default)]with a default value would improve the configuration experience.🔎 Proposed enhancement
#[derive(Deserialize)] struct ApertureConfig { + #[serde(default = "default_listen")] listen: String, } + +fn default_listen() -> String { + "127.0.0.1:8899".to_string() +}
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.locktest-integration/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
Cargo.tomlmagicblock-aperture/src/geyser.rsmagicblock-aperture/src/processor.rsmagicblock-api/src/magic_validator.rsmagicblock-config/src/config/mod.rsmagicblock-ledger/src/store/api.rstest-integration/test-tools/src/toml_to_args.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- magicblock-config/src/config/mod.rs
- magicblock-ledger/src/store/api.rs
🧰 Additional context used
🧠 Learnings (6)
📚 Learning: 2025-10-26T16:53:29.820Z
Learnt from: thlorenz
Repo: magicblock-labs/magicblock-validator PR: 587
File: magicblock-chainlink/src/remote_account_provider/mod.rs:134-0
Timestamp: 2025-10-26T16:53:29.820Z
Learning: In magicblock-chainlink/src/remote_account_provider/mod.rs, the `Endpoint::separate_pubsub_url_and_api_key()` method uses `split_once("?api-key=")` because the api-key parameter is always the only query parameter right after `?`. No additional query parameter parsing is needed for this use case.
Applied to files:
test-integration/test-tools/src/toml_to_args.rs
📚 Learning: 2025-12-03T09:36:01.527Z
Learnt from: Dodecahedr0x
Repo: magicblock-labs/magicblock-validator PR: 639
File: magicblock-chainlink/src/remote_account_provider/mod.rs:1350-1353
Timestamp: 2025-12-03T09:36:01.527Z
Learning: Repo: magicblock-labs/magicblock-validator
File: magicblock-chainlink/src/remote_account_provider/mod.rs
Context: consolidate_fetched_remote_accounts
Learning: For unexpected result counts (>2), the project prefers logging an error and returning an empty Vec over panicking; acceptable during development per maintainer (Dodecahedr0x).
Applied to files:
test-integration/test-tools/src/toml_to_args.rsmagicblock-api/src/magic_validator.rsmagicblock-aperture/src/processor.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/geyser.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-api/src/magic_validator.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-aperture/src/processor.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-aperture/src/processor.rs
🧬 Code graph analysis (1)
magicblock-aperture/src/processor.rs (1)
magicblock-aperture/src/geyser.rs (5)
new(35-89)notify_slot(120-128)notify_block(153-178)notify_account(91-118)notify_transaction(130-151)
🔇 Additional comments (8)
magicblock-api/src/magic_validator.rs (2)
20-20: LGTM! Import change aligns with aperture-based initialization.The import change from
JsonRpcServertoinitialize_aperturecorrectly supports the new aperture-based RPC initialization pattern introduced in this PR.
268-274: LGTM! Aperture-based initialization correctly implemented.The refactoring from
JsonRpcServer::new(config.listen)toinitialize_aperture(&config.aperture, ...)is well-integrated:
- Async/await is correctly used within the existing async context
- All parameters (
config.aperture,shared_state,&dispatch,token) are properly scoped- Error propagation with
?operator is appropriate- The returned
rpcinstance is used consistently with the previous patternThis change successfully introduces the aperture-based initialization path that supports geyser plugins.
test-integration/test-tools/src/toml_to_args.rs (4)
54-59: LGTM!The logic correctly derives the listen address from
aperture.listenwith a sensible default, and the port extraction is properly applied.
96-102: LGTM!The logic correctly identifies and extracts the first HTTP/HTTPS remote URL using
starts_with("http"), which appropriately covers both protocols. The past review comment about formatting issues appears to have been addressed—the current code is properly formatted.
107-115: LGTM!The port extraction logic is correct and includes appropriate fallbacks for both missing configuration (
unwrap_or("127.0.0.1:8899")) and parsing failures (unwrap_or(8899)).
43-45: No changes needed. The function correctly handles the actual address format used throughout the codebase, which is exclusively IPv4 (e.g.,127.0.0.1:8899). IPv6 addresses are not present in any configuration, tests, or utilities in the repository, so this is not a current concern.magicblock-aperture/src/processor.rs (2)
85-104: LGTM! Singleton pattern correctly implemented.The geyser plugin manager is now created once before the loop (lines 96-97) and shared via
Arc::cloneacross all processor instances (line 99). This addresses the previous concern about plugins being loaded multiple times and ensures proper resource sharing.The safety documentation (lines 91-95) appropriately warns operators about FFI compatibility requirements.
106-169: LGTM! Geyser integration follows appropriate error handling pattern.The event loop correctly integrates geyser notifications for all event types (slot/block, accounts, transactions) while using
inspect_errto log failures without stopping event processing. This ensures plugin errors don't interrupt the core event dispatch functionality.
Summary
Introduces comprehensive Geyser plugin support to the Aperture RPC server, enabling real-time streaming of on-chain events to external plugins via the standard Solana Geyser plugin interface. This implementation dynamically loads Geyser plugins from shared libraries and notifies them of:
Additionally, centralizes RPC and WebSocket configuration under a new
aperturesection in the config, allowing RPC binding, event processor settings, and Geyser plugin configurations to be managed together.Fixes #424
Compatibility
listentoaperture.listen; newgeyser-pluginsandevent-processorsconfiguration options added underaperture)Testing
Checklist