-
Notifications
You must be signed in to change notification settings - Fork 23
chore: batch LoaderV3 program data fetching #668
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Manual Deploy AvailableYou can trigger a manual deploy of this PR branch to testnet: Alternative: Comment
Comment updated automatically when the PR is synchronized. |
📝 WalkthroughWalkthroughReplaces per-account remote fetches in fetch_cloner.rs with batched fetching using a collected Possibly related issues
Possibly related PRs
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🧠 Learnings (5)📓 Common learnings📚 Learning: 2025-10-21T14:00:54.642ZApplied to files:
📚 Learning: 2025-11-19T09:34:37.917ZApplied to files:
📚 Learning: 2025-11-18T08:47:39.702ZApplied to files:
📚 Learning: 2025-11-07T14:20:31.457ZApplied to files:
🧬 Code graph analysis (1)magicblock-chainlink/src/chainlink/fetch_cloner.rs (3)
🔇 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 3
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
magicblock-chainlink/src/chainlink/fetch_cloner.rs(2 hunks)test-integration/configs/cloning-conf.devnet.toml(1 hunks)test-integration/test-chainlink/src/programs.rs(1 hunks)test-integration/test-cloning/tests/08_multi_program_cloning.rs(1 hunks)
🧰 Additional context used
🧠 Learnings (4)
📚 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:
test-integration/test-cloning/tests/08_multi_program_cloning.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:
test-integration/test-cloning/tests/08_multi_program_cloning.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:
test-integration/test-cloning/tests/08_multi_program_cloning.rsmagicblock-chainlink/src/chainlink/fetch_cloner.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.rs
🧬 Code graph analysis (2)
test-integration/test-cloning/tests/08_multi_program_cloning.rs (1)
magicblock-validator/src/main.rs (1)
init_logger(13-50)
magicblock-chainlink/src/chainlink/fetch_cloner.rs (2)
magicblock-chainlink/src/remote_account_provider/program_account.rs (1)
get_loaderv3_get_program_data_address(37-41)magicblock-chainlink/src/remote_account_provider/remote_account.rs (1)
account(180-202)
🔇 Additional comments (3)
test-integration/configs/cloning-conf.devnet.toml (1)
47-56: New MiniV3 parallel program entries are consistent with test constantsThe two additional
[[program]]entries forMiniV321...andMiniV331...both point to the sameminiv3binary and use the expectedMiniV4AUTH2.../MiniV4AUTH3...authorities, matching the newPARALLEL_MINIV3_*and*_AUTHconstants intest_chainlink::programs. Config looks consistent with the new multi‑program cloning test.test-integration/test-chainlink/src/programs.rs (1)
53-67: Parallel MiniV3 program constants align with config and testsThe new
PARALLEL_MINIV3_1,PARALLEL_MINIV3_2and corresponding*_AUTHconstants are well-documented and match the IDs/auths incloning-conf.devnet.toml. This gives a clean, typed way for tests (like08_multi_program_cloning.rs) to reference the additional LoaderV3 programs used for batched cloning scenarios.test-integration/test-cloning/tests/08_multi_program_cloning.rs (1)
1-73: Integration test correctly exercises two LoaderV3 programs in a single transactionThe test implementation is sound: it sends a single transaction with two
MiniSdk::log_msg_instructioncalls and asserts both log messages are present in the fetched logs, properly exercising the batched program-data fetching for multiple LoaderV3 programs. Theinit_logger!macro is correctly imported fromtest_kit(defined asmacro_rules! init_loggerin test-kit/src/macros.rs) and used consistently throughout the codebase.
586dfa4 to
f78e602
Compare
* master: chore: preserve undelegating and delegated accounts on startup (#670)
…ing with zip pattern
bmuddha
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
…ompts * master: chore: batch LoaderV3 program data fetching (#668)
Summary
Optimize program executable fetching by batching multiple LoaderV3 program data account requests into a single
get_multiple_accountscall instead of spawning concurrent tasks for each program. This improves efficiency when cloning multiple programs in a single transaction.Details
fetch_cloner.rs
Refactored the LoaderV3 program data fetching logic to use a batched approach:
try_get_multi_until_slots_matchonce to fetch all accounts togetherresolve_account_with_companionlogic into a reusable helper method to handle the resolution of program and delegation accountsThe new approach reduces the number of network round trips and task spawning overhead, particularly beneficial when multiple LoaderV3 programs need to be cloned concurrently.
test-cloning
Added a new integration test
08_multi_program_cloning.rsthat verifies the batched fetching optimization works correctly:PARALLEL_MINIV3_1andPARALLEL_MINIV3_2)test-chainlink/src/programs.rs
Added program constants for the two parallel programs used in the integration test.
configs/cloning-conf.devnet.toml
Added deployment configuration for the two additional parallel LoaderV3 programs.
Summary by CodeRabbit
Performance
Reliability
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.