feat: Add support for CommitDiff to efficiently commit small changes in large delegated accounts
#575
Conversation
snawaz
force-pushed
the
snawaz/commit-diff
branch
6 times, most recently
from
249b1aa to
2f0555f
Compare
snawaz
force-pushed
the
snawaz/commit-diff
branch
2 times, most recently
from
0151da0 to
5c25e73
Compare
snawaz
changed the title
MagicBlockInstruction::ScheduleCommitDiffAndUndelegate to efficiently commit changes in delegated accounts
snawaz
changed the title
MagicBlockInstruction::ScheduleCommitDiffAndUndelegate to efficiently commit changes in delegated accountsScheduleCommitDiffAndUndelegate to efficiently commit changes in delegated accounts
snawaz
force-pushed
the
snawaz/commit-diff
branch
2 times, most recently
from
2ae750b to
725f72c
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. |
snawaz
force-pushed
the
snawaz/commit-diff
branch
from
725f72c to
e5c5e15
Compare
snawaz
changed the title
ScheduleCommitDiffAndUndelegate to efficiently commit changes in delegated accountsScheduleCommitDiffAndUndelegate to efficiently commit changes in delegated accounts
snawaz
changed the title
ScheduleCommitDiffAndUndelegate to efficiently commit changes in delegated accountsScheduleCommitDiffAndUndelegate to efficiently commit changes in delegated accounts
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 29
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/tasks/args_task.rs (1)
229-237: reset_commit_id ignores CommitDiff—update both variants.Commit IDs must update for Commit and CommitDiff.
Apply:
- let ArgsTaskType::Commit(commit_task) = &mut self.task_type else { - log::error!("reset_commit_id"); - return; - }; - - commit_task.commit_id = commit_id; + match &mut self.task_type { + ArgsTaskType::Commit(task) | ArgsTaskType::CommitDiff(task) => { + task.commit_id = commit_id; + } + _ => {} + }
📜 Review details
Configuration used: CodeRabbit UI
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 (28)
Cargo.toml(1 hunks)magicblock-accounts/src/scheduled_commits_processor.rs(3 hunks)magicblock-committor-service/src/tasks/args_task.rs(6 hunks)magicblock-committor-service/src/tasks/mod.rs(1 hunks)magicblock-committor-service/src/tasks/task_builder.rs(3 hunks)magicblock-committor-service/src/tasks/task_visitors/persistor_visitor.rs(1 hunks)magicblock-magic-program-api/src/instruction.rs(1 hunks)magicblock-rpc-client/src/lib.rs(1 hunks)programs/magicblock/src/magic_scheduled_base_intent.rs(7 hunks)programs/magicblock/src/magicblock_processor.rs(4 hunks)programs/magicblock/src/schedule_transactions/process_schedule_commit.rs(2 hunks)programs/magicblock/src/schedule_transactions/process_scheduled_commit_sent.rs(5 hunks)test-integration/Cargo.toml(2 hunks)test-integration/programs/schedulecommit-security/src/lib.rs(2 hunks)test-integration/programs/schedulecommit/src/api.rs(5 hunks)test-integration/programs/schedulecommit/src/lib.rs(9 hunks)test-integration/programs/schedulecommit/src/order_book.rs(1 hunks)test-integration/programs/schedulecommit/src/utils/mod.rs(3 hunks)test-integration/schedulecommit/client/src/schedule_commit_context.rs(6 hunks)test-integration/schedulecommit/client/src/verify.rs(2 hunks)test-integration/schedulecommit/test-scenarios/Cargo.toml(1 hunks)test-integration/schedulecommit/test-scenarios/tests/01_commits.rs(2 hunks)test-integration/schedulecommit/test-scenarios/tests/02_commit_and_undelegate.rs(4 hunks)test-integration/schedulecommit/test-scenarios/tests/03_commits_fee_payer.rs(1 hunks)test-integration/schedulecommit/test-scenarios/tests/utils/mod.rs(1 hunks)test-integration/test-ledger-restore/tests/08_commit_update.rs(2 hunks)test-integration/test-tools/src/integration_test_context.rs(2 hunks)test-integration/test-tools/src/scheduled_commits.rs(3 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-26T08:49:31.543Z
Learnt from: taco-paco
PR: magicblock-labs/magicblock-validator#585
File: magicblock-committor-service/src/tasks/buffer_task.rs:111-115
Timestamp: 2025-10-26T08:49:31.543Z
Learning: In the magicblock-committor-service, compute units returned by the `compute_units()` method in task implementations (such as `BufferTask`, `ArgsTask`, etc.) represent the compute budget for a single task. Transactions can comprise multiple tasks, and the total compute budget for a transaction is computed as the sum of the compute units of all tasks included in that transaction.
Applied to files:
magicblock-committor-service/src/tasks/args_task.rs
🧬 Code graph analysis (15)
test-integration/test-ledger-restore/tests/08_commit_update.rs (1)
test-integration/test-ledger-restore/src/lib.rs (1)
setup_validator_with_local_remote(97-119)
test-integration/schedulecommit/test-scenarios/tests/03_commits_fee_payer.rs (3)
test-integration/programs/schedulecommit/src/api.rs (1)
schedule_commit_with_payer_cpi_instruction(214-232)test-integration/schedulecommit/test-scenarios/tests/utils/mod.rs (3)
assert_two_committees_were_committed(68-91)get_context_with_delegated_committees(16-35)assert_feepayer_was_committed(94-113)test-integration/schedulecommit/client/src/verify.rs (1)
fetch_and_verify_commit_result_from_logs(7-14)
test-integration/programs/schedulecommit-security/src/lib.rs (1)
test-integration/schedulecommit/test-security/tests/01_invocations.rs (1)
create_schedule_commit_ix(41-66)
magicblock-committor-service/src/tasks/args_task.rs (4)
magicblock-committor-service/src/tasks/mod.rs (2)
instruction(66-66)instruction(249-255)magicblock-committor-service/src/tasks/buffer_task.rs (2)
instruction(65-86)new(37-45)magicblock-committor-service/src/config.rs (1)
local(29-35)magicblock-accounts/src/scheduled_commits_processor.rs (2)
new(66-90)new(420-436)
programs/magicblock/src/magicblock_processor.rs (3)
magicblock-committor-service/src/tasks/args_task.rs (1)
instruction(58-160)programs/magicblock/src/schedule_transactions/process_schedule_commit.rs (1)
process_schedule_commit(34-259)magicblock-committor-program/src/state/changeset.rs (1)
request_undelegation(230-232)
test-integration/schedulecommit/test-scenarios/tests/02_commit_and_undelegate.rs (4)
test-integration/programs/schedulecommit/src/api.rs (4)
schedule_commit_and_undelegate_cpi_instruction(234-252)schedule_commit_and_undelegate_cpi_with_mod_after_instruction(287-311)schedule_commit_diff_instruction_for_order_book(193-212)update_order_book_instruction(175-191)test-integration/schedulecommit/test-scenarios/tests/utils/mod.rs (3)
get_context_with_delegated_committees(16-35)assert_one_committee_was_committed(41-65)assert_one_committee_account_was_undelegated_on_chain(190-196)test-integration/schedulecommit/client/src/schedule_commit_context.rs (3)
committees(205-209)ephem_blockhash(322-324)ephem_client(318-320)test-integration/schedulecommit/client/src/verify.rs (1)
fetch_and_verify_order_book_commit_result_from_logs(16-23)
test-integration/programs/schedulecommit/src/api.rs (2)
test-integration/programs/schedulecommit/src/order_book.rs (1)
new(85-94)test-integration/programs/flexi-counter/src/state.rs (1)
pda(32-35)
test-integration/schedulecommit/client/src/verify.rs (2)
test-integration/test-ledger-restore/tests/08_commit_update.rs (1)
ctx(90-91)test-integration/schedulecommit/test-security/tests/01_invocations.rs (1)
ctx(145-146)
magicblock-committor-service/src/tasks/task_builder.rs (2)
magicblock-accounts/src/scheduled_commits_processor.rs (2)
new(66-90)new(420-436)magicblock-committor-service/src/tasks/args_task.rs (1)
new(49-54)
test-integration/programs/schedulecommit/src/lib.rs (2)
test-integration/programs/schedulecommit/src/utils/mod.rs (3)
assert_is_signer(30-44)assert_keys_equal(16-28)allocate_account_and_assign_owner(58-106)test-integration/programs/schedulecommit/src/order_book.rs (1)
new(85-94)
programs/magicblock/src/schedule_transactions/process_schedule_commit.rs (2)
magicblock-committor-service/src/tasks/task_builder.rs (1)
committed_accounts(153-156)test-integration/test-committor-service/tests/test_ix_commit_local.rs (1)
base_intent(544-549)
test-integration/schedulecommit/client/src/schedule_commit_context.rs (1)
test-integration/programs/schedulecommit/src/api.rs (3)
init_order_book_instruction(36-54)init_payer_escrow(77-98)init_account_instruction(16-34)
programs/magicblock/src/magic_scheduled_base_intent.rs (1)
magicblock-committor-service/src/tasks/task_builder.rs (1)
committed_accounts(153-156)
test-integration/schedulecommit/test-scenarios/tests/utils/mod.rs (3)
test-integration/test-ledger-restore/tests/08_commit_update.rs (1)
ctx(90-91)test-integration/schedulecommit/test-security/tests/01_invocations.rs (1)
ctx(145-146)test-integration/schedulecommit/client/src/schedule_commit_context.rs (3)
try_new(72-74)ncommittees(102-121)try_new_random_keys(66-71)
test-integration/schedulecommit/test-scenarios/tests/01_commits.rs (1)
test-integration/schedulecommit/test-scenarios/tests/utils/mod.rs (1)
get_context_with_delegated_committees(16-35)
⏰ 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: run_make_ci_format
- GitHub Check: run_make_ci_test
- GitHub Check: run_make_ci_lint
- GitHub Check: run_make_ci_test
🔇 Additional comments (34)
test-integration/test-ledger-restore/tests/08_commit_update.rs (1)
55-55: LGTM! Correctly updated to match new function signature.Both call sites properly destructure the new 3-tuple return value. The
_tmpdirbinding is necessary to keep theTempDiralive throughout the function scope, preventing premature cleanup of the temporary directory while the validator is still using it.Also applies to: 170-170
test-integration/programs/schedulecommit/src/utils/mod.rs (2)
53-53: LGTM: Type alignment with Solana API.Changing
sizetou64aligns with Solana'ssystem_instruction::allocateAPI, which expects au64parameter. This improves type consistency and eliminates the need for type conversions downstream.
70-90: LGTM: Clean implementation with helpful debugging.The changes correctly handle the type conversion:
- Line 71: The cast to
usizeis necessary forRent::minimum_balanceand is safe given Solana's practical account size limits.- Lines 75-80: The debug log provides useful visibility into lamport requirements during account allocation.
- Line 90: Passing
sizedirectly asu64tosystem_instruction::allocateis cleaner and aligns with the API signature.test-integration/programs/schedulecommit-security/src/lib.rs (2)
2-2: LGTM: Import addition is correct.The
CommitPolicyimport is necessary for the API change on line 149.
143-150: No action required—the two CPI paths use intentionally different APIs.The review comment's concern about consistency is based on a false premise. The code uses two fundamentally different functions from different modules:
- Path 1 (
schedule_commit_cpi_instruction): A local program API wrapper with a fixed, simplified interface- Path 2 (
create_schedule_commit_ix): An external SDK function with extended configuration options includingCommitPolicyThese are not parallel implementations requiring symmetry. The
schedule_commit_cpi_instructionfunction does not supportCommitPolicyby design—it wraps commit scheduling with hardcoded policy defaults. AddingCommitPolicy::UseFullBytestocreate_schedule_commit_ixis the correct change and requires no corresponding update toschedule_commit_cpi_instruction.Likely an incorrect or invalid review comment.
test-integration/test-tools/src/integration_test_context.rs (1)
167-171: Transaction version configuration is correct and environment-aware.The conditional logic properly handles two distinct RPC endpoints:
- Chain (Solana devnet):
max_supported_transaction_version: Some(0)with 50 retries to handle devnet's known quirk of sometimes returning version responses instead of transaction metadata (noted at line 160-161).- Ephemeral (local validator):
Noneto use default behavior on a controlled, local environment without such quirks.Git history confirms this change is part of the current PR (commit 4c75d78). The code is correctly differentiated for each environment. No changes needed.
test-integration/test-tools/src/scheduled_commits.rs (1)
182-213: LGTM! Clear hierarchical naming for log variables.The renaming from
ephem_logs/chain_logstoephem_logs_l1/ephem_logs_l2improves clarity by establishing a consistent naming convention that reflects the two-level log retrieval hierarchy.magicblock-committor-service/src/tasks/task_visitors/persistor_visitor.rs (1)
29-33: LGTM: CommitDiff included in strategy persistence.Cleanly broadens Commit to CommitDiff without changing strategy semantics.
programs/magicblock/src/schedule_transactions/process_schedule_commit.rs (2)
224-230: Diff-aware commit routing is correct; keep logs concise.Choosing StandaloneDiff when request_diff is true is right. Consider demoting the "StandaloneDiff" ic_msg to debug in production builds to reduce log noise.
Also applies to: 233-238
29-32: All call sites properly initialize request_diff—no corrections needed.Verification confirms that every
process_schedule_commit()invocation explicitly sets bothrequest_undelegationandrequest_difffields. All three instruction paths (ScheduleCommit,ScheduleCommitAndUndelegate,ScheduleCommitDiffAndUndelegate) pass values forrequest_diff, withScheduleCommitDiffAndUndelegatecorrectly setting it totrueto trigger theCommitType::StandaloneDiffpath. No uninitialized defaults are used, and no silent routing to full-state commits can occur.programs/magicblock/src/schedule_transactions/process_scheduled_commit_sent.rs (1)
29-30: LGTM: commit_diff added end-to-end with safe logging.Field is carried into printable form and logged only as a flag. Tests initialize the new field.
Also applies to: 44-45, 71-73, 215-217, 254-255
magicblock-committor-service/src/tasks/args_task.rs (1)
197-205: Calibrate compute units for CommitDiff based on diff size.Fixed 65_000 may be inaccurate. Once diff is precomputed, scale CU by diff length/segments or add headroom to avoid CU errors.
Based on learnings
programs/magicblock/src/magicblock_processor.rs (3)
2-2: LGTM: import added is correct.
51-54: ScheduleCommit: explicit request_diff=false is correct.
90-99: New ScheduleCommitDiffAndUndelegate path correctly flips request_diff=true.magicblock-committor-service/src/tasks/task_builder.rs (1)
96-105: Per-account Commit vs CommitDiff task selection is correct.programs/magicblock/src/magic_scheduled_base_intent.rs (6)
104-107: ScheduledBaseIntent::is_commit_diff added — OK.
155-164: MagicBaseIntent::is_commit_diff added — OK.
448-454: CommitType::is_commit_diff implementation — OK.
456-463: Getters updated for StandaloneDiff — OK.Also applies to: 465-473
475-487: is_empty handles StandaloneDiff — OK.
317-323: StandaloneDiff variant already exists and is actively used in the codebase.The review's concern assumes
StandaloneDiffis a new addition that will shift bincode discriminants. However, the variant is already present inprocess_schedule_commit.rs(lines 225–226), indicating it exists before this PR or is not new.If
StandaloneDiffwas already committed to the codebase:
- The bincode discriminant shift already happened (or never was a concern).
- The review comment's warning is outdated or misguided.
If the snippet shows the final state (post-changes) and
StandaloneDiffwas genuinely added in this PR:
- Verify the diff shows it was inserted between existing variants (which would shift discriminants).
- If it was appended after existing variants, discriminants remain stable and no migration is needed.
Critical finding: No backwards compatibility tests, no versioning code, and no serde external tagging exist. If persisted state truly exists and the variant order changed, this is a risk. However, the evidence suggests
StandaloneDiffis not new.Action for developer: Confirm in the PR diff that you are not reordering existing enum variants. If appending new variants after existing ones, bincode compatibility is safe. If inserting between existing variants, migration is required.
test-integration/schedulecommit/test-scenarios/Cargo.toml (1)
19-20: LGTM!The addition of
randandborshas workspace dev-dependencies appropriately supports the new order-book test scenarios introduced in this PR.test-integration/schedulecommit/test-scenarios/tests/01_commits.rs (1)
30-31: LGTM!The addition of deterministic seed
b"magic_schedule_commit"improves test reproducibility by ensuring consistent PDA derivation across test runs.test-integration/schedulecommit/test-scenarios/tests/utils/mod.rs (2)
16-35: LGTM!The seed parameter enables deterministic test context initialization, and explicitly driving the
init_committeesanddelegate_committeesworkflow steps with debug logging improves test clarity and observability.
41-47: LGTM!Making
assert_one_committee_was_committedgeneric over typeTwith appropriate trait bounds enables reuse across different account types (MainAccount, OrderBookOwned, etc.) while maintaining type safety.test-integration/schedulecommit/test-scenarios/tests/03_commits_fee_payer.rs (2)
23-80: LGTM!The test properly validates that attempting to commit a fee payer without escrowing lamports fails with the expected "DoesNotHaveEscrowAccount" error. The test structure and assertions are appropriate.
83-135: LGTM!The test correctly validates the happy path where fee payer commits succeed when lamports are properly escrowed. The verification flow and assertions comprehensively check that both committees and the fee payer were committed and synchronized.
test-integration/schedulecommit/test-scenarios/tests/02_commit_and_undelegate.rs (3)
55-56: LGTM!The addition of deterministic seed
b"magic_schedule_commit"ensures reproducible test behavior across runs.
110-164: LGTM!The
commit_and_undelegate_order_book_accounthelper follows the established pattern of other commit helpers in this file, properly constructing the update and schedule-commit-diff instructions sequence.
244-312: Excellent reproducibility design for randomized testing.The test demonstrates best practices for randomized testing:
- Prints the RNG seed for reproducibility
- Includes the seed in assertion failure messages
- Uses seeded RNG (StdRng) for deterministic behavior given a seed
This allows failures to be reproduced by rerunning with the printed seed value.
One optional enhancement: consider adding a way to override the seed via environment variable for easier failure reproduction:
let rng_seed = std::env::var("TEST_RNG_SEED") .ok() .and_then(|s| s.parse::<u64>().ok()) .unwrap_or_else(|| OsRng.next_u64());test-integration/programs/schedulecommit/src/lib.rs (1)
561-571: Use commit_diff for undelegate path; this looks correct.The diff-based commit is invoked when
undelegateis true; good alignment with the new optimization.Please confirm tests cover both diff and full-commit paths (with/without
undelegate).test-integration/programs/schedulecommit/src/order_book.rs (1)
101-116: Bounds/capacity logic is good; but relies on header being valid.Once header init is fixed in
process_init_order_book, these helpers are fine for basic growth semantics.Ensure tests cover:
- Non-zero existing asks/bids followed by additional inserts (to exercise prefix/suffix behavior).
- Capacity exhaustion returning
None.Also applies to: 118-138, 163-184
test-integration/schedulecommit/client/src/schedule_commit_context.rs (1)
260-267: Seed semantics are correct and consistently applied across the codebase.The verification shows that
delegate_account_cpi_instruction()explicitly branches on two supported canonical seeds:
b"magic_schedule_commit"→ScheduleCommitInstruction::DelegateCpib"order_book"→ScheduleCommitInstruction::DelegateOrderBookAll test utilities and callers supply only these supported seeds. The
delegate_committees()function at lines 260-267 correctly passes&self.user_seed, and all constructors receive only canonical seeds from test callers. PDA derivation and instruction creation use consistent seed semantics.
snawaz
force-pushed
the
snawaz/commit-diff
branch
2 times, most recently
from
9c44347 to
693af7f
Compare
There was a problem hiding this comment.
Actionable comments posted: 9
♻️ Duplicate comments (22)
magicblock-rpc-client/src/lib.rs (1)
430-430: Redundant error prefix persists in log message.The
error!macro already includes ERROR level in its output. The explicit"> ERROR:"prefix creates redundancy and reduces log readability.Apply this diff to remove the redundant prefix:
- error!("> ERROR: {:?}", err); + error!("{:?}", err);Or add context instead:
- error!("> ERROR: {:?}", err); + error!("Transaction processed status failed: {:?}", err);test-integration/test-tools/src/scheduled_commits.rs (2)
198-210: Verbose debug output concern still applies.The past review comment regarding verbose debug output from
println!with{:#?}formatting remains applicable to these new debug statements. Consider making the debug output conditional or using a more compact format.
233-235: Unnecessary clone() on Copy type.The past review comment regarding the redundant
.clone()call onSignature(which implementsCopy) remains applicable. Use*siginstead ofsig.clone().test-integration/test-tools/src/integration_test_context.rs (1)
157-158: Format the panic message withlabel.The panic still prints the literal
rpc_client for [{}] does not exist, so the label never appears when this trips. Please injectlabelinto the message.- let rpc_client = - rpc_client.expect("rpc_client for [{}] does not exist"); + let rpc_client = rpc_client.unwrap_or_else(|| { + panic!("rpc_client for [{}] does not exist", label) + });test-integration/schedulecommit/client/src/verify.rs (1)
16-23: Code duplication: consider the generic implementation suggested in the previous review.This function duplicates
fetch_and_verify_commit_result_from_logsexcept for the type parameter. A generic implementation withT: borsh::BorshDeserializewould eliminate this duplication.test-integration/schedulecommit/client/src/schedule_commit_context.rs (2)
115-118: PDA derivation must match program-side seeds.The PDA is derived using an arbitrary
user_seed, which may not match the on-chain program's expected seeds (e.g.,[b"order_book", book_manager]or[b"magic_schedule_commit", player]). This mismatch will cause transaction failures.
170-203: Validate user_seed explicitly to prevent silent misconfiguration.The implicit
elsebranch will execute for anyuser_seedthat isn'tb"magic_schedule_commit", including invalid values. Add explicit validation to fail fast with a clear error when an unsupported seed is provided.test-integration/programs/schedulecommit/src/api.rs (1)
106-109: Derive delegated PDA with canonical seeds to match on-chain expectations.The PDA is derived using an arbitrary
user_seed, but the on-chain program expects fixed seeds like[b"magic_schedule_commit", player]or[b"order_book", book_manager]. This mismatch will cause thedelegated_accountto not match program-side expectations.Cargo.toml (1)
112-112: CI/CD will fail: local path dependency not available.The path
../delegation-programdoes not exist in the repository or CI environments, causing all builds to fail. Either:
- Revert to a git-based dependency (consistent with the learning about using git branch references)
- Make
delegation-programa workspace member at the correct path- Update CI workflows to checkout the dependency
Verify the intended dependency resolution strategy:
#!/bin/bash # Check if the delegation-program path exists if [ -d "../delegation-program" ]; then echo "✓ Path exists locally" ls -la ../delegation-program/ else echo "✗ Path ../delegation-program does not exist" fi # Check for any git submodule configuration echo "" echo "=== Git submodules ===" git submodule status 2>/dev/null || echo "No submodules configured" # Check recent commits for this dependency change echo "" echo "=== Recent changes to delegation-program dependency ===" git log -n 5 --oneline --all -- Cargo.toml | head -10test-integration/Cargo.toml (1)
40-40: Critical: Local path dependencies will break CI builds.This issue has already been flagged in previous review comments. The local paths
../../ephemeral-rollups-sdk/rust/sdkand../../delegation-programdo not exist in the CI environment and will cause build failures.Also applies to: 60-60
test-integration/programs/schedulecommit/src/lib.rs (4)
302-309: Zero the order-book header immediately after allocation.
OrderBook::newreadsbids_len/asks_lenstraight from account data. Leaving those bytes uninitialized means random counts, which makes the next borrow calculate bogus slices and can walk past the buffer. Please clear the header bytes right after allocation (e.g. borrow data, zero the firstsize_of::<OrderBookHeader>()bytes).Apply this diff:
allocate_account_and_assign_owner(AllocateAndAssignAccountArgs { payer_info: payer, account_info: order_book, owner: &crate::ID, signer_seeds: &[b"order_book", book_manager.key.as_ref(), &[bump]], size: 10 * 1024, })?; + { + let mut data = order_book.try_borrow_mut_data()?; + let header_len = core::mem::size_of::<OrderBookHeader>(); + if data.len() < header_len { + return Err(ProgramError::AccountDataTooSmall); + } + for byte in &mut data[..header_len] { + *byte = 0; + } + } + Ok(())
329-334: Fix the PDA assertion message.The diagnostic still prints
payer.key, so a failing check points at the wrong seed. Swap it forbook_manager.keyas previously requested.Apply this diff:
assert_keys_equal(order_book.key, &pda, || { format!( "PDA for the account ('{}') and for book_manager ('{}') is incorrect", - order_book.key, payer.key + order_book.key, book_manager.key ) })?;
398-403: Update the log label.This path is mutating an existing order book, not initializing one. Please change the
msg!to say “Update order book” (or equivalent) so logs stay accurate.
424-451: Use the right error kind and require the payer to sign.When the accounts slice is the wrong length you must emit
ProgramError::NotEnoughAccountKeys;MissingRequiredSignatureis misleading. More importantly, we still never verifypayer.is_signer, so the CPI can run with an unsigned funder. Addassert_is_signer(payer, "payer")?before the CPI.Apply this diff:
let [payer, order_book_account, magic_context, magic_program] = accounts else { - return Err(ProgramError::MissingRequiredSignature); + return Err(ProgramError::NotEnoughAccountKeys); }; + assert_is_signer(payer, "payer")?; + commit_diff_and_undelegate_accounts( payer, vec![order_book_account], magic_context, magic_program,test-integration/programs/schedulecommit/src/order_book.rs (3)
45-55:deserializestill aliases immutable data mutably (UB).
slice::from_raw_parts_mutonbook_bytesforges a&mut [u8]from shared data, violating Rust’s aliasing rules and instantly triggering undefined behaviour. Please replace this with a safe path—e.g. copy into an owned buffer or implement a parser that reads header/levels straight from the immutable slice.
85-93:OrderBook::newassumes alignment that account data does not guarantee.Casting
header_bytes.as_ptr()to*mut OrderBookHeader(and the level pointer to*mut OrderLevel) requires 4/8‑byte alignment, but Solana account data is byte-aligned. The moment you dereference those pointers you trigger UB. Rework this to treat the buffer as raw bytes and read/write fields withfrom_le_bytes/to_le_bytesinstead of typed references.
140-178: Typed slices over account data remain unsafe.
slice::from_raw_parts(_ as *mut OrderLevel, ...)for bids/asks/buffer helpers creates slices ofOrderLevelout of unaligned memory. That’s the same UB called out earlier. Please keep the backing store as[u8], then decode/encode eachOrderLevelby copying 16 bytes into a local array and usingu64::from_le_bytes/u64::to_le_bytes.magicblock-magic-program-api/src/instruction.rs (1)
111-111: Add documentation for ScheduleCommitDiffAndUndelegate.This issue was already identified in a previous review. The variant lacks documentation while all sibling variants include doc comments describing the instruction purpose and account references.
magicblock-committor-service/src/tasks/args_task.rs (3)
75-103: Stop hard-coding the RPC endpoint in CommitDiff.
ChainConfig::local(ComputeBudgetConfig::new(1_000_000))forces every CommitDiff task to dialhttp://localhost:7799with Processed commitment and re-create an RPC client each timeinstruction()is called. In production that URI does not exist, so we immediately fall back tocommit_stateand never ship a diff; plus every call repeats the network fetch we already warned about. Inject the realChainConfig/RpcClientfrom the service (or precompute/cache the diff during task construction) soinstruction()stays pure and uses the configured endpoint. Prior feedback on this remains unresolved.
115-116: Do not log raw diff payloads.Dumping the entire diff at warn level leaks account data and explodes log volume. Log only bounded metadata (e.g., commit_id, pubkey, diff length) instead.
- log::warn!("DIFF computed: {:?}", args.diff); + log::debug!( + "commit_diff computed: commit_id={} pubkey={} len={}", + value.commit_id, + value.committed_account.pubkey, + args.diff.len() + );
171-173: Remove the CommitDiff panic in optimize().
optimize()is part of the normal pipeline; panicking here will abort the worker whenever a CommitDiff task flows through. ReturnErr(self)(mirroring the other variants) so the caller can fall back cleanly.- ArgsTaskType::CommitDiff(_) => { - panic!("ArgsTaskType::CommitDiff not handled") - } + ArgsTaskType::CommitDiff(_) => Err(self),programs/magicblock/src/magicblock_processor.rs (1)
38-38: Avoid logging entire MagicBlockInstruction payloads.
ic_msg!(..., "{:?}", instruction)dumps the full enum—including the diff bytes you just added—on every invocation. That inflates CU cost and spams logs, an issue we already called out earlier. Drop the log or gate it behind a debug-only feature flag so mainnet builds don’t emit the payload.
📜 Review details
Configuration used: CodeRabbit UI
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 (28)
Cargo.toml(1 hunks)magicblock-accounts/src/scheduled_commits_processor.rs(3 hunks)magicblock-committor-service/src/tasks/args_task.rs(6 hunks)magicblock-committor-service/src/tasks/mod.rs(1 hunks)magicblock-committor-service/src/tasks/task_builder.rs(3 hunks)magicblock-committor-service/src/tasks/task_visitors/persistor_visitor.rs(1 hunks)magicblock-magic-program-api/src/instruction.rs(1 hunks)magicblock-rpc-client/src/lib.rs(1 hunks)programs/magicblock/src/magic_scheduled_base_intent.rs(7 hunks)programs/magicblock/src/magicblock_processor.rs(4 hunks)programs/magicblock/src/schedule_transactions/process_schedule_commit.rs(2 hunks)programs/magicblock/src/schedule_transactions/process_scheduled_commit_sent.rs(5 hunks)test-integration/Cargo.toml(2 hunks)test-integration/programs/schedulecommit-security/src/lib.rs(2 hunks)test-integration/programs/schedulecommit/src/api.rs(5 hunks)test-integration/programs/schedulecommit/src/lib.rs(9 hunks)test-integration/programs/schedulecommit/src/order_book.rs(1 hunks)test-integration/programs/schedulecommit/src/utils/mod.rs(3 hunks)test-integration/schedulecommit/client/src/schedule_commit_context.rs(6 hunks)test-integration/schedulecommit/client/src/verify.rs(2 hunks)test-integration/schedulecommit/test-scenarios/Cargo.toml(1 hunks)test-integration/schedulecommit/test-scenarios/tests/01_commits.rs(2 hunks)test-integration/schedulecommit/test-scenarios/tests/02_commit_and_undelegate.rs(4 hunks)test-integration/schedulecommit/test-scenarios/tests/03_commits_fee_payer.rs(1 hunks)test-integration/schedulecommit/test-scenarios/tests/utils/mod.rs(1 hunks)test-integration/test-ledger-restore/tests/08_commit_update.rs(2 hunks)test-integration/test-tools/src/integration_test_context.rs(2 hunks)test-integration/test-tools/src/scheduled_commits.rs(3 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-tools/src/integration_test_context.rstest-integration/programs/schedulecommit/src/utils/mod.rstest-integration/test-ledger-restore/tests/08_commit_update.rstest-integration/schedulecommit/test-scenarios/tests/03_commits_fee_payer.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-committor-service/src/tasks/mod.rstest-integration/programs/schedulecommit/src/utils/mod.rsmagicblock-committor-service/src/tasks/args_task.rstest-integration/programs/schedulecommit/src/api.rstest-integration/programs/schedulecommit/src/lib.rstest-integration/programs/schedulecommit/src/order_book.rs
📚 Learning: 2025-10-26T16:54:39.084Z
Learnt from: thlorenz
Repo: magicblock-labs/magicblock-validator PR: 587
File: test-manual/Cargo.toml:0-0
Timestamp: 2025-10-26T16:54:39.084Z
Learning: In the magicblock-validator repository, use git branch references (not commit hashes or tags) for the helius-laserstream dependency to allow automatic updates when the branch is pushed to.
Applied to files:
test-integration/Cargo.tomlCargo.toml
📚 Learning: 2025-10-26T08:49:31.543Z
Learnt from: taco-paco
Repo: magicblock-labs/magicblock-validator PR: 585
File: magicblock-committor-service/src/tasks/buffer_task.rs:111-115
Timestamp: 2025-10-26T08:49:31.543Z
Learning: In the magicblock-committor-service, compute units returned by the `compute_units()` method in task implementations (such as `BufferTask`, `ArgsTask`, etc.) represent the compute budget for a single task. Transactions can comprise multiple tasks, and the total compute budget for a transaction is computed as the sum of the compute units of all tasks included in that transaction.
Applied to files:
magicblock-committor-service/src/tasks/args_task.rs
🧬 Code graph analysis (15)
test-integration/programs/schedulecommit-security/src/lib.rs (1)
test-integration/schedulecommit/test-security/tests/01_invocations.rs (1)
create_schedule_commit_ix(41-66)
magicblock-committor-service/src/tasks/task_builder.rs (2)
magicblock-committor-service/src/tasks/args_task.rs (1)
new(49-54)magicblock-accounts/src/scheduled_commits_processor.rs (2)
new(66-90)new(420-436)
programs/magicblock/src/schedule_transactions/process_schedule_commit.rs (2)
magicblock-committor-service/src/tasks/task_builder.rs (1)
committed_accounts(153-156)test-integration/test-committor-service/tests/test_ix_commit_local.rs (1)
base_intent(544-549)
test-integration/schedulecommit/client/src/verify.rs (2)
test-integration/test-ledger-restore/tests/08_commit_update.rs (1)
ctx(90-91)test-integration/schedulecommit/test-security/tests/01_invocations.rs (1)
ctx(145-146)
programs/magicblock/src/magicblock_processor.rs (3)
magicblock-committor-service/src/tasks/args_task.rs (1)
instruction(58-160)programs/magicblock/src/schedule_transactions/process_schedule_commit.rs (1)
process_schedule_commit(34-259)magicblock-committor-program/src/state/changeset.rs (1)
request_undelegation(230-232)
test-integration/schedulecommit/test-scenarios/tests/01_commits.rs (1)
test-integration/schedulecommit/test-scenarios/tests/utils/mod.rs (1)
get_context_with_delegated_committees(16-35)
magicblock-committor-service/src/tasks/args_task.rs (3)
magicblock-committor-service/src/tasks/mod.rs (2)
instruction(66-66)instruction(249-255)magicblock-committor-service/src/tasks/buffer_task.rs (1)
instruction(65-86)magicblock-committor-service/src/config.rs (1)
local(29-35)
test-integration/schedulecommit/test-scenarios/tests/utils/mod.rs (3)
test-integration/test-ledger-restore/tests/08_commit_update.rs (1)
ctx(90-91)test-integration/schedulecommit/test-security/tests/01_invocations.rs (1)
ctx(145-146)test-integration/schedulecommit/client/src/schedule_commit_context.rs (3)
try_new(72-74)ncommittees(102-121)try_new_random_keys(66-71)
programs/magicblock/src/magic_scheduled_base_intent.rs (1)
magicblock-committor-service/src/tasks/task_builder.rs (1)
committed_accounts(153-156)
test-integration/schedulecommit/client/src/schedule_commit_context.rs (2)
test-integration/programs/schedulecommit/src/api.rs (3)
init_order_book_instruction(36-54)init_payer_escrow(77-98)init_account_instruction(16-34)test-integration/test-tools/src/integration_test_context.rs (1)
try_new(113-115)
test-integration/test-ledger-restore/tests/08_commit_update.rs (1)
test-integration/test-ledger-restore/src/lib.rs (1)
setup_validator_with_local_remote(97-119)
test-integration/programs/schedulecommit/src/api.rs (2)
test-integration/programs/schedulecommit/src/order_book.rs (1)
new(85-94)test-integration/programs/flexi-counter/src/state.rs (1)
pda(32-35)
test-integration/schedulecommit/test-scenarios/tests/03_commits_fee_payer.rs (4)
test-integration/test-runner/bin/run_tests.rs (1)
run_test(777-796)test-integration/programs/schedulecommit/src/api.rs (1)
schedule_commit_with_payer_cpi_instruction(214-232)test-integration/schedulecommit/test-scenarios/tests/utils/mod.rs (4)
assert_two_committees_synchronized_count(139-169)assert_two_committees_were_committed(68-91)get_context_with_delegated_committees(16-35)assert_feepayer_was_committed(94-113)test-integration/schedulecommit/client/src/verify.rs (1)
fetch_and_verify_commit_result_from_logs(7-14)
test-integration/programs/schedulecommit/src/lib.rs (2)
test-integration/programs/schedulecommit/src/utils/mod.rs (3)
assert_is_signer(30-44)assert_keys_equal(16-28)allocate_account_and_assign_owner(58-106)test-integration/programs/schedulecommit/src/order_book.rs (1)
new(85-94)
test-integration/schedulecommit/test-scenarios/tests/02_commit_and_undelegate.rs (4)
test-integration/programs/schedulecommit/src/api.rs (3)
schedule_commit_and_undelegate_cpi_with_mod_after_instruction(287-311)schedule_commit_diff_instruction_for_order_book(193-212)update_order_book_instruction(175-191)test-integration/schedulecommit/test-scenarios/tests/utils/mod.rs (3)
get_context_with_delegated_committees(16-35)assert_one_committee_was_committed(41-65)assert_one_committee_account_was_undelegated_on_chain(190-196)test-integration/schedulecommit/client/src/schedule_commit_context.rs (3)
committees(205-209)ephem_blockhash(322-324)ephem_client(318-320)test-integration/schedulecommit/client/src/verify.rs (1)
fetch_and_verify_order_book_commit_result_from_logs(16-23)
⏰ 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_test
- GitHub Check: run_make_ci_lint
- GitHub Check: run_make_ci_test
🔇 Additional comments (20)
test-integration/programs/schedulecommit/src/utils/mod.rs (3)
49-55: LGTM: Type alignment with Solana API.Changing
sizefromusizetou64correctly aligns with Solana'ssystem_instruction::allocatesignature, which expectsspace: u64.
75-79: LGTM: Helpful debug logging.The added logging for lamport requirements and payer balance aids in troubleshooting account allocation issues during testing.
90-90: Critical fix: Correct system_instruction::allocate signature.The updated call now correctly provides both required arguments (
pubkeyandspace) tosystem_instruction::allocate. The previous implementation appears to have been missing the pubkey argument, which would not match the function signature:pub fn allocate(pubkey: &Pubkey, space: u64) -> Instruction.test-integration/test-tools/src/scheduled_commits.rs (1)
182-213: Variable renaming improves clarity.The renaming of log variables to
ephem_logs_l1andephem_logs_l2clearly distinguishes the two-level log fetching process, and the updated error context at line 206 now correctly references the first-level logs. These changes enhance code readability.test-integration/programs/schedulecommit-security/src/lib.rs (2)
2-2: LGTM!The
CommitPolicyimport is correctly added to support the new argument at line 149.
143-150: Ignore this review comment. The original comment incorrectly assumes thatschedule_commit_cpi_instructionandcreate_schedule_commit_ixshould be consistent, but they are two distinct functions from different crates with different purposes:
schedule_commit_cpi_instruction(fromprogram_schedulecommit::api) is a local CPI helper that does not accept aCommitPolicyparametercreate_schedule_commit_ix(fromephemeral_rollups_sdk::ephem) is an SDK function that acceptsCommitPolicyas a parameterThe two functions have intentionally different signatures. There is no test inconsistency to address.
Likely an incorrect or invalid review comment.
test-integration/test-ledger-restore/tests/08_commit_update.rs (1)
55-61: No issues found—code is correct as written.The
TempDirreturned bysetup_validator_with_local_remoteis created specifically for temporary config storage during validator initialization (written instart_magicblock_validator_with_config_structat line 225:fs::write(&config_path, config_toml)). After the validator process is spawned, this directory is no longer needed and is safely dropped. The actual ledger directory is managed separately by the main_tmpdirat line 42 of the test, which persists for the entire test duration. The binding to_tmpdirwith an underscore prefix correctly signals that this return value is intentionally unused beyond validator initialization.test-integration/programs/schedulecommit/src/api.rs (3)
36-54: LGTM!The
init_order_book_instructionfollows the standard instruction builder pattern with appropriate account metadata.
56-75: LGTM!The
grow_order_book_instructioncorrectly constructs the instruction with theadditional_spaceparameter.
175-212: LGTM!Both
update_order_book_instructionandschedule_commit_diff_instruction_for_order_bookfollow correct instruction builder patterns with appropriate account metadata.test-integration/schedulecommit/test-scenarios/tests/01_commits.rs (1)
30-31: LGTM! Deterministic seed ensures reproducible test contexts.The hard-coded seed
b"magic_schedule_commit"provides deterministic PDA derivation and context initialization, which improves test reliability and debugging.Also applies to: 84-85
test-integration/schedulecommit/test-scenarios/tests/03_commits_fee_payer.rs (2)
82-135: LGTM! Fee payer escrow test properly verifies expected behavior.The test correctly verifies that with proper escrow setup, the fee payer commit succeeds and all expected assertions pass.
34-41: Fix struct field destructuring:payer_ephemfield andephem_blockhashretrieval.The code attempts to destructure non-existent struct fields:
- Line 35:
payerfield doesn't exist inScheduleCommitTestContextFields— should bepayer_ephem: payer- Line 39:
ephem_blockhashis not a struct field — must be fetched separately viaephem_client.get_latest_blockhash().unwrap()- Lines 58, 112: Remove dereference (
*) fromephem_blockhashonce correctedSee
01_commits.rs(lines 34, 54) for the correct pattern.Also applies to: 88-95, 112
⛔ Skipped due to learnings
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/579programs/magicblock/src/magic_scheduled_base_intent.rs (3)
104-106: LGTM!The
is_commit_diff()method correctly delegates to the base intent, following the same pattern as the existingis_undelegate()method.
155-163: LGTM!The
is_commit_diff()method correctly handles allMagicBaseIntentvariants, properly delegating to the underlyingCommitTypefor bothCommitandCommitAndUndelegatecases.
458-458: LGTM!The accessor methods correctly handle the new
StandaloneDiffvariant, treating it identically toStandalonesince both containVec<CommittedAccount>.Also applies to: 468-468, 480-482
magicblock-committor-service/src/tasks/task_visitors/persistor_visitor.rs (2)
29-33: LGTM!The match correctly handles both
CommitandCommitDiffvariants identically, as both containCommitTaskand should be persisted the same way.
59-59: ****The review comment is incorrect.
BufferTaskTypeintentionally has only a singleCommitvariant because it represents buffer-based commit operations.CommitDiffis a separate task variant that belongs exclusively toArgsTaskTypeand is handled through a different code path.Evidence:
- ArgsTaskType includes CommitDiff(CommitTask), but BufferTaskType does not
- ArgsTaskType::CommitDiff explicitly panics with "ArgsTaskType::CommitDiff not handled" when attempting conversion, indicating CommitDiff is intentionally not converted to BufferTask
- The refutable pattern at line 59 is safe because BufferTaskType can only ever contain a single variant
No code changes are needed; the refutable pattern is correct as implemented.
Likely an incorrect or invalid review comment.
programs/magicblock/src/schedule_transactions/process_schedule_commit.rs (2)
31-31: LGTM!The
request_difffield addition cleanly extends the options structure to control diff-based commit scheduling.
231-238: LGTM!The
commit_actionis correctly used in both the undelegation and non-undelegation paths, enabling diff-based commits for both scenarios while preserving existing semantics.
programs/magicblock/src/schedule_transactions/process_schedule_commit.rs
Outdated
Show resolved
Hide resolved
test-integration/schedulecommit/test-scenarios/tests/02_commit_and_undelegate.rs
Show resolved
Hide resolved
test-integration/schedulecommit/test-scenarios/tests/02_commit_and_undelegate.rs
Show resolved
Hide resolved
test-integration/schedulecommit/test-scenarios/tests/utils/mod.rs
Outdated
Show resolved
Hide resolved
snawaz
force-pushed
the
snawaz/commit-diff
branch
from
693af7f to
99f2e8f
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (8)
test-integration/Cargo.toml (1)
40-40:⚠️ Path dependency concerns from prior review remain unresolved.The migration from git-based to local path dependencies for
ephemeral-rollups-sdkandmagicblock-delegation-programstill poses the same critical CI/CD risks flagged in the previous review: these paths do not exist in the repository, CI workflows do not clone them, and builds will fail in CI/CD and for contributors without manual setup.The prior review's recommendations still apply:
- Update CI to clone both external repositories, OR
- Switch back to git-based references, OR
- Add clear setup documentation with exact clone commands and relative paths
Also applies to: 60-60
magicblock-committor-service/src/tasks/args_task.rs (2)
228-234:reset_commit_idmust handleCommitDiffvariant.The current implementation only updates the
commit_idforCommittasks and silently returns forCommitDiff. When aCommitDifftask is recycled, the stalecommit_idwill remain, causing desynced submissions.Apply this fix to handle both variants:
fn reset_commit_id(&mut self, commit_id: u64) { - // TODO (snawaz): handle CommitDiff as well? what is it about? - let ArgsTaskType::Commit(commit_task) = &mut self.task_type else { - return; - }; - - commit_task.commit_id = commit_id; + match &mut self.task_type { + ArgsTaskType::Commit(task) | ArgsTaskType::CommitDiff(task) => { + task.commit_id = commit_id; + } + _ => { + // Only Commit and CommitDiff tasks have commit_id + } + } }
74-122: Network I/O insideinstruction()remains a major concern.The
instruction()method performs RPC calls and diff computation on every invocation, which is expensive and risky:
instruction()may be called multiple times (e.g., viainvolved_accounts), multiplying RPC calls- TOCTOU: base-chain account can change between diff calculation and transaction submission
- Coupling to network hinders testing and determinism
Per previous discussion, this is acknowledged as temporary and will be addressed in subsequent PRs. Consider tracking this technical debt.
programs/magicblock/src/schedule_transactions/process_schedule_commit.rs (1)
224-228: Consider enhancing observability as previously suggested.The conditional logic correctly selects the commit type based on
request_diff. However, a past review comment suggests improving the log messages to include the number of committed accounts and the flag value for better operational visibility.test-integration/programs/schedulecommit/src/order_book.rs (2)
99-119: Unaligned typed reference toOrderBookHeaderremains a critical issue.Line 114 creates a typed reference
&mut OrderBookHeadervia raw pointer cast. While alignment is asserted at runtime (lines 102-110), Solana account data does not guarantee alignment for arbitrary types. This is undefined behavior if the account data is not aligned toalign_of::<OrderBookHeader>().The assertion will catch misalignment at runtime, but the proper fix is to avoid typed references entirely. Consider reading/writing the header fields as raw bytes using little-endian encoding.
165-178: Typed slices over potentially unaligned memory (OrderLevel).Lines 166 and 173-174 use
slice::from_raw_partsto create typed slices ofOrderLevel(which containsu64fields requiring 8-byte alignment). Account data alignment is not guaranteed by Solana, leading to undefined behavior on read.Per previous review: store levels as raw bytes and provide accessors that read/write fields via
u64::from_le_bytes/to_le_bytes, or use a packed representation without taking references to fields.test-integration/programs/schedulecommit/src/lib.rs (2)
280-311: Critical: OrderBook header is never initialized.After
allocate_account_and_assign_ownerreturns (line 308), the account data contains uninitialized memory. Subsequent calls toOrderBook::newwill read uninitializedbids_lenandasks_lenvalues, causing undefined behavior.Apply this fix immediately after allocation:
allocate_account_and_assign_owner(AllocateAndAssignAccountArgs { payer_info: payer, account_info: order_book, owner: &crate::ID, signer_seeds: &[b"order_book", book_manager.key.as_ref(), &[bump]], size: 10 * 1024, })?; + // Initialize header to zero + let mut data = order_book.try_borrow_mut_data()?; + let header_size = core::mem::size_of::<OrderBookHeader>(); + for b in &mut data[..header_size] { + *b = 0; + } Ok(())
329-334: Incorrect identifier in PDA assertion error message.Line 332 incorrectly references
payer.keyin the error message, which should bebook_manager.keyto accurately reflect the PDA derivation.Apply this fix:
assert_keys_equal(order_book.key, &pda, || { format!( "PDA for the account ('{}') and for book_manager ('{}') is incorrect", - order_book.key, payer.key + order_book.key, book_manager.key ) })?;
📜 Review details
Configuration used: CodeRabbit UI
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 (8)
magicblock-committor-service/src/tasks/args_task.rs(6 hunks)programs/magicblock/src/magic_scheduled_base_intent.rs(7 hunks)programs/magicblock/src/magicblock_processor.rs(2 hunks)programs/magicblock/src/schedule_transactions/process_schedule_commit.rs(2 hunks)test-integration/Cargo.toml(4 hunks)test-integration/programs/schedulecommit/Cargo.toml(1 hunks)test-integration/programs/schedulecommit/src/lib.rs(9 hunks)test-integration/programs/schedulecommit/src/order_book.rs(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 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-committor-service/src/tasks/args_task.rstest-integration/programs/schedulecommit/src/order_book.rstest-integration/programs/schedulecommit/src/lib.rs
📚 Learning: 2025-10-26T08:49:31.543Z
Learnt from: taco-paco
Repo: magicblock-labs/magicblock-validator PR: 585
File: magicblock-committor-service/src/tasks/buffer_task.rs:111-115
Timestamp: 2025-10-26T08:49:31.543Z
Learning: In the magicblock-committor-service, compute units returned by the `compute_units()` method in task implementations (such as `BufferTask`, `ArgsTask`, etc.) represent the compute budget for a single task. Transactions can comprise multiple tasks, and the total compute budget for a transaction is computed as the sum of the compute units of all tasks included in that transaction.
Applied to files:
magicblock-committor-service/src/tasks/args_task.rs
🧬 Code graph analysis (5)
magicblock-committor-service/src/tasks/args_task.rs (3)
magicblock-committor-service/src/tasks/mod.rs (2)
instruction(66-66)instruction(249-255)magicblock-committor-service/src/tasks/buffer_task.rs (1)
instruction(65-86)magicblock-committor-service/src/config.rs (1)
local(29-35)
programs/magicblock/src/magicblock_processor.rs (1)
programs/magicblock/src/schedule_transactions/process_schedule_commit.rs (1)
process_schedule_commit(34-258)
programs/magicblock/src/schedule_transactions/process_schedule_commit.rs (2)
magicblock-committor-service/src/tasks/task_builder.rs (1)
committed_accounts(153-156)test-integration/test-committor-service/tests/test_ix_commit_local.rs (1)
base_intent(544-549)
programs/magicblock/src/magic_scheduled_base_intent.rs (1)
magicblock-committor-service/src/tasks/task_builder.rs (1)
committed_accounts(153-156)
test-integration/programs/schedulecommit/src/lib.rs (2)
test-integration/programs/schedulecommit/src/utils/mod.rs (3)
assert_is_signer(30-44)assert_keys_equal(16-28)allocate_account_and_assign_owner(58-106)test-integration/programs/schedulecommit/src/order_book.rs (1)
new(99-119)
🔇 Additional comments (11)
test-integration/Cargo.toml (1)
72-72: Now let me check the codebase for actual usage of these dependencies to confirm they are necessary:Based on my verification, I can now generate the rewritten review comment:
Both dependencies are compatible with Solana 2.2 and appropriate for this PR.
rkyv 0.7.45 supports no_std and no_alloc configurations, making it suitable for Solana on-chain programs. It supports no-std builds for targets without a standard library and no-alloc builds for targets where allocations cannot be made. The Solana VM guarantees that account data is byte-aligned to a multiple of 8, which enables rkyv's zero-copy deserialization to function safely on account data. static_assertions 1.1.0 is a stable, widely-adopted crate (225,914,390 downloads all time) providing compile-time assertion macros useful for validating account structure layouts and sizes—a common pattern in Solana programs.
No further verification needed.
test-integration/programs/schedulecommit/Cargo.toml (1)
11-12: LGTM! Dependencies properly support order book implementation.The additions of
rkyvandstatic_assertionsare well-utilized inorder_book.rsfor alignment guarantees and compile-time size/alignment checks.programs/magicblock/src/schedule_transactions/process_schedule_commit.rs (1)
31-31: LGTM! New field enables diff-based commit path.The
request_diffflag properly controls whether to useCommitType::StandaloneDifforCommitType::Standalone, enabling the optimization for large accounts with small changes.programs/magicblock/src/magicblock_processor.rs (2)
44-59: LGTM! Existing instructions properly default to non-diff path.Both
ScheduleCommitandScheduleCommitAndUndelegatecorrectly setrequest_diff: falseto maintain existing behavior.
85-92: LGTM! New instruction variant correctly enables diff-based commit with undelegation.The
ScheduleCommitDiffAndUndelegateinstruction properly sets bothrequest_undelegation: trueandrequest_diff: true, mirroringScheduleCommitAndUndelegatebehavior while enabling the diff optimization.test-integration/programs/schedulecommit/src/order_book.rs (1)
48-75: LGTM! Mutable aliasing UB resolved with aligned copy.The implementation now safely copies the input buffer into
rkyv::AlignedVecbefore creating mutable references, avoiding the undefined behavior from the previous approach. The comment clearly explains the rationale.magicblock-committor-service/src/tasks/args_task.rs (2)
171-172: LGTM! CommitDiff correctly returnsErr(self)in optimize().Returning
Err(self)forCommitDiffis correct since the diff is already optimized by design—it sends only the differences rather than full account data. Converting toBufferTaskwould be counterproductive.Per previous discussion, this is the correct semantic since CommitDiff is already using the most efficient data transfer strategy.
196-203: LGTM! Compute units appropriately set for CommitDiff.The 65,000 compute units for
CommitDiff(vs 70,000 forCommit) reflects the reduced work from transmitting only the diff. This aligns with the test observation showing lower CU usage.Based on learnings: these are compute units per task, and transactions may comprise multiple tasks.
test-integration/programs/schedulecommit/src/lib.rs (3)
203-203: LGTM! Type change tou64is appropriate.Changing
MainAccount::SIZEfromusizetou64aligns with thesizeparameter type inallocate_account_and_assign_owner(line 255) and Solana's account size conventions.
398-414: LGTM! Log message and signer check are correct.Line 402 now uses the accurate log message "Update orderbook", and line 407 properly validates that the payer is a signer before allowing the update.
419-439: LGTM! Handler correctly uses diff-based commit with undelegation.The implementation properly:
- Returns
NotEnoughAccountKeyswhen accounts are missing (line 426)- Validates payer is a signer (line 429)
- Invokes
commit_diff_and_undelegate_accountsfor the optimized diff path (line 431)
snawaz
force-pushed
the
snawaz/commit-diff
branch
from
99f2e8f to
7eaaa13
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
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/tasks/args_task.rs (1)
228-235: Critical:reset_commit_idsilently ignores CommitDiff tasks.When a
CommitDifftask is recycled, the current code silently returns without updating itscommit_id(line 230-232), leaving stale IDs that will cause:
- Desynchronization with actual commit state
- Failed submissions with wrong nonce
- Data integrity issues
A past review correctly identified this issue.
Apply this fix to handle both
CommitandCommitDiff:fn reset_commit_id(&mut self, commit_id: u64) { - // TODO (snawaz): handle CommitDiff as well? what is it about? - let ArgsTaskType::Commit(commit_task) = &mut self.task_type else { - return; - }; - - commit_task.commit_id = commit_id; + match &mut self.task_type { + ArgsTaskType::Commit(task) | ArgsTaskType::CommitDiff(task) => { + task.commit_id = commit_id; + } + _ => { + // Other task types don't have commit_id + } + } }
♻️ Duplicate comments (7)
programs/magicblock/src/magic_scheduled_base_intent.rs (1)
389-446: CRITICAL: try_from_args cannot construct StandaloneDiff.The
try_from_argsmethod only handlesCommitTypeArgs::StandaloneandCommitTypeArgs::WithBaseActions, but does not handleCommitTypeArgs::StandaloneDiff. This means:
- Code paths using
try_from_args(e.g.,process_schedule_base_intent) cannot constructCommitType::StandaloneDiff- Only
process_schedule_commitcan create it programmatically- Any attempt to deserialize
CommitTypeArgs::StandaloneDifffrom instruction data will failThis creates a critical inconsistency where the
StandaloneDiffvariant cannot be fully utilized across all code paths.The method needs a new match arm similar to:
pub fn try_from_args( args: CommitTypeArgs, context: &ConstructionContext<'_, '_>, ) -> Result<CommitType, InstructionError> { match args { CommitTypeArgs::Standalone(accounts) => { // ... existing code ... Ok(CommitType::Standalone(committed_accounts)) } + CommitTypeArgs::StandaloneDiff(accounts) => { + let committed_accounts_ref = Self::extract_commit_accounts( + &accounts, + context.transaction_context, + )?; + Self::validate_accounts(&committed_accounts_ref, context)?; + let committed_accounts = committed_accounts_ref + .into_iter() + .map(|el| { + let mut committed_account: CommittedAccount = el.into(); + committed_account.account.owner = context + .parent_program_id + .unwrap_or(committed_account.account.owner); + committed_account + }) + .collect(); + Ok(CommitType::StandaloneDiff(committed_accounts)) + } CommitTypeArgs::WithBaseActions { ... } => { // ... existing code ... } } }Based on past review comment.
programs/magicblock/src/schedule_transactions/process_schedule_commit.rs (1)
224-228: Add observability logging for commit type selection.The conditional logic correctly selects the commit type, but lacks logging to indicate which path was taken. This makes debugging and auditing more difficult.
Based on past review comment, consider adding:
let commit_action = if opts.request_diff { + ic_msg!( + invoke_context, + "ScheduleCommit: using StandaloneDiff for {} accounts", + committed_accounts.len() + ); CommitType::StandaloneDiff(committed_accounts) } else { + ic_msg!( + invoke_context, + "ScheduleCommit: using Standalone for {} accounts", + committed_accounts.len() + ); CommitType::Standalone(committed_accounts) };magicblock-committor-service/src/tasks/args_task.rs (1)
74-122: Network I/O in instruction() confirmed as unresolved; improve fallback logging.The RPC-in-instruction issue is genuine and remains present despite the past review claiming it was addressed in commit 99f2e8f (which does not exist in the repository).
Confirmed concerns:
instruction() called multiple times per task: The method is invoked from
delivery_preparator.rs(line 377, mapped over cleanup_tasks),tasks/utils.rs(line 58, mapped over tasks), andtask_strategist.rs(line 217, in optimization loop). Each call triggers RPC I/O on the base-chain, multiplying network overhead and creating TOCTOU race conditions.ChainConfig hardcoded locally (line 76): ChainConfig is properly injected into the service/processor but ArgsTask hardcodes
ChainConfig::local()instead of receiving injected config. This remains unresolved.Fallback log incomplete (line 90): The message contains a typo ("commmit_id" with 3 m's) and is missing the
pubkeycontext for debugging. Improve clarity:- log::warn!("Fallback to commit_state and send full-bytes, as rpc failed to fetch the delegated-account from base chain, commmit_id: {} , error: {}", value.commit_id, e); + log::warn!( + "Fallback to commit_state for account {}: RPC fetch failed, commit_id={} - {}", + value.committed_account.pubkey, + value.commit_id, + e + );The architectural concerns about performing RPC I/O during instruction building should be addressed, and the config injection pattern needs to be established.
test-integration/programs/schedulecommit/src/lib.rs (3)
280-311: Header remains uninitialized after allocation—subsequent reads are undefined.After
allocate_account_and_assign_owner, the header fields (bids_len,asks_len) are still uninitialized. Any call toOrderBook::newon this account will read garbage.Apply this fix to zero the header immediately after allocation:
allocate_account_and_assign_owner(AllocateAndAssignAccountArgs { payer_info: payer, account_info: order_book, owner: &crate::ID, signer_seeds: &[b"order_book", book_manager.key.as_ref(), &[bump]], size: 10 * 1024, })?; + // Initialize header to zero + let mut data = order_book.try_borrow_mut_data()?; + data[..core::mem::size_of::<OrderBookHeader>()].fill(0); Ok(())
329-334: Error message references wrong key.The PDA assertion message incorrectly shows
payer.keyinstead ofbook_manager.key, making debugging harder.assert_keys_equal(order_book.key, &pda, || { format!( "PDA for the account ('{}') and for book_manager ('{}') is incorrect", - order_book.key, payer.key + order_book.key, book_manager.key ) })?;
402-402: Inconsistent log message casing.Message says "Update orderbook" but other logs use "OrderBook" with capital letters and space for consistency.
- msg!("Update orderbook"); + msg!("Update order book");test-integration/Cargo.toml (1)
40-40: Local path dependencies will break CI/CD builds.These local path dependencies point to repositories outside the current codebase (
../../ephemeral-rollups-sdkand../../delegation-program) that are not cloned by the CI workflow. This will cause build failures for all contributors and in continuous integration.Choose one of the following solutions:
- Revert to git-based dependencies (recommended for now):
-ephemeral-rollups-sdk = { path = "../../ephemeral-rollups-sdk/rust/sdk"} +ephemeral-rollups-sdk = { git = "https://github.com/magicblock-labs/ephemeral-rollups-sdk.git", rev = "..." }
Update CI to clone required repositories before build steps in
.github/workflows/ci-test-integration.ymlDocument the local setup in README with explicit clone instructions if local paths are intended for development only
Based on learnings: This appears to be a transition to local development setup but lacks the corresponding CI infrastructure updates.
Also applies to: 60-60
📜 Review details
Configuration used: CodeRabbit UI
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 (8)
magicblock-committor-service/src/tasks/args_task.rs(6 hunks)programs/magicblock/src/magic_scheduled_base_intent.rs(7 hunks)programs/magicblock/src/magicblock_processor.rs(2 hunks)programs/magicblock/src/schedule_transactions/process_schedule_commit.rs(2 hunks)test-integration/Cargo.toml(4 hunks)test-integration/programs/schedulecommit/Cargo.toml(1 hunks)test-integration/programs/schedulecommit/src/lib.rs(9 hunks)test-integration/programs/schedulecommit/src/order_book.rs(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 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-committor-service/src/tasks/args_task.rstest-integration/programs/schedulecommit/src/lib.rstest-integration/programs/schedulecommit/src/order_book.rs
📚 Learning: 2025-10-26T08:49:31.543Z
Learnt from: taco-paco
Repo: magicblock-labs/magicblock-validator PR: 585
File: magicblock-committor-service/src/tasks/buffer_task.rs:111-115
Timestamp: 2025-10-26T08:49:31.543Z
Learning: In the magicblock-committor-service, compute units returned by the `compute_units()` method in task implementations (such as `BufferTask`, `ArgsTask`, etc.) represent the compute budget for a single task. Transactions can comprise multiple tasks, and the total compute budget for a transaction is computed as the sum of the compute units of all tasks included in that transaction.
Applied to files:
magicblock-committor-service/src/tasks/args_task.rs
🧬 Code graph analysis (5)
magicblock-committor-service/src/tasks/args_task.rs (4)
magicblock-committor-service/src/tasks/mod.rs (2)
instruction(66-66)instruction(249-255)magicblock-committor-service/src/tasks/buffer_task.rs (2)
instruction(65-86)new(37-45)magicblock-committor-service/src/config.rs (1)
local(29-35)magicblock-accounts/src/scheduled_commits_processor.rs (2)
new(66-90)new(420-436)
programs/magicblock/src/magic_scheduled_base_intent.rs (1)
magicblock-committor-service/src/tasks/task_builder.rs (1)
committed_accounts(153-156)
programs/magicblock/src/magicblock_processor.rs (1)
programs/magicblock/src/schedule_transactions/process_schedule_commit.rs (1)
process_schedule_commit(34-258)
programs/magicblock/src/schedule_transactions/process_schedule_commit.rs (2)
magicblock-committor-service/src/tasks/task_builder.rs (1)
committed_accounts(153-156)test-integration/test-committor-service/tests/test_ix_commit_local.rs (1)
base_intent(544-549)
test-integration/programs/schedulecommit/src/lib.rs (2)
test-integration/programs/schedulecommit/src/utils/mod.rs (3)
assert_is_signer(30-44)assert_keys_equal(16-28)allocate_account_and_assign_owner(58-106)test-integration/programs/schedulecommit/src/order_book.rs (1)
new(105-125)
⏰ 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_format
- GitHub Check: run_make_ci_test
- GitHub Check: run_make_ci_test
🔇 Additional comments (14)
programs/magicblock/src/schedule_transactions/process_schedule_commit.rs (1)
29-32: Field addition looks correct.The new
request_difffield is properly scoped aspub(crate)and theDefaulttrait will initialize it tofalse, maintaining backward compatibility.programs/magicblock/src/magicblock_processor.rs (2)
44-51: ScheduleCommit handler correctly configured.The handler properly sets
request_diff: falseto maintain the existing full-commit behavior for standard commits.
85-92: New instruction handler correctly implemented.The
ScheduleCommitDiffAndUndelegatehandler properly enables both undelegation and diff-based commits by setting both flags totrue. The implementation follows the established pattern from the other handlers.programs/magicblock/src/magic_scheduled_base_intent.rs (4)
104-106: Delegation method correctly implemented.The
is_commit_diff()method appropriately delegates to the underlyingbase_intent, following the established pattern for other query methods in this struct.
155-163: Commit diff detection logic is correct.The method correctly returns
falseforBaseActionsand properly delegates to the commit-related variants to determine if differential commits are enabled.
317-327: StandaloneDiff variant added correctly.The new variant follows the same structure as
Standalone, holdingVec<CommittedAccount>. Note that full account data is stored here; the actual diff computation likely occurs downstream during commit processing.
448-484: Accessor methods properly handle StandaloneDiff.All match expressions are now exhaustive and correctly handle the new
StandaloneDiffvariant. Theis_commit_diff()method correctly returnstrueonly forStandaloneDiff, and the accessor methods treat it consistently withStandalone.magicblock-committor-service/src/tasks/args_task.rs (4)
1-24: LGTM: Imports support the CommitDiff feature.All new imports are necessary for the RPC client setup, diff computation, and configuration.
30-30: LGTM: CommitDiff variant added.Reusing
CommitTaskfor theCommitDiffvariant is appropriate since they share the same data requirements.
171-172: LGTM: ReturningErr(self)for CommitDiff is correct.The past review discussion thoroughly analyzed this and concluded that
CommitDiffshould returnErr(self)(no optimization) because:
CommitDiffis already optimized by design—it transmits only the diff (e.g., 286 bytes) vs. full account data (10,240 bytes per your test)- Converting
CommitDifftoBufferTaskwould be counterproductive and defeat its purposeErr(self)correctly signals "no further optimization possible"Suggestion: The TODO on line 171 can be resolved and removed based on the past review's analysis.
199-199: LGTM: Compute units appropriately reduced for CommitDiff.The 65,000 compute unit allocation (5k less than
Commit's 70k) aligns with your test observations and reflects the reduced data processing for diff-based commits.test-integration/programs/schedulecommit/src/lib.rs (1)
419-439: LGTM—signer validation and error handling are now correct.The function now properly returns
NotEnoughAccountKeyswhen the accounts array doesn't match, and validates thatpayeris a signer before proceeding with the CPI call.test-integration/programs/schedulecommit/Cargo.toml (1)
11-12: LGTM—dependencies appropriately support zero-copy OrderBook.The
rkyvcrate providesAlignedVecfor safe buffer allocation, andstatic_assertionsenables compile-time layout verification. Both are correctly utilized inorder_book.rs.test-integration/Cargo.toml (1)
72-72: LGTM—workspace dependencies properly declared.The
rkyvandstatic_assertionscrates are correctly added at the workspace level, enabling their use across integration test crates for zero-copy structures and compile-time assertions.Also applies to: 91-91
snawaz
force-pushed
the
snawaz/commit-diff
branch
from
7eaaa13 to
c05f313
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
magicblock-committor-service/src/tasks/account_fetcher.rs (1)
55-60: Consider adding observability.The method correctly delegates to the RPC client and propagates errors. As noted in previous reviews, adding tracing (e.g.,
#[tracing::instrument]or manual spans) would help diagnose fetch failures, latency, and per-account performance in production.Example instrumentation:
+ #[tracing::instrument(skip(self), fields(pubkey = %pubkey))] pub async fn fetch_account( &self, pubkey: &Pubkey, ) -> Result<Account, solana_rpc_client_api::client_error::Error> { + tracing::debug!("Fetching account from RPC"); - self.rpc_client.get_account(pubkey).await + let result = self.rpc_client.get_account(pubkey).await; + if let Err(ref e) = result { + tracing::warn!("Failed to fetch account: {}", e); + } + result }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
magicblock-committor-service/src/tasks/account_fetcher.rs(1 hunks)
🧰 Additional context used
🧠 Learnings (5)
📚 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-committor-service/src/tasks/account_fetcher.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-committor-service/src/tasks/account_fetcher.rs
📚 Learning: 2025-10-26T16:53:29.820Z
Learnt from: thlorenz
Repo: magicblock-labs/magicblock-validator PR: 587
File: magicblock-chainlink/src/remote_account_provider/mod.rs:134-0
Timestamp: 2025-10-26T16:53:29.820Z
Learning: In magicblock-chainlink/src/remote_account_provider/mod.rs, the `Endpoint::separate_pubsub_url_and_api_key()` method uses `split_once("?api-key=")` because the api-key parameter is always the only query parameter right after `?`. No additional query parameter parsing is needed for this use case.
Applied to files:
magicblock-committor-service/src/tasks/account_fetcher.rs
📚 Learning: 2025-11-07T18:19:48.996Z
Learnt from: Dodecahedr0x
Repo: magicblock-labs/magicblock-validator PR: 614
File: test-integration/programs/schedulecommit/Cargo.toml:19-20
Timestamp: 2025-11-07T18:19:48.996Z
Learning: In Solana programs (crates depending on solana-program), the empty feature flags `custom-heap = []` and `custom-panic = []` are standard declarations required to avoid compiler warnings. These should not be flagged as unused features in Cargo.toml files for Solana program crates.
Applied to files:
magicblock-committor-service/src/tasks/account_fetcher.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-committor-service/src/tasks/account_fetcher.rs
🧬 Code graph analysis (1)
magicblock-committor-service/src/tasks/account_fetcher.rs (2)
magicblock-committor-service/src/tasks/mod.rs (1)
new(124-155)magicblock-committor-service/src/config.rs (2)
local(29-35)mainnet(21-27)
⏰ 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). (2)
- GitHub Check: Build Project
- GitHub Check: run_make_ci_test
🔇 Additional comments (1)
magicblock-committor-service/src/tasks/account_fetcher.rs (1)
1-11: LGTM! Clean struct definition.The imports are appropriate and the struct is simple and focused, wrapping a single
RpcClientto handle account fetching responsibilities.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
magicblock-committor-service/src/tasks/task_strategist.rs (2)
159-171: Tx length calculation TODO correctly flags recomputation; future optimization only.
calculate_tx_lengthreassembles the full transaction on every call, which is expensive but acceptable given current usage. The TODO is appropriate—if this becomes hot, consider reusing already computed instructions/budgets or caching the encoded size instead of rebuilding the tx each time.
253-292: Async test helper and join_all usage look correct; consider decoupling from real RPC if tests get slow/flaky.The async
create_test_commit_taskhelper and its use across the strategy tests are idiomatic and align with the newCommitTask::new(...).awaitAPI. The only caveat is that usingAccountFetcher::new()in unit tests ties them to whatever RPC endpoint is configured at runtime; if that ever becomes a source of slowness or flakiness, it would be worth introducing a test-local/mockAccountFetcher(or feature-gated dev config) so these tests remain deterministic and offline-friendly.Also applies to: 326-383, 385-457, 459-515
♻️ Duplicate comments (2)
magicblock-committor-service/src/tasks/mod.rs (2)
110-219: CommitTask CommitDiff/CommitState selection logic looks sound; minor follow‑ups only.The async constructor and fields (
base_account,force_commit_state) correctly implement the intended behavior:
- For accounts with
data.len() > COMMIT_STATE_SIZE_THRESHOLD, you eagerly fetchbase_accountviaAccountFetcher; on failure you log with pubkey+commit_id and fall back cleanly to CommitState.is_commit_diff()is consistent with the constructor (threshold +base_account.is_some()+ not forced), andcreate_commit_ix()switches between diff/state based solely on the presence ofbase_account, withforce_commit_state()providing an explicit override.create_commit_state_ix()andcreate_commit_diff_ix()build the appropriate DLP args; computing the diff fromReadableAccount::data()is correct, and the early return whenforce_commit_stateis set avoids unnecessary diff work.Two minor, non-blocking considerations:
AccountFetcheris consumed by value; if call sites later want to reuse a shared fetcher, changing this to accept&AccountFetcheror anArc<AccountFetcher>might be preferable.- Long term, splitting “commit full state” vs “commit diff” into separate task types (as already discussed in review) would let you drop the
Option<Account>+force_commit_statefrom a single struct and simplify invariants.Overall, the implementation is coherent and matches the CommitDiff design goals.
1-5: AccountFetcher imports/module exposure are fine; glob can be scoped to tests if desired.
pub mod account_fetcher;plususe solana_account::{Account, ReadableAccount};correctly support the newCommitTasklogic. The top-leveluse account_fetcher::*;appears to be used only indirectly by the test module; if production code doesn’t rely on those symbols, you could either gate that glob with#[cfg(test)]or importAccountFetcherdirectly inside the test module to keep the main module namespace tighter.Also applies to: 21-21, 36-37
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
magicblock-committor-service/src/tasks/mod.rs(7 hunks)magicblock-committor-service/src/tasks/task_strategist.rs(11 hunks)
🧰 Additional context used
🧠 Learnings (4)
📚 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/tasks/task_strategist.rsmagicblock-committor-service/src/tasks/mod.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-committor-service/src/tasks/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-committor-service/src/tasks/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-committor-service/src/tasks/mod.rs
🧬 Code graph analysis (2)
magicblock-committor-service/src/tasks/task_strategist.rs (1)
magicblock-committor-service/src/tasks/mod.rs (1)
new(126-157)
magicblock-committor-service/src/tasks/mod.rs (1)
magicblock-committor-service/src/tasks/account_fetcher.rs (1)
new(20-53)
⏰ 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_test
- GitHub Check: Build Project
- GitHub Check: run_make_ci_lint
🔇 Additional comments (1)
magicblock-committor-service/src/tasks/mod.rs (1)
421-443: Serialization tests correctly follow the async CommitTask API; consider isolating RPC behavior.The three serialization tests now:
- Use
CommitTask::new(...).awaitwith very small payloads (3 bytes) in the first two tests, which keeps them on the CommitState path and avoids unnecessary RPC.- Use a 1024‑byte payload in
test_preparation_instructions_serialization, which will crossCOMMIT_STATE_SIZE_THRESHOLDand triggerAccountFetcher::fetch_accountfor each run before falling back to buffer preparation logic.Functionally this is fine—the tests still exercise the instruction/serialization paths—but it does couple the last test to the configured RPC endpoint and can make it slower or environment‑dependent (especially since the pubkeys are random and fetches will likely fail). If that becomes an issue, consider:
- Injecting a mock/test
AccountFetcherfor these tests, or- Using a payload size below the threshold when you only care about buffer chunking/serialization, or
- Moving any “real RPC” coverage to an integration test that expects network access.
Also worth double‑checking that
fetch_accountuses a non-blocking pattern (e.g.,spawn_blocking) or that these tests always run on a multi-thread runtime, to avoid accidentally blocking the executor thread.Also applies to: 485-507, 512-535
⛔ Skipped due to learnings
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
snawaz
force-pushed
the
snawaz/commit-diff
branch
from
59b7935 to
c07a177
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
magicblock-committor-service/src/tasks/task_strategist.rs (1)
343-362: Use multi_thread runtime for tests that trigger blocking RPC calls.This test creates a 1000-byte account, which exceeds
COMMIT_STATE_SIZE_THRESHOLD(256 bytes).CommitTask::new().awaitwill callfetch_account(), which performs blocking RPC operations and requires a multi-threaded Tokio runtime to avoid hanging.Apply this diff:
- #[tokio::test] + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn test_build_strategy_optimizes_to_buffer_when_needed() {Also applies to: Lines 364-383, 385-409, 411-436, 438-457, 459-474, 476-515
magicblock-committor-service/src/tasks/mod.rs (1)
512-549: Use multi_thread runtime for test with 1024-byte account.This test creates a 1024-byte account, which exceeds
COMMIT_STATE_SIZE_THRESHOLD(256 bytes).CommitTask::new().awaitwill callfetch_account()that performs blocking RPC operations, requiring a multi-threaded Tokio runtime.The other two tests (
test_args_task_instruction_serializationandtest_buffer_task_instruction_serialization) use 3-byte accounts (below threshold) and correctly use the default#[tokio::test].Apply this diff:
- #[tokio::test] + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn test_preparation_instructions_serialization() {
♻️ Duplicate comments (2)
test-integration/schedulecommit/test-scenarios/tests/01_commits.rs (2)
100-101: Same issue: duplicate hardcoded seed.This is the second occurrence of the hardcoded
b"magic_schedule_commit"seed. See the comment on lines 46-47 for the suggestion to extract this to a constant.
232-232: Same issue: third occurrence of hardcoded seed.This is the third occurrence of the hardcoded
b"magic_schedule_commit"seed in the helper function. See the comment on lines 46-47 for the suggestion to extract this to a constant.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
magicblock-committor-service/src/tasks/mod.rs(7 hunks)magicblock-committor-service/src/tasks/task_strategist.rs(11 hunks)test-integration/schedulecommit/test-scenarios/tests/01_commits.rs(3 hunks)
🧰 Additional context used
🧠 Learnings (4)
📚 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-committor-service/src/tasks/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-committor-service/src/tasks/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-committor-service/src/tasks/mod.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/tasks/mod.rsmagicblock-committor-service/src/tasks/task_strategist.rstest-integration/schedulecommit/test-scenarios/tests/01_commits.rs
🧬 Code graph analysis (3)
magicblock-committor-service/src/tasks/mod.rs (1)
magicblock-committor-service/src/tasks/account_fetcher.rs (1)
new(20-53)
magicblock-committor-service/src/tasks/task_strategist.rs (1)
magicblock-committor-service/src/tasks/mod.rs (1)
new(126-157)
test-integration/schedulecommit/test-scenarios/tests/01_commits.rs (1)
test-integration/schedulecommit/test-scenarios/tests/utils/mod.rs (1)
get_context_with_delegated_committees(16-35)
⏰ 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: Build Project
- GitHub Check: run_make_ci_test
- GitHub Check: run_make_ci_lint
🔇 Additional comments (4)
magicblock-committor-service/src/tasks/mod.rs (4)
119-124: COMMIT_STATE_SIZE_THRESHOLD rationale is clear and well-documented.The comment explains the threshold choice (256 bytes can hold 8 u32 or 4 u64 fields) and the trade-off between CommitDiff (lower instruction size) vs CommitState (no RPC overhead). This addresses previous review feedback requesting documentation of this constant.
126-157: Async constructor properly handles base account fetch with fallback.The implementation correctly:
- Fetches base account only when data exceeds threshold
- Logs warnings on fetch failure and falls back to
commit_state- Handles the Result properly
This addresses multiple past review concerns about RPC integration and error handling.
159-168: Commit path selection logic is correct and defensive.The
is_commit_diff()check ensures all three conditions are met (not forced, exceeds threshold, and base_account fetched successfully) before using the diff path. Theforce_commit_state()method provides a clean escape hatch if needed.
170-219: Instruction builders correctly implement diff/state paths.The implementation properly:
- Selects diff or state path based on
base_accountavailability- Uses
compute_diffto generate minimal changeset- Falls back to state if
force_commit_stateis set (defensive, though redundant per past reviews)- Passes correct arguments to delegation program instruction builders
| ) -> Result<usize, SignerError> { | ||
| // Get initial transaction size | ||
| let calculate_tx_length = |tasks: &[Box<dyn BaseTask>]| { | ||
| // TODO (snawaz): we seem to discard lots of heavy computations here |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
TODO is valid but consider filing a tracking issue.
The TODO correctly identifies that calculate_tx_length builds and discards full transactions multiple times during optimization. This is a legitimate performance concern, but it's a pre-existing issue you're now documenting rather than introducing. Consider filing a GitHub issue to track this optimization opportunity so it doesn't get lost.
🤖 Prompt for AI Agents
magicblock-committor-service/src/tasks/task_strategist.rs around line 160: the
TODO noting that calculate_tx_length builds and discards full transactions
during optimization should be tracked externally; create a GitHub issue
describing the performance problem, expected impact, and possible remediation
approaches, then update the TODO comment to reference the new issue number
(e.g., "TODO (snawaz): see GH-<issue-number> — avoid rebuilding full txs in
calculate_tx_length") and include labels/assignee as appropriate so the
optimization isn’t lost.
| let ctx = | ||
| get_context_with_delegated_committees(1, b"magic_schedule_commit"); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Extract the hardcoded seed to a test constant.
The seed value b"magic_schedule_commit" is duplicated across multiple test functions (lines 47, 101, 232). Consider extracting it to a module-level constant for maintainability.
Apply this diff to define and use a constant:
+const TEST_USER_SEED: &[u8] = b"magic_schedule_commit";
+
#[test]
fn test_committing_one_account() {
run_test!({
let ctx =
- get_context_with_delegated_committees(1, b"magic_schedule_commit");
+ get_context_with_delegated_committees(1, TEST_USER_SEED);Then update the other two occurrences on lines 101 and 232 similarly.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In test-integration/schedulecommit/test-scenarios/tests/01_commits.rs around
lines 46-47, 101, and 232, the byte string seed b"magic_schedule_commit" is
duplicated; extract it to a module-level constant (e.g., const
MAGIC_SCHEDULE_COMMIT_SEED: &[u8] = b"magic_schedule_commit";) placed near the
top of the file, then replace the three hardcoded occurrences with the constant
name so all tests use the shared constant.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
magicblock-committor-service/src/tasks/mod.rs (3)
63-71:involved_accountsTODO is valid—recomputing full instructions is expensiveThe TODO accurately notes that
involved_accountscurrently rebuilds the full instruction (including diff computation for large commit tasks) just to collect pubkeys. That’s acceptable for now but potentially costly as CommitDiff usage grows. A future helper that extracts account metas without constructing args/diffs would be a worthwhile optimization.
110-117: CommitTask structure and threshold rationale are reasonableAdding
base_account: Option<Account>andforce_commit_statetoCommitTask, along withCOMMIT_STATE_SIZE_THRESHOLD = 256and its explanatory comment, gives a clear mechanism to decide between CommitDiff and CommitState. The threshold documentation (8×u32 or 4×u64) is helpful; if you later tune this constant empirically, the comment already sets the expectation that it’s about payload size trade-offs.Also applies to: 119-125
412-423: Fix tests:AccountFetcher::new()has been removed—construct viaAccountFetcher::with_client()with a testMagicblockRpcClientIn the serialization tests and other test locations,
CommitTask::new()is called withAccountFetcher::new(), butAccountFetcher::new()no longer exists. TheAccountFetcherstruct now requires construction viawith_client(rpc_client: MagicblockRpcClient).Affected locations:
magicblock-committor-service/src/tasks/mod.rs: lines 445, 509, 537 (inserialization_safety_test)test-integration/test-committor-service/tests/test_transaction_preparator.rs: lines 44, 103, 115, 207test-integration/test-committor-service/tests/common.rs: line 165magicblock-committor-service/src/tasks/task_strategist.rs: line 288Update all these calls to construct an
AccountFetcherviawith_client(). For unit tests, construct a testMagicblockRpcClientusing the pattern fromtest-integration/test-committor-service/tests/common.rs(lines 34–40), or create a shared helper to avoid duplication. For example:let rpc_client = RpcClient::new_with_commitment( "http://localhost:7799".to_string(), CommitmentConfig::confirmed() ); let account_fetcher = AccountFetcher::with_client(MagicblockRpcClient::new(Arc::new(rpc_client)));Apply the same pattern consistently across all affected test files.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
magicblock-committor-service/src/intent_executor/task_info_fetcher.rs(3 hunks)magicblock-committor-service/src/tasks/account_fetcher.rs(1 hunks)magicblock-committor-service/src/tasks/mod.rs(7 hunks)magicblock-committor-service/src/tasks/task_builder.rs(6 hunks)
🧰 Additional context used
🧠 Learnings (7)
📚 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-committor-service/src/tasks/account_fetcher.rsmagicblock-committor-service/src/intent_executor/task_info_fetcher.rsmagicblock-committor-service/src/tasks/mod.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-committor-service/src/tasks/account_fetcher.rsmagicblock-committor-service/src/intent_executor/task_info_fetcher.rs
📚 Learning: 2025-10-26T16:53:29.820Z
Learnt from: thlorenz
Repo: magicblock-labs/magicblock-validator PR: 587
File: magicblock-chainlink/src/remote_account_provider/mod.rs:134-0
Timestamp: 2025-10-26T16:53:29.820Z
Learning: In magicblock-chainlink/src/remote_account_provider/mod.rs, the `Endpoint::separate_pubsub_url_and_api_key()` method uses `split_once("?api-key=")` because the api-key parameter is always the only query parameter right after `?`. No additional query parameter parsing is needed for this use case.
Applied to files:
magicblock-committor-service/src/tasks/account_fetcher.rs
📚 Learning: 2025-11-07T18:19:48.996Z
Learnt from: Dodecahedr0x
Repo: magicblock-labs/magicblock-validator PR: 614
File: test-integration/programs/schedulecommit/Cargo.toml:19-20
Timestamp: 2025-11-07T18:19:48.996Z
Learning: In Solana programs (crates depending on solana-program), the empty feature flags `custom-heap = []` and `custom-panic = []` are standard declarations required to avoid compiler warnings. These should not be flagged as unused features in Cargo.toml files for Solana program crates.
Applied to files:
magicblock-committor-service/src/tasks/account_fetcher.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-committor-service/src/tasks/account_fetcher.rsmagicblock-committor-service/src/tasks/mod.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/tasks/task_builder.rsmagicblock-committor-service/src/tasks/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-committor-service/src/tasks/mod.rs
🧬 Code graph analysis (3)
magicblock-committor-service/src/tasks/task_builder.rs (2)
magicblock-committor-service/src/transaction_preparator/delivery_preparator.rs (2)
join_all(467-476)new(49-59)magicblock-committor-service/src/tasks/mod.rs (1)
new(126-162)
magicblock-committor-service/src/intent_executor/task_info_fetcher.rs (1)
magicblock-committor-service/src/tasks/account_fetcher.rs (1)
with_client(54-56)
magicblock-committor-service/src/tasks/mod.rs (2)
magicblock-committor-service/src/tasks/args_task.rs (1)
new(36-41)magicblock-committor-service/src/tasks/buffer_task.rs (2)
new(37-45)new_preparation_required(30-35)
🪛 GitHub Actions: Run CI - Lint
magicblock-committor-service/src/tasks/account_fetcher.rs
[error] Build failed: 6 previous errors prevented compilation (lint step).
magicblock-committor-service/src/tasks/task_builder.rs
[error] Build failed: 6 previous errors prevented compilation (lint step).
magicblock-committor-service/src/intent_executor/task_info_fetcher.rs
[error] Build failed: 6 previous errors prevented compilation (lint step).
magicblock-committor-service/src/tasks/mod.rs
[error] 445-445: AccountFetcher::new() not found. Use AccountFetcher::with_client(...) or implement a new constructor.
[error] 509-509: AccountFetcher::new() not found. Use AccountFetcher::with_client(...) or implement a new constructor.
[error] Build failed: 6 previous errors prevented compilation (lint step).
⏰ 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). (2)
- GitHub Check: run_make_ci_test
- GitHub Check: Build Project
🔇 Additional comments (7)
magicblock-committor-service/src/intent_executor/task_info_fetcher.rs (1)
14-15: Centralizing AccountFetcher creation via TaskInfoFetcher is a good directionWiring
new_account_fetcherintoTaskInfoFetcherand implementing it inCacheTaskInfoFetcherby cloning the sharedMagicblockRpcClientkeeps RPC config and client lifecycle out of task code, while still allowing per-task fetchers. This aligns with earlier concerns about constructing clients per account and is a solid intermediate step until all chain reads are fully moved into the fetcher.Also applies to: 41-42, 270-272
magicblock-committor-service/src/tasks/task_builder.rs (2)
47-50: Parallel CommitTask construction with TaskInfoFetcher wiring looks correctUsing
task_info_fetcher.fetch_next_commit_idsfollowed byjoin_allto asynchronously constructCommitTask::new(..., task_info_fetcher.new_account_fetcher()).awaitper account is a reasonable way to parallelize commit task creation while keeping RPC configuration centralized behind the fetcher. Aside from the persister bug above, the commit path wiring itself is coherent.Also applies to: 71-79, 89-103
108-110: Finalize path rename totask_info_fetchermaintains consistencyRenaming the finalize API parameter to
task_info_fetcherand using it to drivefetch_rent_reimbursementskeeps both commit and finalize flows on the same abstraction. The rest of the finalize logic is unchanged and remains straightforward.Also applies to: 165-175
magicblock-committor-service/src/tasks/mod.rs (4)
1-5: Publicly exposingaccount_fetcherand CommitDiff args integration is consistentRe-exporting
account_fetcherat the module level and importingCommitDiffArgs/CommitStateArgspluscompute_diffkeeps the commit-task module self-contained and ready to build either state or diff instructions. This matches the new CommitDiff flow described in the PR objectives.Also applies to: 36-37
126-162: Async CommitTask::new correctly gates base-account fetch and logs fallbacksThe async constructor:
- Only attempts a base-chain fetch when
committed_account.account.data.len() > COMMIT_STATE_SIZE_THRESHOLD.- Uses
account_fetcher.fetch_accountand distinguishes betweenOk(Some),Ok(None), andErr(e).- Logs clear warnings on not-found and RPC failure, then falls back to full CommitState.
This is a robust pattern that prevents CommitDiff from silently failing to engage without operator visibility, while still ensuring commits proceed via the state path.
183-196: CommitState/CommitDiff instruction builders are well-structuredBoth
create_commit_state_ixandcreate_commit_diff_ixbuild their respective args (including lamports, cloned data or diff, andallow_undelegation) and delegate to the appropriatedlp::instruction_builderfunctions. UsingReadableAccount::data()for the base account and computing the diff againstcommitted_account.account.data()is consistent with the CommitDiff semantics.Please double-check against the DLP
compute_diffandcommit_diff/commit_stateAPIs to ensure the argument ordering and data slices match the program’s expectations.Also applies to: 198-224
516-539: Consider whethertest_preparation_instructions_serializationshould trigger base-account fetchThis test uses
data: vec![0; 1024], which is aboveCOMMIT_STATE_SIZE_THRESHOLD(256), soCommitTask::newwill attempt to fetch the base account viaAccountFetcher. If yourMagicblockRpcClientperforms blocking I/O under the hood, this may require a multi-threaded Tokio runtime (or a stubbed client) to avoid hangs in unit tests. Alternatively, if you want this test to remain purely local/serialization-only, you could lower the test data size below the threshold so no RPC is attempted.
| pub struct AccountFetcher { | ||
| rpc_client: MagicblockRpcClient, | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
AccountFetcher injection-based design looks good; consider dropping dead constructor scaffolding
AccountFetcher now cleanly wraps an injected MagicblockRpcClient via with_client and fetch_account simply delegates to get_account, which aligns with earlier feedback about avoiding per-account RpcClient construction. The large commented-out new()/Default scaffolding no longer reflects how this type is used; consider removing it to reduce confusion and keep the module focused.
Also applies to: 54-63
🤖 Prompt for AI Agents
magicblock-committor-service/src/tasks/account_fetcher.rs lines 8-10 (and also
remove dead scaffolding at lines 54-63): the AccountFetcher struct now uses
dependency injection via with_client and the old commented-out/new()/Default
constructor scaffolding is dead code and misleading; delete the commented-out
new()/Default implementations and any unused constructor scaffolding (including
the block at lines 54-63) so the file only contains the injected constructor and
fetch_account delegation, and run cargo fmt/check to ensure no leftover imports
or warnings remain.
9da0edd to
cf6229d
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
magicblock-committor-service/src/tasks/task_strategist.rs (2)
253-292: Async helper wiring forcreate_test_commit_tasklooks correct.Using an async helper with
CommitTask::new(..., &Arc::new(NullTaskInfoFetcher)).awaitkeeps the tests aligned with the new constructor while intentionally avoiding real base‑account RPC. If you ever need many tasks in a single test, you could hoistlet fetcher = Arc::new(NullTaskInfoFetcher);and pass&fetcherto avoid constructing multiple Arcs, but that’s a micro‑optimization for tests.
459-474:test_optimize_strategy_prioritizes_largest_tasksis potentially brittle.Here both “larger” tasks are created with the same
data_size(1000), but the test assumestasks[1]is the one optimized toTaskStrategy::Buffer. The optimizer pulls from a heap keyed by size and index, so with equal sizes it may legitimately pick the last task (index 2) first, which could break this assertion depending on thresholds.Consider either:
- Making the “largest” task strictly larger, or
- Relaxing the assertion to check that the small task stays
Argsand at least one of the two large tasks becomesBuffer.For example:
- Box::new(create_test_commit_task(2, 1000).await) + Box::new(create_test_commit_task(2, 2000).await)This guarantees that
tasks[1]is unambiguously the largest and should be optimized first.test-integration/test-committor-service/tests/common.rs (1)
162-181:create_commit_taskhelper disables commit‑diff viaNullTaskInfoFetcher.The async helper now uses a static
AtomicU64for unique commit IDs andCommitTask::new(..., &Arc::new(NullTaskInfoFetcher)).await, which is a sensible default for tests that only care about commit‑state behavior and not diffs. If you later want this helper to be usable in commit‑diff scenarios, consider parameterizing it over a&Arc<impl TaskInfoFetcher>instead of hard‑wiringNullTaskInfoFetcher.
♻️ Duplicate comments (2)
magicblock-committor-service/src/tasks/task_builder.rs (1)
75-88: Fix persister call:&Option<P>doesn’t haveset_commit_id.
persisteris&Option<P>, but the code callspersister.set_commit_id(...)directly, which won’t compile and also ignores theNonecase conceptually.Guard the call with an
if let Some(persister):- // Persist commit ids for commitees - commit_ids - .iter() - .for_each(|(pubkey, commit_id) | { - if let Err(err) = persister.set_commit_id(base_intent.id, pubkey, *commit_id) { - error!("Failed to persist commit id: {}, for message id: {} with pubkey {}: {}", commit_id, base_intent.id, pubkey, err); - } - }); + // Persist commit ids for commitees (if a persister is configured) + if let Some(persister) = persister { + commit_ids.iter().for_each(|(pubkey, commit_id)| { + if let Err(err) = + persister.set_commit_id(base_intent.id, pubkey, *commit_id) + { + error!( + "Failed to persist commit id: {}, for message id: {} with pubkey {}: {}", + commit_id, base_intent.id, pubkey, err + ); + } + }); + }magicblock-committor-service/src/tasks/mod.rs (1)
112-171: CommitTask diff/state selection looks correct; consider decoupling the fetcher and diff-specific stateThe core behavior here looks good:
CommitTask::newgates base-account fetch behindCOMMIT_STATE_SIZE_THRESHOLDand cleanly falls back to CommitState with warnings onOk(None)andErr(_).force_commit_stateandis_commit_diffbehave coherently with the constructor invariants, andcreate_commit_ixdelegates to the appropriate builder.Two non-blocking improvements to consider:
The signature
pub async fn new<C: TaskInfoFetcher>(..., task_info_fetcher: &Arc<C>)unnecessarily couples the API toArc, forcing tests to pass&Arc::new(NullTaskInfoFetcher). A more flexible shape likepub async fn new<F>(..., fetcher: &F) -> Self where F: TaskInfoFetcher + ?Sized,would let callers use either a plain fetcher or an
Arc<F>without changing this type.
CommitTasknow represents both full-state and diff commits viabase_account: Option<Account>andforce_commit_state: bool. Long term, splitting this into explicit variants (e.g. a separateCommitDiffTaskor a task enum) would eliminate theOption<Account>/flag coupling and align with earlier suggestions to distinguish “commit full state” vs “commit diff” at the type level.Also applies to: 177-226
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (6)
magicblock-committor-service/src/intent_executor/task_info_fetcher.rs(4 hunks)magicblock-committor-service/src/tasks/mod.rs(6 hunks)magicblock-committor-service/src/tasks/task_builder.rs(6 hunks)magicblock-committor-service/src/tasks/task_strategist.rs(11 hunks)test-integration/test-committor-service/tests/common.rs(6 hunks)test-integration/test-committor-service/tests/test_transaction_preparator.rs(3 hunks)
🧰 Additional context used
🧠 Learnings (7)
📚 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/tasks/task_strategist.rsmagicblock-committor-service/src/tasks/task_builder.rsmagicblock-committor-service/src/tasks/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-committor-service/src/tasks/task_strategist.rsmagicblock-committor-service/src/tasks/mod.rstest-integration/test-committor-service/tests/common.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-committor-service/src/intent_executor/task_info_fetcher.rsmagicblock-committor-service/src/tasks/mod.rstest-integration/test-committor-service/tests/common.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-committor-service/src/intent_executor/task_info_fetcher.rsmagicblock-committor-service/src/tasks/mod.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-committor-service/src/intent_executor/task_info_fetcher.rstest-integration/test-committor-service/tests/common.rs
📚 Learning: 2025-11-12T09:46:27.553Z
Learnt from: Dodecahedr0x
Repo: magicblock-labs/magicblock-validator PR: 614
File: magicblock-task-scheduler/src/db.rs:26-0
Timestamp: 2025-11-12T09:46:27.553Z
Learning: In magicblock-task-scheduler, task parameter validation (including ensuring iterations > 0 and enforcing minimum execution intervals) is performed in the Magic program (on-chain) before ScheduleTaskRequest instances reach the scheduler service. The From<&ScheduleTaskRequest> conversion in db.rs does not need additional validation because inputs are already validated at the program level.
Applied to files:
magicblock-committor-service/src/tasks/mod.rs
📚 Learning: 2025-10-21T13:06:38.900Z
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 578
File: magicblock-aperture/src/requests/http/get_program_accounts.rs:17-25
Timestamp: 2025-10-21T13:06:38.900Z
Learning: The magicblock validator does not support ledger forking, so commitment-based state queries (processed/confirmed/finalized) are not applicable. RPC methods can safely ignore commitment and minContextSlot parameters from Solana RPC config objects.
Applied to files:
test-integration/test-committor-service/tests/common.rs
🧬 Code graph analysis (6)
test-integration/test-committor-service/tests/test_transaction_preparator.rs (3)
magicblock-committor-service/src/tasks/mod.rs (1)
new(128-164)test-integration/test-committor-service/tests/common.rs (2)
new(53-56)create_task_info_fetcher(117-119)magicblock-committor-service/src/tasks/buffer_task.rs (2)
new(37-45)new_preparation_required(30-35)
magicblock-committor-service/src/tasks/task_strategist.rs (2)
magicblock-committor-service/src/tasks/mod.rs (1)
new(128-164)magicblock-committor-service/src/intent_executor/mod.rs (1)
new(108-120)
magicblock-committor-service/src/intent_executor/task_info_fetcher.rs (2)
test-integration/test-committor-service/tests/common.rs (5)
get_base_account(146-151)fetch_next_commit_ids(126-131)fetch_rent_reimbursements(133-138)peek_commit_id(140-142)reset(144-144)magicblock-committor-service/src/intent_executor/mod.rs (4)
fetch_next_commit_ids(799-804)fetch_rent_reimbursements(806-811)peek_commit_id(813-815)reset(817-817)
magicblock-committor-service/src/tasks/task_builder.rs (3)
magicblock-committor-service/src/transaction_preparator/delivery_preparator.rs (2)
join_all(467-476)new(49-59)magicblock-committor-service/src/tasks/mod.rs (1)
new(128-164)magicblock-committor-service/src/intent_executor/mod.rs (1)
new(108-120)
magicblock-committor-service/src/tasks/mod.rs (3)
magicblock-committor-service/src/tasks/args_task.rs (2)
instruction(45-85)new(36-41)magicblock-committor-service/src/tasks/buffer_task.rs (3)
instruction(65-86)new(37-45)new_preparation_required(30-35)magicblock-committor-service/src/intent_executor/task_info_fetcher.rs (1)
new(61-69)
test-integration/test-committor-service/tests/common.rs (3)
magicblock-committor-service/src/intent_executor/task_info_fetcher.rs (4)
new(61-69)get_base_account(42-47)get_base_account(276-281)get_base_account(322-327)magicblock-committor-service/src/tasks/mod.rs (1)
new(128-164)magicblock-committor-service/src/intent_executor/mod.rs (1)
new(108-120)
⏰ 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: Build Project
- GitHub Check: run_make_ci_lint
- GitHub Check: run_make_ci_test
🔇 Additional comments (11)
magicblock-committor-service/src/tasks/task_strategist.rs (3)
159-171: TODO oncalculate_tx_lengthis fine and non‑behavioral.The added TODO correctly notes the repeated transaction construction cost but doesn’t alter behavior; the closure remains side‑effect‑free and safe to call within the optimization loop.
326-457: New async strategy tests exercise buffer/ALT paths well.The updated async tests (
test_build_strategy_with_single_small_task, buffer optimization, multi‑buffer, ALT usage, and failure when overfitting) are consistent with the strategy behavior and correctly driveTaskStrategist::build_strategyviaArgsTask/CommitTaskplusjoin_all. No issues spotted in the async usage or assertions.
476-515: Mixed task‑type optimization test is well‑structured.
test_mixed_task_types_with_optimizationgives a good end‑to‑end check: one large commit task plus finalize/base/undelegate, asserting that only the commit is buffered, others stayArgs, and ALTs are engaged. The expectations align with the intended strategy semantics.magicblock-committor-service/src/intent_executor/task_info_fetcher.rs (3)
11-15: Newget_base_accounthook onTaskInfoFetcheris well‑designed.Adding
async fn get_base_account(...) -> MagicBlockRpcClientResult<Option<Account>>with a defaultOk(None)cleanly extends the trait for commit‑diff support without breaking existing implementors. This keeps the “no base account available” case explicit and cheap.Also applies to: 41-47
275-281:CacheTaskInfoFetcher::get_base_accountcorrectly delegates to RPC.The implementation simply forwards to
self.rpc_client.get_account(pubkey).await, lettingMagicBlockRpcClientErrorflow through the existingTaskInfoFetcherError::MagicBlockRpcClientErrorpath. Given commit‑diff is an optimization, a single RPC attempt with upstream logging/fallback is reasonable here.
296-328:NullTaskInfoFetcheris appropriate for tests/dev utilities.For test/dev contexts, returning deterministic zeros and
Ok(None)fromget_base_accountkeeps behavior predictable and avoids external RPC dependencies, while still satisfying theTaskInfoFetchercontract.magicblock-committor-service/src/tasks/task_builder.rs (3)
23-37: Task builder trait signature updates are consistent with the new fetcher model.Switching both
commit_tasksandfinalize_tasksto accepttask_info_fetcher: &Arc<C>matches the updatedCommitTask::newAPI and centralizes all account/nonce lookups throughTaskInfoFetcher. The generics remain straightforward and keep the builder decoupled from a concrete fetcher implementation.Also applies to: 47-50
89-101: Parallel commit task construction viajoin_allis a good improvement.Building all commit tasks with
join_allover the committed accounts and a sharedtask_info_fetcheravoids repeated fetcher instantiation and keeps per‑account async work nicely parallelized. The closure correctly pullscommit_idfrom the pre‑fetched map and awaitsCommitTask::new(...)before boxing the task.
171-175: Finalize path correctly reusestask_info_fetcherfor rent reimbursements.Using
task_info_fetcher.fetch_rent_reimbursements(&pubkeys).awaitin the CommitAndUndelegate branch keeps rent‑payer resolution consistent with the commit side and centralizes the logic inTaskInfoFetcher.test-integration/test-committor-service/tests/common.rs (2)
9-18: Fixture now cleanly injects a task‑info fetcher into the intent executor.
TestFixture::create_intent_executordelegating toself.create_task_info_fetcher()(returningArc<MockTaskInfoFetcher>) aligns the integration tests with the production pattern of passing an Arc intoIntentExecutorImpl::new. This keeps tests exercising the same abstraction boundary.Also applies to: 102-119
122-152:MockTaskInfoFetcherimplementation matches theTaskInfoFetchercontract.Wrapping
MagicblockRpcClientand implementingfetch_next_commit_ids,fetch_rent_reimbursements,peek_commit_id,reset, andget_base_accountprovides a realistic but deterministic fetcher for tests. Delegatingget_base_accounttoself.0.get_account(pubkey).awaitensures commit‑diff logic can be exercised in integration tests without special hooks.
| let tasks = vec![ | ||
| Box::new(ArgsTask::new(ArgsTaskType::Commit(CommitTask { | ||
| commit_id: 1, | ||
| committed_account: committed_account.clone(), | ||
| allow_undelegation: true, | ||
| }))) as Box<dyn BaseTask>, | ||
| Box::new(ArgsTask::new(ArgsTaskType::Commit( | ||
| CommitTask::new( | ||
| 1, | ||
| true, | ||
| committed_account.clone(), | ||
| &fixture.create_task_info_fetcher(), | ||
| ) | ||
| .await, | ||
| ))) as Box<dyn BaseTask>, | ||
| Box::new(ArgsTask::new(ArgsTaskType::Finalize(FinalizeTask { |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Async CommitTask::new wiring in tests looks good.
All three tests correctly await CommitTask::new(..., &fixture.create_task_info_fetcher()) before wrapping in ArgsTask/BufferTask, so they exercise the new constructor and task‑info fetcher path end‑to‑end. If you want to trim a bit of duplication, you could bind a single let task_info_fetcher = fixture.create_task_info_fetcher(); per test and pass &task_info_fetcher into each CommitTask::new call instead of recreating it.
Also applies to: 96-105, 109-117, 200-209
🤖 Prompt for AI Agents
In test-integration/test-committor-service/tests/test_transaction_preparator.rs
around lines 37 to 47 (also apply similar change at 96-105, 109-117, 200-209):
tests repeatedly call fixture.create_task_info_fetcher() inline for each
CommitTask::new; bind let task_info_fetcher =
fixture.create_task_info_fetcher(); once at the start of the test and pass
&task_info_fetcher into each CommitTask::new invocation to remove duplication
while preserving the awaited constructor usage.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
magicblock-committor-service/src/intent_executor/task_info_fetcher.rs(4 hunks)magicblock-committor-service/src/tasks/args_task.rs(3 hunks)test-integration/test-committor-service/tests/test_delivery_preparator.rs(6 hunks)test-integration/test-committor-service/tests/test_ix_commit_local.rs(9 hunks)
🧰 Additional context used
🧠 Learnings (4)
📚 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-committor-service/tests/test_delivery_preparator.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:
test-integration/test-committor-service/tests/test_ix_commit_local.rsmagicblock-committor-service/src/tasks/args_task.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:
test-integration/test-committor-service/tests/test_ix_commit_local.rsmagicblock-committor-service/src/tasks/args_task.rsmagicblock-committor-service/src/intent_executor/task_info_fetcher.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-committor-service/src/intent_executor/task_info_fetcher.rs
🧬 Code graph analysis (4)
test-integration/test-committor-service/tests/test_delivery_preparator.rs (1)
test-integration/test-committor-service/tests/common.rs (2)
create_commit_task(163-181)new(53-56)
test-integration/test-committor-service/tests/test_ix_commit_local.rs (1)
magicblock-validator/src/main.rs (1)
init_logger(13-50)
magicblock-committor-service/src/tasks/args_task.rs (2)
magicblock-committor-service/src/tasks/mod.rs (1)
new(128-164)magicblock-committor-service/src/tasks/buffer_task.rs (2)
new(37-45)new_preparation_required(30-35)
magicblock-committor-service/src/intent_executor/task_info_fetcher.rs (1)
test-integration/test-committor-service/tests/common.rs (5)
get_base_account(146-151)fetch_next_commit_ids(126-131)fetch_rent_reimbursements(133-138)peek_commit_id(140-142)reset(144-144)
⏰ 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: Build Project
- GitHub Check: run_make_ci_test
🔇 Additional comments (7)
test-integration/test-committor-service/tests/test_delivery_preparator.rs (2)
2-2: Parallel commit-task construction viajoin_alllooks correct.Importing
join_alland using it overdatas.iter().map(|data| async { ... })to buildBufferTask/ArgsTaskinstances concurrently is sound, andjoin_allpreserves input order so the laterdatas[i]assertions stay valid.Also applies to: 90-96, 167-172
25-25: Good reuse ofcreate_commit_taskhelper in tests.Switching these tests to the shared
create_commit_task(&data).awaithelper keeps commit-id generation andCommitTaskconstruction consistent across test cases and avoids duplication; the addedmutbindings for tasks that are later mutated are appropriate.Also applies to: 212-212, 301-303
test-integration/test-committor-service/tests/test_ix_commit_local.rs (2)
65-68: Single-account 100-byte path now correctly asserts Args strategy.Expecting
CommitStrategy::Argsfor a 100-byte single-account commit aligns with the new preference for arg-based commits on small accounts and avoids unnecessary buffer usage.
151-152: Large-commit TODO documentation is clear and non-invasive.The extended TODOs describing 1 MB/10 MB and “max Solana account size” scenarios are well scoped and informative; leaving them in this test file as forward-looking notes is reasonable for now.
Also applies to: 465-486
magicblock-committor-service/src/tasks/args_task.rs (2)
1-1: Instruction path now correctly delegates toCommitTask::create_commit_ix.Routing
ArgsTaskType::Committhroughvalue.create_commit_ix(validator)and usingCallHandlerArgsfor base actions centralizes commit/commit-diff encoding inCommitTaskand keepsinstruction()free of RPC/side effects, which is a cleaner separation of concerns.Also applies to: 44-85
87-102: CommitDiff-awareoptimizebehavior is a reasonable interim downgrade strategy.The
value.is_commit_diff()guard plusvalue.force_commit_state()before wrapping inBufferTask::new_preparation_required(BufferTaskType::Commit(value))ensures that BufferTask only handles full commit-state payloads, while diff-capable tasks can still run via Args whenoptimize()is not invoked. This avoids panics and respects current BufferTask limitations; the TODO comment clearly marks the technical debt.magicblock-committor-service/src/intent_executor/task_info_fetcher.rs (1)
11-15: Newget_base_accountplumbing andNullTaskInfoFetcherlook sound.Extending
TaskInfoFetcherwithget_base_account(defaulting toOk(None)), wiringCacheTaskInfoFetcherthroughrpc_client.get_account, and introducingNullTaskInfoFetcherfor tests/dev contexts gives CommitTask a clean, injectable way to obtain base accounts for diff computation while preserving backward compatibility for existing fetchers. The error typing viaMagicBlockRpcClientResultis consistent with the RPC client, and returningOk(None)gracefully triggers the CommitState fallback.Also applies to: 42-47, 276-281, 296-328
| async fn get_base_account( | ||
| &self, | ||
| _pubkey: &Pubkey, | ||
| ) -> MagicBlockRpcClientResult<Option<Account>> { | ||
| Ok(None) // AccountNotFound | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider batched base-account fetching for high-volume CommitDiff usage.
get_base_account currently issues one get_account RPC per pubkey via CacheTaskInfoFetcher, which is fine initially but may become a bottleneck when many large delegated accounts are committed in one run. Longer term, you might want a batched variant (e.g., get_base_accounts(&[Pubkey]) backed by get_multiple_accounts) to reduce round-trips and better align with the existing metadata-fetch retry logic.
Also applies to: 276-281
|
I made the following changes:
The decision to make what variant of I'm not resolving Rabbit's feedbacks yet — they're mostly minor and I'll see them in next week. The last 3 commits are the latest ones with these commit messages (commit-ids might change):
|
taco-paco
left a comment
taco-paco
left a comment
There was a problem hiding this comment.
Looks much better, bet there still some things we shall improve, especially testing for Diff.
| async fn get_base_account( | ||
| &self, | ||
| _pubkey: &Pubkey, | ||
| ) -> MagicBlockRpcClientResult<Option<Account>> { | ||
| Ok(None) // AccountNotFound | ||
| } |
There was a problem hiding this comment.
Is it needed for tests?
| async fn get_base_account( | |
| &self, | |
| _pubkey: &Pubkey, | |
| ) -> MagicBlockRpcClientResult<Option<Account>> { | |
| Ok(None) // AccountNotFound | |
| } | |
| async fn get_base_account( | |
| &self, | |
| _pubkey: &Pubkey, | |
| ) -> MagicBlockRpcClientResult<Option<Account>>; |
Let's let implementors define default behaviour.
| #[cfg(any(test, feature = "dev-context-only-utils"))] | ||
| pub struct NullTaskInfoFetcher; | ||
|
|
||
| #[cfg(any(test, feature = "dev-context-only-utils"))] | ||
| #[async_trait] | ||
| impl TaskInfoFetcher for NullTaskInfoFetcher { | ||
| async fn fetch_next_commit_ids( | ||
| &self, | ||
| _pubkeys: &[Pubkey], | ||
| ) -> TaskInfoFetcherResult<HashMap<Pubkey, u64>> { | ||
| Ok(Default::default()) | ||
| } | ||
|
|
||
| async fn fetch_rent_reimbursements( | ||
| &self, | ||
| _pubkeys: &[Pubkey], | ||
| ) -> TaskInfoFetcherResult<Vec<Pubkey>> { | ||
| Ok(Default::default()) | ||
| } | ||
|
|
||
| fn peek_commit_id(&self, _pubkey: &Pubkey) -> Option<u64> { | ||
| None | ||
| } | ||
|
|
||
| fn reset(&self, _: ResetType) {} | ||
|
|
||
| async fn get_base_account( | ||
| &self, | ||
| _pubkey: &Pubkey, | ||
| ) -> MagicBlockRpcClientResult<Option<Account>> { | ||
| Ok(None) // AccountNotFound | ||
| } | ||
| } |
There was a problem hiding this comment.
Let's move this in separate tests.rs file or somehing like mock.rs
| let base_account = if committed_account.account.data.len() | ||
| > CommitTask::COMMIT_STATE_SIZE_THRESHOLD | ||
| { | ||
| match task_info_fetcher |
There was a problem hiding this comment.
Why don't we fetch this outside?
We can supply base_account as an argument instead.
- We would remove async from constructor
- We wouldn't need TaskInfoFetcher at all in
CommitTask - Task could still decide for itself if it needs to upgrade or not as was your idea
There was a problem hiding this comment.
No. We should not do that. base_account is conditionally fetched by CommitTask.
There was a problem hiding this comment.
I used CommitTaskBuilder which fetches the base account and then creates the task.
| } | ||
|
|
||
| pub fn create_commit_ix(&self, validator: &Pubkey) -> Instruction { | ||
| if let Some(fetched_account) = self.base_account.as_ref() { |
There was a problem hiding this comment.
Shouldn't we check is_commit_diff here?
There was a problem hiding this comment.
Yes. That can be used as well, but we still need to unwrap base_account here. So I kept it simple here.
| (logs, sig) | ||
| }; | ||
|
|
||
| println!("Ephem Logs level-1: {:#?}", ephem_logs_l1); |
There was a problem hiding this comment.
Looks like specific to a particular test.
This shall be removed
| committed_account: account.clone(), | ||
| }); | ||
| account.clone(), | ||
| task_info_fetcher, |
There was a problem hiding this comment.
Here we could supply base_account directly
There was a problem hiding this comment.
There's no need for constructor to be async
There was a problem hiding this comment.
I explained in the previous comment why I passed task_info_fetcher instead.
There was a problem hiding this comment.
We missing tests for Diff here. Right now NullTaskInfoFetcher::get_base_account always returns Ok(None).
So we only test direct commit.
We shall have various test that ensure that resulting task is Diff as expected or is Direct commit as expected kind of like here
assert!(matches!(
strategy.optimized_tasks[0].strategy(),
TaskStrategy::Buffer
));
| } | ||
|
|
||
| pub fn is_commit_diff(&self) -> bool { | ||
| !self.force_commit_state |
There was a problem hiding this comment.
This seems rather complicated, comparing to the case if we would have separate type for Diff.
We would just need to check the type.
match commit_task_type {
CommitTaskType::CommitDirect => false,
CommitTaskType::CommitDiff => true
}
There was a problem hiding this comment.
Your suggestion merely replaced boolean with enum. The value of enum (i.e commit_task_type) still needs to be computed. Its not decided by the caller, but the task itself.
| CommitType::StandaloneDiff(accounts) => { | ||
| accounts.iter().map(finalize_task).collect() | ||
| } |
There was a problem hiding this comment.
Not going to modify the working old branch in this PR. Note that I'm a new developer and I do not want to be too clever in my first PR.
| let base_account = if committed_account.account.data.len() | ||
| > CommitTask::COMMIT_STATE_SIZE_THRESHOLD | ||
| { | ||
| match task_info_fetcher |
There was a problem hiding this comment.
No. We should not do that. base_account is conditionally fetched by CommitTask.
| } | ||
|
|
||
| pub fn is_commit_diff(&self) -> bool { | ||
| !self.force_commit_state |
There was a problem hiding this comment.
Your suggestion merely replaced boolean with enum. The value of enum (i.e commit_task_type) still needs to be computed. Its not decided by the caller, but the task itself.
| } | ||
|
|
||
| pub fn create_commit_ix(&self, validator: &Pubkey) -> Instruction { | ||
| if let Some(fetched_account) = self.base_account.as_ref() { |
There was a problem hiding this comment.
Yes. That can be used as well, but we still need to unwrap base_account here. So I kept it simple here.
| committed_account: account.clone(), | ||
| }); | ||
| account.clone(), | ||
| task_info_fetcher, |
There was a problem hiding this comment.
I explained in the previous comment why I passed task_info_fetcher instead.
snawaz
force-pushed
the
snawaz/commit-diff
branch
from
9e8da2d to
34d41c2
Compare
📝 WalkthroughWalkthroughThis PR extends the committor service with adaptive commit strategy selection based on account data availability. It introduces a Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
test-integration/test-committor-service/tests/test_ix_commit_local.rs (1)
65-345: Updated CommitStrategy expectations look coherent with Args/CommitDiff-first planner.The shifts from
FromBuffertoArgs(and mixed Args/FromBufferWithLookupTable in bundle tests) are internally consistent and match the documented behavior where small deltas on reasonably sized accounts stay in-args and larger/more crowded bundles still use buffers + ALTs.If you expect commit-planner thresholds to keep evolving, consider, in a later PR, relaxing some of these exact-count assertions (e.g., asserting minimum Args usage or upper bounds on buffer commits) to avoid brittleness.
test-integration/test-committor-service/tests/test_transaction_preparator.rs (1)
3-13: Import forCommitTaskBuilderis missing (compile error)This file uses
CommitTaskBuilder::create_commit_task(...)but never importsCommitTaskBuilderfrommagicblock_committor_service::tasks, so it won’t compile.Add
CommitTaskBuilderto thetasksimport list:use magicblock_committor_service::{ persist::IntentPersisterImpl, tasks::{ args_task::{ArgsTask, ArgsTaskType}, buffer_task::{BufferTask, BufferTaskType}, task_strategist::{TaskStrategist, TransactionStrategy}, utils::TransactionUtils, - BaseActionTask, BaseTask, CommitTask, FinalizeTask, PreparationState, + BaseActionTask, BaseTask, CommitTask, CommitTaskBuilder, + FinalizeTask, PreparationState, UndelegateTask, },After that, all the new
CommitTaskBuilder::create_commit_task(...)usages in this test should compile as intended.If you want to reduce churn in tests, you can also bind
let task_info_fetcher = fixture.create_task_info_fetcher();once per test and pass&task_info_fetcherinto each builder call instead of recreating it each time.Also applies to: 38-46, 96-105, 109-117, 200-209
♻️ Duplicate comments (8)
test-integration/test-tools/src/scheduled_commits.rs (1)
182-196: Reduce or remove unconditional ephem-logprintln!debug outputThese two
println!calls:println!("Ephem Logs level-1: {:#?}", ephem_logs_l1); println!("Ephem Logs level-2: {:#?}", ephem_logs_l2);will dump entire log vectors on every successful run of
fetch_schedule_commit_result, making all integration tests very noisy and harder to scan, especially when accounts/logs are large. The error-context messages around the fetches already include the logs when something goes wrong, so keeping these unconditional prints provides limited extra value.Given the prior review note (“This shall be removed”) about similar debug prints here, I’d recommend:
- Either removing these two
println!s before merging, or- Gating them behind an explicit debug toggle (e.g., an env var like
MAGICBLOCK_DEBUG_LOGSor aboolargument) so they only run when you’re actively debugging.Minor nit (optional): consider renaming
scheduled_commmit_sent_sig→scheduled_commit_sent_sigto avoid the doublemmtypo and improve readability.Also applies to: 198-211, 213-213
test-integration/schedulecommit/test-scenarios/tests/utils/mod.rs (1)
41-47: Remove unnecessary trait bounds onTinassert_one_committee_was_committed.The function body never inspects or compares values of type
T; it only looks at theScheduledCommitResult’s structural fields. The bounds
std::fmt::Debugborsh::BorshDeserializePartialEq + Eqare therefore unused and can be dropped to make this helper usable with any
Tcarried inScheduledCommitResult.-pub fn assert_one_committee_was_committed<T>( +pub fn assert_one_committee_was_committed<T>( ctx: &ScheduleCommitTestContext, - res: &ScheduledCommitResult<T>, - is_single_stage: bool, -) where - T: std::fmt::Debug + borsh::BorshDeserialize + PartialEq + Eq, -{ + res: &ScheduledCommitResult<T>, + is_single_stage: bool, +) {test-integration/programs/schedulecommit/src/order_book.rs (1)
127-130: Capacity exhaustion inupdate_fromstill silently no-ops.
update_fromignores theOptionreturned byadd_bids/add_asks, so if an update doesn’t fit, it is silently dropped instead of panicking or surfacing an error. For a test helper, this makes it harder to detect when the book’s configured capacity is too small for a scenario.If you want capacity issues to be loud (as previously discussed), consider tightening this to panic:
pub fn update_from(&mut self, updates: BookUpdate) { - self.add_bids(&updates.bids); - self.add_asks(&updates.asks); + self.add_bids(&updates.bids) + .expect("order_book capacity exhausted while adding bids"); + self.add_asks(&updates.asks) + .expect("order_book capacity exhausted while adding asks"); }This keeps the semantics simple for tests: either the full update applies or the test fails immediately.
test-integration/schedulecommit/client/src/verify.rs (1)
2-23: Duplication for OrderBook-owned variant is acceptable given prior discussion.The new
fetch_and_verify_order_book_commit_result_from_logsmirrorsfetch_and_verify_commit_result_from_logsbut forOrderBookOwned. Since you've already decided not to genericize this helper in earlier review, the current duplication is fine and keeps call sites simple.magicblock-committor-service/src/tasks/task_strategist.rs (2)
159-171: TODO about repeatedassemble_tasks_txwork is valid; fine to leave as tracked debt.The comment at Line 160 correctly notes that
calculate_tx_lengthrebuilds and encodes full transactions multiple times during optimization. That’s real performance debt, but it’s pre-existing behavior you’re just documenting. Keeping the TODO here is fine for now; just ensure it’s tracked in an issue so it doesn’t get lost.
271-294: Strategist tests exercise buffer/ALT paths but not CommitDiff behavior.The new async helper
create_test_commit_taskusesNullTaskInfoFetcher, sobase_accountis alwaysNoneand these tests only cover CommitState→Buffer/ALT behavior. As taco-paco noted, you’ll eventually want strategist-level tests whereTaskInfoFetcherreturns a base account so you can assert:
- When a diff-eligible task stays on the Args path, and
- How it behaves once buffer optimization kicks in.
Not required for this PR, but worth adding once CommitDiff semantics settle.
Also applies to: 328-439
test-integration/programs/schedulecommit/src/api.rs (1)
36-75: Order-book grow/update helpers still bypassbook_managerauthorization.
init_order_book_instructionrequiresbook_manageras a signer (AccountMeta::new_readonly(book_manager, true)), but:
grow_order_book_instructionusesAccountMeta::new_readonly(book_manager, false), andupdate_order_book_instructionomitsbook_managerentirely.This lets any payer invoke grow/update on any order book in these helpers. Even though this is under
test-integration/programs, if the intent is to model realistic authorization, consider requiringbook_manageras a signer for grow and including it (and validating it) for update as well, or at least documenting the relaxed model for tests.Also applies to: 178-195
test-integration/programs/schedulecommit/src/lib.rs (1)
334-351: Prevent size overflow before realloc
new_size = data_len() + additional_space as usizetrusts attacker-controlled input. On BPF this wraps in release mode, so a largeadditional_spacecan underflow the sum and shrink the buffer, truncating order book data. Use checked addition and error out instead.- let new_size = order_book.data_len() + additional_space as usize; + let additional = additional_space as usize; + let new_size = order_book + .data_len() + .checked_add(additional) + .ok_or(ProgramError::InvalidArgument)?;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (3)
Cargo.lockis excluded by!**/*.locktest-integration/Cargo.lockis excluded by!**/*.locktest-integration/schedulecommit/elfs/dlp.sois excluded by!**/*.so
📒 Files selected for processing (32)
Cargo.toml(1 hunks)magicblock-committor-service/Cargo.toml(1 hunks)magicblock-committor-service/src/intent_execution_manager/intent_execution_engine.rs(2 hunks)magicblock-committor-service/src/intent_executor/mod.rs(3 hunks)magicblock-committor-service/src/intent_executor/null_task_info_fetcher.rs(1 hunks)magicblock-committor-service/src/intent_executor/task_info_fetcher.rs(3 hunks)magicblock-committor-service/src/tasks/args_task.rs(3 hunks)magicblock-committor-service/src/tasks/mod.rs(6 hunks)magicblock-committor-service/src/tasks/task_builder.rs(6 hunks)magicblock-committor-service/src/tasks/task_strategist.rs(11 hunks)programs/magicblock/src/magic_scheduled_base_intent.rs(1 hunks)test-integration/Cargo.toml(4 hunks)test-integration/programs/schedulecommit/Cargo.toml(1 hunks)test-integration/programs/schedulecommit/src/api.rs(5 hunks)test-integration/programs/schedulecommit/src/lib.rs(8 hunks)test-integration/programs/schedulecommit/src/order_book.rs(1 hunks)test-integration/programs/schedulecommit/src/utils/mod.rs(3 hunks)test-integration/schedulecommit/client/src/schedule_commit_context.rs(6 hunks)test-integration/schedulecommit/client/src/verify.rs(2 hunks)test-integration/schedulecommit/test-scenarios/Cargo.toml(1 hunks)test-integration/schedulecommit/test-scenarios/tests/01_commits.rs(3 hunks)test-integration/schedulecommit/test-scenarios/tests/02_commit_and_undelegate.rs(4 hunks)test-integration/schedulecommit/test-scenarios/tests/utils/mod.rs(1 hunks)test-integration/schedulecommit/test-security/tests/01_invocations.rs(1 hunks)test-integration/test-committor-service/tests/common.rs(6 hunks)test-integration/test-committor-service/tests/test_delivery_preparator.rs(6 hunks)test-integration/test-committor-service/tests/test_ix_commit_local.rs(9 hunks)test-integration/test-committor-service/tests/test_transaction_preparator.rs(3 hunks)test-integration/test-committor-service/tests/utils/transactions.rs(1 hunks)test-integration/test-ledger-restore/tests/08_commit_update.rs(2 hunks)test-integration/test-tools/src/integration_test_context.rs(2 hunks)test-integration/test-tools/src/scheduled_commits.rs(3 hunks)
🧰 Additional context used
🧠 Learnings (18)
📚 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:
test-integration/test-committor-service/tests/test_ix_commit_local.rsmagicblock-committor-service/src/intent_executor/task_info_fetcher.rsmagicblock-committor-service/src/intent_executor/null_task_info_fetcher.rsmagicblock-committor-service/src/intent_executor/mod.rsmagicblock-committor-service/src/tasks/mod.rsprograms/magicblock/src/magic_scheduled_base_intent.rsmagicblock-committor-service/src/tasks/task_builder.rsmagicblock-committor-service/src/tasks/task_strategist.rstest-integration/test-committor-service/tests/common.rstest-integration/test-committor-service/tests/test_delivery_preparator.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-committor-service/tests/test_ix_commit_local.rstest-integration/test-tools/src/integration_test_context.rsmagicblock-committor-service/src/intent_executor/task_info_fetcher.rstest-integration/test-ledger-restore/tests/08_commit_update.rsmagicblock-committor-service/src/intent_executor/mod.rstest-integration/schedulecommit/test-scenarios/tests/utils/mod.rsmagicblock-committor-service/src/tasks/mod.rstest-integration/test-committor-service/tests/test_delivery_preparator.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:
test-integration/test-committor-service/tests/test_ix_commit_local.rstest-integration/programs/schedulecommit/src/utils/mod.rsmagicblock-committor-service/src/intent_executor/task_info_fetcher.rstest-integration/programs/schedulecommit/src/api.rsmagicblock-committor-service/src/intent_executor/mod.rsmagicblock-committor-service/src/tasks/mod.rstest-integration/schedulecommit/client/src/schedule_commit_context.rstest-integration/programs/schedulecommit/src/order_book.rsprograms/magicblock/src/magic_scheduled_base_intent.rsmagicblock-committor-service/src/tasks/args_task.rstest-integration/programs/schedulecommit/src/lib.rstest-integration/schedulecommit/test-scenarios/tests/01_commits.rstest-integration/test-committor-service/tests/common.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-tools/src/integration_test_context.rstest-integration/programs/schedulecommit/src/utils/mod.rstest-integration/schedulecommit/test-security/tests/01_invocations.rstest-integration/schedulecommit/test-scenarios/tests/02_commit_and_undelegate.rstest-integration/schedulecommit/client/src/verify.rstest-integration/schedulecommit/test-scenarios/tests/utils/mod.rsmagicblock-committor-service/src/tasks/mod.rstest-integration/test-committor-service/tests/test_transaction_preparator.rsmagicblock-committor-service/src/tasks/task_builder.rsmagicblock-committor-service/src/tasks/task_strategist.rstest-integration/schedulecommit/test-scenarios/tests/01_commits.rstest-integration/test-committor-service/tests/test_delivery_preparator.rs
📚 Learning: 2025-10-26T16:54:39.084Z
Learnt from: thlorenz
Repo: magicblock-labs/magicblock-validator PR: 587
File: test-manual/Cargo.toml:0-0
Timestamp: 2025-10-26T16:54:39.084Z
Learning: In the magicblock-validator repository, use git branch references (not commit hashes or tags) for the helius-laserstream dependency to allow automatic updates when the branch is pushed to.
Applied to files:
Cargo.tomltest-integration/Cargo.toml
📚 Learning: 2025-11-21T13:56:03.885Z
Learnt from: snawaz
Repo: magicblock-labs/magicblock-validator PR: 575
File: test-integration/programs/schedulecommit/src/utils/mod.rs:71-71
Timestamp: 2025-11-21T13:56:03.885Z
Learning: In Solana programs (BPF runtime), defensive checked conversions to `usize` (e.g., `try_into()`) are unnecessary when casting from `u64`. Solana exclusively runs in 64-bit environments where `usize` is 64 bits, so truncation is not a concern and simple `as usize` casts are appropriate.
Applied to files:
test-integration/programs/schedulecommit/src/utils/mod.rstest-integration/programs/schedulecommit/src/order_book.rstest-integration/programs/schedulecommit/src/lib.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:
test-integration/programs/schedulecommit/src/utils/mod.rsmagicblock-committor-service/src/intent_executor/task_info_fetcher.rstest-integration/test-ledger-restore/tests/08_commit_update.rsmagicblock-committor-service/src/intent_executor/mod.rsmagicblock-committor-service/src/tasks/mod.rstest-integration/programs/schedulecommit/src/order_book.rstest-integration/Cargo.tomlmagicblock-committor-service/src/tasks/args_task.rsmagicblock-committor-service/src/tasks/task_strategist.rstest-integration/test-committor-service/tests/common.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-committor-service/src/intent_executor/task_info_fetcher.rsmagicblock-committor-service/src/intent_executor/mod.rsmagicblock-committor-service/src/tasks/mod.rstest-integration/schedulecommit/client/src/schedule_commit_context.rstest-integration/programs/schedulecommit/src/order_book.rsprograms/magicblock/src/magic_scheduled_base_intent.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-committor-service/src/intent_executor/task_info_fetcher.rsmagicblock-committor-service/src/intent_execution_manager/intent_execution_engine.rsmagicblock-committor-service/src/intent_executor/mod.rstest-integration/test-committor-service/tests/common.rs
📚 Learning: 2025-10-26T16:53:29.820Z
Learnt from: thlorenz
Repo: magicblock-labs/magicblock-validator PR: 587
File: magicblock-chainlink/src/remote_account_provider/mod.rs:134-0
Timestamp: 2025-10-26T16:53:29.820Z
Learning: In magicblock-chainlink/src/remote_account_provider/mod.rs, the `Endpoint::separate_pubsub_url_and_api_key()` method uses `split_once("?api-key=")` because the api-key parameter is always the only query parameter right after `?`. No additional query parameter parsing is needed for this use case.
Applied to files:
magicblock-committor-service/src/intent_executor/task_info_fetcher.rs
📚 Learning: 2025-11-19T12:55:48.931Z
Learnt from: Dodecahedr0x
Repo: magicblock-labs/magicblock-validator PR: 639
File: test-integration/test-tools/src/validator.rs:193-197
Timestamp: 2025-11-19T12:55:48.931Z
Learning: In the magicblock-validator codebase, when constructing arguments for the light CLI test validator in start_light_validator_with_config (test-integration/test-tools/src/validator.rs), the shlex::split approach is necessary because the light CLI does not handle whitespaces properly in validator arguments. The string concatenation + shlex parsing pattern should not be refactored to direct argument construction in this specific case.
Applied to files:
test-integration/test-ledger-restore/tests/08_commit_update.rs
📚 Learning: 2025-11-20T08:57:07.217Z
Learnt from: thlorenz
Repo: magicblock-labs/magicblock-validator PR: 650
File: magicblock-chainlink/src/submux/subscription_task.rs:13-99
Timestamp: 2025-11-20T08:57:07.217Z
Learning: In the magicblock-validator repository, avoid posting review comments that merely confirm code is correct or matches intended behavior without providing actionable feedback, suggestions for improvement, or identifying potential issues. Such confirmatory comments are considered unhelpful noise by the maintainers.
Applied to files:
test-integration/test-tools/src/scheduled_commits.rs
📚 Learning: 2025-11-21T11:03:26.756Z
Learnt from: thlorenz
Repo: magicblock-labs/magicblock-validator PR: 664
File: magicblock-chainlink/src/testing/mod.rs:342-370
Timestamp: 2025-11-21T11:03:26.756Z
Learning: In the magicblock-validator codebase, avoid leaving review comments that merely acknowledge code is correct or well-structured when there is no actionable suggestion, improvement, or issue to flag. Only comment when there is something specific to recommend, fix, or clarify.
Applied to files:
test-integration/test-tools/src/scheduled_commits.rs
📚 Learning: 2025-11-07T18:19:48.996Z
Learnt from: Dodecahedr0x
Repo: magicblock-labs/magicblock-validator PR: 614
File: test-integration/programs/schedulecommit/Cargo.toml:19-20
Timestamp: 2025-11-07T18:19:48.996Z
Learning: In Solana programs (crates depending on solana-program), the empty feature flags `custom-heap = []` and `custom-panic = []` are standard declarations required to avoid compiler warnings. These should not be flagged as unused features in Cargo.toml files for Solana program crates.
Applied to files:
test-integration/programs/schedulecommit/Cargo.toml
📚 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-committor-service/src/intent_executor/mod.rs
📚 Learning: 2025-11-12T09:46:27.553Z
Learnt from: Dodecahedr0x
Repo: magicblock-labs/magicblock-validator PR: 614
File: magicblock-task-scheduler/src/db.rs:26-0
Timestamp: 2025-11-12T09:46:27.553Z
Learning: In magicblock-task-scheduler, task parameter validation (including ensuring iterations > 0 and enforcing minimum execution intervals) is performed in the Magic program (on-chain) before ScheduleTaskRequest instances reach the scheduler service. The From<&ScheduleTaskRequest> conversion in db.rs does not need additional validation because inputs are already validated at the program level.
Applied to files:
magicblock-committor-service/src/tasks/mod.rs
📚 Learning: 2025-11-20T17:25:23.444Z
Learnt from: JMirval
Repo: magicblock-labs/magicblock-validator PR: 656
File: programs/guinea/src/lib.rs:33-33
Timestamp: 2025-11-20T17:25:23.444Z
Learning: In the magicblock-validator codebase, task IDs (task_id) are i64 values that can be negative. The task scheduler is designed to handle any i64 value including negative task IDs. Task IDs are randomly generated using rand::random() which produces values across the full i64 range. No validation is needed to restrict task IDs to positive values.
Applied to files:
magicblock-committor-service/src/tasks/task_strategist.rs
📚 Learning: 2025-10-21T13:06:38.900Z
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 578
File: magicblock-aperture/src/requests/http/get_program_accounts.rs:17-25
Timestamp: 2025-10-21T13:06:38.900Z
Learning: The magicblock validator does not support ledger forking, so commitment-based state queries (processed/confirmed/finalized) are not applicable. RPC methods can safely ignore commitment and minContextSlot parameters from Solana RPC config objects.
Applied to files:
test-integration/test-committor-service/tests/common.rs
🧬 Code graph analysis (20)
test-integration/test-committor-service/tests/test_ix_commit_local.rs (1)
magicblock-validator/src/main.rs (1)
init_logger(13-50)
magicblock-committor-service/src/intent_executor/task_info_fetcher.rs (4)
magicblock-committor-service/src/intent_execution_manager/intent_execution_engine.rs (1)
get_base_account(804-809)magicblock-committor-service/src/intent_executor/mod.rs (1)
get_base_account(827-832)magicblock-committor-service/src/intent_executor/null_task_info_fetcher.rs (1)
get_base_account(36-41)test-integration/test-committor-service/tests/common.rs (1)
get_base_account(146-151)
test-integration/test-ledger-restore/tests/08_commit_update.rs (1)
test-integration/test-ledger-restore/src/lib.rs (1)
setup_validator_with_local_remote(97-119)
test-integration/schedulecommit/test-security/tests/01_invocations.rs (1)
test-integration/schedulecommit/client/src/schedule_commit_context.rs (2)
try_new(72-74)try_new_random_keys(66-71)
magicblock-committor-service/src/intent_execution_manager/intent_execution_engine.rs (3)
magicblock-committor-service/src/intent_executor/null_task_info_fetcher.rs (1)
get_base_account(36-41)magicblock-committor-service/src/intent_executor/task_info_fetcher.rs (2)
get_base_account(42-45)get_base_account(274-279)test-integration/test-committor-service/tests/common.rs (1)
get_base_account(146-151)
magicblock-committor-service/src/intent_executor/null_task_info_fetcher.rs (4)
magicblock-committor-service/src/intent_execution_manager/intent_execution_engine.rs (6)
fetch_next_commit_ids(784-789)fetch_rent_reimbursements(791-796)peek_commit_id(798-800)None(393-393)reset(802-802)get_base_account(804-809)magicblock-committor-service/src/intent_executor/mod.rs (7)
fetch_next_commit_ids(807-812)fetch_rent_reimbursements(814-819)peek_commit_id(821-823)None(844-844)None(860-860)reset(825-825)get_base_account(827-832)magicblock-committor-service/src/intent_executor/task_info_fetcher.rs (10)
fetch_next_commit_ids(25-28)fetch_next_commit_ids(167-234)fetch_rent_reimbursements(31-34)fetch_rent_reimbursements(236-251)peek_commit_id(37-37)peek_commit_id(254-257)reset(40-40)reset(260-272)get_base_account(42-45)get_base_account(274-279)test-integration/test-committor-service/tests/common.rs (5)
fetch_next_commit_ids(126-131)fetch_rent_reimbursements(133-138)peek_commit_id(140-142)reset(144-144)get_base_account(146-151)
test-integration/schedulecommit/test-scenarios/tests/02_commit_and_undelegate.rs (4)
test-integration/programs/schedulecommit/src/api.rs (3)
schedule_commit_and_undelegate_cpi_instruction(237-255)schedule_commit_diff_instruction_for_order_book(196-215)update_order_book_instruction(178-194)test-integration/schedulecommit/test-scenarios/tests/utils/mod.rs (2)
get_context_with_delegated_committees(16-35)assert_one_committee_was_committed(41-65)test-integration/programs/schedulecommit/src/order_book.rs (1)
bids(171-173)test-integration/schedulecommit/client/src/verify.rs (1)
fetch_and_verify_order_book_commit_result_from_logs(16-23)
test-integration/schedulecommit/client/src/verify.rs (2)
test-integration/schedulecommit/test-security/tests/01_invocations.rs (1)
ctx(148-149)test-integration/test-ledger-restore/tests/08_commit_update.rs (1)
ctx(90-91)
magicblock-committor-service/src/intent_executor/mod.rs (3)
magicblock-committor-service/src/intent_execution_manager/intent_execution_engine.rs (2)
get_base_account(804-809)None(393-393)magicblock-committor-service/src/intent_executor/null_task_info_fetcher.rs (1)
get_base_account(36-41)magicblock-committor-service/src/intent_executor/task_info_fetcher.rs (2)
get_base_account(42-45)get_base_account(274-279)
test-integration/schedulecommit/test-scenarios/tests/utils/mod.rs (3)
test-integration/schedulecommit/test-security/tests/01_invocations.rs (1)
ctx(148-149)test-integration/test-ledger-restore/tests/08_commit_update.rs (1)
ctx(90-91)test-integration/schedulecommit/client/src/schedule_commit_context.rs (3)
try_new(72-74)ncommittees(102-121)try_new_random_keys(66-71)
magicblock-committor-service/src/tasks/mod.rs (4)
magicblock-committor-service/src/tasks/args_task.rs (2)
instruction(45-85)new(36-41)magicblock-committor-service/src/tasks/buffer_task.rs (2)
instruction(65-86)new(37-45)test-integration/test-committor-service/tests/common.rs (2)
create_commit_task(163-181)new(53-56)magicblock-committor-service/src/intent_executor/task_info_fetcher.rs (1)
new(59-67)
test-integration/schedulecommit/client/src/schedule_commit_context.rs (1)
test-integration/programs/schedulecommit/src/api.rs (3)
init_order_book_instruction(36-54)init_payer_escrow(77-98)init_account_instruction(16-34)
test-integration/test-committor-service/tests/test_transaction_preparator.rs (4)
test-integration/test-committor-service/tests/common.rs (3)
new(53-56)create_commit_task(163-181)create_task_info_fetcher(117-119)magicblock-committor-service/src/tasks/args_task.rs (1)
new(36-41)magicblock-committor-service/src/tasks/buffer_task.rs (2)
new(37-45)new_preparation_required(30-35)magicblock-committor-service/src/tasks/mod.rs (1)
create_commit_task(121-157)
magicblock-committor-service/src/tasks/args_task.rs (1)
magicblock-committor-service/src/tasks/buffer_task.rs (2)
new(37-45)new_preparation_required(30-35)
test-integration/programs/schedulecommit/src/lib.rs (2)
test-integration/programs/schedulecommit/src/utils/mod.rs (3)
assert_is_signer(30-44)assert_keys_equal(16-28)allocate_account_and_assign_owner(58-106)test-integration/programs/schedulecommit/src/order_book.rs (1)
new(105-125)
magicblock-committor-service/src/tasks/task_builder.rs (2)
magicblock-committor-service/src/tasks/mod.rs (1)
create_commit_task(121-157)test-integration/test-committor-service/tests/common.rs (2)
create_commit_task(163-181)new(53-56)
magicblock-committor-service/src/tasks/task_strategist.rs (2)
test-integration/test-committor-service/tests/common.rs (2)
new(53-56)create_commit_task(163-181)magicblock-committor-service/src/tasks/mod.rs (1)
create_commit_task(121-157)
test-integration/schedulecommit/test-scenarios/tests/01_commits.rs (1)
test-integration/schedulecommit/test-scenarios/tests/utils/mod.rs (1)
get_context_with_delegated_committees(16-35)
test-integration/test-committor-service/tests/common.rs (3)
magicblock-committor-service/src/intent_executor/task_info_fetcher.rs (3)
new(59-67)get_base_account(42-45)get_base_account(274-279)magicblock-committor-service/src/intent_executor/null_task_info_fetcher.rs (1)
get_base_account(36-41)magicblock-committor-service/src/tasks/mod.rs (1)
create_commit_task(121-157)
test-integration/test-committor-service/tests/test_delivery_preparator.rs (1)
test-integration/test-committor-service/tests/common.rs (2)
create_commit_task(163-181)new(53-56)
🔇 Additional comments (34)
test-integration/programs/schedulecommit/src/utils/mod.rs (4)
53-53: Good API alignment improvement.Changing the
sizefield tou64aligns with Solana'ssystem_instruction::allocateAPI and eliminates the need for type conversions at the call site.
71-71: Cast is appropriate for Solana's 64-bit environment.The
as usizecast is safe here since Solana programs run exclusively in 64-bit environments whereusizeequalsu64. This approach is consistent with established patterns.Based on learnings.
75-79: Helpful observability addition.The logging message improves debuggability by showing the required lamports and available payer balance, which is useful for diagnosing allocation failures.
90-90: Cleaner API usage.Passing
sizedirectly tosystem_instruction::allocateeliminates the previous conversion and potential panic point, making the code more straightforward and robust.test-integration/test-ledger-restore/tests/08_commit_update.rs (2)
55-61: KeepTempDirbound to avoid premature directory cleanupCapturing the first tuple element as
_tmpdir(instead of_) correctly keeps theTempDiralive for the entirewrite()scope, preventing the underlying temporary directory from being deleted while the validator/ctx are still in use. The leading underscore avoids unused-variable warnings and documents the intent.
Based on learnings
170-176: Same lifetime fix forread()setupMirroring
write(), binding theTempDiras_tmpdirhere ensures the directory survives for the wholeread()scope, avoiding early cleanup while the validator and cloned accounts are being used. This is the right pattern for resource guards you need to keep alive but never otherwise reference.
Based on learningstest-integration/schedulecommit/test-scenarios/Cargo.toml (1)
19-20: LGTM — test dependencies are reasonable.
randandborshare appropriately placed as dev-dependencies to support test scenario generation and serialization testing.test-integration/Cargo.toml (2)
40-40: LGTM —ephemeral-rollups-sdkremains a git-based dependency.The reordering maintains the git-based reference, avoiding the CI path-dependency issues from previous feedback. No functional change.
60-60: I'll search for information about this specific commit and CommitDiff features in the magicblock-delegation-program project.I was unable to find specific information about commit e8d03936 or a feature called "CommitDiff" in the publicly available documentation. Let me search more specifically for commit history and this feature.
Unable to verify CommitDiff feature existence in commit e8d03936.
After multiple web searches, I could not find any reference to a feature called "CommitDiff" in the magicblock-labs/delegation-program documentation, changelog, or codebase. The delegation-program supports core functionality including account delegation, state commits (synchronization back to Solana), and account undelegation, but there is no evidence of a specific "CommitDiff" capability. Additionally, I could not access specific details about commit e8d03936 from public sources.
Without being able to confirm the existence or compatibility status of this feature, please manually verify that:
- Commit e8d03936 is the correct revision for your use case
- The delegation-program at this revision meets your workspace requirements
- Integration tests in test-integration/Cargo.toml run successfully with this dependency version
test-integration/programs/schedulecommit/Cargo.toml (1)
11-12: No issues found — both dependencies are actively used.Verification confirms that
rkyvandstatic_assertionsare actively used in the program:
rkyv::AlignedVecis used inorder_book.rs:64for serializationstatic_assertions::const_assertis used inorder_book.rs:7for compile-time assertionsThe dependencies appropriately support the CommitDiff serialization feature. No unused transitive dependencies.
Cargo.toml (2)
111-114: Themagicblock-aperturepath dependency is valid and properly integrated.Verification confirms:
- Directory exists at workspace root (
./magicblock-aperture)- Contains valid Cargo.toml with proper workspace configuration
- Declared in workspace members list
- Follows the same pattern as 20+ other legitimate path dependencies in the workspace (magicblock-committor-service, magicblock-config, magicblock-core, etc.)
This is not a temporary workaround like previous CI concerns—it is permanent workspace infrastructure. The workspace resolver handles this correctly.
112-114: CommitDiff instruction support is present in revision e8d03936.Commit e8d03936 introduces an optimization to CommitDiff using efficient merge_diff_copy that avoids heap allocation, confirming the new revision includes the CommitDiff functionality required by this PR. The revision upgrade is compatible.
test-integration/test-tools/src/integration_test_context.rs (1)
157-175: Stricter client presence + explicit v0 handling look good.Turning a missing
rpc_clientinto a panic with a labeled message and explicitly settingmax_supported_transaction_versionfor"chain"improves diagnostics and makes the version behavior explicit without changing intended semantics in this test context.test-integration/schedulecommit/test-security/tests/01_invocations.rs (1)
28-36: Context creation correctly updated for newuser_seedAPI.The
prepare_ctx_with_account_to_commithelper now passes the expecteduser_seedinto bothtry_newandtry_new_random_keys, preserving the previous behavior while satisfying the new constructor signature.programs/magicblock/src/magic_scheduled_base_intent.rs (1)
310-465: CommittedAccount abstraction and CommitType refactor look sound.The introduction of
CommittedAccountRefandCommittedAccountcleanly separates on‑chain account views from the owned representation used in commits, andCommitType::try_from_argsnow consistently validates delegation/ownership before snapshotting and rewriting the owner toparent_program_id. I don’t see correctness or safety issues in this refactor.test-integration/schedulecommit/test-scenarios/tests/utils/mod.rs (1)
16-35: User seed correctly threaded into context helper.
get_context_with_delegated_committeesnow acceptsuser_seedand forwards it to bothtry_newvariants before initializing and delegating committees, which matches the updated client API and keeps the helper’s behavior intact.test-integration/test-committor-service/tests/utils/transactions.rs (1)
123-133: CommitState-vs-CommitDiff log matching behavior is clear and aligned with intent.The special-casing of
"CommitState"to also match"CommitDiff"is well-documented here and keeps existing tests stable while allowing the optimized path.magicblock-committor-service/src/intent_executor/mod.rs (2)
7-12: NullTaskInfoFetcher module gating and re-export are well-scoped.The
cfg(any(test, feature = "dev-context-only-utils"))gating keepsNullTaskInfoFetcherconfined to tests/dev utilities while still letting downstream code import it fromintent_executorwhen needed.
787-833: MockInfoFetcher implementation correctly adapts to new TaskInfoFetcher API.Adding
get_base_accountthat returnsOk(None)is an appropriate no-op implementation for these tests and keeps the trait object-safe and coherent across production and test code.magicblock-committor-service/src/intent_executor/task_info_fetcher.rs (2)
11-46: New TaskInfoFetcher::get_base_account signature fits existing error model and callers.Extending the trait with
get_base_accountreturningMagicBlockRpcClientResult<Option<Account>>integrates cleanly with the existingTaskInfoFetcherError::MagicBlockRpcClientErrorand matches how tests and null/buffer fetchers are implemented elsewhere.
274-279: CacheTaskInfoFetcher get_base_account implementation is straightforward and correct.Delegating to
self.rpc_client.get_account(pubkey).awaitis an appropriate baseline implementation; any future need for batching can be handled separately without affecting the trait contract.test-integration/test-committor-service/tests/test_delivery_preparator.rs (1)
2-304: Asynccreate_commit_taskintegration and join_all usage look correct.All call sites now properly await
create_commit_task, and thejoin_allpatterns for building multiple buffer/args tasks are sound (no lifetime issues withdatas, and results are collected beforedatasdrops). The tests accurately exercise both initial and re-prepare flows with mutated commit data.test-integration/schedulecommit/test-scenarios/tests/01_commits.rs (1)
46-47: User seed propagation into context and delegation looks correctPassing
b"magic_schedule_commit"intoget_context_with_delegated_committeesanddelegate_account_cpi_instructionaligns with the updated helper signature and keeps the delegation context/seed consistent across tests.Also applies to: 100-101, 232-233
magicblock-committor-service/src/intent_execution_manager/intent_execution_engine.rs (1)
347-349: MockInfoFetcher update cleanly satisfies new TaskInfoFetcher APIThe added
MagicBlockRpcClientResult/Accountimports andget_base_accountimplementation (Ok(None)) correctly extendMockInfoFetcherto match the updatedTaskInfoFetchertrait without affecting existing test behavior.Also applies to: 804-809
magicblock-committor-service/src/intent_executor/null_task_info_fetcher.rs (1)
1-41: NullTaskInfoFetcher is a correct null-object implementationAll
TaskInfoFetchermethods return empty collections/None/Ok(None)as expected from a no-op fetcher, andget_base_accountaligns with the commit‑diff fallback logic (treating missing base accounts as AccountNotFound).test-integration/schedulecommit/test-scenarios/tests/02_commit_and_undelegate.rs (2)
55-57: Seeded contexts and order-book CommitDiff test look sound
- Using
get_context_with_delegated_committees(…, b"magic_schedule_commit")in the single/two‑account helpers andb"order_book"for the order‑book helper correctly aligns with the updated context initializer that now requires a user seed.commit_and_undelegate_order_book_accountcomposes theupdate_order_book_instructionandschedule_commit_diff_instruction_for_order_bookin a single transaction against the delegated committee account, which matches the intended CommitDiff flow.test_committing_and_undelegating_huge_order_book_account’s seeded RNG setup, detailed failure messages (includingrng_seed), and strict equality checks on bids/asks provide robust, reproducible coverage for large order‑book commits.Also applies to: 173-174, 110-164, 244-312
150-161: Replaceprintln!withdebug!/info!for consistent loggingThis file otherwise uses the
logmacros; the remainingprintln!calls (e.g., incommit_and_undelegate_order_book_accountand the huge order‑book test) are inconsistent and bypass the configured logger.Consider switching them to
debug!orinfo!, for example:- println!("txhash (scheduled_commit): {:?}", tx_res); + debug!("txhash (scheduled_commit): {:?}", tx_res); - println!("Important: use {rng_seed} as seed to regenerate the random inputs in case of test failure"); + info!("Important: use {rng_seed} as seed to regenerate the random inputs in case of test failure"); - println!( + info!( "BookUpdate: total = {}, bids = {}, asks = {}", update.bids.len() + update.asks.len(), update.bids.len(), update.asks.len() );[ suggest_optional_refactor]
[ duplicate_comment]Also applies to: 252-275, 268-273
magicblock-committor-service/src/tasks/task_builder.rs (2)
27-32: Async commit task construction viaTaskInfoFetcherlooks correct.Using
task_info_fetcher: &Arc<C>plus a singlefetch_next_commit_idscall and thenjoin_alloverCommitTaskBuilder::create_commit_taskgives you one RPC batch for IDs and concurrent per-account base‑account fetching, while preserving task ordering. Theexpectoncommit_ids.get(&account.pubkey)is reasonable given the documented invariant that the fetcher must return IDs for all input pubkeys. No changes needed here.Also applies to: 48-51, 72-80, 90-103
35-38: Finalize path’s switch toTaskInfoFetcheris consistent and localized.Threading
task_info_fetcher: &Arc<C>intofinalize_tasksand usingfetch_rent_reimbursements(&pubkeys)keeps all chain/RPC I/O behind the same trait as the commit side, and the error mapping toFinalizedTasksBuildErroris clear. Just ensure yourTaskInfoFetcherimplementations guarantee a 1:1 mapping betweenpubkeysandrent_reimbursements, since the subsequentzipwill silently drop mismatches.Also applies to: 107-111, 167-176
magicblock-committor-service/src/tasks/args_task.rs (2)
1-2: Delegating commit instruction building toCommitTaskis a good consolidation.Having
ArgsTaskType::Commitcallvalue.create_commit_ix(validator)centralizes Commit vs CommitDiff encoding inCommitTaskand keepsArgsTaskfree of DLP‑specific argument wiring. This also supports the earlier goal of avoiding any RPC or diff computation insideinstruction(). Looks good.Also applies to: 44-83
87-107: Confirm intended downgrade of diff-capable commits during buffer optimization.For
Committasks wherevalue.is_commit_diff()is true,optimizenow callsvalue.force_commit_state()and then wraps it in aBufferTask. That means CommitDiff is only exercised when the unoptimized Args transaction already fits, and any task that needs buffer optimization will fall back to CommitState semantics, as your TODO notes.If this is the intended interim behavior, fine; just ensure:
- You have tests that cover both cases (diff path taken vs downgraded when buffers are needed), and
- Future work to support diff via buffers removes
force_commit_state()and/or routes such tasks to a dedicated diff-aware buffer variant instead of silently downgrading them.test-integration/programs/schedulecommit/src/api.rs (2)
100-145: Seed-based delegation split (main vs order-book) is clear; constrain callers to known seeds.
delegate_account_cpi_instructionnow:
- Derives the delegated PDA as
[user_seed, player_or_book_manager], and- Chooses between
DelegateCpiandDelegateOrderBookbased onuser_seed == b"magic_schedule_commit".This matches the two PDA schemes you’re exercising from the client. Just make sure all call sites only ever pass the two canonical seeds (
"magic_schedule_commit"and"order_book"); anything else will derive a mismatched PDA but still go down the order-book branch.
196-215: Double-check account order forScheduleCommitForOrderBook.
schedule_commit_diff_instruction_for_order_bookwires accounts as[payer, order_book, magic_context_id, magic_program_id]. Please verify this matches the on-chainScheduleCommitForOrderBookhandler’s expected account order; a drift here would be hard to diagnose from tests.test-integration/schedulecommit/client/src/schedule_commit_context.rs (1)
34-35:user_seedwiring across PDAs, init, and delegation is coherent.Storing
user_seedinScheduleCommitTestContext, deriving committee PDAs with[user_seed, payer_ephem], branching ininit_committeesbetweeninit_account_instructionandinit_order_book_instruction, and then reusing the same seed indelegate_account_cpi_instructionkeeps the main-account and order-book flows consistent end to end. The fallback error for unsupported seeds ininit_committeesis also helpful. No issues from a correctness standpoint.Also applies to: 66-80, 103-121, 153-159, 165-211, 268-276
| rand = "0.8.5" | ||
| random-port = "0.1.1" | ||
| rayon = "1.10.0" | ||
| rkyv = "0.7.45" |
There was a problem hiding this comment.
🧩 Analysis chain
Verify rkyv and static_assertions workspace dependency versions.
Lines 74 and 93 add rkyv = "0.7.45" and static_assertions = "1.1.0" to workspace.dependencies. Ensure these versions are:
- Compatible with other Solana/workspace dependencies (especially
solana-program,solana-sdk) - Consistent across the workspace (check if they appear in root
Cargo.tomlor other workspace members) - Not introducing transitive dependency conflicts
Also applies to: 93-93
🏁 Script executed:
#!/bin/bash
# Check workspace-wide usage of rkyv and static_assertions
echo "=== Checking rkyv usage in workspace ==="
rg -n "rkyv" --type toml Cargo.toml test-integration/Cargo.toml
echo ""
echo "=== Checking static_assertions usage in workspace ==="
rg -n "static_assertions" --type toml Cargo.toml test-integration/Cargo.toml
echo ""
echo "=== Checking for version consistency in root Cargo.toml ==="
grep -n "static_assertions\|rkyv" Cargo.tomlLength of output: 337
🏁 Script executed:
#!/bin/bash
# Check Solana dependencies in root Cargo.toml to assess compatibility
echo "=== Solana dependencies in root Cargo.toml ==="
rg -A2 -B2 "solana-program|solana-sdk" --type toml Cargo.toml | head -50
echo ""
echo "=== Check if test-integration actually uses rkyv ==="
rg -n "rkyv" --type rust test-integration/src/ || echo "No rkyv usage found in test-integration/src"
echo ""
echo "=== Check if rkyv is in root workspace.dependencies (workspace.dependencies section) ==="
sed -n '/\[workspace\.dependencies\]/,/^\[/p' Cargo.toml | grep -n "rkyv" || echo "rkyv NOT found in root workspace.dependencies"
echo ""
echo "=== List all workspace.dependencies in root Cargo.toml ==="
sed -n '/\[workspace\.dependencies\]/,/^\[/p' Cargo.toml | head -30Length of output: 1575
🏁 Script executed:
#!/bin/bash
# Check Solana dependencies and full workspace.dependencies section
echo "=== Solana dependencies (from root Cargo.toml) ==="
grep -n "solana-program\|solana-sdk" Cargo.toml | head -10
echo ""
echo "=== Full workspace.dependencies section in root Cargo.toml ==="
sed -n '/\[workspace\.dependencies\]/,/^$/p' Cargo.toml
echo ""
echo "=== Check if test-integration Cargo.toml references rkyv in code ==="
grep -r "rkyv" test-integration/Cargo.toml
echo ""
echo "=== Get the dependencies section from test-integration/Cargo.toml ==="
sed -n '/\[dependencies\]/,/^\[/p' test-integration/Cargo.toml | grep -E "rkyv|static_assertions"Length of output: 3365
🏁 Script executed:
#!/bin/bash
# Check test-integration/Cargo.toml structure and any workspace definition
echo "=== Full test-integration/Cargo.toml content ==="
cat test-integration/Cargo.toml
echo ""
echo "=== Check for rkyv usage anywhere in the codebase ==="
rg -n "rkyv" --type rust . 2>/dev/null | head -20 || echo "No rkyv Rust usage found"
echo ""
echo "=== Check if test-integration is a workspace member ==="
grep -A 20 "\[workspace\]" Cargo.toml | grep -E "members|test-integration"Length of output: 4143
</web_search>
🌐 Web query:
rkyv 0.7.45 Solana compatibility
💡 Result:
Short answer: Yes — rkyv 0.7.45 can be used in Solana (BPF) programs, but you must build it for no‑std/32‑bit BPF by adjusting features.
What to do
- Use the rkyv 0.7.45 crate (release exists). [1][2]
- Disable/std opt out default features and enable the minimal features required for BPF:
- compile rkyv with no std (disable the "std" feature),
- enable alloc only if you need heap types (Vec/String) and your program supplies an allocator,
- enable pointer_width_32 so usize/isize are serialized as 32‑bit (matches Solana BPF). [3]
- Ensure every dependency in your dependency graph is also no_std/no‑alloc (Cargo features are unioned).
- Avoid/disable features that pull in bytecheck/std or other non‑BPF‑compatible code, or provide no_std versions of those crates.
References
- rkyv crate documentation (features, pointer_width_32, alloc/std notes). [3]
- rkyv 0.7.45 release/package listing. [2][4]
If you want, tell me your Cargo.toml and I’ll produce the exact dependency lines / feature flags to compile rkyv 0.7.45 for Solana BPF.
🏁 Script executed:
#!/bin/bash
# Check how rkyv is declared in the actual programs using it
echo "=== Search for rkyv declaration in schedulecommit program ==="
find test-integration/programs -name "Cargo.toml" -exec grep -H "rkyv" {} \;
echo ""
echo "=== Check schedulecommit program's Cargo.toml dependencies ==="
cat test-integration/programs/schedulecommit/Cargo.toml | grep -A 3 -B 3 "rkyv\|dependencies"
echo ""
echo "=== Check for any feature configuration in test-integration workspace.dependencies ==="
grep -A 1 "^rkyv" test-integration/Cargo.toml
echo ""
echo "=== Verify rkyv usage context in order_book.rs ==="
sed -n '60,70p' test-integration/programs/schedulecommit/src/order_book.rsLength of output: 1228
Reconfigure rkyv with BPF-compatible feature flags in test-integration/Cargo.toml line 74.
rkyv = "0.7.45" is used by the schedulecommit Solana program but lacks critical Solana BPF compatibility features. Change to:
rkyv = { version = "0.7.45", default-features = false, features = ["alloc", "pointer_width_32"] }
The default features include std, which is incompatible with Solana BPF programs. The pointer_width_32 and alloc flags are required for BPF serialization (the code uses rkyv::AlignedVec).
static_assertions = "1.1.0" is consistent across both workspaces and requires no changes.
🤖 Prompt for AI Agents
In test-integration/Cargo.toml around line 74, the rkyv dependency is currently
declared as rkyv = "0.7.45" which brings default (std) features incompatible
with Solana BPF; update the dependency to explicitly disable default features
and enable alloc and pointer_width_32 by changing the declaration to use a table
with version = "0.7.45", default-features = false, and features =
["alloc","pointer_width_32"] so the crate builds for 32-bit BPF and supports
rkyv::AlignedVec.
There was a problem hiding this comment.
You're wrong.
See the code carefully again. rkyv is used by code which does NOT run inside the Solana BPF VM.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find all actual rkyv usage in the codebase
echo "=== All rkyv imports and usage in Rust files ==="
rg -n "use.*rkyv|rkyv::" --type rust test-integration/
echo ""
echo "=== Check schedulecommit program's crate-type and target ==="
cat test-integration/programs/schedulecommit/Cargo.toml
echo ""
echo "=== Check for cfg attributes around rkyv usage in order_book.rs ==="
cat test-integration/programs/schedulecommit/src/order_book.rs | head -100
echo ""
echo "=== Check how OrderBook is used - is it only in tests? ==="
rg -n "OrderBook" --type rust test-integration/schedulecommit/ -C 2 | head -50
Length of output: 4911
@snawaz, you're absolutely right, and I apologize for the incorrect analysis.
Looking at the actual usage, rkyv appears only in the BorshDeserialize implementation for OrderBookOwned in order_book.rs line 64, which is explicitly marked in the comment as "test code." This deserialization method is called exclusively by off-chain test/verification code (in schedulecommit/client/src/verify.rs), not by the on-chain BPF program.
The on-chain code uses the zero-copy OrderBook<'a> structure that operates directly on account data without any serialization dependencies. When the schedulecommit program compiles to BPF (via the "cdylib" target), the OrderBookOwned deserialization path with rkyv::AlignedVec is never executed.
The current declaration rkyv = "0.7.45" with default features (including std) is correct for this off-chain usage. No BPF-specific feature flags are needed.
🧠 Learnings used
Learnt from: Dodecahedr0x
Repo: magicblock-labs/magicblock-validator PR: 614
File: test-integration/programs/schedulecommit/Cargo.toml:19-20
Timestamp: 2025-11-07T18:19:48.996Z
Learning: In Solana programs (crates depending on solana-program), the empty feature flags `custom-heap = []` and `custom-panic = []` are standard declarations required to avoid compiler warnings. These should not be flagged as unused features in Cargo.toml files for Solana program crates.
Learnt from: snawaz
Repo: magicblock-labs/magicblock-validator PR: 575
File: test-integration/programs/schedulecommit/src/utils/mod.rs:71-71
Timestamp: 2025-11-21T13:56:03.885Z
Learning: In Solana programs (BPF runtime), defensive checked conversions to `usize` (e.g., `try_into()`) are unnecessary when casting from `u64`. Solana exclusively runs in 64-bit environments where `usize` is 64 bits, so truncation is not a concern and simple `as usize` casts are appropriate.
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.
7d6f839 to
faed161
Compare
…fficiently commit changes in delegated accounts
snawaz
force-pushed
the
snawaz/commit-diff
branch
from
faed161 to
9c11945
Compare
snawaz
force-pushed
the
snawaz/commit-diff
branch
from
9c11945 to
a42745b
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
test-integration/test-committor-service/tests/test_ix_commit_local.rs (1)
465-488: Large‑commit TODO is clear; consider linking to a tracking issueThe expanded TODO block for 1 MB / “max Solana account size” scenarios is well described. To avoid this stress‑test work getting lost, it would be helpful to reference a tracking issue or design doc here once one exists (e.g.,
// TODO(...): ... (tracked in issue #NNN)).test-integration/test-committor-service/tests/common.rs (1)
162-180: Confirm whether tests using create_commit_task are meant to exercise CommitDiff
create_commit_tasknow always usesNullTaskInfoFetcher, which guaranteesbase_accountisNoneand thus forces the CommitState path even for large accounts. That’s fine for pure serialization tests, but if any callers of this helper are intended to cover CommitDiff behavior, they won’t actually hit the diff path.If you need CommitDiff coverage through this helper, consider either:
- Accepting a
TaskInfoFetcher(orArc<dyn TaskInfoFetcher>) parameter, or- Switching to a real/test
TaskInfoFetcherthat returnsSome(Account)for large accounts in the relevant tests.Otherwise, leaving it as-is is reasonable for state-only tests.
♻️ Duplicate comments (8)
test-integration/programs/schedulecommit/src/lib.rs (1)
327-332: Fixprocess_grow_order_bookPDA error message to referencebook_managerThe PDA assertion still formats the message with
payer.keyinstead ofbook_manager.key:format!( "PDA for the account ('{}') and for book_manager ('{}') is incorrect", order_book.key, payer.key )For debugging, this should show the book manager whose key is used in the PDA derivation:
- "PDA for the account ('{}') and for book_manager ('{}') is incorrect", - order_book.key, payer.key + "PDA for the account ('{}') and for book_manager ('{}') is incorrect", + order_book.key, book_manager.key[suggest_minor_issue]
test-integration/test-tools/src/scheduled_commits.rs (1)
229-231: Unconditionaldump_chain_logsstill makes happy-path tests noisy and slowThis helper will hit RPC and dump full chain logs for every commit signature on every successful run. That was already called out earlier; the concern still applies.
Consider either:
- Removing these calls now that debugging is done, or
- Gating them behind an env flag / debug mode, or
- Moving them into specific failing paths/tests that actually need deep logs.
test-integration/schedulecommit/client/src/schedule_commit_context.rs (1)
193-207: Remove stale commented-out grow-order-book block; track via issue insteadThis 10+ line commented block under the
b"order_book"arm is now just noise in a hot helper://// TODO... // ixs.extend(self.committees.iter().flat_map(|(payer, committee)| { // [grow_order_book_instruction(...)] // }));Given CommitDiff/grow behavior is already being exercised elsewhere, and this TODO has been called out before, consider:
- Moving the “relax account size / grow order book” work into a GitHub issue, then
- Deleting this commented code entirely, possibly leaving a 1‑line comment with the issue reference if you want breadcrumbs.
This keeps the test context focused and easier to scan.
test-integration/programs/schedulecommit/src/order_book.rs (1)
138-141: Avoid silently dropping updates when order-book capacity is exhausted
update_fromcurrently ignores theOptionreturned byadd_bids/add_asks:pub fn update_from(&mut self, updates: BookUpdate) { self.add_bids(&updates.bids); self.add_asks(&updates.asks); }If capacity is insufficient, those helpers return
Noneand the update is silently dropped (or partially applied). Earlier discussion indicated you’d rather have tests panic than fail quietly.To align with that intent and keep the API simple, consider:
pub fn update_from(&mut self, updates: BookUpdate) { - self.add_bids(&updates.bids); - self.add_asks(&updates.asks); + self.add_bids(&updates.bids) + .expect("order_book capacity exhausted while adding bids"); + self.add_asks(&updates.asks) + .expect("order_book capacity exhausted while adding asks"); }This keeps the zero-copy layout as-is but guarantees you’ll see an immediate, clear failure instead of silent truncation.
test-integration/schedulecommit/test-security/tests/01_invocations.rs (1)
28-36: Context initialization correctly forwardsuser_seed(consider centralizing the literal)Updating
prepare_ctx_with_account_to_committo passb"magic_schedule_commit"into bothtry_newandtry_new_random_keysmatches the new ScheduleCommitTestContext API and keeps the test context wiring consistent. As this same seed string appears in other schedulecommit tests, you may eventually want a shared constant to avoid drift across files.test-integration/schedulecommit/test-scenarios/tests/01_commits.rs (1)
46-47: Avoid repeating the literalb"magic_schedule_commit"in multiple call sitesThe new
user_seedplumbing is correct, but the same byte string is now hardcoded in three places (twoget_context_with_delegated_committeescalls andinit_and_delegate_player). Consider extracting a module-level test constant (e.g.const TEST_USER_SEED: &[u8] = b"magic_schedule_commit";) and using it everywhere to keep these tests in sync if the seed ever changes.Also applies to: 100-101, 216-233
magicblock-committor-service/src/intent_executor/task_info_fetcher.rs (1)
12-16:get_base_accountdesign is fine; consider a batched path later for performanceExtending
TaskInfoFetcherwith asyncget_base_accountand wiringCacheTaskInfoFetchertorpc_client.get_accountis a reasonable first step to support CommitDiff logic. For workloads that touch many large delegated accounts in one run, callingget_accountper pubkey may become a bottleneck; a futureget_base_accounts(&[Pubkey])backed byget_multiple_accountscould reduce round‑trips and better match the existing batched metadata fetch.Also applies to: 43-47, 276-281
test-integration/schedulecommit/test-scenarios/tests/02_commit_and_undelegate.rs (1)
55-57: Order‑book CommitDiff test flow is solid; align logging with existing log macrosThe new order‑book helper and
test_committing_and_undelegating_huge_order_book_accountcorrectly:
- apply an
UpdateOrderBook,- schedule a CommitDiff via
schedule_commit_diff_instruction_for_order_book,- verify the committed book exactly matches the generated
BookUpdate, and- assert the committee account was undelegated on‑chain.
The only nit is consistency: the
println!calls (e.g., for the scheduled‑commit tx hash and RNG seed/update sizes) bypass the logging pipeline while the rest of this file usesinfo!/debug!. Consider swapping thoseprintln!s fordebug!/info!so test output respects the configured log level.Also applies to: 110-164, 244-312
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (3)
Cargo.lockis excluded by!**/*.locktest-integration/Cargo.lockis excluded by!**/*.locktest-integration/schedulecommit/elfs/dlp.sois excluded by!**/*.so
📒 Files selected for processing (32)
Cargo.toml(1 hunks)magicblock-committor-service/src/intent_execution_manager/intent_execution_engine.rs(2 hunks)magicblock-committor-service/src/intent_executor/mod.rs(4 hunks)magicblock-committor-service/src/intent_executor/null_task_info_fetcher.rs(1 hunks)magicblock-committor-service/src/intent_executor/task_info_fetcher.rs(3 hunks)magicblock-committor-service/src/tasks/args_task.rs(3 hunks)magicblock-committor-service/src/tasks/mod.rs(6 hunks)magicblock-committor-service/src/tasks/task_builder.rs(6 hunks)magicblock-committor-service/src/tasks/task_strategist.rs(11 hunks)magicblock-processor/src/executor/processing.rs(0 hunks)programs/magicblock/src/magic_scheduled_base_intent.rs(1 hunks)test-integration/Cargo.toml(4 hunks)test-integration/programs/schedulecommit/Cargo.toml(1 hunks)test-integration/programs/schedulecommit/src/api.rs(5 hunks)test-integration/programs/schedulecommit/src/lib.rs(8 hunks)test-integration/programs/schedulecommit/src/order_book.rs(1 hunks)test-integration/programs/schedulecommit/src/utils/mod.rs(3 hunks)test-integration/schedulecommit/client/src/schedule_commit_context.rs(6 hunks)test-integration/schedulecommit/client/src/verify.rs(2 hunks)test-integration/schedulecommit/test-scenarios/Cargo.toml(1 hunks)test-integration/schedulecommit/test-scenarios/tests/01_commits.rs(3 hunks)test-integration/schedulecommit/test-scenarios/tests/02_commit_and_undelegate.rs(4 hunks)test-integration/schedulecommit/test-scenarios/tests/utils/mod.rs(1 hunks)test-integration/schedulecommit/test-security/tests/01_invocations.rs(1 hunks)test-integration/test-committor-service/tests/common.rs(5 hunks)test-integration/test-committor-service/tests/test_delivery_preparator.rs(6 hunks)test-integration/test-committor-service/tests/test_ix_commit_local.rs(9 hunks)test-integration/test-committor-service/tests/test_transaction_preparator.rs(4 hunks)test-integration/test-committor-service/tests/utils/transactions.rs(1 hunks)test-integration/test-ledger-restore/tests/08_commit_update.rs(2 hunks)test-integration/test-tools/src/integration_test_context.rs(2 hunks)test-integration/test-tools/src/scheduled_commits.rs(3 hunks)
💤 Files with no reviewable changes (1)
- magicblock-processor/src/executor/processing.rs
🧰 Additional context used
🧠 Learnings (19)
📚 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-committor-service/tests/test_ix_commit_local.rsmagicblock-committor-service/src/intent_executor/mod.rstest-integration/test-ledger-restore/tests/08_commit_update.rstest-integration/test-tools/src/integration_test_context.rstest-integration/test-committor-service/tests/test_delivery_preparator.rstest-integration/schedulecommit/test-scenarios/tests/utils/mod.rsmagicblock-committor-service/src/tasks/mod.rsmagicblock-committor-service/src/intent_executor/task_info_fetcher.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:
test-integration/test-committor-service/tests/test_ix_commit_local.rsmagicblock-committor-service/src/intent_executor/mod.rstest-integration/programs/schedulecommit/src/utils/mod.rsprograms/magicblock/src/magic_scheduled_base_intent.rstest-integration/programs/schedulecommit/src/lib.rsmagicblock-committor-service/src/tasks/args_task.rsmagicblock-committor-service/src/tasks/mod.rstest-integration/programs/schedulecommit/src/api.rsmagicblock-committor-service/src/intent_executor/task_info_fetcher.rstest-integration/programs/schedulecommit/src/order_book.rstest-integration/schedulecommit/client/src/schedule_commit_context.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:
test-integration/test-committor-service/tests/test_ix_commit_local.rsmagicblock-committor-service/src/intent_executor/mod.rsmagicblock-committor-service/src/tasks/task_builder.rsmagicblock-committor-service/src/intent_executor/null_task_info_fetcher.rsprograms/magicblock/src/magic_scheduled_base_intent.rstest-integration/test-committor-service/tests/common.rsmagicblock-committor-service/src/tasks/task_strategist.rstest-integration/test-committor-service/tests/test_delivery_preparator.rsmagicblock-committor-service/src/tasks/args_task.rsmagicblock-committor-service/src/tasks/mod.rstest-integration/test-committor-service/tests/test_transaction_preparator.rs
📚 Learning: 2025-11-07T18:19:48.996Z
Learnt from: Dodecahedr0x
Repo: magicblock-labs/magicblock-validator PR: 614
File: test-integration/programs/schedulecommit/Cargo.toml:19-20
Timestamp: 2025-11-07T18:19:48.996Z
Learning: In Solana programs (crates depending on solana-program), the empty feature flags `custom-heap = []` and `custom-panic = []` are standard declarations required to avoid compiler warnings. These should not be flagged as unused features in Cargo.toml files for Solana program crates.
Applied to files:
test-integration/programs/schedulecommit/Cargo.tomltest-integration/Cargo.toml
📚 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-committor-service/src/intent_executor/mod.rstest-integration/Cargo.tomltest-integration/programs/schedulecommit/src/utils/mod.rstest-integration/test-ledger-restore/tests/08_commit_update.rsmagicblock-committor-service/src/tasks/task_strategist.rsmagicblock-committor-service/src/tasks/args_task.rsmagicblock-committor-service/src/tasks/mod.rsmagicblock-committor-service/src/intent_executor/task_info_fetcher.rstest-integration/programs/schedulecommit/src/order_book.rs
📚 Learning: 2025-10-21T13:06:38.900Z
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 578
File: magicblock-aperture/src/requests/http/get_program_accounts.rs:17-25
Timestamp: 2025-10-21T13:06:38.900Z
Learning: The magicblock validator does not support ledger forking, so commitment-based state queries (processed/confirmed/finalized) are not applicable. RPC methods can safely ignore commitment and minContextSlot parameters from Solana RPC config objects.
Applied to files:
magicblock-committor-service/src/intent_executor/mod.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-committor-service/src/intent_executor/mod.rsmagicblock-committor-service/src/intent_execution_manager/intent_execution_engine.rstest-integration/test-committor-service/tests/common.rsmagicblock-committor-service/src/intent_executor/task_info_fetcher.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-committor-service/src/intent_executor/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-committor-service/src/intent_executor/mod.rsprograms/magicblock/src/magic_scheduled_base_intent.rstest-integration/programs/schedulecommit/src/lib.rsmagicblock-committor-service/src/tasks/mod.rsmagicblock-committor-service/src/intent_executor/task_info_fetcher.rstest-integration/programs/schedulecommit/src/order_book.rstest-integration/schedulecommit/client/src/schedule_commit_context.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/tasks/task_builder.rstest-integration/programs/schedulecommit/src/utils/mod.rstest-integration/schedulecommit/test-scenarios/tests/01_commits.rstest-integration/schedulecommit/test-security/tests/01_invocations.rstest-integration/schedulecommit/test-scenarios/tests/02_commit_and_undelegate.rsmagicblock-committor-service/src/tasks/task_strategist.rstest-integration/test-committor-service/tests/test_delivery_preparator.rstest-integration/schedulecommit/client/src/verify.rstest-integration/schedulecommit/test-scenarios/tests/utils/mod.rsmagicblock-committor-service/src/tasks/mod.rstest-integration/test-committor-service/tests/test_transaction_preparator.rs
📚 Learning: 2025-11-21T13:56:03.885Z
Learnt from: snawaz
Repo: magicblock-labs/magicblock-validator PR: 575
File: test-integration/programs/schedulecommit/src/utils/mod.rs:71-71
Timestamp: 2025-11-21T13:56:03.885Z
Learning: In Solana programs (BPF runtime), defensive checked conversions to `usize` (e.g., `try_into()`) are unnecessary when casting from `u64`. Solana exclusively runs in 64-bit environments where `usize` is 64 bits, so truncation is not a concern and simple `as usize` casts are appropriate.
Applied to files:
test-integration/Cargo.tomltest-integration/programs/schedulecommit/src/utils/mod.rstest-integration/programs/schedulecommit/src/lib.rstest-integration/programs/schedulecommit/src/order_book.rs
📚 Learning: 2025-10-26T16:54:39.084Z
Learnt from: thlorenz
Repo: magicblock-labs/magicblock-validator PR: 587
File: test-manual/Cargo.toml:0-0
Timestamp: 2025-10-26T16:54:39.084Z
Learning: In the magicblock-validator repository, use git branch references (not commit hashes or tags) for the helius-laserstream dependency to allow automatic updates when the branch is pushed to.
Applied to files:
test-integration/Cargo.tomlCargo.toml
📚 Learning: 2025-11-20T08:57:07.217Z
Learnt from: thlorenz
Repo: magicblock-labs/magicblock-validator PR: 650
File: magicblock-chainlink/src/submux/subscription_task.rs:13-99
Timestamp: 2025-11-20T08:57:07.217Z
Learning: In the magicblock-validator repository, avoid posting review comments that merely confirm code is correct or matches intended behavior without providing actionable feedback, suggestions for improvement, or identifying potential issues. Such confirmatory comments are considered unhelpful noise by the maintainers.
Applied to files:
test-integration/test-tools/src/scheduled_commits.rs
📚 Learning: 2025-11-21T11:03:26.756Z
Learnt from: thlorenz
Repo: magicblock-labs/magicblock-validator PR: 664
File: magicblock-chainlink/src/testing/mod.rs:342-370
Timestamp: 2025-11-21T11:03:26.756Z
Learning: In the magicblock-validator codebase, avoid leaving review comments that merely acknowledge code is correct or well-structured when there is no actionable suggestion, improvement, or issue to flag. Only comment when there is something specific to recommend, fix, or clarify.
Applied to files:
test-integration/test-tools/src/scheduled_commits.rs
📚 Learning: 2025-11-19T12:55:48.931Z
Learnt from: Dodecahedr0x
Repo: magicblock-labs/magicblock-validator PR: 639
File: test-integration/test-tools/src/validator.rs:193-197
Timestamp: 2025-11-19T12:55:48.931Z
Learning: In the magicblock-validator codebase, when constructing arguments for the light CLI test validator in start_light_validator_with_config (test-integration/test-tools/src/validator.rs), the shlex::split approach is necessary because the light CLI does not handle whitespaces properly in validator arguments. The string concatenation + shlex parsing pattern should not be refactored to direct argument construction in this specific case.
Applied to files:
test-integration/test-ledger-restore/tests/08_commit_update.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:
test-integration/schedulecommit/test-scenarios/tests/01_commits.rs
📚 Learning: 2025-11-12T09:46:27.553Z
Learnt from: Dodecahedr0x
Repo: magicblock-labs/magicblock-validator PR: 614
File: magicblock-task-scheduler/src/db.rs:26-0
Timestamp: 2025-11-12T09:46:27.553Z
Learning: In magicblock-task-scheduler, task parameter validation (including ensuring iterations > 0 and enforcing minimum execution intervals) is performed in the Magic program (on-chain) before ScheduleTaskRequest instances reach the scheduler service. The From<&ScheduleTaskRequest> conversion in db.rs does not need additional validation because inputs are already validated at the program level.
Applied to files:
test-integration/test-committor-service/tests/common.rsmagicblock-committor-service/src/tasks/mod.rs
📚 Learning: 2025-11-20T17:25:23.444Z
Learnt from: JMirval
Repo: magicblock-labs/magicblock-validator PR: 656
File: programs/guinea/src/lib.rs:33-33
Timestamp: 2025-11-20T17:25:23.444Z
Learning: In the magicblock-validator codebase, task IDs (task_id) are i64 values that can be negative. The task scheduler is designed to handle any i64 value including negative task IDs. Task IDs are randomly generated using rand::random() which produces values across the full i64 range. No validation is needed to restrict task IDs to positive values.
Applied to files:
magicblock-committor-service/src/tasks/task_strategist.rs
📚 Learning: 2025-10-26T16:53:29.820Z
Learnt from: thlorenz
Repo: magicblock-labs/magicblock-validator PR: 587
File: magicblock-chainlink/src/remote_account_provider/mod.rs:134-0
Timestamp: 2025-10-26T16:53:29.820Z
Learning: In magicblock-chainlink/src/remote_account_provider/mod.rs, the `Endpoint::separate_pubsub_url_and_api_key()` method uses `split_once("?api-key=")` because the api-key parameter is always the only query parameter right after `?`. No additional query parameter parsing is needed for this use case.
Applied to files:
magicblock-committor-service/src/intent_executor/task_info_fetcher.rs
🧬 Code graph analysis (22)
test-integration/test-committor-service/tests/test_ix_commit_local.rs (1)
magicblock-validator/src/main.rs (1)
init_logger(13-50)
magicblock-committor-service/src/intent_executor/mod.rs (2)
magicblock-committor-service/src/intent_executor/null_task_info_fetcher.rs (1)
get_base_account(36-41)magicblock-committor-service/src/intent_executor/task_info_fetcher.rs (2)
get_base_account(43-46)get_base_account(276-281)
magicblock-committor-service/src/tasks/task_builder.rs (3)
magicblock-committor-service/src/transaction_preparator/delivery_preparator.rs (2)
join_all(467-476)new(49-59)magicblock-committor-service/src/tasks/mod.rs (1)
create_commit_task(121-157)test-integration/test-committor-service/tests/common.rs (2)
create_commit_task(162-180)new(52-55)
magicblock-committor-service/src/intent_executor/null_task_info_fetcher.rs (1)
magicblock-committor-service/src/intent_executor/task_info_fetcher.rs (10)
fetch_next_commit_ids(26-29)fetch_next_commit_ids(169-236)fetch_rent_reimbursements(32-35)fetch_rent_reimbursements(238-253)peek_commit_id(38-38)peek_commit_id(256-259)reset(41-41)reset(262-274)get_base_account(43-46)get_base_account(276-281)
test-integration/test-ledger-restore/tests/08_commit_update.rs (1)
test-integration/test-ledger-restore/src/lib.rs (1)
setup_validator_with_local_remote(97-119)
magicblock-committor-service/src/intent_execution_manager/intent_execution_engine.rs (3)
magicblock-committor-service/src/intent_executor/null_task_info_fetcher.rs (1)
get_base_account(36-41)magicblock-committor-service/src/intent_executor/task_info_fetcher.rs (2)
get_base_account(43-46)get_base_account(276-281)test-integration/test-committor-service/tests/common.rs (1)
get_base_account(145-150)
test-integration/schedulecommit/test-scenarios/tests/01_commits.rs (1)
test-integration/schedulecommit/test-scenarios/tests/utils/mod.rs (1)
get_context_with_delegated_committees(16-35)
test-integration/test-tools/src/integration_test_context.rs (1)
magicblock-chainlink/src/remote_account_provider/mod.rs (2)
rpc_client(1068-1070)rpc_client(1080-1082)
test-integration/schedulecommit/test-security/tests/01_invocations.rs (1)
test-integration/schedulecommit/client/src/schedule_commit_context.rs (2)
try_new(72-74)try_new_random_keys(66-71)
test-integration/test-committor-service/tests/common.rs (4)
magicblock-committor-service/src/intent_executor/mod.rs (2)
new(113-125)get_base_account(826-831)magicblock-committor-service/src/intent_executor/task_info_fetcher.rs (3)
new(60-68)get_base_account(43-46)get_base_account(276-281)magicblock-committor-service/src/intent_executor/null_task_info_fetcher.rs (1)
get_base_account(36-41)magicblock-committor-service/src/tasks/mod.rs (1)
create_commit_task(121-157)
test-integration/programs/schedulecommit/src/lib.rs (2)
test-integration/programs/schedulecommit/src/utils/mod.rs (3)
assert_is_signer(30-44)assert_keys_equal(16-28)allocate_account_and_assign_owner(58-106)test-integration/programs/schedulecommit/src/order_book.rs (1)
new(105-136)
test-integration/schedulecommit/test-scenarios/tests/02_commit_and_undelegate.rs (5)
test-integration/programs/schedulecommit/src/api.rs (2)
schedule_commit_diff_instruction_for_order_book(196-215)update_order_book_instruction(178-194)test-integration/schedulecommit/test-scenarios/tests/utils/mod.rs (3)
get_context_with_delegated_committees(16-35)assert_one_committee_was_committed(41-65)assert_one_committee_account_was_undelegated_on_chain(190-196)test-integration/schedulecommit/client/src/schedule_commit_context.rs (3)
committees(213-217)ephem_blockhash(331-333)ephem_client(327-329)test-integration/programs/schedulecommit/src/order_book.rs (1)
bids(182-184)test-integration/schedulecommit/client/src/verify.rs (1)
fetch_and_verify_order_book_commit_result_from_logs(16-23)
magicblock-committor-service/src/tasks/task_strategist.rs (2)
magicblock-committor-service/src/tasks/args_task.rs (1)
new(36-41)magicblock-committor-service/src/tasks/mod.rs (1)
create_commit_task(121-157)
test-integration/test-committor-service/tests/test_delivery_preparator.rs (1)
test-integration/test-committor-service/tests/common.rs (2)
create_commit_task(162-180)new(52-55)
test-integration/schedulecommit/client/src/verify.rs (2)
test-integration/schedulecommit/test-security/tests/01_invocations.rs (1)
ctx(148-149)test-integration/test-ledger-restore/tests/08_commit_update.rs (1)
ctx(90-91)
test-integration/schedulecommit/test-scenarios/tests/utils/mod.rs (3)
test-integration/schedulecommit/test-security/tests/01_invocations.rs (1)
ctx(148-149)test-integration/test-ledger-restore/tests/08_commit_update.rs (1)
ctx(90-91)test-integration/schedulecommit/client/src/schedule_commit_context.rs (3)
try_new(72-74)ncommittees(102-121)try_new_random_keys(66-71)
magicblock-committor-service/src/tasks/args_task.rs (1)
magicblock-committor-service/src/tasks/buffer_task.rs (2)
new(37-45)new_preparation_required(30-35)
magicblock-committor-service/src/tasks/mod.rs (2)
test-integration/test-committor-service/tests/common.rs (2)
create_commit_task(162-180)new(52-55)magicblock-committor-service/src/intent_executor/task_info_fetcher.rs (1)
new(60-68)
test-integration/test-committor-service/tests/test_transaction_preparator.rs (3)
test-integration/test-committor-service/tests/common.rs (3)
new(52-55)create_commit_task(162-180)create_task_info_fetcher(116-118)magicblock-committor-service/src/tasks/buffer_task.rs (2)
new(37-45)new_preparation_required(30-35)magicblock-committor-service/src/tasks/mod.rs (1)
create_commit_task(121-157)
test-integration/programs/schedulecommit/src/api.rs (1)
test-integration/programs/schedulecommit/src/order_book.rs (1)
new(105-136)
magicblock-committor-service/src/intent_executor/task_info_fetcher.rs (4)
magicblock-committor-service/src/intent_execution_manager/intent_execution_engine.rs (1)
get_base_account(804-809)magicblock-committor-service/src/intent_executor/mod.rs (1)
get_base_account(826-831)magicblock-committor-service/src/intent_executor/null_task_info_fetcher.rs (1)
get_base_account(36-41)test-integration/test-committor-service/tests/common.rs (1)
get_base_account(145-150)
test-integration/schedulecommit/client/src/schedule_commit_context.rs (1)
test-integration/programs/schedulecommit/src/api.rs (3)
init_order_book_instruction(36-54)init_payer_escrow(77-98)init_account_instruction(16-34)
🔇 Additional comments (33)
test-integration/programs/schedulecommit/Cargo.toml (1)
11-12: LGTM! Dependencies added for test infrastructure.The addition of
rkyvandstatic_assertionsas workspace dependencies is appropriate. Based on the codebase analysis,rkyvis used exclusively in off-chain test/verification code (e.g.,OrderBookOwneddeserialization), not in the BPF program itself, so the default features (including std) are correct.static_assertionsenables compile-time checks for the test program.test-integration/test-committor-service/tests/utils/transactions.rs (1)
123-133: LGTM! Pragmatic solution with clear documentation.The special-case handling for "CommitState" to also match "CommitDiff" is well-justified and clearly documented in the inline comment. Since
CommitDiffis an optimized variant ofCommitStatechosen automatically by the committor service, this approach appropriately abstracts the implementation detail from test assertions.Cargo.toml (1)
111-114: LGTM! Dependency updates aligned with PR objectives.The addition of
magicblock-apertureand the revision update formagicblock-delegation-program(from aa1de56d90c to e8d03936) support the CommitDiff functionality introduced in this PR. The local path dependencies are acknowledged as temporary and will be converted to git references before merge.test-integration/schedulecommit/test-scenarios/Cargo.toml (1)
19-20: LGTM! Test dependencies added appropriately.The addition of
randandborshas workspace dev-dependencies is appropriate for test scenarios. These enable random data generation and serialization/deserialization support in test code.test-integration/test-tools/src/integration_test_context.rs (2)
158-160: LGTM! Error message now properly formatted.The change from
.expect()to.unwrap_or_else()correctly fixes the format placeholder issue, ensuring the panic message includes the actuallabelvalue when the RPC client doesn't exist.
169-176: LGTM! Transaction version handling documented.The conditional
max_supported_transaction_versionlogic is well-documented. SettingSome(0)for the chain cluster enables explicit v0 transaction support, while the ephemeral cluster uses default version handling. The inline comment clearly explains this distinction.test-integration/test-ledger-restore/tests/08_commit_update.rs (2)
55-55: LGTM! Proper temporary directory handling.The destructuring now correctly captures the
_tmpdirvalue returned bysetup_validator_with_local_remote. Binding it (even as_tmpdir) keeps the temporary directory alive for the scope, preventing premature cleanup. This is the standard Rust pattern for managing temporary resources.
170-170: LGTM! Consistent temporary directory handling.Same proper handling of the temporary directory as in the
writefunction above.test-integration/Cargo.toml (3)
40-40: LGTM! Dependency reordering.The reordering of
ephemeral-rollups-sdkhas no functional impact and likely improves organization.
60-62: LGTM! Delegation program revision updated.The revision update for
magicblock-delegation-program(from aa1de56d90c to e8d03936) aligns with the CommitDiff functionality. The local path dependency concern noted in past reviews is acknowledged as temporary and will be addressed before merge.
74-74: LGTM! Test infrastructure dependencies added.The additions of
rkyv = "0.7.45"andstatic_assertions = "1.1.0"to workspace dependencies are appropriate. As confirmed in the codebase analysis,rkyvis used exclusively in off-chain test/verification code (not BPF programs), so the default features (including std) are correct.static_assertionsprovides compile-time checks for test infrastructure.Also applies to: 93-93
test-integration/programs/schedulecommit/src/utils/mod.rs (4)
53-53: LGTM! Type improved for consistency with Solana APIs.Changing the
sizefield fromusizetou64aligns better with Solana's system instruction APIs (e.g.,system_instruction::allocateexpectsu64) and provides consistency across the codebase.
70-71: LGTM! Safe cast for 64-bit Solana environment.The cast
size as usizeis safe because Solana exclusively runs in 64-bit environments whereusizeis 64 bits, so no truncation can occur. Based on learnings.
75-80: LGTM! Helpful debug logging added.The debug log printing
required_lamportsand payer balance aids in troubleshooting account allocation issues during testing.
90-90: LGTM! Cleaner API usage.Passing
sizedirectly tosystem_instruction::allocate(which acceptsu64) is cleaner than the previoustry_into().unwrap()conversion and aligns with the updated field type.test-integration/test-committor-service/tests/test_ix_commit_local.rs (2)
158-282: UpdatedCommitStrategyexpectations look consistent with Args‑first plannerThe revised
expect_strategies(...)maps across the multi‑account and bundle‑size tests (moreCommitStrategy::Args, adjustedFromBufferWithLookupTablecounts) match the described behavior of the new planner that prefers Args when the intent fits and falls back to buffers+ALTs only when needed. The comments explaining when ALTs are required (e.g., undelegate‑all scenarios and large bundles) still align with the expectations.Also applies to: 284-345
786-787:MockInfoFetcher::get_base_accountstub is adequate for current testsAdding
get_base_accountreturningOk(None)keeps the testMockInfoFetchercompatible with the extendedTaskInfoFetchertrait, and is appropriate as long as these tests don't rely on diff behavior that needs the base account contents. If future tests start exercising CommitDiff based on base-account size or data, you'll likely want to extend this mock to return a syntheticAccountinstead ofNone.test-integration/schedulecommit/client/src/verify.rs (1)
2-3: Order‑book verify helper matches existing patternThe new
fetch_and_verify_order_book_commit_result_from_logsmirrors the existingMainAccounthelper and cleanly exposesScheduledCommitResult<OrderBookOwned>without changing the underlying flow. This keeps call sites simple and is fine even with the small duplication.Also applies to: 16-23
magicblock-committor-service/src/intent_executor/mod.rs (2)
7-9: cfg‑gatedNullTaskInfoFetcherexport is well‑scopedThe new
null_task_info_fetchermodule andpub useare both guarded by#[cfg(any(test, feature = "dev-context-only-utils"))], so tests and dev‑context utilities can rely onNullTaskInfoFetcherwithout affecting production builds. This is a good pattern for exposing test‑only helpers.Also applies to: 28-29
786-787: TestMockInfoFetcherfully implements the extended traitThe added imports and
get_base_accountimplementation in the test module keepMockInfoFetcherin sync with the updatedTaskInfoFetchertrait while clearly signaling “no base account available” viaOk(None). That’s appropriate for this unit test, which only needs commit IDs and rent reimbursements.Also applies to: 803-832
test-integration/test-committor-service/tests/test_delivery_preparator.rs (1)
2-3: Asynccreate_commit_task+join_allusage is appropriate for test setupMigrating to the async
create_commit_taskhelper and usingjoin_allto build multipleBufferTask/ArgsTaskinstances keeps the tests aligned with the new builder API while avoiding unnecessary sequential awaits. The closures only capture references into the localdatasarray and are awaited viajoin_all(not spawned), so lifetimes and ordering remain correct for the subsequent assertions.Also applies to: 25-31, 90-97, 167-172, 211-213, 300-303
programs/magicblock/src/magic_scheduled_base_intent.rs (1)
310-325:CommittedAccountabstraction cleans up commit‑account handlingIntroducing the
CommittedAccountstruct (withCommittedAccountRefand theFromimpl) and then using it insideCommitType::try_from_argsremoves ad‑hoc tuple handling and centralizes theAccountSharedData→Accountconversion. The subsequent owner override viacontext.parent_program_id.unwrap_or(...)preserves the previous behavior while making the code easier to follow.Also applies to: 413-463
magicblock-committor-service/src/intent_execution_manager/intent_execution_engine.rs (1)
347-369: Test TaskInfoFetcher extension is consistent and safeThe added
MagicBlockRpcClientResult/Accountimports andMockInfoFetcher::get_base_accountstub cleanly track theTaskInfoFetchertrait extension and correctly model “account not found” asOk(None)for tests. No issues from a correctness or async standpoint.Also applies to: 780-809
magicblock-committor-service/src/intent_executor/null_task_info_fetcher.rs (1)
1-42: NullTaskInfoFetcher implementation matches intended null‑object semantics
NullTaskInfoFetcherreturns empty maps, empty vectors,Nonefor peek, andOk(None)forget_base_account, which is exactly what you want from a no‑op test/dummy fetcher. The implementation is simple, deterministic, and correctly satisfies theTaskInfoFetchertrait.test-integration/test-committor-service/tests/test_transaction_preparator.rs (1)
10-12: CommitTaskBuilder + TaskInfoFetcher wiring in transaction‑preparator tests looks correctAll three tests now construct commit tasks via
CommitTaskBuilder::create_commit_task(..., &fixture.create_task_info_fetcher()).await, which correctly exercises the async builder andTaskInfoFetcherintegration without changing the behavioral expectations of the tests. No functional or async‑lifetime issues stand out.Also applies to: 37-47, 96-117, 200-209
test-integration/schedulecommit/test-scenarios/tests/utils/mod.rs (1)
16-35: Seeded context helper and generic commit assertion integrate cleanlyThreading
user_seedthroughget_context_with_delegated_committeesto theScheduleCommitTestContextconstructors, and performing bothinit_committees()anddelegate_committees()inside the helper, matches how the tests use this function and is required for the new order‑book scenarios.The generic
assert_one_committee_was_committed<T>overScheduledCommitResult<T>is a reasonable generalization for non‑MainAccountpayloads (like order books) and doesn’t change the assertion semantics.Also applies to: 40-47
magicblock-committor-service/src/tasks/task_builder.rs (1)
24-38: Async commit‑task building via TaskInfoFetcher is correctly wiredThe
TasksBuilder::commit_tasksrefactor to use a singletask_info_fetcher: &Arc<C>plusjoin_allto buildCommitTasks is sound:
- Commit ids are fetched once via
fetch_next_commit_idsand persisted before task construction.- Each async closure looks up its pubkey’s commit id and calls
CommitTaskBuilder::create_commit_task(..., task_info_fetcher).await, enabling concurrent base‑account RPCs while reusing the same fetcher.- The
expecton thecommit_idsmap is appropriate given the “all or error” contract.- Finalize‑stage usage of
fetch_rent_reimbursementsis unchanged aside from the parameter rename and remains coherent.Overall this aligns
TaskBuilderImplwith the new CommitDiff‑capable builder without introducing new correctness or concurrency issues.Also applies to: 72-89, 90-105, 108-199
magicblock-committor-service/src/tasks/args_task.rs (1)
1-12: CommitTask‑driven instruction generation and guarded CommitDiff downgrade look correctRouting commit instructions through
CommitTask::create_commit_ix(validator)makesArgsTask::instructionpure and keeps diff/state decision logic encapsulated inCommitTask, which is a good separation of concerns.In
optimize(), the explicitvalue.is_commit_diff()check plusforce_commit_state()before wrapping in aBufferTaskmatches the documented intent: CommitDiff tasks remain on the Args path unless they’re being converted into buffer‑based commits, at which point they are intentionally downgraded to CommitState until BufferTask learns how to handle diffs. This is consistent with the current design and avoids accidental use of CommitDiff in unsupported paths.Also applies to: 44-85, 87-112, 131-137, 145-165
magicblock-committor-service/src/tasks/task_strategist.rs (1)
159-171: TaskStrategist remains correct; new async tests cover builder‑based commit tasksThe core strategist logic is unchanged aside from the TODO, and the new test module wiring looks good:
create_test_commit_taskbuilds commit tasks viaCommitTaskBuilder::create_commit_task(..., &Arc::new(NullTaskInfoFetcher)).await, so the strategist is exercised against the same commit‑task shape used in production (minus base‑account fetching).- The suite of
#[tokio::test]cases verifies:
- small commit stays as Args,
- large commit(s) convert to Buffer,
- multiple commits lead to multiple buffers and, when necessary, ALTs,
- failure when even ALTs can’t make the tx fit, and
- that optimization prioritizes the largest tasks and leaves non‑commit tasks (Finalize/BaseAction/Undelegate) as Args.
This gives solid coverage of the sizing/optimization behavior with the new async commit‑task construction.
Also applies to: 253-269, 271-276, 328-518
test-integration/test-committor-service/tests/common.rs (2)
101-122: MockTaskInfoFetcher wiring with IntentExecutor looks correctUsing
MockTaskInfoFetcheras a thin wrapper overMagicblockRpcClientand injecting it viacreate_task_info_fetcher()intoIntentExecutorImpl::newcleanly aligns tests with the realTaskInfoFetcherflow, includingget_base_account. No issues from a correctness or design perspective here.
145-150: Thin get_base_account wrapper is appropriate for integration testsDelegating
MockTaskInfoFetcher::get_base_accountdirectly toself.0.get_account(pubkey).awaitis a straightforward way to exercise real RPC-based base-account lookups in integration tests and matches theTaskInfoFetchertrait semantics. This is consistent with the new CommitDiff flow and needs no changes.magicblock-committor-service/src/tasks/mod.rs (2)
160-230: CommitTask diff/state selection and instruction builders are coherentThe extended
CommitTaskimplementation behaves as expected:
is_commit_diff()requires:
!force_commit_state,- account data length >
COMMIT_STATE_SIZE_THRESHOLD,base_account.is_some().
create_commit_ix()chooses:
- Diff path when
base_accountisSome(with a defensiveforce_commit_statecheck insidecreate_commit_diff_ix), otherwise- CommitState.
create_commit_state_ix()andcreate_commit_diff_ix()constructCommitStateArgs/CommitDiffArgswith consistent fields, andcompute_diff(base, target)usesReadableAccount::data()for both account views.This matches the intended semantics: CommitDiff is used only when a base account is available and not forced off, with a clean fallback to CommitState in all other scenarios. No functional issues here.
418-563: Async serialization tests now exercise the builder path without real RPC, which is appropriateThe updated tests:
- Construct
CommitTasks viaCommitTaskBuilder::create_commit_task(...).awaitand wrap them inArgsTask/BufferTaskvariants.- Use
NullTaskInfoFetchereverywhere in this module, so no real RPC calls occur; even the 1024‑byte case intest_preparation_instructions_serializationonly hits the builder with a cheapOk(None).- Assert that instructions from ArgsTask, BufferTask (including preparation init/realloc/write Ixs) are serializable via
bincode.This gives good coverage of the new builder-based construction and ensures the public instruction shapes remain serializable, without introducing blocking I/O into the tests. Looks solid.
This PR does the following things:
CommitDiff. So now when a commit is scheduled, it triggers eitherCommitStateorCommitDiffin the delegation-program, depending on the account size and theCOMMIT_STATE_SIZE_THRESHOLD.CommitDiffis effectively an optimization that reduces the amount of data transferred when committing small changes in large accounts, which improves performance and reduces costs.args, notbufferaccount.OrderBookaccount (10 KB). I initially wanted to try a few MB, but realized currently that’s not possible yet.CommitDiffwas invoked (in the delegation-program) with onlydiff len = 286whereasdata len = 10240(see first screenshot).diff len = 8(encoding the size of account on ER and the number of offset-pairs), and logs a warning (see second screenshot).Update dlp.so
Updated
test-integration/schedulecommit/elfs/dlp.soto use latest version of DLP withCommitDiffix.Changes in the existing tests.
Two kinds of changes:
Since it is
CommitTaskwhich decides the exact DLP instructionCommitStateorCommitDiff, and many tests which earlier used to get executed asBufferTask, will now be executed asArgsTaskbecause large accounts with small changes can be executed asArgsTaskas long as the transaction size stays within limit. So made the related changes in the tests.For some tests, I made the following change:
It is because now
CommitTask::new()uses blockingRpcClientto fetch account from the base chain, so the tests fail because they run in a single-threaded environment, giving this error:Using
multi_threadwith 2 threads makes the tests work again.Screenshots
Summary by CodeRabbit
Release Notes
New Features
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.