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. |
📝 WalkthroughWalkthroughThis PR refactors the RPC and configuration architecture by reorganizing the aperture module. Key changes include: adding workspace dependencies for agave-geyser-plugin-interface and libloading while removing jsonrpc-related dependencies; moving the listen address and event processor configuration into a new ApertureConfig structure under an Possibly related PRs
Suggested reviewers
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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.
| #[derive(Serialize, Debug, thiserror::Error)] | ||
| pub struct RpcError { | ||
| code: i16, | ||
| message: String, | ||
| } | ||
|
|
||
| impl Display for RpcError { | ||
| fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { | ||
| write!( | ||
| f, | ||
| "RPC Error. Code: {}. Message: {}", | ||
| self.code, self.message | ||
| ) | ||
| write!(f, "Code: {}. Message: {}", self.code, self.message) | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Custom Display impl overrides thiserror generated one — ensure this is intentional.
When deriving thiserror::Error, a Display impl is typically generated from #[error("...")]. Since RpcError has both a manual Display impl and the derive, the manual one takes precedence. If this custom format ("Code: {code}. Message: {message}") is intentional, consider adding an #[error] attribute instead to make it explicit, or remove the manual impl.
-#[derive(Serialize, Debug, thiserror::Error)]
+#[derive(Serialize, Debug)]
pub struct RpcError {
code: i16,
message: String,
}
+
+impl std::error::Error for RpcError {}Or alternatively, use the #[error] attribute and remove the manual Display:
#[derive(Serialize, Debug, thiserror::Error)]
+#[error("Code: {code}. Message: {message}")]
pub struct RpcError {
code: i16,
message: String,
}
-
-impl Display for RpcError {
- fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
- write!(f, "Code: {}. Message: {}", self.code, self.message)
- }
-}📝 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.
| #[derive(Serialize, Debug, thiserror::Error)] | |
| pub struct RpcError { | |
| code: i16, | |
| message: String, | |
| } | |
| impl Display for RpcError { | |
| fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { | |
| write!( | |
| f, | |
| "RPC Error. Code: {}. Message: {}", | |
| self.code, self.message | |
| ) | |
| write!(f, "Code: {}. Message: {}", self.code, self.message) | |
| } | |
| } | |
| #[derive(Serialize, Debug)] | |
| pub struct RpcError { | |
| code: i16, | |
| message: String, | |
| } | |
| impl Display for RpcError { | |
| fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { | |
| write!(f, "Code: {}. Message: {}", self.code, self.message) | |
| } | |
| } | |
| impl std::error::Error for RpcError {} |
| #[derive(Serialize, Debug, thiserror::Error)] | |
| pub struct RpcError { | |
| code: i16, | |
| message: String, | |
| } | |
| impl Display for RpcError { | |
| fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { | |
| write!( | |
| f, | |
| "RPC Error. Code: {}. Message: {}", | |
| self.code, self.message | |
| ) | |
| write!(f, "Code: {}. Message: {}", self.code, self.message) | |
| } | |
| } | |
| #[derive(Serialize, Debug, thiserror::Error)] | |
| #[error("Code: {code}. Message: {message}")] | |
| pub struct RpcError { | |
| code: i16, | |
| message: String, | |
| } |
🤖 Prompt for AI Agents
In magicblock-aperture/src/error.rs around lines 23 to 33, the manual Display
impl for RpcError overrides the Display produced by #[derive(thiserror::Error)];
decide and make it explicit: either remove the manual impl and add an
#[error("Code: {code}. Message: {message}")] attribute on the struct so
thiserror generates the Display, or keep the manual impl but remove the derive
for Display by changing the derive to exclude Display (or keep derive and
document intent with a comment/attribute). Update the file accordingly so only
one Display implementation exists and the chosen format is explicit.
| impl GeyserPluginManager { | ||
| pub(crate) unsafe fn new( | ||
| configs: &[PathBuf], | ||
| ) -> Result<Self, GeyserPluginError> { | ||
| let mut plugins = Vec::with_capacity(configs.len()); | ||
| let mut _libs = Vec::with_capacity(configs.len()); | ||
| for file_path in configs { | ||
| let config = fs::read_to_string(file_path)?; | ||
| let config: Value = json::from_str(&config).map_err(|e| { | ||
| GeyserPluginError::ConfigFileReadError { | ||
| msg: format!( | ||
| "Failed to parse plugin configuration file: {e}" | ||
| ), | ||
| } | ||
| })?; | ||
| let path = config | ||
| .get("path") | ||
| .ok_or(GeyserPluginError::ConfigFileReadError { | ||
| msg: | ||
| "Plugin configuration file doesn't contain `path` field" | ||
| .into(), | ||
| })? | ||
| .as_str() | ||
| .ok_or(GeyserPluginError::ConfigFileReadError { | ||
| msg: | ||
| "The `path` field in the configuration must be a string" | ||
| .into(), | ||
| })?; | ||
| let lib = Library::new(path).map_err(|e| { | ||
| GeyserPluginError::ConfigFileReadError { | ||
| msg: format!( | ||
| "Failed to load plugin shared library object file: {e}" | ||
| ), | ||
| } | ||
| })?; | ||
| let create_plugin: Symbol<PluginCreate> = lib.get(ENTRYPOINT_SYMBOL).map_err(|e| { | ||
| GeyserPluginError::ConfigFileReadError { | ||
| msg: format!( | ||
| "Failed to read entry point symbol from plugin object file: {e}" | ||
| ), | ||
| } | ||
| })?; | ||
| let plugin_raw: *mut dyn GeyserPlugin = create_plugin(); | ||
| let mut plugin: Box<dyn GeyserPlugin> = Box::from_raw(plugin_raw); | ||
| plugin.on_load(&file_path.to_string_lossy(), false)?; | ||
| plugin.notify_end_of_startup()?; | ||
| plugins.push(plugin); | ||
| _libs.push(lib); | ||
| } | ||
| Ok(Self { plugins, _libs }) | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider documenting the safety contract for plugin loading.
The new function is correctly marked unsafe, but the safety requirements could be more explicit in documentation. The caller must ensure:
- Config files point to valid, trusted plugin libraries
- Plugins are ABI-compatible with this validator version
+ /// Creates a new GeyserPluginManager by loading plugins from the provided config files.
+ ///
+ /// # Safety
+ ///
+ /// The caller must ensure that:
+ /// - All plugin configuration files point to valid, trusted shared libraries
+ /// - The loaded plugins are ABI-compatible with the current validator version
+ /// - The plugin libraries implement the `_create_plugin` symbol correctly
pub(crate) unsafe fn new(
configs: &[PathBuf],
) -> Result<Self, GeyserPluginError> {📝 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.
| impl GeyserPluginManager { | |
| pub(crate) unsafe fn new( | |
| configs: &[PathBuf], | |
| ) -> Result<Self, GeyserPluginError> { | |
| let mut plugins = Vec::with_capacity(configs.len()); | |
| let mut _libs = Vec::with_capacity(configs.len()); | |
| for file_path in configs { | |
| let config = fs::read_to_string(file_path)?; | |
| let config: Value = json::from_str(&config).map_err(|e| { | |
| GeyserPluginError::ConfigFileReadError { | |
| msg: format!( | |
| "Failed to parse plugin configuration file: {e}" | |
| ), | |
| } | |
| })?; | |
| let path = config | |
| .get("path") | |
| .ok_or(GeyserPluginError::ConfigFileReadError { | |
| msg: | |
| "Plugin configuration file doesn't contain `path` field" | |
| .into(), | |
| })? | |
| .as_str() | |
| .ok_or(GeyserPluginError::ConfigFileReadError { | |
| msg: | |
| "The `path` field in the configuration must be a string" | |
| .into(), | |
| })?; | |
| let lib = Library::new(path).map_err(|e| { | |
| GeyserPluginError::ConfigFileReadError { | |
| msg: format!( | |
| "Failed to load plugin shared library object file: {e}" | |
| ), | |
| } | |
| })?; | |
| let create_plugin: Symbol<PluginCreate> = lib.get(ENTRYPOINT_SYMBOL).map_err(|e| { | |
| GeyserPluginError::ConfigFileReadError { | |
| msg: format!( | |
| "Failed to read entry point symbol from plugin object file: {e}" | |
| ), | |
| } | |
| })?; | |
| let plugin_raw: *mut dyn GeyserPlugin = create_plugin(); | |
| let mut plugin: Box<dyn GeyserPlugin> = Box::from_raw(plugin_raw); | |
| plugin.on_load(&file_path.to_string_lossy(), false)?; | |
| plugin.notify_end_of_startup()?; | |
| plugins.push(plugin); | |
| _libs.push(lib); | |
| } | |
| Ok(Self { plugins, _libs }) | |
| } | |
| impl GeyserPluginManager { | |
| /// Creates a new GeyserPluginManager by loading plugins from the provided config files. | |
| /// | |
| /// # Safety | |
| /// | |
| /// The caller must ensure that: | |
| /// - All plugin configuration files point to valid, trusted shared libraries | |
| /// - The loaded plugins are ABI-compatible with the current validator version | |
| /// - The plugin libraries implement the `_create_plugin` symbol correctly | |
| pub(crate) unsafe fn new( | |
| configs: &[PathBuf], | |
| ) -> Result<Self, GeyserPluginError> { | |
| let mut plugins = Vec::with_capacity(configs.len()); | |
| let mut _libs = Vec::with_capacity(configs.len()); | |
| for file_path in configs { | |
| let config = fs::read_to_string(file_path)?; | |
| let config: Value = json::from_str(&config).map_err(|e| { | |
| GeyserPluginError::ConfigFileReadError { | |
| msg: format!( | |
| "Failed to parse plugin configuration file: {e}" | |
| ), | |
| } | |
| })?; | |
| let path = config | |
| .get("path") | |
| .ok_or(GeyserPluginError::ConfigFileReadError { | |
| msg: | |
| "Plugin configuration file doesn't contain `path` field" | |
| .into(), | |
| })? | |
| .as_str() | |
| .ok_or(GeyserPluginError::ConfigFileReadError { | |
| msg: | |
| "The `path` field in the configuration must be a string" | |
| .into(), | |
| })?; | |
| let lib = Library::new(path).map_err(|e| { | |
| GeyserPluginError::ConfigFileReadError { | |
| msg: format!( | |
| "Failed to load plugin shared library object file: {e}" | |
| ), | |
| } | |
| })?; | |
| let create_plugin: Symbol<PluginCreate> = lib.get(ENTRYPOINT_SYMBOL).map_err(|e| { | |
| GeyserPluginError::ConfigFileReadError { | |
| msg: format!( | |
| "Failed to read entry point symbol from plugin object file: {e}" | |
| ), | |
| } | |
| })?; | |
| let plugin_raw: *mut dyn GeyserPlugin = create_plugin(); | |
| let mut plugin: Box<dyn GeyserPlugin> = Box::from_raw(plugin_raw); | |
| plugin.on_load(&file_path.to_string_lossy(), false)?; | |
| plugin.notify_end_of_startup()?; | |
| plugins.push(plugin); | |
| _libs.push(lib); | |
| } | |
| Ok(Self { plugins, _libs }) | |
| } |
🤖 Prompt for AI Agents
In magicblock-aperture/src/geyser.rs around lines 21 to 71, add a clear doc
comment on the unsafe fn new describing the safety contract: state that the
caller must ensure config paths point to valid, trusted plugin shared libraries,
that the loaded libraries are ABI-compatible with this validator (matching
symbol signatures and expectations), that the ENTRYPOINT_SYMBOL returns a valid
pointer to a GeyserPlugin instance and that ownership/stack/threading rules for
the plugin instance are satisfied (e.g., Box::from_raw is valid and plugin
methods are safe to call from this context); also document any lifetime or
concurrency constraints and examples of correct caller behavior so future
callers understand when using unsafe is valid.
| let plugin_raw: *mut dyn GeyserPlugin = create_plugin(); | ||
| let mut plugin: Box<dyn GeyserPlugin> = Box::from_raw(plugin_raw); |
There was a problem hiding this comment.
Missing null check before Box::from_raw — potential null pointer dereference.
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.
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);📝 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 plugin_raw: *mut dyn GeyserPlugin = create_plugin(); | |
| let mut plugin: Box<dyn GeyserPlugin> = Box::from_raw(plugin_raw); | |
| 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); |
🤖 Prompt for AI Agents
In magicblock-aperture/src/geyser.rs around lines 63-64, the code calls
Box::from_raw on the pointer returned by create_plugin() without checking for
null; add an explicit null check (if plugin_raw.is_null()) immediately after
create_plugin() and handle the failure path (log an error/return Err or cleanly
abort plugin initialization) instead of calling Box::from_raw on a null pointer,
and only call Box::from_raw when plugin_raw is non-null so you avoid undefined
behavior; ensure any required cleanup or error propagation is performed in the
null branch.
| pub fn notify_slot(&self, slot: u64) -> Result<(), GeyserPluginError> { | ||
| let status = &SlotStatus::Rooted; | ||
| let parent = Some(slot.saturating_sub(1)); | ||
| for plugin in &self.plugins { | ||
| plugin.update_slot_status(slot, parent, status)?; | ||
| } | ||
| Ok(()) |
There was a problem hiding this comment.
Edge case: slot 0 would have itself as parent.
slot.saturating_sub(1) returns 0 when slot == 0, making slot 0 its own parent. Consider using slot.checked_sub(1) to return None for slot 0:
pub fn notify_slot(&self, slot: u64) -> Result<(), GeyserPluginError> {
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(())
}📝 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.
| pub fn notify_slot(&self, slot: u64) -> Result<(), GeyserPluginError> { | |
| let status = &SlotStatus::Rooted; | |
| let parent = Some(slot.saturating_sub(1)); | |
| for plugin in &self.plugins { | |
| plugin.update_slot_status(slot, parent, status)?; | |
| } | |
| Ok(()) | |
| pub fn notify_slot(&self, slot: u64) -> Result<(), GeyserPluginError> { | |
| let status = &SlotStatus::Rooted; | |
| let parent = slot.checked_sub(1); | |
| for plugin in &self.plugins { | |
| plugin.update_slot_status(slot, parent, status)?; | |
| } | |
| Ok(()) | |
| } |
🤖 Prompt for AI Agents
In magicblock-aperture/src/geyser.rs around lines 102 to 108, passing
Some(slot.saturating_sub(1)) makes slot 0 its own parent; change parent to use
slot.checked_sub(1) so it yields None for slot 0 (i.e., let parent =
slot.checked_sub(1);) and pass that Option<u64> to plugin.update_slot_status;
preserve the rest of the loop and error handling.
| pub(crate) fn start( | ||
| config: &ApertureConfig, | ||
| state: &SharedState, | ||
| channels: &DispatchEndpoints, | ||
| instances: usize, | ||
| cancel: CancellationToken, | ||
| ) { | ||
| for id in 0..instances { | ||
| let processor = EventProcessor::new(channels, state); | ||
| ) -> ApertureResult<()> { | ||
| for id in 0..config.event_processors { | ||
| let processor = | ||
| EventProcessor::new(channels, state, &config.geyser_plugins)?; | ||
| tokio::spawn(processor.run(id, cancel.clone())); | ||
| } | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
Each EventProcessor creates its own GeyserPluginManager — plugins loaded multiple times.
The start method creates config.event_processors instances, each calling EventProcessor::new which loads all plugins independently. This means each plugin is loaded and initialized event_processors 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 GeyserPluginManager once and sharing it via Arc:
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(())
}This would require adding a new constructor that accepts an existing Arc<GeyserPluginManager>.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In magicblock-aperture/src/processor.rs around lines 91 to 103, each loop
iteration calls EventProcessor::new which constructs its own GeyserPluginManager
causing plugins to be loaded multiple times; instead, create a single
GeyserPluginManager before the loop, wrap it in an Arc, and pass a cloned Arc
reference into EventProcessor (add a new EventProcessor::with_plugin_manager or
extend the existing constructor to accept Arc<GeyserPluginManager>). Instantiate
the manager once using the same config.geyser_plugins, wrap it in Arc, and
inside the loop call the new constructor with manager.clone() so every spawned
processor shares the same plugin manager and no plugin is initialized more than
once.
| offset: usize, | ||
| ) -> Result<(), ChunksError> { | ||
| if offset % self.chunk_size as usize != 0 { | ||
| if offset % self.chunk_size as usize > 0 { |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Standardize offset validation checks for consistency.
Line 100 uses offset % self.chunk_size as usize > 0 while the identical validation in is_offset_delivered (line 115) uses offset % self.chunk_size as usize != 0. While these are logically equivalent for unsigned integers, using inconsistent forms reduces code clarity and maintainability.
Recommend standardizing on != 0 across both methods, as it's more idiomatic for this type of zero-check pattern in Rust.
pub fn set_offset_delivered(
&mut self,
offset: usize,
) -> Result<(), ChunksError> {
- if offset % self.chunk_size as usize > 0 {
+ if offset % self.chunk_size as usize != 0 {
Err(ChunksError::InvalidOffsetError(offset, self.chunk_size))
} else {
let idx = offset / self.chunk_size as usize;
self.set_chunk_delivered(idx)?;
Ok(())
}
}📝 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.
| if offset % self.chunk_size as usize > 0 { | |
| pub fn set_offset_delivered( | |
| &mut self, | |
| offset: usize, | |
| ) -> Result<(), ChunksError> { | |
| if offset % self.chunk_size as usize != 0 { | |
| Err(ChunksError::InvalidOffsetError(offset, self.chunk_size)) | |
| } else { | |
| let idx = offset / self.chunk_size as usize; | |
| self.set_chunk_delivered(idx)?; | |
| Ok(()) | |
| } | |
| } |
🤖 Prompt for AI Agents
magicblock-committor-program/src/state/chunks.rs around lines 100 and 115: the
offset validation at line 100 uses `offset % self.chunk_size as usize > 0` while
the same check at line 115 uses `offset % self.chunk_size as usize != 0`;
standardize both to use `!= 0` for clarity and idiomatic zero-checking in Rust
by replacing the `> 0` expression at line 100 with `!= 0`.
| /// Aperture-specific configuration. | ||
| pub aperture: ApertureConfig, | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for any remaining references to config.listen that should be config.aperture.listen
rg -n --type rust '\.listen\.' | grep -v 'aperture\.listen' | grep -v 'prepare_lookup_tables' | grep -v '_test\.' | head -30Repository: magicblock-labs/magicblock-validator
Length of output: 528
🏁 Script executed:
#!/bin/bash
# Check the ValidatorParams structure in lib.rs around lines 50-60
sed -n '50,60p' magicblock-config/src/lib.rsRepository: magicblock-labs/magicblock-validator
Length of output: 406
🏁 Script executed:
#!/bin/bash
# Check the context around the found references
echo "=== magicblock-aperture/src/lib.rs:42 ==="
sed -n '38,46p' magicblock-aperture/src/lib.rs
echo -e "\n=== test-integration/test-tools/src/validator.rs:215 ==="
sed -n '211,220p' test-integration/test-tools/src/validator.rs
echo -e "\n=== test-integration/test-tools/src/validator.rs:258 ==="
sed -n '254,263p' test-integration/test-tools/src/validator.rs
echo -e "\n=== test-integration/test-tools/src/toml_to_args.rs:59 ==="
sed -n '55,65p' test-integration/test-tools/src/toml_to_args.rsRepository: magicblock-labs/magicblock-validator
Length of output: 1872
🏁 Script executed:
#!/bin/bash
# Check the function signature and context of toml_to_args.rs
sed -n '50,65p' test-integration/test-tools/src/toml_to_args.rsRepository: magicblock-labs/magicblock-validator
Length of output: 507
Breaking changes must be addressed before merging: update all .listen references to .aperture.listen
The refactoring to group aperture configuration is sound, but the migration is incomplete. Four locations still reference the old config.listen field and will break at runtime:
magicblock-aperture/src/lib.rs:42—config.listen.0should beconfig.aperture.listen.0test-integration/test-tools/src/validator.rs:215, 258—config.listenassignments and property access should useconfig.aperture.listentest-integration/test-tools/src/toml_to_args.rs:59—config.listenshould beconfig.aperture.listen
🤖 Prompt for AI Agents
In magicblock-config/src/lib.rs around lines 53-55 and in the listed call sites,
update all accesses to the moved listen field to use the new nested path:
replace config.listen with config.aperture.listen; specifically change
magicblock-aperture/src/lib.rs line ~42 from config.listen.0 to
config.aperture.listen.0, update both assignments and property accesses in
test-integration/test-tools/src/validator.rs at lines ~215 and ~258 to use
config.aperture.listen, and change
test-integration/test-tools/src/toml_to_args.rs line ~59 from config.listen to
config.aperture.listen so all references point to the new ApertureConfig.listen
field.
| .filter(|acc| { | ||
| if !owner.map_or(true, |owner| acc.owner.eq(&owner)) { | ||
| if !owner.is_none_or(|owner| acc.owner.eq(&owner)) { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Owner filter logic is correct; consider MSRV and readability tweaks
The is_none_or usage is semantically correct and preserves the intended behavior of “no filter when owner is None, exact owner match when Some”. However:
Option::is_none_oris a relatively new std method; ensure your Rust toolchain/MSRV is new enough to support it, otherwise this will break builds.- The closure parameter name
|owner|shadows the outerowner: Option<Pubkey>, which slightly hurts readability.
You might consider one of:
// Keep is_none_or, avoid shadowing
if !owner.is_none_or(|filter_owner| acc.owner == filter_owner) {
return false;
}
// Or fall back to the clearer pattern if MSRV is a concern:
if let Some(filter_owner) = owner {
if acc.owner != filter_owner {
return false;
}
}🤖 Prompt for AI Agents
In tools/ledger-stats/src/accounts.rs around lines 126 to 129, the owner filter
uses Option::is_none_or with a closure that shadows the outer owner variable; to
fix, either rename the closure parameter to avoid shadowing (e.g., filter_owner)
while keeping is_none_or, or replace the is_none_or usage with an explicit if
let Some(filter_owner) = owner check and early-return when acc.owner !=
filter_owner to preserve MSRV compatibility and improve readability.
This PR introduces a mechanism to load and use agave geyser plugins from shared libraries. The implementation simply hooks into aperture event processing and invokes loaded plugins from there.
For now only account and slot updates are supported, with transaction and block updates are scheduled to be added after magicblock-ledger crate has been rewritten.
Summary by CodeRabbit
Release Notes
New Features
Configuration Changes
[aperture]section.geyser-pluginsconfiguration option for plugin paths.Chores
✏️ Tip: You can customize this high-level summary in your review settings.