fix: disable Chainlink for replicas#1238
Conversation
Amp-Thread-ID: https://ampcode.com/threads/T-019e4e54-d4d3-7100-b5c4-0dfd04062e67 Co-authored-by: Amp <amp@ampcode.com>
Amp-Thread-ID: https://ampcode.com/threads/T-019e4e56-716b-7509-a8c6-4bb779631fce Co-authored-by: Amp <amp@ampcode.com>
Amp-Thread-ID: https://ampcode.com/threads/T-019e4e58-89d1-7259-889e-776f73bbc9ef Co-authored-by: Amp <amp@ampcode.com>
Amp-Thread-ID: https://ampcode.com/threads/T-019e4e5a-3144-736e-9142-6ab0468f58df Co-authored-by: Amp <amp@ampcode.com>
Amp-Thread-ID: https://ampcode.com/threads/T-019e4e5b-d14b-711f-8f25-43a48585028c Co-authored-by: Amp <amp@ampcode.com>
Amp-Thread-ID: https://ampcode.com/threads/T-019e4e5d-7b9a-7004-a6d1-283c7008204f Co-authored-by: Amp <amp@ampcode.com>
|
Caution Review failedFailed to post review comments 📝 WalkthroughWalkthroughThis PR makes chainlink replication-mode aware by adding ReplicationModeAwareChainlink (enabled/disabled variants), introduces production type aliases (ProdInnerChainlink/ProdChainlink), and a new AccountsBankResetter trait. RPC handlers are gated by CoordinationMode (require_primary_rpc_method/needs_onchain_interactions) to reject primary-only operations in replica mode. The replicator and related contexts are generalized to accept an AccountsBankResetter and invoke reset during role transitions. Tests and test setups are updated to construct InnerChainlink and wrap it as enabled where appropriate. Assessment against linked issues
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 |
Amp-Thread-ID: https://ampcode.com/threads/T-019e4e6a-2ea0-7699-8450-7cdd096250b8 Co-authored-by: Amp <amp@ampcode.com>
Amp-Thread-ID: https://ampcode.com/threads/T-019e4e6b-7d58-7651-9236-cfe16954f89b Co-authored-by: Amp <amp@ampcode.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
magicblock-api/src/magic_validator.rs (2)
236-246:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReplace the startup
.expect()with a typed error.Line 238 will panic the validator if
replication_messageswas already taken or not initialized. Please return anApiErrorhere instead of aborting startup.As per coding guidelines, "Treat any usage of
.unwrap()or.expect()in production Rust code as a MAJOR issue."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@magicblock-api/src/magic_validator.rs` around lines 236 - 246, The code currently calls dispatch.replication_messages.take().expect(...) which can panic; replace this .expect() with a typed ApiError return so startup fails gracefully. Update the block constructing ReplicationService (where you access dispatch.replication_messages and call ReplicationService::new) to call dispatch.replication_messages.take().ok_or_else(|| ApiError::new("...")) or map_err to convert the None case into an ApiError and propagate it (using ?), ensuring the enclosing function's signature returns Result<..., ApiError> so the error is returned instead of panicking. Ensure the ApiError includes a clear message like "replication channel missing or already taken" and adjust any call sites to handle the propagated error.
216-218:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftReplica mode still eagerly boots the commit pipeline.
init_committor_servicestill always startsCommittorService, andscheduled_commits_processoris still built from it, soReplicationMode::Replicakeeps the primary-only commit path alive even though this PR disables Chainlink/RPC on replicas. Please gate both on the same replica-only predicate used for Chainlink, or make the replica-safe invariant explicit and tested.Also applies to: 293-300, 450-468
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@magicblock-api/src/magic_validator.rs` around lines 216 - 218, The code currently always starts the CommittorService via Self::init_committor_service(...) and constructs scheduled_commits_processor even when running in ReplicationMode::Replica; gate both the CommittorService startup and any construction/use of scheduled_commits_processor behind the same replica predicate used for Chainlink/RPC (i.e., only start when NOT ReplicationMode::Replica) or make an explicit replica-safe invariant with tests; update the call sites that invoke init_committor_service and build scheduled_commits_processor (including the other occurrences noted around init_committor_service/CommittorService usage) to check the replication mode/config before initializing the service or processor, and add a unit/integration test that verifies replicas do not start the commit pipeline.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@magicblock-api/src/magic_validator.rs`:
- Around line 236-246: The code currently calls
dispatch.replication_messages.take().expect(...) which can panic; replace this
.expect() with a typed ApiError return so startup fails gracefully. Update the
block constructing ReplicationService (where you access
dispatch.replication_messages and call ReplicationService::new) to call
dispatch.replication_messages.take().ok_or_else(|| ApiError::new("...")) or
map_err to convert the None case into an ApiError and propagate it (using ?),
ensuring the enclosing function's signature returns Result<..., ApiError> so the
error is returned instead of panicking. Ensure the ApiError includes a clear
message like "replication channel missing or already taken" and adjust any call
sites to handle the propagated error.
- Around line 216-218: The code currently always starts the CommittorService via
Self::init_committor_service(...) and constructs scheduled_commits_processor
even when running in ReplicationMode::Replica; gate both the CommittorService
startup and any construction/use of scheduled_commits_processor behind the same
replica predicate used for Chainlink/RPC (i.e., only start when NOT
ReplicationMode::Replica) or make an explicit replica-safe invariant with tests;
update the call sites that invoke init_committor_service and build
scheduled_commits_processor (including the other occurrences noted around
init_committor_service/CommittorService usage) to check the replication
mode/config before initializing the service or processor, and add a
unit/integration test that verifies replicas do not start the commit pipeline.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9a5500b7-ffec-40c0-a4bb-d7a76c9e0a02
📒 Files selected for processing (17)
magicblock-accounts/src/scheduled_commits_processor.rsmagicblock-aperture/src/requests/http/mod.rsmagicblock-aperture/src/requests/http/send_transaction.rsmagicblock-aperture/src/requests/http/simulate_transaction.rsmagicblock-aperture/src/state/mod.rsmagicblock-aperture/src/tests.rsmagicblock-aperture/tests/setup.rsmagicblock-api/src/magic_validator.rsmagicblock-chainlink/src/chainlink/errors.rsmagicblock-chainlink/src/chainlink/mod.rsmagicblock-chainlink/tests/utils/test_context.rsmagicblock-replicator/src/service/context.rsmagicblock-replicator/src/service/mod.rsmagicblock-replicator/src/service/primary.rsmagicblock-replicator/src/service/replica.rstest-integration/test-chainlink/src/ixtest_context.rstest-integration/test-chainlink/src/test_context.rs
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@CodeRabbit resume |
✅ Actions performedReviews resumed. |
* master: fix: replication protocol (#1247) fix: delegated account evictions (#1216) fix(replicator): disable automatic failover (#1228) Fix same-slot account refetch (#1242) fix: onchain startup setup (#1244) fix: prohobit commits of confined & ephemeral accs via IntentBundles (#1236) fix: reset owner to default when closing buffer accounts (#1241) Fix eATA projection min-context refetch (#1240) Stop RPC servers promptly on shutdown (#1237) fix: validate min_interval (#1234) feat: derive crank signer from task authority (#1188) feat: batch cranks (#1190)
* master: fix: various correctness related enhancements (#1248)
| // Magic Router compatibility methods. | ||
| pub(crate) mod get_delegation_status; | ||
|
|
||
| #[cfg(test)] |
There was a problem hiding this comment.
these tests verify something which is obvious from reading the code, they are overly verbose and add very little value, if you think they are absolutely necessary, then please add a proper full coverage in tests directory to respective modules
| if Self::replication_mode_uses_disabled_chainlink( | ||
| &config.validator.replication_mode, | ||
| ) { | ||
| return ChainlinkImpl::disabled( |
There was a problem hiding this comment.
arguably could've just used Option to indicate disabled state
|
|
||
| /// Shared state for both primary and replica roles. | ||
| pub struct ReplicationContext { | ||
| pub struct ReplicationContext<R> |
There was a problem hiding this comment.
please remove the generic, it's no longer needed, as we do not perform the reset
| pub enum Service { | ||
| Primary(Primary), | ||
| Replica(Replica), | ||
| pub enum Service<R> |
There was a problem hiding this comment.
ditto, no generic needed, master code doesn't perform the reset or transitions between modes
|
|
||
| use std::time::Duration; | ||
|
|
||
| use magicblock_chainlink::AccountsBankResetter; |
There was a problem hiding this comment.
ditto: no need for the trait
| bytes = { workspace = true } | ||
| futures = { workspace = true } | ||
| magicblock-accounts-db = { workspace = true } | ||
| magicblock-chainlink = { workspace = true } |
There was a problem hiding this comment.
no need for the dependency
2 participants
Summary
Disable Chainlink-dependent behavior when the validator runs as a replica, while keeping Chainlink enabled for standalone and primary modes.
CLOSES: #1203
NOTE: this does not disable the comittor service in those replica modes, but it totally could. That part could be done in a follow up PR if we want to implement it.
Details
Chainlink
Adds a replication-mode-aware Chainlink wrapper that can run either with the full Chainlink stack or in a disabled mode for replicas. Disabled mode avoids remote account fetching and subscriptions while preserving the account-bank reset behavior needed during replication startup.
Aperture RPC
Gates RPC paths that require on-chain interaction so replicas serve local account reads but reject primary-only transaction flows with a clear error. This prevents replica nodes from attempting Chainlink-backed fetches or transaction account preparation.
Validator and replication services
Initializes disabled Chainlink for replica mode and passes a reset-capable Chainlink handle into replication instead of relying on a Chainlink-shaped stub. Replication services are made generic over the account-bank resetter interface so they only depend on the cleanup behavior they need.
Tests
Adds coverage for mode-aware Chainlink behavior, replica RPC gating, validator replication-mode selection, and the resetter plumbing used by replication services.
Summary by CodeRabbit
New Features
Bug Fixes
Tests