perf: skip account ensure for noop transfers #768
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. |
📝 WalkthroughWalkthroughAdds a new 📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
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: 0
🧹 Nitpick comments (2)
magicblock-chainlink/src/filters/noop_system_transfer.rs (1)
33-36: Consider avoiding the Vec allocation.The code collects all instructions into a
Vecjust to check the length. For better performance, consider checking the iterator directly without collecting.🔎 Proposed optimization
- let instructions = message.program_instructions_iter().collect::<Vec<_>>(); - if instructions.len() != 1 { + let mut instructions = message.program_instructions_iter(); + let Some(first) = instructions.next() else { return false; - } + }; + if instructions.next().is_some() { + return false; + } - let (program_id, instruction) = instructions[0]; + let (program_id, instruction) = first;magicblock-chainlink/src/chainlink/mod.rs (1)
290-296: Consider adding metrics tracking for skipped noop transfers.While the optimization logic is correct, tracking how often this code path is taken would provide valuable insights into the optimization's impact.
🔎 Suggested metric addition
) -> ChainlinkResult<FetchAndCloneResult> { if is_noop_system_transfer(tx) { trace!( "Skipping account ensure for noop system transfer transaction {}", tx.signature() ); + magicblock_metrics::metrics::inc_noop_system_transfer_skipped(); return Ok(Default::default()); }Note: You'll need to add the corresponding metric function in the magicblock-metrics crate.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
magicblock-chainlink/src/chainlink/mod.rsmagicblock-chainlink/src/filters/mod.rsmagicblock-chainlink/src/filters/noop_system_transfer.rsmagicblock-chainlink/src/lib.rs
🧰 Additional context used
🧠 Learnings (3)
📚 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/mod.rsmagicblock-chainlink/src/lib.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/mod.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/lib.rs
🧬 Code graph analysis (3)
magicblock-chainlink/src/filters/noop_system_transfer.rs (2)
magicblock-chainlink/src/chainlink/mod.rs (2)
tx(298-303)tx(520-524)magicblock-committor-program/src/state/changeset.rs (1)
account_keys(255-257)
magicblock-chainlink/src/filters/mod.rs (1)
magicblock-chainlink/src/filters/noop_system_transfer.rs (1)
is_noop_system_transfer(29-79)
magicblock-chainlink/src/chainlink/mod.rs (1)
magicblock-chainlink/src/filters/noop_system_transfer.rs (1)
is_noop_system_transfer(29-79)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Build Project
- GitHub Check: run_make_ci_lint
- GitHub Check: run_make_ci_format
- GitHub Check: run_make_ci_test
🔇 Additional comments (7)
magicblock-chainlink/src/filters/noop_system_transfer.rs (4)
4-28: LGTM!The documentation is comprehensive, clearly explaining the function's purpose, performance characteristics, arguments, and return value.
48-52: LGTM!The deserialization and pattern matching correctly identifies only Transfer instructions, filtering out other system instruction types.
56-78: LGTM!The account validation and bounds checking are correctly implemented. The safe use of
get()prevents potential panics from out-of-bounds access.
81-179: LGTM!The test suite provides comprehensive coverage of the main scenarios: self-transfer detection, different sender/recipient, multiple instructions, empty transactions, and non-system instructions.
magicblock-chainlink/src/filters/mod.rs (1)
1-2: LGTM!The module structure correctly declares and re-exports the
is_noop_system_transferfunction with appropriate crate-level visibility.magicblock-chainlink/src/chainlink/mod.rs (1)
27-27: LGTM!The import is correctly added to bring in the
is_noop_system_transferfunction.magicblock-chainlink/src/lib.rs (1)
10-10: LGTM!The module declaration correctly introduces the
filtersmodule as a private crate-level module, consistent with thepub(crate)visibility ofis_noop_system_transfer.
thlorenz
requested review from
GabrielePicco and
lucacillario
as code owners
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
magicblock-chainlink/src/filters/noop_system_transfer.rs (3)
10-19: Clarify performance claim regarding line 33.The documentation states "Single iteration through at most 1 instruction (bounded to 1)" but line 33 collects all instructions into a
Vec, which is O(n) in the number of instructions. While Solana transactions have bounded instruction counts (making this effectively constant-time in practice), the documentation could be more precise.🔎 Consider rewording the performance notes
/// # Performance Notes /// -/// This function is O(1) in terms of iterations: +/// This function has bounded complexity due to Solana's instruction limits: /// - Early return after checking instruction count (constant time) -/// - Single iteration through at most 1 instruction (bounded to 1) +/// - Collects all instructions (bounded by Solana's max instructions per transaction) /// - Account key lookup is O(1) array indexing /// - System instruction deserialization is O(1) for fixed-size dataAlternatively, to make it truly O(1) without allocations, you could avoid the
collect():let mut instructions = message.program_instructions_iter(); let Some((program_id, instruction)) = instructions.next() else { return false; }; if instructions.next().is_some() { return false; } // ... rest of the logic
32-36: Consider avoiding Vec allocation.The current implementation collects all instructions into a
Vec, which creates an unnecessary allocation. You can check for exactly one instruction more efficiently using iterator methods.🔎 More efficient alternative
- // Early exit: Must have exactly 1 instruction - let instructions = message.program_instructions_iter().collect::<Vec<_>>(); - if instructions.len() != 1 { + // Early exit: Must have exactly 1 instruction + let mut instructions = message.program_instructions_iter(); + let Some((program_id, instruction)) = instructions.next() else { return false; - } + }; + if instructions.next().is_some() { + return false; + } - let (program_id, instruction) = instructions[0]; - // Check if this is the system program instructionThis avoids the
Vecallocation while maintaining the same logic.
81-180: Excellent test coverage!The test suite comprehensively covers the key scenarios:
- Positive case: self-transfer detection
- Different sender/recipient accounts
- Multiple instructions
- Empty transactions
- Non-system program instructions
Optional: Consider additional edge case tests
While the current coverage is sufficient, you could add tests for:
#[test] fn test_non_transfer_system_instruction() { // Test that other SystemInstructions (e.g., Allocate) with same account are not detected let payer = Keypair::new(); let account = Keypair::new(); let allocate_ix = system_instruction::allocate(&account.pubkey(), 1024); let message = Message::new(&[allocate_ix], Some(&payer.pubkey())); let tx = Transaction::new_unsigned(message); let sanitized_tx = SanitizedTransaction::from_transaction_for_tests(tx); assert!(!is_noop_system_transfer(&sanitized_tx)); } #[test] fn test_transfer_with_wrong_account_count() { // Test Transfer instruction with incorrect number of accounts // (though this would be caught by Solana runtime, it's good to verify the filter behavior) }These are optional as the current tests already provide strong coverage.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/CODEOWNERSmagicblock-chainlink/src/filters/noop_system_transfer.rs
💤 Files with no reviewable changes (1)
- .github/CODEOWNERS
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: run_make_ci_lint
- GitHub Check: run_make_ci_test
- GitHub Check: Build Project
🔇 Additional comments (3)
magicblock-chainlink/src/filters/noop_system_transfer.rs (3)
1-3: LGTM!The imports are minimal and appropriate for the functionality.
46-58: LGTM!The instruction parsing and validation logic is correct:
- Proper error handling for deserialization failures
- Correctly filters for
Transfervariant only- Appropriate validation that Transfer instructions have exactly 2 accounts
60-79: LGTM!The account resolution and comparison logic is robust:
- Safe casting of account indices
- Proper bounds checking with
Optionpattern prevents potential panics- Correct pubkey comparison
Summary
Optimize fetching accounts for transactions by skipping account ensure operations for noop system transfer
transactions (where from == to).
This prevents unnecessary account cloning and processing overhead for self-transfer
transactions, which are no-ops that waste transaction fees without meaningful state changes.
Details
A new filter module has been added to
magicblock-chainlinkto detect noop system transferswith O(1) performance characteristics.
is_noop_system_transfer filter
The filter identifies transactions that consist of a single system program transfer instruction
where the sender and recipient are the same account. This is detected by:
When detected, the chainlink processor skips the account ensure operation, avoiding unnecessary
account fetches for these no-op transactions.
The implementation includes comprehensive unit tests covering:
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.