feat: lock free chainlink #804
Conversation
📝 WalkthroughWalkthroughThis PR adds workspace dependencies Assessment against linked issues
Out-of-scope changes
Suggested reviewers
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (1){magicblock-*,programs,storage-proto}/**⚙️ CodeRabbit configuration file
Files:
🧠 Learnings (6)📚 Learning: 2025-12-17T12:46:36.207ZApplied to files:
📚 Learning: 2025-12-03T09:36:01.527ZApplied to files:
📚 Learning: 2025-11-07T14:20:31.457ZApplied to files:
📚 Learning: 2025-12-01T16:02:05.367ZApplied to files:
📚 Learning: 2025-10-21T14:00:54.642ZApplied to files:
📚 Learning: 2025-11-19T09:34:37.917ZApplied to files:
🧬 Code graph analysis (1)magicblock-chainlink/src/chainlink/fetch_cloner/ata_projection.rs (1)
🔇 Additional comments (1)
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 |
This stack of pull requests is managed by Graphite. Learn more about stacking. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Fix all issues with AI Agents 🤖
In @magicblock-chainlink/src/chainlink/fetch_cloner/pipeline.rs:
- Around line 180-199: The current partition_not_found function uses
mark_empty_if_not_found: Option<&[Pubkey]> and repeatedly calls
mark_empty.contains(p) inside partition, which is O(n) per check; to make
lookups O(1) when the list may grow, convert the incoming slice into a HashSet
before partitioning (e.g., when mark_empty_if_not_found is Some, build a HashSet
from mark_empty and use that set in the partition predicate) so the contains
call inside partition becomes constant-time; update references to mark_empty in
partition_not_found accordingly.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.locktest-integration/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
magicblock-chainlink/Cargo.tomlmagicblock-chainlink/src/chainlink/fetch_cloner/mod.rsmagicblock-chainlink/src/chainlink/fetch_cloner/pipeline.rsmagicblock-chainlink/src/chainlink/fetch_cloner/types.rsmagicblock-chainlink/src/remote_account_provider/chain_pubsub_client.rsmagicblock-chainlink/src/remote_account_provider/lru_cache.rs
🧰 Additional context used
📓 Path-based instructions (1)
{magicblock-*,programs,storage-proto}/**
⚙️ CodeRabbit configuration file
{magicblock-*,programs,storage-proto}/**: Treat any usage of.unwrap()or.expect()in production Rust code as a MAJOR issue.
These should not be categorized as trivial or nit-level concerns.
Request proper error handling or explicit justification with invariants.
Files:
magicblock-chainlink/Cargo.tomlmagicblock-chainlink/src/chainlink/fetch_cloner/mod.rsmagicblock-chainlink/src/remote_account_provider/chain_pubsub_client.rsmagicblock-chainlink/src/chainlink/fetch_cloner/types.rsmagicblock-chainlink/src/chainlink/fetch_cloner/pipeline.rsmagicblock-chainlink/src/remote_account_provider/lru_cache.rs
🧠 Learnings (14)
📓 Common learnings
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.
📚 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-chainlink/Cargo.toml
📚 Learning: 2025-12-17T12:46:36.207Z
Learnt from: GabrielePicco
Repo: magicblock-labs/magicblock-validator PR: 738
File: magicblock-chainlink/src/chainlink/fetch_cloner.rs:1109-1196
Timestamp: 2025-12-17T12:46:36.207Z
Learning: In magicblock-chainlink/src/chainlink/fetch_cloner.rs, eATA account subscriptions are intentionally NOT cancelled even when the eATA doesn't exist or delegation conversion fails. The subscriptions are kept active to watch for future creation or delegation of eATA accounts (per maintainer GabrielePicco).
Applied to files:
magicblock-chainlink/src/chainlink/fetch_cloner/mod.rsmagicblock-chainlink/src/chainlink/fetch_cloner/types.rsmagicblock-chainlink/src/chainlink/fetch_cloner/pipeline.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-chainlink/src/chainlink/fetch_cloner/mod.rsmagicblock-chainlink/src/remote_account_provider/chain_pubsub_client.rsmagicblock-chainlink/src/chainlink/fetch_cloner/types.rsmagicblock-chainlink/src/chainlink/fetch_cloner/pipeline.rsmagicblock-chainlink/src/remote_account_provider/lru_cache.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-chainlink/src/chainlink/fetch_cloner/mod.rsmagicblock-chainlink/src/remote_account_provider/chain_pubsub_client.rsmagicblock-chainlink/src/chainlink/fetch_cloner/pipeline.rsmagicblock-chainlink/src/remote_account_provider/lru_cache.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-chainlink/src/chainlink/fetch_cloner/mod.rsmagicblock-chainlink/src/remote_account_provider/chain_pubsub_client.rsmagicblock-chainlink/src/chainlink/fetch_cloner/pipeline.rs
📚 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-chainlink/src/chainlink/fetch_cloner/mod.rsmagicblock-chainlink/src/remote_account_provider/chain_pubsub_client.rsmagicblock-chainlink/src/chainlink/fetch_cloner/pipeline.rsmagicblock-chainlink/src/remote_account_provider/lru_cache.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-chainlink/src/chainlink/fetch_cloner/mod.rs
📚 Learning: 2025-11-07T13:20:13.793Z
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 589
File: magicblock-processor/src/scheduler/coordinator.rs:227-238
Timestamp: 2025-11-07T13:20:13.793Z
Learning: In magicblock-processor's ExecutionCoordinator (scheduler/coordinator.rs), the `account_contention` HashMap intentionally does not call `shrink_to_fit()`. Maintaining slack capacity is beneficial for performance by avoiding frequent reallocations during high transaction throughput. As long as empty entries are removed from the map (which `clear_account_contention` does), the capacity overhead is acceptable.
Applied to files:
magicblock-chainlink/src/chainlink/fetch_cloner/mod.rsmagicblock-chainlink/src/remote_account_provider/lru_cache.rs
📚 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-chainlink/src/chainlink/fetch_cloner/mod.rsmagicblock-chainlink/src/remote_account_provider/chain_pubsub_client.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-chainlink/src/remote_account_provider/chain_pubsub_client.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-chainlink/src/remote_account_provider/chain_pubsub_client.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-chainlink/src/chainlink/fetch_cloner/pipeline.rs
📚 Learning: 2025-11-13T09:38:43.804Z
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 589
File: magicblock-processor/src/scheduler/locks.rs:64-102
Timestamp: 2025-11-13T09:38:43.804Z
Learning: In magicblock-processor's TransactionScheduler (scheduler/mod.rs line 59), the executor count is clamped to MAX_SVM_EXECUTORS (63) at initialization time, and executor IDs are assigned sequentially from 0 to count-1. This architectural guarantee ensures that executor IDs used in the bitmask-based AccountLock (scheduler/locks.rs) will always be within valid bounds for bit shifting operations, making runtime bounds checks unnecessary.
Applied to files:
magicblock-chainlink/src/remote_account_provider/lru_cache.rs
🧬 Code graph analysis (2)
magicblock-chainlink/src/chainlink/fetch_cloner/pipeline.rs (1)
magicblock-chainlink/src/chainlink/fetch_cloner/mod.rs (5)
not_found(639-642)owned_by_deleg(647-650)programs(112-112)programs(651-654)atas(655-658)
magicblock-chainlink/src/remote_account_provider/lru_cache.rs (2)
magicblock-metrics/src/metrics/mod.rs (1)
inc_evicted_accounts_count(599-601)magicblock-chainlink/src/submux/subscribed_accounts_tracker.rs (2)
subscribed_accounts(18-18)subscribed_accounts(50-52)
🔇 Additional comments (23)
magicblock-chainlink/src/remote_account_provider/lru_cache.rs (2)
1-6: LGTM: Import changes align with parking_lot adoption.The switch from
std::sync::Mutextoparking_lot::Mutexis appropriate and consistent with the broader codebase migration indicated in the PR objectives.
50-50: Excellent refactor: Removes unwrap() calls while improving performance.The migration to
parking_lot::Mutexeliminates all.unwrap()calls from lock acquisitions (lines 50, 65, 92, 101, 111, 128, 135), directly addressing the coding guideline that treats.unwrap()and.expect()as MAJOR issues in production Rust code. Additionally,parking_lot::Mutexprovides better performance characteristics than the standard library mutex.The changes maintain thread safety and correctness while simplifying error handling by removing the poisoning concept.
Based on coding guidelines, this change removes MAJOR issues (unwrap calls) from production code.
Also applies to: 65-65, 92-92, 101-101, 111-111, 128-128, 135-135
magicblock-chainlink/src/remote_account_provider/chain_pubsub_client.rs (9)
364-367: LGTM: Correct import changes for parking_lot migration.The import changes appropriately introduce
parking_lot::Mutexfor the mock module while maintaining necessary std imports at the module level.
408-413: LGTM: Correct parking_lot migration.The removal of
.unwrap()calls is appropriate sinceparking_lot::Mutex::lock()returnsMutexGuarddirectly. The lock-and-mutate pattern correctly updates all state under proper synchronization.
416-418: LGTM: Correct lock usage.The change appropriately uses
parking_lot::Mutex::lock()without.unwrap()for direct guard access.
420-428: LGTM: Proper lock-and-clone pattern.The lock-and-clone pattern correctly avoids holding the mutex guard across the async
send()operation, preventing potential deadlocks or blocking.
454-466: LGTM: Correct state management.The reconnection control methods and state checks properly use
parking_lot::Mutexfor synchronization. The multiple lock acquisitions inis_connected_and_resubscribedare safe as they only read state and release immediately.
471-479: LGTM: Correct usage with intentional panic.The change correctly uses
parking_lot::Mutex::lock()without.unwrap(). The.expect()on theOptionis intentional and documented to surface logic bugs early iftake_updatesis called more than once.
480-519: LGTM: Correct subscription management.The subscription methods properly use
parking_lot::Mutexfor state checks and mutations. Error handling for disconnected state is appropriate, and the lock-and-mutate patterns are correct.
525-554: LGTM: Correct query implementation.The subscription query methods properly use
parking_lot::Mutexto safely read and collect subscription state without holding locks longer than necessary.
556-593: LGTM: Correct reconnection logic with test optimization.The reconnection methods properly use
parking_lot::Mutexfor state management. The delay reduction from 50ms to 10ms inresub_multipleis a reasonable test optimization that keeps test execution time minimal without compromising test behavior.magicblock-chainlink/Cargo.toml (1)
17-17: LGTM! Dependencies support the lock-free refactor.The additions of
parking_lotandsccare appropriate for the lock-free chainlink implementation.sccprovides the lock-freeHashMapused forpending_requests, andparking_lotoffers optimized synchronization primitives.Also applies to: 37-37
magicblock-chainlink/src/chainlink/fetch_cloner/types.rs (2)
55-58: LGTM! Clean data structure for partitioned results.The
PartitionedNotFoundstruct provides a clear, typed return value replacing tuple-based returns. This improves code readability and makes the API self-documenting.
60-111: LGTM!The
FetchAndCloneResultstruct with its helper methods andDisplayimplementation is well-designed. Theis_ok()method provides a clear way to check success, and theDisplayimplementation gives useful debugging output.magicblock-chainlink/src/chainlink/fetch_cloner/pipeline.rs (3)
62-91: LGTM!The refactored
classify_remote_accountscleanly delegates toclassify_single_account, improving readability. Thezipiteration correctly pairs accounts with their pubkeys.
93-155: LGTM! Well-structured classification logic.The helper correctly handles all
RemoteAccountvariants:
NotFound→ adds to not_found listFound(Fresh)→ classifies by owner/executable/ATAFound(Bank)→ logs error for unexpected state (appropriate since this shouldn't occur)The inline attribute is appropriate for this hot-path helper.
157-178: LGTM! Defensive filtering of native loader programs.The helper correctly filters out native loader programs with a warning, providing a safety net in case new native programs bypass the blacklist. This aligns with the project's preference for logging errors over panicking.
magicblock-chainlink/src/chainlink/fetch_cloner/mod.rs (6)
20-20: LGTM! Lock-free HashMap type change.The switch from
Arc<Mutex<std::collections::HashMap>>toArc<scc::HashMap>correctly removes mutex contention. Thescc::HashMapprovides thread-safe operations without explicit locking.Also applies to: 75-75
165-282: Concurrent subscription processing looks correct.The refactor to spawn per-update tasks enables parallel processing of subscription updates. The out-of-order slot check (Lines 175-193) correctly guards against stale updates being applied. The idempotent nature of
clone_account(newer slot wins) ensures correctness even with concurrent updates for the same account.
664-667: LGTM!Clean destructuring of the new
PartitionedNotFoundtype.
917-931: LGTM! Correct lock-free deduplication with Entry API.The
scc::HashMap::entry()API provides atomic check-and-modify semantics:
Occupied: Another fetch is in progress → add sender to pending listVacant: First request → insert entry and add to fetch listThis correctly deduplicates concurrent fetch requests without mutex locks.
964-974: LGTM! Correct cleanup and notification sequence.The cleanup correctly:
- Removes the entry atomically via
scc::HashMap::remove()- Notifies all pending requesters after
fetch_and_clone_accountscompletesThe
let _ = sender.send(())pattern appropriately handles dropped receivers. Requesters waking up can safely read from the bank since cloning completes before notification.
119-119: LGTM!The initialization of
scc::HashMap::new()is correct for the lock-free pending requests tracking.
bmuddha
left a comment
bmuddha
left a comment
There was a problem hiding this comment.
Looks legit, still some room for improvement in terms of existing locks, but we can address that in later PRs
GabrielePicco
force-pushed
the
chore/refactor-cloning-pipeline-v2
branch
from
de09c36 to
381c479
Compare
Manual Deploy AvailableYou can trigger a manual deploy of this PR branch to testnet: Alternative: Comment
Comment updated automatically when the PR is synchronized. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @magicblock-chainlink/src/chainlink/fetch_cloner/ata_projection.rs:
- Line 47: The ATA subscription result is currently ignored; change the call to
handle the Result from subscribe_to_account(ata_pubkey).await and log a warning
on Err just like the eATA path does (use the same warn-level message pattern and
include ata_pubkey and the error). Locate the let _ =
this.subscribe_to_account(ata_pubkey).await line and replace it with a match or
if let Err(e) = ... that calls the existing warn logger with context (e.g.,
"failed to subscribe to ATA", ata_pubkey, e).
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
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)
magicblock-chainlink/Cargo.tomlmagicblock-chainlink/src/chainlink/fetch_cloner/ata_projection.rsmagicblock-chainlink/src/chainlink/fetch_cloner/mod.rsmagicblock-chainlink/src/chainlink/fetch_cloner/pipeline.rsmagicblock-chainlink/src/chainlink/fetch_cloner/types.rsmagicblock-chainlink/src/remote_account_provider/chain_pubsub_client.rsmagicblock-chainlink/src/remote_account_provider/lru_cache.rs
🧰 Additional context used
📓 Path-based instructions (1)
{magicblock-*,programs,storage-proto}/**
⚙️ CodeRabbit configuration file
{magicblock-*,programs,storage-proto}/**: Treat any usage of.unwrap()or.expect()in production Rust code as a MAJOR issue.
These should not be categorized as trivial or nit-level concerns.
Request proper error handling or explicit justification with invariants.
Files:
magicblock-chainlink/Cargo.tomlmagicblock-chainlink/src/chainlink/fetch_cloner/mod.rsmagicblock-chainlink/src/chainlink/fetch_cloner/types.rsmagicblock-chainlink/src/remote_account_provider/lru_cache.rsmagicblock-chainlink/src/chainlink/fetch_cloner/ata_projection.rsmagicblock-chainlink/src/chainlink/fetch_cloner/pipeline.rsmagicblock-chainlink/src/remote_account_provider/chain_pubsub_client.rs
🧠 Learnings (12)
📚 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-chainlink/Cargo.toml
📚 Learning: 2025-12-17T12:46:36.207Z
Learnt from: GabrielePicco
Repo: magicblock-labs/magicblock-validator PR: 738
File: magicblock-chainlink/src/chainlink/fetch_cloner.rs:1109-1196
Timestamp: 2025-12-17T12:46:36.207Z
Learning: In magicblock-chainlink/src/chainlink/fetch_cloner.rs, eATA account subscriptions are intentionally NOT cancelled even when the eATA doesn't exist or delegation conversion fails. The subscriptions are kept active to watch for future creation or delegation of eATA accounts (per maintainer GabrielePicco).
Applied to files:
magicblock-chainlink/src/chainlink/fetch_cloner/mod.rsmagicblock-chainlink/src/chainlink/fetch_cloner/types.rsmagicblock-chainlink/src/chainlink/fetch_cloner/ata_projection.rsmagicblock-chainlink/src/chainlink/fetch_cloner/pipeline.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-chainlink/src/chainlink/fetch_cloner/mod.rsmagicblock-chainlink/src/chainlink/fetch_cloner/types.rsmagicblock-chainlink/src/remote_account_provider/lru_cache.rsmagicblock-chainlink/src/chainlink/fetch_cloner/ata_projection.rsmagicblock-chainlink/src/chainlink/fetch_cloner/pipeline.rsmagicblock-chainlink/src/remote_account_provider/chain_pubsub_client.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-chainlink/src/chainlink/fetch_cloner/mod.rsmagicblock-chainlink/src/chainlink/fetch_cloner/types.rsmagicblock-chainlink/src/chainlink/fetch_cloner/ata_projection.rsmagicblock-chainlink/src/chainlink/fetch_cloner/pipeline.rsmagicblock-chainlink/src/remote_account_provider/chain_pubsub_client.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-chainlink/src/chainlink/fetch_cloner/mod.rsmagicblock-chainlink/src/remote_account_provider/lru_cache.rsmagicblock-chainlink/src/chainlink/fetch_cloner/ata_projection.rsmagicblock-chainlink/src/chainlink/fetch_cloner/pipeline.rsmagicblock-chainlink/src/remote_account_provider/chain_pubsub_client.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-chainlink/src/chainlink/fetch_cloner/mod.rsmagicblock-chainlink/src/remote_account_provider/lru_cache.rs
📚 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-chainlink/src/chainlink/fetch_cloner/mod.rsmagicblock-chainlink/src/remote_account_provider/lru_cache.rsmagicblock-chainlink/src/remote_account_provider/chain_pubsub_client.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-chainlink/src/chainlink/fetch_cloner/mod.rsmagicblock-chainlink/src/chainlink/fetch_cloner/ata_projection.rs
📚 Learning: 2025-11-13T09:38:43.804Z
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 589
File: magicblock-processor/src/scheduler/locks.rs:64-102
Timestamp: 2025-11-13T09:38:43.804Z
Learning: In magicblock-processor's TransactionScheduler (scheduler/mod.rs line 59), the executor count is clamped to MAX_SVM_EXECUTORS (63) at initialization time, and executor IDs are assigned sequentially from 0 to count-1. This architectural guarantee ensures that executor IDs used in the bitmask-based AccountLock (scheduler/locks.rs) will always be within valid bounds for bit shifting operations, making runtime bounds checks unnecessary.
Applied to files:
magicblock-chainlink/src/remote_account_provider/lru_cache.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-chainlink/src/chainlink/fetch_cloner/pipeline.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-chainlink/src/remote_account_provider/chain_pubsub_client.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-chainlink/src/remote_account_provider/chain_pubsub_client.rs
🧬 Code graph analysis (2)
magicblock-chainlink/src/remote_account_provider/lru_cache.rs (2)
magicblock-metrics/src/metrics/mod.rs (1)
inc_evicted_accounts_count(599-601)magicblock-chainlink/src/submux/subscribed_accounts_tracker.rs (2)
subscribed_accounts(18-18)subscribed_accounts(50-52)
magicblock-chainlink/src/chainlink/fetch_cloner/ata_projection.rs (1)
magicblock-core/src/token_programs.rs (1)
try_derive_eata_address_and_bump(81-89)
🔇 Additional comments (17)
magicblock-chainlink/src/chainlink/fetch_cloner/ata_projection.rs (1)
52-52: LGTM! Error handling appropriately relaxed.Downgrading eATA subscription failures from error to warning aligns with the resilient subscription design, where subscriptions are kept active to watch for future account creation or delegation.
Based on learnings, eATA subscriptions are intentionally maintained even on initial failures.
magicblock-chainlink/src/remote_account_provider/lru_cache.rs (1)
1-1: LGTM! Clean migration to parking_lot::Mutex.The migration from
std::sync::Mutextoparking_lot::Mutexis straightforward and correct. Removed poison error handling is appropriate since parking_lot mutexes don't poison on panic. All functional logic remains unchanged.Also applies to: 6-6, 50-50, 65-65, 92-92, 101-101, 111-111, 128-128, 135-135
magicblock-chainlink/Cargo.toml (1)
17-17: LGTM! Dependencies added for lock-free refactoring.The additions of
parking_lot(for more efficient mutexes) andscc(for lock-free concurrent collections) align with the PR objectives to implement lock-free Chainlink patterns. Both are properly declared as workspace dependencies.Also applies to: 37-37
magicblock-chainlink/src/remote_account_provider/chain_pubsub_client.rs (3)
364-364: LGTM! Mock migrated to parking_lot::Mutex.The migration from
std::sync::Mutextoparking_lot::Mutexin the mock implementation is consistent with changes across the codebase. This improves test performance and simplifies lock handling.Also applies to: 367-367
409-412: LGTM! Lock handling updated for parking_lot.All lock acquisitions correctly updated to use
parking_lot::Mutex's API directly without.unwrap()calls. The removal of poison error handling is appropriate since parking_lot mutexes don't poison.Also applies to: 417-417, 421-421, 455-455, 459-459, 463-465, 476-476, 484-484, 491-491, 500-500, 516-516, 530-530, 543-543, 559-559, 566-566, 576-576
589-589: LGTM! Reasonable test delay reduction.The resubscribe delay reduction from 50ms to 10ms speeds up test execution while still providing a meaningful delay for testing timing-sensitive logic.
magicblock-chainlink/src/chainlink/fetch_cloner/types.rs (1)
55-58: LGTM! New type supports pipeline refactoring.The
PartitionedNotFoundstruct cleanly separates accounts that should be cloned as empty from those that are genuinely not found. This supports the refactored classification pipeline mentioned in the PR objectives.magicblock-chainlink/src/chainlink/fetch_cloner/pipeline.rs (3)
62-91: LGTM! Cleaner classification logic.The refactor from fold to explicit loop with helper functions significantly improves readability while preserving the original logic. This aligns well with the PR objective to "simplify large fold statements into more readable code."
93-155: LGTM! Well-structured classification helper.The helper function cleanly encapsulates per-account classification logic, improving modularity. All branches (delegation program owner, executable, ATA, plain) are handled correctly.
157-178: LGTM! Appropriate defensive filtering.The helper correctly filters out native loader programs with appropriate warning logs. This defensive approach prevents issues if a new native program is introduced that bypasses the blacklist.
magicblock-chainlink/src/chainlink/fetch_cloner/mod.rs (7)
1-17: LGTM! Imports aligned with lock-free architecture.The import changes correctly support the lock-free refactor using the
scccrate's concurrent HashMap.
60-87: LGTM! Core lock-free data structure change.The removal of
Mutexin favor ofscc::HashMapis the foundation of the lock-free approach. Thescccrate's lock-free HashMap supports concurrent access through its Entry API without explicit locking.Also applies to: 116-116
411-413: LGTM! Helpful documentation.The comment clarifies the subscription handling for undelegated accounts.
564-567: LGTM! Consistent with pipeline.rs type changes.The destructuring correctly uses the new
PartitionedNotFoundstruct.
817-832: LGTM! Correct lock-free deduplication with Entry API.The Entry API usage correctly implements lock-free request deduplication:
- Occupied: Appends new requester to existing list
- Vacant: Reserves entry and marks account for fetch
The
insert_entrymethod is part of scc's HashMap VacantEntry API and is used correctly here.
864-874: LGTM! Proper cleanup and notification.The cleanup correctly:
- Removes the entry from pending requests
- Signals all waiting requesters that the fetch completed
- Appropriately ignores send errors (receivers may be dropped if those requests timed out or were cancelled)
152-282: Concurrent processing with bounded channel is sound.The refactor to spawn a task per subscription update allows concurrent resolution of delegation records and account updates, directly addressing the PR objective to "allow concurrent account resolutions to handle spikes."
The
subscription_updateschannel is bounded with capacity 100 (created inmagicblock-chainlink/src/chainlink/mod.rsline 117), providing backpressure and preventing unbounded task spawning. The Arc cloning pattern and error handling with proper logging are correct. No.unwrap()or.expect()calls found in the handler.
magicblock-chainlink/src/chainlink/fetch_cloner/ata_projection.rs
Outdated
Show resolved
Hide resolved
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Summary
Compatibility
Testing
Checklist
Summary by CodeRabbit
Refactor
Bug Fixes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.