feat: first compression iteration #639
Conversation
also use the latest SVM patch
* bmuddha/epic/rpc: tests: added fee test, fixed mutator tests
* bmuddha/epic/rpc: chore: rename magicblock-gateway to magicblock-aperture
* bmuddha/epic/rpc: fix: use port retries in test, fix blockstore
* bmuddha/epic/rpc: fixes: use random ports with retries in tests
since accountsdb snapshots take place in async context (due to the slot advancement happening in tickers), we end up blocking the runtime, while trying to acquire a sync lock. In order to avoid that, this PR moves the whole lock acquisition and snapshot logic a separate thread and performs it in the background, which makes slot advancement a non-blocking operation.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 27
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
magicblock-config/src/validator.rs (1)
37-38: Inconsistent pattern with other optional fields.The
base_feesfield now lacks a#[serde(default)]attribute, while thefqdnfield (lines 30-33) retains#[serde(default = "default_fqdn")]despite both beingOptiontypes. SinceOption<T>automatically defaults toNoneduring deserialization when the field is missing, the explicit serde default is technically redundant in both cases.For consistency and maintainability, consider either:
- Removing
#[serde(default = "default_fqdn")]fromfqdnto match the simpler pattern now used forbase_fees, or- Adding back
#[serde(default = "default_base_fees")]tobase_feesto maintain the explicit pattern used forfqdn</comment_end>
magicblock-aperture/src/state/blocks.rs (1)
55-69: Consider seeding the expiring cache with the initiallatestblock inBlocksCache::new
BlocksCache::newnow accepts alatest: LastCachedBlockand stores it inself.latest, but the underlyingExpiringCachestarts empty and only receives entries viaset_latest. That means the block returned byledger.latest_block().load()is immediately visible throughget_latest()/block_height(), but not through cache lookups until the firstBlockUpdatearrives.If any callers rely on validating the current latest blockhash via the cache (using the
Derefinterface) before the firstset_latestcall, this is a behavioral change. If you want the initial ledger block to be usable for cache‑based validation as well, consider pushing it intocacheinsidenew:pub(crate) fn new(blocktime: u64, latest: LastCachedBlock) -> Self { const BLOCK_CACHE_TTL: Duration = Duration::from_secs(60); assert!(blocktime != 0, "blocktime cannot be zero"); let blocktime_ratio = SOLANA_BLOCK_TIME / blocktime as f64; let block_validity = blocktime_ratio * MAX_VALID_BLOCKHASH_SLOTS; - let cache = ExpiringCache::new(BLOCK_CACHE_TTL); - Self { - latest: ArcSwapAny::new(latest.into()), - block_validity: block_validity as u64, - cache, - } + let mut cache = ExpiringCache::new(BLOCK_CACHE_TTL); + cache.push(latest.blockhash, BlockMeta { slot: latest.slot, /* fill other fields as appropriate */ }); + Self { + latest: ArcSwapAny::new(latest.into()), + block_validity: block_validity as u64, + cache, + } }(Adjust the
BlockMetaconstruction to match your actual type.) If no callers depend on cache visibility for the initial block, leaving it as‑is is fine, but it’s worth confirming the intended semantics.
♻️ Duplicate comments (9)
magicblock-processor/src/executor/processing.rs (1)
342-410: Critical security vulnerability: gasless feepayer protection is ineffective due to mutating inner status instead of converting to hard error.This is a duplicate of the previously flagged issue. Despite being marked as addressed, both security concerns persist in the current code:
Issue 1: Account mutations are still persisted despite error status
Lines 368-369 and 401 set
executed.execution_details.status = Err(...), but leaveresultasOk(ProcessedTransaction::Executed(...)). This means:
- Line 76 still calls
commit_accounts(feepayer, &processed, is_replay)- For failed transactions, lines 290-295 persist the feepayer account (first account)
- Lines 323-326 write dirty feepayer accounts to AccountsDb
- Illegal feepayer mutations get persisted despite the error, defeating the protection described in lines 353-357
Issue 2: Auto-airdrop condition polarity appears inverted
Line 361 uses
!self.is_auto_airdrop_lamports_enabled, applying the guard only when auto-airdrop is disabled. However:
- The comment (lines 353-357) aims to prevent "validator losing funds upon balance settling"
- This risk only exists when auto-airdrop is enabled (validator funds accounts)
- Current logic is backwards
Apply the previously suggested fix:
fn verify_account_states(&self, processed: &mut ProcessedTransaction) { - let ProcessedTransaction::Executed(executed) = processed else { + let ProcessedTransaction::Executed(ref mut executed) = processed else { return; }; let txn = &executed.loaded_transaction; // ... existing checks ... if gasless { let undelegated_feepayer_was_modified = feepayer .map(|acc| { (acc.1.is_dirty() - && !self.is_auto_airdrop_lamports_enabled + && self.is_auto_airdrop_lamports_enabled && (acc.1.lamports() != 0 || rollback_lamports != 0)) && !acc.1.delegated() && !acc.1.privileged() }) .unwrap_or_default(); if undelegated_feepayer_was_modified { - executed.execution_details.status = - Err(TransactionError::InvalidAccountForFee); - let logs = executed - .execution_details - .log_messages - .get_or_insert_default(); - let msg = "Feepayer balance has been modified illegally".into(); - logs.push(msg); - return; + // Convert to hard error so no account mutations are committed + *processed = ProcessedTransaction::FeesOnly(/* construct appropriate FeesOnly variant */); + // OR better: change process() to return Err(TransactionError::InvalidAccountForFee) + // when verify_account_states detects violations } } }Better approach: Have
verify_account_statesreturn aResult, and inprocess()(lines 174-177) convert theOk(processed)toErr(...)when validation fails. This ensures the execution takes the early-return path inexecute()(lines 56-68), which skipscommit_accountsentirely.Based on learnings.
programs/magicblock/src/mutate_accounts/process_mutate_accounts.rs (1)
329-331: Fix the rent_epoch assertion to match the modified value.The modification at line 329 sets
rent_epoch: Some(88), which is applied by the processor at lines 202-209. However, the assertion at line 383 still expectsrent_epoch: u64::MAX. The test should verify that the account'srent_epochis 88, not u64::MAX.The compressed field handling (lines 331 and 375) is correct.
Apply this diff to fix the assertion:
assert_matches!( modified_account.into(), Account { lamports: 200, owner: owner_key, executable: true, data, - rent_epoch: u64::MAX, + rent_epoch: 88, } => { assert_eq!(data, modification.data.unwrap()); assert_eq!(owner_key, modification.owner.unwrap()); } );Also applies to: 375-388
magicblock-committor-service/src/intent_executor/task_info_fetcher.rs (1)
233-251: Guard against mismatched lengths between Photon results and requested pubkeys
compressed_accounts.items.into_iter().zip(pubkeys.iter())will silently drop trailingpubkeysifitems.len() < pubkeys.len(), so some requested accounts would never get aCompressedDelegationRecordor cachedcommit_id. That’s a latent correctness bug if the indexer ever returns fewer items than requested (even transiently).Consider asserting or explicitly handling a length mismatch before zipping, e.g.:
- let compressed_delegation_records = compressed_accounts - .items + let items = compressed_accounts.items; + + debug_assert_eq!( + items.len(), + pubkeys.len(), + "expected one compressed account per pubkey ({} pubkeys, {} items)", + pubkeys.len(), + items.len(), + ); + + let compressed_delegation_records = items .into_iter() .zip(pubkeys.iter()) .map(|(acc, pubkey)| {If you’d rather fail loudly in release builds too, replace the
debug_assert_eq!with a runtime check that returns a suitableTaskInfoFetcherErrorwhen the lengths differ.test-integration/test-chainlink/src/ixtest_context.rs (1)
98-101: Fix PackedAddressTreeInfo queue index wiring and remove unusedpda_seeds.Two things to tighten up in
delegate_compressed_counter:
address_queue_pubkey_indexis set to the merkle-tree indexIn the non‑redelegate branch:
let address_merkle_tree_pubkey_index = remaining_accounts.insert_or_get(ADDRESS_TREE_PUBKEY); let state_queue_pubkey_index = remaining_accounts.insert_or_get(OUTPUT_QUEUE_PUBKEY); let packed_address_tree_info = PackedAddressTreeInfo { root_index: rpc_result.addresses[0].root_index, address_merkle_tree_pubkey_index, address_queue_pubkey_index: address_merkle_tree_pubkey_index, };
address_queue_pubkey_indexshould reference the address‑queue account index, not the merkle‑tree index. Otherwise the Light CPI will see the wrong account in that slot. Consider:
- Introducing a dedicated address‑queue constant (e.g.,
ADDRESS_QUEUE_PUBKEY) and inserting it intoremaining_accountsto obtainaddress_queue_pubkey_index, or- If
OUTPUT_QUEUE_PUBKEYis truly the address queue, rename the variable (and constant) accordingly and pass that index intoaddress_queue_pubkey_index.Similarly, double‑check that
state_queue_pubkey_indexreally is the intended value to feed intooutput_state_tree_index; the name suggests a queue rather than the state tree.Remove unused
pda_seedsIn the same function:
let pda_seeds = FlexiCounter::seeds(&auth); let (pda, bump) = FlexiCounter::pda(&auth);
pda_seedsis never read, so this binding can be dropped.Also applies to: 401-494
magicblock-committor-service/src/tasks/args_task.rs (1)
1-8: Wire UndelegateArgs into compressed undelegate instructions (builder still incomplete).
ArgsTaskType::CompressedUndelegatenow sets payer, delegated account, owner program, system program, and remaining accounts, but it never passes the actual undelegate arguments:ArgsTaskType::CompressedUndelegate(value) => { compressed_delegation_client::UndelegateBuilder::new() .payer(*validator) .delegated_account(value.delegated_account) .owner_program(value.owner_program) .system_program(system_program::ID) .add_remaining_accounts(&value.compressed_data.remaining_accounts) .instruction() }By contrast, the test helper in
ixtest_context.rscalls.args(UndelegateArgs { validity_proof, delegation_record_account_meta, compressed_delegated_record, .. })before.add_remaining_accounts(..).instruction(). Here you already have:
value.compressed_data.proofvalue.compressed_data.account_metavalue.compressed_data.compressed_delegation_record_bytesbut they are never used.
To avoid building an underspecified CPI:
- Construct an
UndelegateArgsfrom the compressed data (decodingCompressedDelegationRecordfrom the stored bytes, as done in the helper), and- Pass it via
.args(UndelegateArgs { .. })on the builder before.add_remaining_accounts(..).Also double‑check whether
*validatoris the intended payer for compressed undelegation or whether this should instead be the delegated account owner (mirroring the helper).Also applies to: 65-167, 203-213, 247-302
magicblock-committor-service/src/tasks/task_builder.rs (1)
260-279: Compressed finalize with base actions still silently downgrades on compressed-data fetch errorsIn
process_commitforCommitType::WithBaseActions, the compressed path still swallowsget_compressed_dataerrors:if is_compressed { let photon_client = photon_client.as_ref().ok_or( TaskBuilderError::PhotonClientNotFound, )?; compressed_data.push( get_compressed_data( &account.pubkey, photon_client, None, ) .await .ok(), // <-- error converted to None ); } else { compressed_data.push(None); }Whereas the
CommitType::Standalone+is_compressedbranch now correctly propagates failures (.await?).If compressed finalize is expected to be required whenever the intent is compressed, silently converting failures into
None(falling back to non-compressedFinalizeTask) can hide Photon/indexer issues and produce inconsistent behavior between pure compressed commits and compressed+base-action commits.Options:
- If silent downgrade is desired, at least log/metric the error before pushing
Noneso operators can see compressed finalize is failing:match get_compressed_data(&account.pubkey, photon_client, None).await { Ok(data) => compressed_data.push(Some(data)), Err(err) => { warn!( "Failed to fetch compressed data for {} in finalize+base-actions: {err}", account.pubkey ); compressed_data.push(None); } }
- If downgrade is not desired, mirror the
Standalonepath and propagateTaskBuilderError::CompressedDataFetchErrorinstead of using.ok().This would make compressed behavior consistent across commit/finalize paths.
magicblock-chainlink/src/remote_account_provider/mod.rs (2)
1177-1193: Critical: Unwrap will panic on Photon indexer errors.Line 1180 contains
.unwrap()on the Photon client call which will panic if the indexer is unavailable, returns an error, or times out.This was previously flagged and marked as addressed (commits 3dd8b8d to fcf3260), but appears to still be present. If this is a regression, it should be re-fixed.
Apply proper error handling:
- // TODO: @@@ unwrap and/or retry - let (compressed_accounts, slot) = photon_client + let (compressed_accounts, slot) = photon_client .get_multiple_accounts(&pubkeys, Some(min_context_slot)) .await - .unwrap(); + .map_err(|e| { + error!("Failed to fetch from Photon: {:?}", e); + RemoteAccountProviderError::FailedFetchingAccounts( + format!("Photon fetch failed: {e:?}") + ) + })?;Update return type to:
async fn fetch_from_photon(...) -> Result<(FetchedRemoteAccounts, u64, u64), RemoteAccountProviderError>
1161-1169: Critical: Double unwrap will panic on task spawn or RPC errors.Line 1161 contains
.await.unwrap().unwrap()which will panic on either:
JoinErrorif the spawned task fails- The inner
Resulterror from RPC operationsThis was flagged and marked as addressed in previous reviews (commits 3dd8b8d to fcf3260), but appears to still be present in the current code. If this is a regression, it needs to be re-fixed.
Apply the previous fix pattern:
- }).await.unwrap().unwrap(); - // TODO: @@@ unwrap - - ( - FetchedRemoteAccounts::Rpc(remote_accounts), + }).await.unwrap()?; + + Ok(( + FetchedRemoteAccounts::Rpc(remote_accounts), found_count, not_found_count, - ) + ))Then update the return type of
fetch_from_rpcto:async fn fetch_from_rpc(...) -> Result<(FetchedRemoteAccounts, u64, u64), RemoteAccountProviderError>And handle the error in the consolidation caller.
test-integration/programs/flexi-counter/src/processor.rs (1)
650-687: Tightenprocess_external_undelegate_compressedrefund and data-restore logicA couple of minor improvements here:
- Skip zero-lamport transfers
You already guard against underflow with
checked_sub, but you still construct a transfer for amount0when the delegated account is already at the recorded lamports. You can avoid an unnecessary CPI:- invoke( - &system_instruction::transfer( - payer.key, - delegated_account.key, - args.delegation_record - .lamports - .checked_sub(delegated_account.lamports()) - .ok_or(ProgramError::ArithmeticOverflow)?, - ), - &[payer.clone(), delegated_account.clone()], - )?; + let target_lamports = args.delegation_record.lamports; + let current_lamports = delegated_account.lamports(); + let delta = target_lamports + .checked_sub(current_lamports) + .ok_or(ProgramError::ArithmeticOverflow)?; + if delta > 0 { + invoke( + &system_instruction::transfer( + payer.key, + delegated_account.key, + delta, + ), + &[payer.clone(), delegated_account.clone()], + )?; + }
- Optional: assert owner before realloc
Given this program is meant to restore the original flexi-counter account, you may also want an explicit owner check before
realloc:+ if delegated_account.owner != &crate::ID { + msg!( + "Unexpected owner for delegated account: {}", + delegated_account.owner + ); + return Err(ProgramError::InvalidAccountData); + }This would fail fast with a clearer error if the compressed delegation flow ever calls this entrypoint without reassigning the account back to
crate::IDfirst, improving debuggability of the integration tests.
magicblock-chainlink/src/chainlink/account_still_undelegating_on_chain.rs
Show resolved
Hide resolved
magicblock-chainlink/src/chainlink/account_still_undelegating_on_chain.rs
Show resolved
Hide resolved
| #[test] | ||
| fn test_account_was_undelegated_and_remained_so() { | ||
| // Case A: The account was undelegated and remained so. | ||
| // Conditions: | ||
| // - is_delegated: false (account is not delegated to us on chain) | ||
| // - deleg_record: None (no delegation record associated) | ||
| // Expected: true (should override/clone as is) | ||
|
|
||
| let pubkey = Pubkey::default(); | ||
| let is_delegated = false; | ||
| let remote_slot = 100; | ||
| let deleg_record = None; | ||
|
|
||
| assert!(!account_still_undelegating_on_chain( | ||
| &pubkey, | ||
| is_delegated, | ||
| remote_slot, | ||
| deleg_record, | ||
| &Pubkey::default(), | ||
| )); | ||
| } |
There was a problem hiding this comment.
Correct the test comment - expected value is false, not true.
Line 114 states "Expected: true (should override/clone as is)" but the assertion on line 121 expects false (via !account_still_undelegating_on_chain(...)). The assertion is correct since Case A should return false to allow cloning the remote state. The comment should be updated to match.
- // Expected: true (should override/clone as is)
+ // Expected: false (should override/clone remote state)🤖 Prompt for AI Agents
In magicblock-chainlink/src/chainlink/account_still_undelegating_on_chain.rs
around lines 108 to 128, the test comment for Case A incorrectly says "Expected:
true"; update that line to "Expected: false (should override/clone as is)" so
the comment matches the assertion on line 121 which expects false; no code
changes are required beyond correcting the comment text to reflect the actual
expected value.
programs/magicblock/src/schedule_transactions/process_schedule_commit.rs
Outdated
Show resolved
Hide resolved
Co-authored-by: Thorsten Lorenz <thlorenz@gmx.de> Co-authored-by: Luca Cillario <luca@magicblock.gg> Co-authored-by: Gabriele Picco <piccogabriele@gmail.com> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
test-integration/programs/schedulecommit/src/api.rs (1)
220-270: Remove duplicate function definition.The function
schedule_commit_and_undelegate_cpi_twiceis defined twice (lines 220-244 and lines 246-270) with identical signatures and implementations. This will cause a compilation error.Remove one of the duplicate definitions and update the remaining one to use the
build_instructionhelper for consistency:-pub fn schedule_commit_and_undelegate_cpi_twice( - payer: Pubkey, - magic_program_id: Pubkey, - magic_context_id: Pubkey, - players: &[Pubkey], - committees: &[Pubkey], -) -> Instruction { - let program_id = crate::id(); - let mut account_metas = vec![ - AccountMeta::new(payer, true), - AccountMeta::new(magic_context_id, false), - AccountMeta::new_readonly(magic_program_id, false), - ]; - for committee in committees { - account_metas.push(AccountMeta::new(*committee, false)); - } - - Instruction::new_with_borsh( - program_id, - &ScheduleCommitInstruction::ScheduleCommitAndUndelegateCpiTwice( - players.to_vec(), - ), - account_metas, - ) -} - pub fn schedule_commit_and_undelegate_cpi_twice( payer: Pubkey, magic_program_id: Pubkey, magic_context_id: Pubkey, players: &[Pubkey], committees: &[Pubkey], ) -> Instruction { let program_id = crate::id(); let mut account_metas = vec![ AccountMeta::new(payer, true), AccountMeta::new(magic_context_id, false), AccountMeta::new_readonly(magic_program_id, false), ]; for committee in committees { account_metas.push(AccountMeta::new(*committee, false)); } - Instruction::new_with_borsh( + let ix = ScheduleCommitInstruction::ScheduleCommitAndUndelegateCpiTwice( + players.to_vec(), + ); + build_instruction(program_id, ix, account_metas) - program_id, - &ScheduleCommitInstruction::ScheduleCommitAndUndelegateCpiTwice( - players.to_vec(), - ), - account_metas, - ) }magicblock-committor-service/src/transaction_preparator/delivery_preparator.rs (1)
592-620: Remove dead code variants fromInternalErrorenumThe search confirms that
CompressedDataNotFound,TransactionCreationError, andTransactionSigningErrorare never constructed or used anywhere in the codebase—they only appear in the enum definition.Remove these three variants from the
InternalErrorenum inmagicblock-committor-service/src/transaction_preparator/delivery_preparator.rs:595,604-607to keep the error surface clean and avoid confusion about which error paths are actually reachable.magicblock-chainlink/src/chainlink/fetch_cloner.rs (1)
678-749: Fix fold accumulator arity when introducingowned_by_deleg_compressedThe
foldthat partitions fetched accounts now destructures five accumulators (not_found,plain,owned_by_deleg,owned_by_deleg_compressed,programs) but the initializer only provides fourvec![]. This won’t compile and also makes the intent less clear.Initialize all five buckets explicitly:
- let (not_found, plain, owned_by_deleg, owned_by_deleg_compressed, programs) = - accs.into_iter().zip(pubkeys).fold( - (vec![], vec![], vec![], vec![]), + let (not_found, plain, owned_by_deleg, owned_by_deleg_compressed, programs) = + accs.into_iter().zip(pubkeys).fold( + (vec![], vec![], vec![], vec![], vec![]), |( mut not_found, mut plain, mut owned_by_deleg, mut owned_by_deleg_compressed, mut programs, ), (acc, &pubkey)| { … }, );
♻️ Duplicate comments (5)
magicblock-committor-service/src/intent_execution_manager/intent_execution_engine.rs (1)
778-784: Previous review suggestions have been addressed.The addition of the
_compressedparameter and the idiomatic closure pattern are correct and align with the trait signature changes for compressed delegation support.programs/magicblock/src/schedule_transactions/process_schedule_commit.rs (1)
267-283: Intent selection for compressed vs non‑compressed flows looks correct; optional match refactor.The four branches correctly cover all combinations of
request_undelegationandcompressed, mapping to the expectedMagicBaseIntentvariants and usingCommitType::Standalone(committed_accounts)consistently. Ownership ofcommitted_accountsis sound in this if/else chain.As previously noted in an earlier review, you could switch to a
matchon(opts.request_undelegation, compressed)to make the exhaustiveness explicit and a bit more idiomatic, but this is purely stylistic and non‑blocking.magicblock-committor-service/src/intent_executor/two_stage_executor.rs (1)
109-119: [Duplicate] Avoid silently swallowing errors when fetching commit transaction slotThe commit slot fetch at lines 111–119 uses
.ok()to discard RPC errors without logging. Any network issue, node error, or transaction pruning collapses toNone, losing observability into why the finalize preparation runs without slot information.Consider logging the error before converting to
None:- let commit_slot = self - .rpc_client - .get_transaction( - &commit_signature, - Some(RpcTransactionConfig::default()), - ) - .await - .map(|tx| tx.slot) - .ok(); + let commit_slot = match self + .rpc_client + .get_transaction( + &commit_signature, + Some(RpcTransactionConfig::default()), + ) + .await + { + Ok(tx) => Some(tx.slot), + Err(err) => { + warn!("Failed to fetch commit transaction slot for {}: {err}", commit_signature); + None + } + };This preserves existing behavior (finalize proceeds with
None) while improving debuggability for Photon-indexed flows.magicblock-chainlink/src/remote_account_provider/mod.rs (1)
900-1210: HandleJoinSet::join_allresults and async errors instead of unwrappingThe new parallel RPC+Photon fetch path has a few correctness issues that can lead to panics or ignored errors:
JoinSet::join_all()result is mis‑destructuredAt lines around 937–952:
let (remote_accounts_results, found_count, not_found_count) = join_set.join_all().await.into_iter().fold( (vec![], 0, 0), |(remote_accounts_results, found_count, not_found_count), (accs, found_cnt, not_found_cnt)| { ( [remote_accounts_results, vec![accs]].concat(), found_count + found_cnt, not_found_count + not_found_cnt ) }, );
JoinSet::join_all()returnsVec<Result<T, JoinError>>, notVec<T>. Destructuring directly as(accs, found_cnt, not_found_cnt)will not compile and, even if adjusted, would be ignoring potentialJoinErrors. You need to pattern match each element, handleOk((...))vsErr(e), and decide how to treat failed fetch tasks.
- Double
.unwrap()ontokio::spawninfetch_from_rpcIn
fetch_from_rpc:let (remote_accounts, found_count, not_found_count) = tokio::spawn(async move { … Ok((pubkeys.iter().zip(response.value).map(...).collect(), found_count, not_found_count)) }).await.unwrap().unwrap(); // TODO: @@@ unwrapThis can panic in two ways:
- If the spawned task panics or is cancelled (
JoinError).- If the inner
ResultisErr(RemoteAccountProviderError::FailedFetchingAccounts(_)), which you already use to signal hard failures.Because
notify_errorinside the spawned future already sendsErrto all pending requests for these pubkeys, the outer.unwrap().unwrap()is both redundant and dangerous: it can crash the task after the error was already handled and reported.
- Unwrap on photon fetch
In
fetch_from_photon:let (compressed_accounts, slot) = photon_client .get_multiple_accounts(&pubkeys, Some(min_context_slot)) .await .unwrap(); // TODO: @@@ unwrap and/or retryAny indexer/network error will panic here, again taking down the task instead of being reported as a structured failure. That’s particularly risky since this code runs in the background spawned by
fetch, not in a test.
A minimal, localized fix would:
- Treat both RPC and Photon fetches as fallible, and never unwrap their results.
- Accumulate only successful
(FetchedRemoteAccounts, found_count, not_found_count)into theremote_accounts_resultsvector and log+metric‑increment failures.- If all fetch tasks fail, return early from the outer
tokio::spawninfetch(pending requests will already have been signalled vianotify_erroron the RPC side; for Photon failures you can just log and let RPC be the source of truth).Sketch of the pattern inside
fetch(omitting metrics/trace details for brevity):- tokio::spawn(async move { - let mut join_set = JoinSet::new(); - join_set.spawn(Self::fetch_from_rpc(...)); - if let Some(photon_client) = photon_client { - let photon_client = photon_client.clone(); - join_set.spawn(Self::fetch_from_photon(...)); - } - - let (remote_accounts_results, found_count, not_found_count) = - join_set.join_all().await.into_iter().fold( - (vec![], 0, 0), - |(remote_accounts_results, found_count, not_found_count), - (accs, found_cnt, not_found_cnt)| { - ( - [remote_accounts_results, vec![accs]].concat(), - found_count + found_cnt, - not_found_count + not_found_cnt, - ) - }, - ); + tokio::spawn(async move { + let mut join_set = JoinSet::new(); + join_set.spawn(Self::fetch_from_rpc( + rpc_client, + pubkeys.clone(), + fetching_accounts.clone(), + mark_empty_if_not_found, + min_context_slot, + )); + if let Some(photon_client) = photon_client { + let photon_client = photon_client.clone(); + join_set.spawn(Self::fetch_from_photon( + photon_client, + pubkeys.clone(), + min_context_slot, + )); + } + + let mut remote_accounts_results = Vec::new(); + let mut found_count = 0u64; + let mut not_found_count = 0u64; + + for res in join_set.join_all().await { + match res { + Ok((accs, found_cnt, not_found_cnt)) => { + remote_accounts_results.push(accs); + found_count += found_cnt; + not_found_count += not_found_cnt; + } + Err(join_err) => { + warn!("Background fetch task failed: {join_err:?}"); + // Optionally inc_account_fetches_failed(pubkeys.len() as u64); + } + } + } + + if remote_accounts_results.is_empty() { + // All tasks failed; pending requests should already have been + // signalled via notify_error or equivalent. Nothing more to do. + return; + } @@ - let (remote_accounts, found_count, not_found_count) = - tokio::spawn(async move { + let (remote_accounts, found_count, not_found_count) = + tokio::spawn(async move { … - Ok((pubkeys + Ok((pubkeys .iter() .zip(response.value) .map(|(pubkey, acc)| { … }) - .collect(), found_count, not_found_count)) - }).await.unwrap().unwrap(); - // TODO: @@@ unwrap + .collect(), found_count, not_found_count)) + }).await + .map_err(|e| { + RemoteAccountProviderError::FailedFetchingAccounts( + format!("RPC fetch join failed: {e:?}") + ) + })? + .map_err(|e| { + // notify_error already ran; just propagate/log + e + })?; @@ - let (compressed_accounts, slot) = photon_client - .get_multiple_accounts(&pubkeys, Some(min_context_slot)) - .await - .unwrap(); + let (compressed_accounts, slot) = match photon_client + .get_multiple_accounts(&pubkeys, Some(min_context_slot)) + .await + { + Ok(res) => res, + Err(err) => { + warn!("Photon fetch failed for [{}]: {err:?}", pubkeys_str(&pubkeys)); + // Treat as "no compressed data" and let RPC results drive behavior. + return (FetchedRemoteAccounts::Compressed(vec![]), 0, 0); + } + };You can refine the error mapping to your existing
RemoteAccountProviderErrorvariants and metrics, but the key points are:
- No
.unwrap()on async joins or external calls.- Explicit handling of
JoinErrorand Photon errors.- Avoiding double‑signalling (first with
notify_error, then via panic or second Ok).What is the return type of `tokio::task::JoinSet::join_all` in the current stable Tokio, and does it wrap the task result in `Result<T, JoinError>`?magicblock-committor-service/src/intent_executor/mod.rs (1)
333-376: UseTransactionStrategy::compressedand assert invariants instead of rescanning tasks for compression
handle_commit_id_errornow passes acompressedflag intofetch_next_commit_idsby re‑scanningstrategy.optimized_taskswithany(|task| task.is_compressed()), and there’s still a TODO about mixed compressed/non‑compressed tasks. SinceTransactionStrategyalready carries acompressedflag (and you propagate it through all error handlers), you can:
- Treat
strategy.compressedas the single source of truth for this flag, and- Add a
debug_assert!that all tasks agree with it, so any future mixed‑compression regression fails loudly in debug builds.This removes the TODO, avoids recomputing the same property, and codifies the “no mixed compression in a strategy” invariant you’ve introduced via
InconsistentTaskCompression.A concrete refactor:
- let commit_ids = self - .task_info_fetcher - .fetch_next_commit_ids( - committed_pubkeys, - // TODO(dode): Handle cases where some tasks are compressed and some are not - strategy - .optimized_tasks - .iter() - .any(|task| task.is_compressed()), - ) + debug_assert!( + strategy + .optimized_tasks + .iter() + .all(|task| task.is_compressed()) == strategy.compressed, + "TransactionStrategy.compressed inconsistent with tasks; mixed compressed/non-compressed tasks are not supported", + ); + + let commit_ids = self + .task_info_fetcher + .fetch_next_commit_ids( + committed_pubkeys, + strategy.compressed, + )This keeps
fetch_next_commit_idsdeterministic with respect to the strategy and aligned with the newInconsistentTaskCompressionerror path. Based on learnings, this also addresses the earlier concern about mixed compressed/non‑compressed handling without re‑opening that design discussion.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.locktest-integration/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (40)
Cargo.toml(5 hunks)magicblock-accounts/src/scheduled_commits_processor.rs(2 hunks)magicblock-api/src/magic_validator.rs(8 hunks)magicblock-chainlink/src/chainlink/fetch_cloner.rs(19 hunks)magicblock-chainlink/src/chainlink/mod.rs(8 hunks)magicblock-chainlink/src/remote_account_provider/mod.rs(26 hunks)magicblock-chainlink/src/testing/mod.rs(1 hunks)magicblock-chainlink/tests/01_ensure-accounts.rs(3 hunks)magicblock-chainlink/tests/03_deleg_after_sub.rs(2 hunks)magicblock-chainlink/tests/06_redeleg_us_separate_slots.rs(3 hunks)magicblock-chainlink/tests/07_redeleg_us_same_slot.rs(2 hunks)magicblock-chainlink/tests/utils/test_context.rs(4 hunks)magicblock-committor-service/src/intent_execution_manager/intent_execution_engine.rs(1 hunks)magicblock-committor-service/src/intent_executor/error.rs(2 hunks)magicblock-committor-service/src/intent_executor/mod.rs(27 hunks)magicblock-committor-service/src/intent_executor/single_stage_executor.rs(4 hunks)magicblock-committor-service/src/intent_executor/two_stage_executor.rs(4 hunks)magicblock-committor-service/src/tasks/args_task.rs(7 hunks)magicblock-committor-service/src/tasks/buffer_task.rs(3 hunks)magicblock-committor-service/src/tasks/mod.rs(7 hunks)magicblock-committor-service/src/transaction_preparator/delivery_preparator.rs(10 hunks)magicblock-metrics/src/metrics/mod.rs(3 hunks)programs/magicblock/src/magic_scheduled_base_intent.rs(8 hunks)programs/magicblock/src/schedule_transactions/process_schedule_base_intent.rs(1 hunks)programs/magicblock/src/schedule_transactions/process_schedule_commit.rs(2 hunks)test-integration/programs/flexi-counter/src/processor.rs(10 hunks)test-integration/programs/flexi-counter/src/state.rs(1 hunks)test-integration/programs/schedulecommit/src/api.rs(6 hunks)test-integration/schedulecommit/test-scenarios/tests/01_commits.rs(1 hunks)test-integration/test-chainlink/src/programs.rs(1 hunks)test-integration/test-chainlink/src/test_context.rs(4 hunks)test-integration/test-chainlink/tests/ix_01_ensure-accounts.rs(1 hunks)test-integration/test-chainlink/tests/ix_03_deleg_after_sub.rs(1 hunks)test-integration/test-chainlink/tests/ix_06_redeleg_us_separate_slots.rs(4 hunks)test-integration/test-chainlink/tests/ix_07_redeleg_us_same_slot.rs(1 hunks)test-integration/test-chainlink/tests/ix_remote_account_provider.rs(2 hunks)test-integration/test-committor-service/tests/test_intent_executor.rs(14 hunks)test-integration/test-committor-service/tests/test_ix_commit_local.rs(23 hunks)test-integration/test-committor-service/tests/utils/instructions.rs(3 hunks)test-integration/test-committor-service/tests/utils/transactions.rs(7 hunks)
🧰 Additional context used
🧠 Learnings (25)
📓 Common learnings
Learnt from: Dodecahedr0x
Repo: magicblock-labs/magicblock-validator PR: 639
File: test-integration/test-chainlink/tests/ix_01_ensure-accounts.rs:85-114
Timestamp: 2025-11-19T11:31:24.218Z
Learning: In the magicblock-validator codebase, compressed delegation records are never subscribed to by design. Tests for compressed delegation flows should focus on verifying that the delegated account (e.g., counter PDA) is cloned as delegated and not subscribed, but do not need to check for subscription to delegation-record-like PDAs in the compressed path because subscriptions to compressed delegation records never occur.
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.
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
📚 Learning: 2025-11-19T11:31:24.218Z
Learnt from: Dodecahedr0x
Repo: magicblock-labs/magicblock-validator PR: 639
File: test-integration/test-chainlink/tests/ix_01_ensure-accounts.rs:85-114
Timestamp: 2025-11-19T11:31:24.218Z
Learning: In the magicblock-validator codebase, compressed delegation records are never subscribed to by design. Tests for compressed delegation flows should focus on verifying that the delegated account (e.g., counter PDA) is cloned as delegated and not subscribed, but do not need to check for subscription to delegation-record-like PDAs in the compressed path because subscriptions to compressed delegation records never occur.
Applied to files:
test-integration/test-chainlink/tests/ix_03_deleg_after_sub.rsmagicblock-committor-service/src/intent_executor/error.rstest-integration/test-chainlink/tests/ix_01_ensure-accounts.rstest-integration/test-chainlink/tests/ix_07_redeleg_us_same_slot.rstest-integration/test-chainlink/tests/ix_06_redeleg_us_separate_slots.rsmagicblock-chainlink/tests/01_ensure-accounts.rsmagicblock-metrics/src/metrics/mod.rsmagicblock-committor-service/src/intent_execution_manager/intent_execution_engine.rstest-integration/test-chainlink/tests/ix_remote_account_provider.rsmagicblock-committor-service/src/tasks/args_task.rsmagicblock-chainlink/tests/03_deleg_after_sub.rstest-integration/test-committor-service/tests/utils/transactions.rsmagicblock-api/src/magic_validator.rstest-integration/programs/flexi-counter/src/processor.rstest-integration/test-committor-service/tests/test_ix_commit_local.rstest-integration/test-committor-service/tests/utils/instructions.rsmagicblock-chainlink/src/chainlink/fetch_cloner.rsmagicblock-chainlink/tests/07_redeleg_us_same_slot.rsmagicblock-chainlink/src/remote_account_provider/mod.rsmagicblock-chainlink/tests/06_redeleg_us_separate_slots.rsprograms/magicblock/src/magic_scheduled_base_intent.rsmagicblock-committor-service/src/tasks/mod.rsCargo.tomlmagicblock-committor-service/src/intent_executor/mod.rstest-integration/programs/schedulecommit/src/api.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-chainlink/tests/ix_03_deleg_after_sub.rstest-integration/schedulecommit/test-scenarios/tests/01_commits.rstest-integration/test-chainlink/tests/ix_01_ensure-accounts.rstest-integration/test-chainlink/tests/ix_07_redeleg_us_same_slot.rstest-integration/test-chainlink/tests/ix_06_redeleg_us_separate_slots.rstest-integration/programs/flexi-counter/src/state.rstest-integration/test-committor-service/tests/utils/transactions.rsprograms/magicblock/src/schedule_transactions/process_schedule_base_intent.rstest-integration/programs/flexi-counter/src/processor.rsmagicblock-chainlink/src/testing/mod.rstest-integration/test-chainlink/src/programs.rsmagicblock-committor-service/src/intent_executor/mod.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/test-chainlink/tests/ix_03_deleg_after_sub.rstest-integration/schedulecommit/test-scenarios/tests/01_commits.rstest-integration/test-chainlink/tests/ix_01_ensure-accounts.rstest-integration/test-chainlink/tests/ix_07_redeleg_us_same_slot.rstest-integration/test-chainlink/tests/ix_06_redeleg_us_separate_slots.rsmagicblock-chainlink/tests/01_ensure-accounts.rsmagicblock-metrics/src/metrics/mod.rstest-integration/test-chainlink/tests/ix_remote_account_provider.rsmagicblock-committor-service/src/tasks/args_task.rsmagicblock-chainlink/tests/03_deleg_after_sub.rstest-integration/test-committor-service/tests/utils/transactions.rsmagicblock-api/src/magic_validator.rstest-integration/programs/flexi-counter/src/processor.rstest-integration/test-committor-service/tests/test_ix_commit_local.rstest-integration/test-committor-service/tests/utils/instructions.rsmagicblock-chainlink/src/chainlink/fetch_cloner.rsmagicblock-chainlink/tests/07_redeleg_us_same_slot.rsmagicblock-chainlink/src/remote_account_provider/mod.rsmagicblock-chainlink/tests/06_redeleg_us_separate_slots.rsprograms/magicblock/src/magic_scheduled_base_intent.rsmagicblock-committor-service/src/intent_executor/mod.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-chainlink/tests/ix_03_deleg_after_sub.rstest-integration/test-chainlink/tests/ix_01_ensure-accounts.rstest-integration/test-chainlink/tests/ix_07_redeleg_us_same_slot.rstest-integration/test-chainlink/tests/ix_06_redeleg_us_separate_slots.rsmagicblock-chainlink/tests/01_ensure-accounts.rsmagicblock-metrics/src/metrics/mod.rsmagicblock-committor-service/src/intent_execution_manager/intent_execution_engine.rstest-integration/test-chainlink/tests/ix_remote_account_provider.rsmagicblock-chainlink/tests/03_deleg_after_sub.rstest-integration/test-committor-service/tests/utils/transactions.rsmagicblock-api/src/magic_validator.rstest-integration/programs/flexi-counter/src/processor.rstest-integration/test-committor-service/tests/test_ix_commit_local.rsmagicblock-accounts/src/scheduled_commits_processor.rsmagicblock-chainlink/src/testing/mod.rsmagicblock-chainlink/tests/utils/test_context.rstest-integration/test-chainlink/src/programs.rsmagicblock-chainlink/src/chainlink/fetch_cloner.rstest-integration/test-chainlink/src/test_context.rsmagicblock-chainlink/tests/07_redeleg_us_same_slot.rsmagicblock-chainlink/src/remote_account_provider/mod.rsmagicblock-chainlink/tests/06_redeleg_us_separate_slots.rsmagicblock-committor-service/src/tasks/mod.rsCargo.tomlmagicblock-committor-service/src/intent_executor/mod.rstest-integration/programs/schedulecommit/src/api.rsmagicblock-chainlink/src/chainlink/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:
test-integration/test-chainlink/tests/ix_03_deleg_after_sub.rstest-integration/test-chainlink/tests/ix_01_ensure-accounts.rstest-integration/test-chainlink/tests/ix_06_redeleg_us_separate_slots.rsmagicblock-metrics/src/metrics/mod.rsmagicblock-committor-service/src/intent_execution_manager/intent_execution_engine.rstest-integration/test-chainlink/tests/ix_remote_account_provider.rsmagicblock-chainlink/tests/03_deleg_after_sub.rsmagicblock-api/src/magic_validator.rstest-integration/programs/flexi-counter/src/processor.rsmagicblock-accounts/src/scheduled_commits_processor.rsmagicblock-chainlink/tests/utils/test_context.rsmagicblock-chainlink/src/chainlink/fetch_cloner.rstest-integration/test-chainlink/src/test_context.rsmagicblock-chainlink/tests/07_redeleg_us_same_slot.rsmagicblock-chainlink/src/remote_account_provider/mod.rsmagicblock-chainlink/tests/06_redeleg_us_separate_slots.rsmagicblock-committor-service/src/intent_executor/two_stage_executor.rsCargo.tomlmagicblock-chainlink/src/chainlink/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:
test-integration/test-chainlink/tests/ix_03_deleg_after_sub.rsmagicblock-committor-service/src/intent_executor/error.rstest-integration/schedulecommit/test-scenarios/tests/01_commits.rstest-integration/test-chainlink/tests/ix_01_ensure-accounts.rstest-integration/test-chainlink/tests/ix_07_redeleg_us_same_slot.rstest-integration/test-chainlink/tests/ix_06_redeleg_us_separate_slots.rsmagicblock-chainlink/tests/01_ensure-accounts.rsmagicblock-metrics/src/metrics/mod.rsmagicblock-committor-service/src/intent_execution_manager/intent_execution_engine.rstest-integration/test-chainlink/tests/ix_remote_account_provider.rsmagicblock-committor-service/src/tasks/args_task.rsmagicblock-chainlink/tests/03_deleg_after_sub.rstest-integration/test-committor-service/tests/utils/transactions.rsmagicblock-api/src/magic_validator.rstest-integration/programs/flexi-counter/src/processor.rstest-integration/test-committor-service/tests/test_ix_commit_local.rstest-integration/test-committor-service/tests/utils/instructions.rsmagicblock-chainlink/src/testing/mod.rsmagicblock-chainlink/src/chainlink/fetch_cloner.rsprograms/magicblock/src/schedule_transactions/process_schedule_commit.rsmagicblock-chainlink/tests/07_redeleg_us_same_slot.rsmagicblock-chainlink/src/remote_account_provider/mod.rsmagicblock-chainlink/tests/06_redeleg_us_separate_slots.rsmagicblock-committor-service/src/intent_executor/two_stage_executor.rsprograms/magicblock/src/magic_scheduled_base_intent.rsmagicblock-committor-service/src/tasks/mod.rsCargo.tomlmagicblock-committor-service/src/intent_executor/mod.rsmagicblock-committor-service/src/transaction_preparator/delivery_preparator.rstest-integration/programs/schedulecommit/src/api.rsmagicblock-chainlink/src/chainlink/mod.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:
magicblock-committor-service/src/intent_executor/error.rsmagicblock-committor-service/src/intent_execution_manager/intent_execution_engine.rsmagicblock-committor-service/src/intent_executor/single_stage_executor.rstest-integration/test-committor-service/tests/test_ix_commit_local.rsprograms/magicblock/src/schedule_transactions/process_schedule_commit.rstest-integration/test-committor-service/tests/test_intent_executor.rsmagicblock-committor-service/src/intent_executor/two_stage_executor.rsprograms/magicblock/src/magic_scheduled_base_intent.rsmagicblock-committor-service/src/tasks/mod.rsmagicblock-committor-service/src/intent_executor/mod.rsmagicblock-committor-service/src/transaction_preparator/delivery_preparator.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/schedulecommit/test-scenarios/tests/01_commits.rstest-integration/test-committor-service/tests/test_intent_executor.rs
📚 Learning: 2025-10-21T11:00:18.396Z
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 578
File: magicblock-aperture/src/encoder.rs:176-187
Timestamp: 2025-10-21T11:00:18.396Z
Learning: In the magicblock validator, the current slot is always the root slot. The SlotEncoder in magicblock-aperture/src/encoder.rs correctly sets `root: slot` because there is no lag between current and root slots in this architecture.
Applied to files:
magicblock-chainlink/tests/01_ensure-accounts.rsmagicblock-chainlink/src/testing/mod.rsmagicblock-chainlink/tests/07_redeleg_us_same_slot.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:
magicblock-committor-service/src/intent_execution_manager/intent_execution_engine.rstest-integration/test-chainlink/tests/ix_remote_account_provider.rsmagicblock-api/src/magic_validator.rstest-integration/programs/flexi-counter/src/processor.rsmagicblock-chainlink/src/testing/mod.rsmagicblock-chainlink/tests/07_redeleg_us_same_slot.rsmagicblock-committor-service/src/tasks/mod.rsCargo.tomlmagicblock-committor-service/src/intent_executor/mod.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:
magicblock-committor-service/src/intent_execution_manager/intent_execution_engine.rsmagicblock-api/src/magic_validator.rstest-integration/programs/flexi-counter/src/processor.rsmagicblock-chainlink/src/testing/mod.rsmagicblock-chainlink/tests/07_redeleg_us_same_slot.rsmagicblock-chainlink/src/remote_account_provider/mod.rsmagicblock-committor-service/src/tasks/mod.rsCargo.tomlmagicblock-committor-service/src/intent_executor/mod.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.rsmagicblock-committor-service/src/tasks/buffer_task.rsmagicblock-committor-service/src/intent_executor/mod.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:
magicblock-api/src/magic_validator.rsmagicblock-chainlink/src/testing/mod.rsmagicblock-chainlink/tests/07_redeleg_us_same_slot.rsCargo.toml
📚 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-api/src/magic_validator.rsmagicblock-chainlink/src/remote_account_provider/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:
test-integration/programs/flexi-counter/src/processor.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:
test-integration/programs/flexi-counter/src/processor.rsmagicblock-chainlink/src/remote_account_provider/mod.rsmagicblock-chainlink/src/chainlink/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:
test-integration/programs/flexi-counter/src/processor.rs
📚 Learning: 2025-11-24T14:21:00.948Z
Learnt from: Dodecahedr0x
Repo: magicblock-labs/magicblock-validator PR: 639
File: Cargo.toml:58-58
Timestamp: 2025-11-24T14:21:00.948Z
Learning: In the magicblock-validator codebase, magicblock-api/Cargo.toml intentionally uses borsh = "1.5.3" (instead of the workspace version 0.10.4) because it needs to deserialize types from the magic-domain-program external dependency, which requires borsh 1.5.x compatibility. This is an intentional exception for interoperability with the magic domain program.
Applied to files:
test-integration/test-committor-service/tests/test_ix_commit_local.rsCargo.tomlmagicblock-committor-service/src/transaction_preparator/delivery_preparator.rs
📚 Learning: 2025-10-28T13:15:42.706Z
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 596
File: magicblock-processor/src/scheduler.rs:1-1
Timestamp: 2025-10-28T13:15:42.706Z
Learning: In magicblock-processor, transaction indexes were always set to 0 even before the changes in PR #596. The proper transaction indexing within slots will be addressed during the planned ledger rewrite.
Applied to files:
magicblock-chainlink/src/testing/mod.rsmagicblock-chainlink/tests/07_redeleg_us_same_slot.rsCargo.tomlmagicblock-committor-service/src/transaction_preparator/delivery_preparator.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-chainlink/src/testing/mod.rsmagicblock-chainlink/tests/07_redeleg_us_same_slot.rsCargo.toml
📚 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:
magicblock-chainlink/src/testing/mod.rsmagicblock-chainlink/tests/07_redeleg_us_same_slot.rsCargo.toml
📚 Learning: 2025-11-04T13:22:38.811Z
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 589
File: magicblock-aperture/src/requests/http/get_fee_for_message.rs:25-31
Timestamp: 2025-11-04T13:22:38.811Z
Learning: In magicblock-aperture, request size validation (including limits on message size) is enforced at the request ingestion layer, before handlers like get_fee_for_message are invoked. Therefore, handlers do not need additional size guards on decoded data.
Applied to files:
magicblock-chainlink/src/testing/mod.rsmagicblock-chainlink/tests/07_redeleg_us_same_slot.rsCargo.toml
📚 Learning: 2025-10-21T10:34:59.140Z
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 578
File: magicblock-accounts-db/src/lib.rs:63-72
Timestamp: 2025-10-21T10:34:59.140Z
Learning: In magicblock-validator, the AccountsDb "stop-the-world" synchronizer is managed at the processor/executor level, not at the AccountsDb API level. Transaction executors in magicblock-processor hold a read lock (sync.read()) for the duration of each slot and release it only at slot boundaries, ensuring all account writes happen under the read lock. Snapshot operations acquire a write lock, blocking until all executors release their read locks. This pattern ensures mutual exclusion between writes and snapshots without requiring read guards in AccountsDb write APIs.
Applied to files:
magicblock-chainlink/src/testing/mod.rsmagicblock-chainlink/tests/07_redeleg_us_same_slot.rs
📚 Learning: 2025-11-24T08:45:11.154Z
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 675
File: Cargo.toml:145-145
Timestamp: 2025-11-24T08:45:11.154Z
Learning: The reqwest dependency version 0.11 is intentionally used in the magicblock-validator project to maintain compatibility with the Solana client dependencies.
Applied to files:
Cargo.toml
🧬 Code graph analysis (16)
test-integration/test-chainlink/tests/ix_06_redeleg_us_separate_slots.rs (2)
test-integration/test-chainlink/src/ixtest_context.rs (2)
init(103-108)counter_pda(209-211)test-integration/test-chainlink/src/lib.rs (1)
sleep_ms(8-11)
magicblock-chainlink/tests/03_deleg_after_sub.rs (1)
magicblock-chainlink/tests/utils/accounts.rs (2)
compressed_account_shared_with_owner_and_slot(22-49)account_shared_with_owner_and_slot(12-20)
magicblock-committor-service/src/intent_executor/single_stage_executor.rs (2)
magicblock-committor-service/src/intent_executor/mod.rs (2)
None(918-918)None(938-938)test-integration/test-committor-service/tests/test_intent_executor.rs (14)
None(149-149)None(150-150)None(200-200)None(201-201)None(261-261)None(262-262)None(319-319)None(320-320)None(368-368)None(419-419)None(468-468)None(527-527)None(599-599)None(600-600)
test-integration/test-committor-service/tests/utils/transactions.rs (2)
test-integration/test-committor-service/tests/utils/instructions.rs (2)
init_account_and_delegate_compressed_ixs(81-159)init_account_and_delegate_ixs(48-79)test-integration/test-committor-service/tests/utils/mod.rs (1)
sleep_millis(16-18)
magicblock-api/src/magic_validator.rs (1)
magicblock-chainlink/src/remote_account_provider/mod.rs (1)
new(300-359)
magicblock-committor-service/src/tasks/buffer_task.rs (6)
magicblock-committor-service/src/tasks/args_task.rs (5)
is_compressed(248-255)set_compressed_data(257-270)get_compressed_data(272-285)delegated_account(287-303)value(307-314)magicblock-committor-service/src/tasks/mod.rs (4)
is_compressed(100-100)set_compressed_data(103-103)get_compressed_data(106-106)delegated_account(109-109)magicblock-committor-service/src/tasks/task_builder.rs (1)
get_compressed_data(412-463)magicblock-committor-service/src/intent_executor/mod.rs (3)
None(918-918)None(938-938)value(75-83)test-integration/test-committor-service/tests/test_intent_executor.rs (13)
None(149-149)None(150-150)None(200-200)None(201-201)None(261-261)None(262-262)None(319-319)None(320-320)None(368-368)None(419-419)None(468-468)None(527-527)None(599-599)magicblock-committor-service/src/intent_executor/error.rs (1)
value(113-120)
test-integration/programs/flexi-counter/src/processor.rs (3)
compressed-delegation-client/src/generated/instructions/undelegate_compressed.rs (2)
args(112-115)args(303-306)compressed-delegation-client/src/generated/instructions/delegate_compressed.rs (2)
args(162-165)args(430-433)compressed-delegation-client/src/lib.rs (1)
id(61-63)
test-integration/test-committor-service/tests/utils/instructions.rs (4)
magicblock-core/src/compression/mod.rs (1)
derive_cda_from_pda(10-22)test-integration/programs/flexi-counter/src/instruction.rs (2)
create_delegate_compressed_ix(514-533)create_init_ix(227-240)test-integration/programs/flexi-counter/src/state.rs (2)
pda(45-48)new(27-33)test-integration/test-committor-service/tests/common.rs (1)
new(64-67)
magicblock-chainlink/tests/utils/test_context.rs (2)
magicblock-chainlink/src/remote_account_provider/photon_client.rs (1)
new(27-29)magicblock-chainlink/src/testing/photon_client_mock.rs (1)
new(26-30)
magicblock-chainlink/src/chainlink/fetch_cloner.rs (2)
magicblock-chainlink/src/remote_account_provider/photon_client.rs (1)
new(27-29)compressed-delegation-client/src/lib.rs (1)
id(61-63)
test-integration/test-chainlink/src/test_context.rs (1)
magicblock-chainlink/tests/utils/test_context.rs (1)
init(62-132)
magicblock-chainlink/tests/06_redeleg_us_separate_slots.rs (3)
magicblock-chainlink/src/testing/accounts.rs (1)
account_shared_with_owner(4-10)magicblock-chainlink/src/testing/utils.rs (1)
init_logger(51-59)magicblock-chainlink/tests/utils/accounts.rs (2)
compressed_account_shared_with_owner_and_slot(22-49)account_shared_with_owner_and_slot(12-20)
magicblock-committor-service/src/intent_executor/two_stage_executor.rs (2)
magicblock-committor-service/src/intent_executor/mod.rs (2)
None(918-918)None(938-938)test-integration/test-committor-service/tests/test_intent_executor.rs (14)
None(149-149)None(150-150)None(200-200)None(201-201)None(261-261)None(262-262)None(319-319)None(320-320)None(368-368)None(419-419)None(468-468)None(527-527)None(599-599)None(600-600)
programs/magicblock/src/magic_scheduled_base_intent.rs (4)
magicblock-committor-service/src/tasks/mod.rs (1)
is_compressed(100-100)test-integration/test-committor-service/tests/test_ix_commit_local.rs (1)
base_intent(811-816)magicblock-committor-service/src/tasks/task_builder.rs (1)
committed_accounts(280-286)magicblock-committor-service/src/intent_executor/task_info_fetcher.rs (2)
Self(305-313)Self(315-323)
magicblock-committor-service/src/tasks/mod.rs (3)
magicblock-committor-service/src/tasks/args_task.rs (4)
is_compressed(248-255)set_compressed_data(257-270)get_compressed_data(272-285)delegated_account(287-303)magicblock-committor-service/src/tasks/buffer_task.rs (4)
is_compressed(147-149)set_compressed_data(151-156)get_compressed_data(158-162)delegated_account(164-170)magicblock-committor-service/src/tasks/task_builder.rs (1)
get_compressed_data(412-463)
magicblock-committor-service/src/transaction_preparator/delivery_preparator.rs (2)
magicblock-committor-service/src/tasks/args_task.rs (2)
get_compressed_data(272-285)from(40-42)magicblock-committor-service/src/tasks/task_builder.rs (1)
get_compressed_data(412-463)
| async fn fetch_rent_reimbursements( | ||
| &self, | ||
| pubkeys: &[Pubkey], | ||
| accounts: &[Pubkey], | ||
| ) -> TaskInfoFetcherResult<Vec<Pubkey>> { | ||
| Ok(pubkeys.iter().map(|_| Pubkey::new_unique()).collect()) | ||
| Ok(accounts.iter().map(|_| Pubkey::new_unique()).collect()) | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Make MockInfoFetcher::fetch_rent_reimbursements preserve input pubkeys for clearer semantics
The mock currently returns a vector of Pubkey::new_unique() with the same length as accounts, which breaks the natural 1:1 relationship between the input accounts and their “reimbursement” pubkeys. If tests start asserting on actual keys (not just length), this behaviour becomes misleading.
Since this is a test helper and you don’t care about deriving new values, you can simply echo the inputs back:
- async fn fetch_rent_reimbursements(
- &self,
- accounts: &[Pubkey],
- ) -> TaskInfoFetcherResult<Vec<Pubkey>> {
- Ok(accounts.iter().map(|_| Pubkey::new_unique()).collect())
- }
+ async fn fetch_rent_reimbursements(
+ &self,
+ accounts: &[Pubkey],
+ ) -> TaskInfoFetcherResult<Vec<Pubkey>> {
+ Ok(accounts.to_vec())
+ }This keeps the mock behaviour intuitive and future‑proofs tests that may rely on the actual identities of reimbursed accounts.
🤖 Prompt for AI Agents
In magicblock-committor-service/src/intent_executor/mod.rs around lines 891 to
896, the mock implementation of fetch_rent_reimbursements constructs new random
Pubkeys instead of returning the input accounts, breaking the 1:1 mapping and
confusing tests; change the body to simply return the input pubkeys (e.g.,
Ok(accounts.to_vec()) or accounts.iter().cloned().collect()) so the mock echoes
the provided Pubkeys in order.
There was a problem hiding this comment.
@Dodecahedr0x Understood, we'll keep the current implementation. The mock generating unique pubkeys works fine for the existing tests.
🧠 Learnings used
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.
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
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.
Learnt from: Dodecahedr0x
Repo: magicblock-labs/magicblock-validator PR: 639
File: test-integration/test-chainlink/tests/ix_01_ensure-accounts.rs:85-114
Timestamp: 2025-11-19T11:31:24.218Z
Learning: In the magicblock-validator codebase, compressed delegation records are never subscribed to by design. Tests for compressed delegation flows should focus on verifying that the delegated account (e.g., counter PDA) is cloned as delegated and not subscribed, but do not need to check for subscription to delegation-record-like PDAs in the compressed path because subscriptions to compressed delegation records never occur.
| fn task_type(&self) -> TaskType { | ||
| match &self.task_type { | ||
| ArgsTaskType::Commit(_) => TaskType::Commit, | ||
| ArgsTaskType::CompressedCommit(_) => TaskType::CompressedCommit, | ||
| ArgsTaskType::BaseAction(_) => TaskType::Action, | ||
| ArgsTaskType::Undelegate(_) => TaskType::Undelegate, | ||
| ArgsTaskType::CompressedUndelegate(_) => { | ||
| TaskType::CompressedUndelegate | ||
| } | ||
| ArgsTaskType::Finalize(_) => TaskType::Finalize, | ||
| ArgsTaskType::CompressedFinalize(_) => TaskType::CompressedFinalize, | ||
| } | ||
| } |
There was a problem hiding this comment.
LabelValue match is non‑exhaustive for new compressed variants and currently moves task_type
ArgsTaskType now has compressed variants, but LabelValue::value still only matches the four legacy cases:
impl LabelValue for ArgsTask {
fn value(&self) -> &str {
match self.task_type {
ArgsTaskType::Commit(_) => "args_commit",
ArgsTaskType::BaseAction(_) => "args_action",
ArgsTaskType::Finalize(_) => "args_finalize",
ArgsTaskType::Undelegate(_) => "args_undelegate",
}
}
}Issues:
- The match is now non‑exhaustive and will fail to compile once compressed variants are enabled.
match self.task_typeattempts to movetask_typeout of&self; this only works ifArgsTaskTypeisCopy(which it isn’t, given#[derive(Clone)]and the inner types).
You probably want both exhaustive coverage and to keep task_type borrowed:
impl LabelValue for ArgsTask {
fn value(&self) -> &str {
- match self.task_type {
- ArgsTaskType::Commit(_) => "args_commit",
- ArgsTaskType::BaseAction(_) => "args_action",
- ArgsTaskType::Finalize(_) => "args_finalize",
- ArgsTaskType::Undelegate(_) => "args_undelegate",
- }
+ match &self.task_type {
+ ArgsTaskType::Commit(_) => "args_commit",
+ ArgsTaskType::CompressedCommit(_) => "args_compressed_commit",
+ ArgsTaskType::BaseAction(_) => "args_action",
+ ArgsTaskType::Finalize(_) => "args_finalize",
+ ArgsTaskType::CompressedFinalize(_) => "args_compressed_finalize",
+ ArgsTaskType::Undelegate(_) => "args_undelegate",
+ ArgsTaskType::CompressedUndelegate(_) => "args_compressed_undelegate",
+ }
}
}This restores exhaustiveness and keeps metric labeling in sync with the new compressed task variants.
Also applies to: 306-314
🤖 Prompt for AI Agents
In magicblock-committor-service/src/tasks/args_task.rs around lines 221-233 (and
also apply the same change at 306-314), the LabelValue::value match is
non-exhaustive for the new compressed ArgsTaskType variants and currently
attempts to move self.task_type; change the match to borrow the enum (match
&self.task_type) and add arms for the compressed variants mapping them to the
same label strings as their legacy counterparts (e.g., CompressedCommit ->
"args_commit", CompressedFinalize -> "args_finalize", CompressedUndelegate ->
"args_undelegate", etc.) so all variants are covered and no move occurs from
&self.
There was a problem hiding this comment.
Actionable comments posted: 8
♻️ Duplicate comments (2)
magicblock-committor-service/src/tasks/args_task.rs (1)
126-136: Add missing.args()call toUndelegateBuilderand importUndelegateArgs.The
UndelegateBuilder.instruction()method panics with "args is not set" if.args()is not called. The code at lines 126-136 is missing this mandatory call. The requiredUndelegateArgsstruct needsvalidity_proof,delegation_record_account_meta, andcompressed_delegated_record, which are all available invalue.compressed_data.Add to imports (line 1):
use compressed_delegation_client::types::{CommitArgs, FinalizeArgs, UndelegateArgs};Update lines 126-136 to include
.args():ArgsTaskType::CompressedUndelegate(value) => { compressed_delegation_client::UndelegateBuilder::new() .payer(*validator) .delegated_account(value.delegated_account) .owner_program(value.owner_program) .system_program(system_program::ID) .args(UndelegateArgs { validity_proof: value.compressed_data.proof, delegation_record_account_meta: value.compressed_data.account_meta, compressed_delegated_record: CompressedDelegationRecord::try_from_slice( &value.compressed_data.compressed_delegation_record_bytes, ).expect("failed to deserialize compressed delegation record"), }) .add_remaining_accounts( &value.compressed_data.remaining_accounts, ) .instruction() }Also add
CompressedDelegationRecordto imports fromcompressed_delegation_client::types.test-integration/programs/flexi-counter/src/processor.rs (1)
548-555: Improve error semantics when refunding excess lamports inprocess_delegate_compressedThe
checked_subhere correctly avoids a panic if the counter PDA somehow has fewer lamports thanmin_rent(0), but surfacing that asProgramError::ArithmeticOverflowmakes debugging harder and conflates an impossible arithmetic condition with a logical “account under‑funded” state:let min_rent = rent.minimum_balance(0); **payer_info.try_borrow_mut_lamports()? += counter_pda_info .lamports() .checked_sub(min_rent) .ok_or(ProgramError::ArithmeticOverflow)?; **counter_pda_info.try_borrow_mut_lamports()? = min_rent;Consider making the under‑funded case explicit and returning a more descriptive error (and/or logging):
- let min_rent = rent.minimum_balance(0); - **payer_info.try_borrow_mut_lamports()? += counter_pda_info - .lamports() - .checked_sub(min_rent) - .ok_or(ProgramError::ArithmeticOverflow)?; - **counter_pda_info.try_borrow_mut_lamports()? = min_rent; + let min_rent = rent.minimum_balance(0); + let current_lamports = counter_pda_info.lamports(); + if current_lamports < min_rent { + msg!( + "Counter PDA has {} lamports, expected at least {} before delegation", + current_lamports, + min_rent + ); + return Err(ProgramError::InsufficientFunds); + } + let refund = current_lamports - min_rent; + **payer_info.try_borrow_mut_lamports()? += refund; + **counter_pda_info.try_borrow_mut_lamports()? = min_rent;This keeps the behavior safe but turns a low‑signal arithmetic error into a clear “bad precondition” signal for tests.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
test-integration/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (9)
magicblock-chainlink/src/chainlink/fetch_cloner.rs(19 hunks)magicblock-chainlink/tests/06_redeleg_us_separate_slots.rs(3 hunks)magicblock-committor-service/src/tasks/args_task.rs(9 hunks)magicblock-committor-service/src/transaction_preparator/delivery_preparator.rs(10 hunks)programs/magicblock/src/schedule_transactions/process_schedule_commit.rs(2 hunks)test-integration/programs/flexi-counter/src/processor.rs(10 hunks)test-integration/programs/schedulecommit/src/api.rs(7 hunks)test-integration/test-committor-service/tests/test_ix_commit_local.rs(23 hunks)test-integration/test-committor-service/tests/utils/transactions.rs(7 hunks)
🧰 Additional context used
🧠 Learnings (17)
📓 Common learnings
Learnt from: Dodecahedr0x
Repo: magicblock-labs/magicblock-validator PR: 639
File: test-integration/test-chainlink/tests/ix_01_ensure-accounts.rs:85-114
Timestamp: 2025-11-19T11:31:24.218Z
Learning: In the magicblock-validator codebase, compressed delegation records are never subscribed to by design. Tests for compressed delegation flows should focus on verifying that the delegated account (e.g., counter PDA) is cloned as delegated and not subscribed, but do not need to check for subscription to delegation-record-like PDAs in the compressed path because subscriptions to compressed delegation records never occur.
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.
📚 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/api.rstest-integration/programs/flexi-counter/src/processor.rstest-integration/test-committor-service/tests/utils/transactions.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/programs/schedulecommit/src/api.rsprograms/magicblock/src/schedule_transactions/process_schedule_commit.rsmagicblock-chainlink/tests/06_redeleg_us_separate_slots.rstest-integration/test-committor-service/tests/test_ix_commit_local.rsmagicblock-committor-service/src/tasks/args_task.rstest-integration/programs/flexi-counter/src/processor.rsmagicblock-committor-service/src/transaction_preparator/delivery_preparator.rstest-integration/test-committor-service/tests/utils/transactions.rsmagicblock-chainlink/src/chainlink/fetch_cloner.rs
📚 Learning: 2025-11-19T11:31:24.218Z
Learnt from: Dodecahedr0x
Repo: magicblock-labs/magicblock-validator PR: 639
File: test-integration/test-chainlink/tests/ix_01_ensure-accounts.rs:85-114
Timestamp: 2025-11-19T11:31:24.218Z
Learning: In the magicblock-validator codebase, compressed delegation records are never subscribed to by design. Tests for compressed delegation flows should focus on verifying that the delegated account (e.g., counter PDA) is cloned as delegated and not subscribed, but do not need to check for subscription to delegation-record-like PDAs in the compressed path because subscriptions to compressed delegation records never occur.
Applied to files:
test-integration/programs/schedulecommit/src/api.rsmagicblock-chainlink/tests/06_redeleg_us_separate_slots.rstest-integration/test-committor-service/tests/test_ix_commit_local.rsmagicblock-committor-service/src/tasks/args_task.rstest-integration/programs/flexi-counter/src/processor.rstest-integration/test-committor-service/tests/utils/transactions.rsmagicblock-chainlink/src/chainlink/fetch_cloner.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/programs/schedulecommit/src/api.rsmagicblock-chainlink/tests/06_redeleg_us_separate_slots.rstest-integration/test-committor-service/tests/test_ix_commit_local.rstest-integration/programs/flexi-counter/src/processor.rstest-integration/test-committor-service/tests/utils/transactions.rsmagicblock-chainlink/src/chainlink/fetch_cloner.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:
programs/magicblock/src/schedule_transactions/process_schedule_commit.rstest-integration/test-committor-service/tests/test_ix_commit_local.rsmagicblock-committor-service/src/transaction_preparator/delivery_preparator.rs
📚 Learning: 2025-11-18T08:47:39.702Z
Learnt from: Dodecahedr0x
Repo: magicblock-labs/magicblock-validator PR: 639
File: magicblock-chainlink/tests/04_redeleg_other_separate_slots.rs:158-165
Timestamp: 2025-11-18T08:47:39.702Z
Learning: In magicblock-chainlink tests involving compressed accounts, `set_remote_slot()` sets the slot of the `AccountSharedData`, while `compressed_account_shared_with_owner_and_slot()` sets the slot of the delegation record. These are two different fields and both calls are necessary.
Applied to files:
magicblock-chainlink/tests/06_redeleg_us_separate_slots.rstest-integration/test-committor-service/tests/test_ix_commit_local.rsmagicblock-committor-service/src/tasks/args_task.rstest-integration/programs/flexi-counter/src/processor.rstest-integration/test-committor-service/tests/utils/transactions.rsmagicblock-chainlink/src/chainlink/fetch_cloner.rs
📚 Learning: 2025-11-25T11:07:19.985Z
Learnt from: Dodecahedr0x
Repo: magicblock-labs/magicblock-validator PR: 639
File: test-integration/programs/flexi-counter/src/processor.rs:643-680
Timestamp: 2025-11-25T11:07:19.985Z
Learning: In the compressed delegation architecture, ownership reassignment during undelegation is handled by the delegation program itself, not by the owner program's external undelegate handler. The external undelegate handler (e.g., `process_external_undelegate_compressed` in flexi-counter) is a callback for program-specific cleanup (restoring lamports and data) after the delegation program has already reassigned ownership back to the original owner stored in the CompressedDelegationRecord.
Applied to files:
magicblock-chainlink/tests/06_redeleg_us_separate_slots.rsmagicblock-committor-service/src/tasks/args_task.rstest-integration/programs/flexi-counter/src/processor.rstest-integration/test-committor-service/tests/utils/transactions.rsmagicblock-chainlink/src/chainlink/fetch_cloner.rs
📚 Learning: 2025-11-07T14:20:31.457Z
Learnt from: thlorenz
Repo: magicblock-labs/magicblock-validator PR: 621
File: magicblock-chainlink/src/remote_account_provider/chain_pubsub_actor.rs:457-495
Timestamp: 2025-11-07T14:20:31.457Z
Learning: In magicblock-chainlink/src/remote_account_provider/chain_pubsub_client.rs, the unsubscribe closure returned by PubSubConnection::account_subscribe(...) resolves to () (unit), not a Result. Downstream code should not attempt to inspect an unsubscribe result and can optionally wrap it in a timeout to guard against hangs.
Applied to files:
magicblock-chainlink/tests/06_redeleg_us_separate_slots.rstest-integration/programs/flexi-counter/src/processor.rsmagicblock-chainlink/src/chainlink/fetch_cloner.rs
📚 Learning: 2025-11-24T14:21:00.948Z
Learnt from: Dodecahedr0x
Repo: magicblock-labs/magicblock-validator PR: 639
File: Cargo.toml:58-58
Timestamp: 2025-11-24T14:21:00.948Z
Learning: In the magicblock-validator codebase, magicblock-api/Cargo.toml intentionally uses borsh = "1.5.3" (instead of the workspace version 0.10.4) because it needs to deserialize types from the magic-domain-program external dependency, which requires borsh 1.5.x compatibility. This is an intentional exception for interoperability with the magic domain program.
Applied to files:
test-integration/test-committor-service/tests/test_ix_commit_local.rsmagicblock-committor-service/src/transaction_preparator/delivery_preparator.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
📚 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/programs/flexi-counter/src/processor.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/programs/flexi-counter/src/processor.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/programs/flexi-counter/src/processor.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:
test-integration/programs/flexi-counter/src/processor.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:
test-integration/programs/flexi-counter/src/processor.rs
📚 Learning: 2025-10-28T13:15:42.706Z
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 596
File: magicblock-processor/src/scheduler.rs:1-1
Timestamp: 2025-10-28T13:15:42.706Z
Learning: In magicblock-processor, transaction indexes were always set to 0 even before the changes in PR #596. The proper transaction indexing within slots will be addressed during the planned ledger rewrite.
Applied to files:
magicblock-committor-service/src/transaction_preparator/delivery_preparator.rs
🧬 Code graph analysis (6)
programs/magicblock/src/schedule_transactions/process_schedule_commit.rs (4)
programs/magicblock/src/utils/instruction_utils.rs (1)
schedule_commit(33-40)magicblock-chainlink/src/remote_account_provider/remote_account.rs (1)
compressed(154-156)test-integration/test-committor-service/tests/test_ix_commit_local.rs (1)
base_intent(811-816)magicblock-committor-service/src/tasks/task_builder.rs (1)
committed_accounts(280-286)
magicblock-chainlink/tests/06_redeleg_us_separate_slots.rs (1)
magicblock-chainlink/tests/utils/accounts.rs (2)
compressed_account_shared_with_owner_and_slot(22-49)account_shared_with_owner_and_slot(12-20)
test-integration/test-committor-service/tests/test_ix_commit_local.rs (6)
magicblock-core/src/compression/mod.rs (1)
derive_cda_from_pda(10-22)magicblock-committor-service/src/tasks/args_task.rs (2)
new(46-63)is_compressed(248-255)magicblock-committor-service/src/config.rs (1)
local(29-35)magicblock-chainlink/src/remote_account_provider/remote_account.rs (1)
compressed(154-156)magicblock-config/src/compression.rs (1)
default(24-29)compressed-delegation-client/src/lib.rs (1)
id(61-63)
magicblock-committor-service/src/transaction_preparator/delivery_preparator.rs (2)
magicblock-committor-service/src/tasks/buffer_task.rs (3)
get_compressed_data(158-162)strategy(122-124)delegated_account(164-170)magicblock-committor-service/src/tasks/task_builder.rs (1)
get_compressed_data(412-463)
test-integration/test-committor-service/tests/utils/transactions.rs (2)
test-integration/test-committor-service/tests/utils/instructions.rs (1)
init_account_and_delegate_compressed_ixs(81-159)test-integration/test-committor-service/tests/utils/mod.rs (1)
sleep_millis(16-18)
magicblock-chainlink/src/chainlink/fetch_cloner.rs (2)
magicblock-chainlink/src/remote_account_provider/photon_client.rs (1)
new(27-29)compressed-delegation-client/src/lib.rs (1)
id(61-63)
🔇 Additional comments (26)
test-integration/test-committor-service/tests/test_ix_commit_local.rs (7)
7-22: LGTM!The new imports for compressed delegation support are well-organized and appropriate for the added functionality.
130-135: LGTM!The
CommitAccountModeenum cleanly captures the four test scenarios and simplifies the mode-driven branching throughout the test helpers.
613-654: LGTM!The
create_bundleshelper cleanly handles both regular and compressed account initialization with efficient parallel spawning viaJoinSet.
680-689: LGTM!Good use of
matches!macro to determine the compressed flag from the mode enum.
840-875: LGTM!The compressed account validation path correctly:
- Validates the on-chain PDA is owned by
compressed_delegation_client::IDwith minimal rent- Derives the CDA address and fetches compressed state from the Photon indexer
- Validates the compressed delegation record contains expected data
1026-1042: LGTM!The
validate_compressed_accountfunction correctly parses theCompressedDelegationRecordfrom the compressed account data and validates the embedded state matches expectations.
440-478: LGTM!Good test coverage for single compressed account commits with both small (100 bytes) and medium (500 bytes) data sizes, including commit-only and commit-and-undelegate variants.
test-integration/programs/schedulecommit/src/api.rs (2)
28-32: Consistent use ofbuild_instructionacross all builders looks goodAll updated builders now delegate to
build_instructionwith the appropriateprogram_id,ScheduleCommitInstructionvariant, andaccount_metas. This removes duplication from the file and keeps the serialization/encoding path consistent for all schedule-commit flows.Also applies to: 86-90, 193-195, 214-218, 237-243, 249-253
285-295: Helper neatly centralizes Borsh serialization for schedule-commit instructions
build_instructioncorrectly wrapsScheduleCommitInstructionserialization viaborsh::to_vecand constructs theInstructionwith the providedprogram_idandaccount_metas. Centralizing this logic improves maintainability and aligns this test program with the broader instruction-building pattern used elsewhere in the PR.magicblock-committor-service/src/transaction_preparator/delivery_preparator.rs (5)
1-1: LGTM: Imports properly support PhotonIndexer integration.The added imports correctly support the new compressed delegation data flow with proper type safety (BufferPreparationTask, TaskBuilderError) and PhotonIndexer client integration.
Also applies to: 5-5, 36-39
66-90: LGTM: PhotonIndexer context properly threaded through preparation pipeline.The
photon_clientandcommit_slotparameters are correctly propagated to task preparation, enabling compressed delegation data flows while remaining optional for non-compressed paths.
171-192: Note: Past review comments about this section appear to be outdated.Previous comments mentioned:
- A TODO with a 1000ms sleep that should be replaced (lines 171-177 in past version)
- Error handling that discarded context using
let Ok(...)pattern (lines 159-165 in past version)The current code no longer contains these issues:
- No sleep is present in the compressed path
- Error handling at line 190 uses the
?operator, which preserves error context via theFrom<TaskBuilderError>impl (line 615)
592-620: LGTM: Error variants comprehensively support compressed delegation flows.The new
InternalErrorvariants properly model all failure modes in the compressed path:
CompressedDataNotFound,DelegatedAccountNotFound,PhotonClientNotFoundprovide specific error contextTaskBuilderErrorwithFromimpl enables clean error propagation via?operator at line 190- All variants follow Rust best practices with proper
#[error]and#[from]attributes
128-170: LGTM: Buffer path refactored with improved type safety.The use of
BufferPreparationTaskas a specific type (instead of generic preparation task) improves type safety throughout the buffer initialization flow. State transitions are clearly modeled and persisted at each stage.Also applies to: 264-265, 313-313
magicblock-committor-service/src/tasks/args_task.rs (6)
23-31: LGTM!The new compressed task variants are well-structured and follow the existing pattern consistently.
45-63: LGTM!The constructor properly initializes
preparation_stateper variant at construction time, addressing the previous concern about returning references to temporaries.
83-102: LGTM!The
CompressedCommitinstruction builder is well-structured, properly sourcing all arguments from the task's compressed data.
109-124: LGTM!The
CompressedFinalizeinstruction builder correctly mirrors theCompressedCommitpattern with appropriate arguments.
192-202: LGTM!The state transition now properly persists the new state, addressing the previous concern about compressed tasks remaining in
Requiredstate indefinitely.
306-319: LGTM!All compressed variants are now covered, resolving the previous non-exhaustive match concern. The implementation correctly handles metrics labeling for all task types.
programs/magicblock/src/schedule_transactions/process_schedule_commit.rs (2)
34-55: Wrapper refactor cleanly centralizes commit scheduling logicDelegating both
process_schedule_commitandprocess_schedule_compressed_commitinto a singleschedule_commit(..., compressed: bool)keeps the external API clear while centralizing validation and scheduling logic. The ownership ofsignersandoptsis preserved and there’s no behavior change for the non‑compressed path.
267-285: Intent selection for compressed vs non‑compressed paths looks correctThe
match (opts.request_undelegation, compressed)cleanly and exhaustively maps the four combinations to the expectedMagicBaseIntentvariants, and forcompressed == falsepreserves the priorCommit/CommitAndUndelegatebehavior. Movingcommitted_accountsintoCommitType::Standalonein each arm is consistent with the previous implementation and only consumed once.magicblock-chainlink/src/chainlink/fetch_cloner.rs (4)
49-56: LGTM: PhotonClient generic parameter properly threaded through the codebase.The addition of the
P: PhotonClientgeneric parameter and its propagation throughFetchCloner, impl blocks, and helper functions is consistent and correct.Also applies to: 135-142, 145-145, 1702-1707
1796-1958: LGTM: Test scaffolding properly updated for PhotonClient integration.All test type signatures, helpers, and setup functions correctly include
PhotonClientMockas the new generic parameter. The updates are consistent across the test suite.
878-902: Verify whether compressed delegation authority handling is intentionally different.The code shows an inconsistency in authority checks between standard and compressed delegation processing:
Standard delegation (lines 949-952):
delegation_record.authority.eq(&self.validator_pubkey) || // TODO(thlorenz): @ once the delegation program supports // delegating to specific authority we need to remove the below delegation_record.authority.eq(&Pubkey::default());Compressed delegation (lines 889-896):
if delegation_record.authority.eq(&self.validator_pubkey) { account.set_delegated(true); } else { account.set_delegated(false); }The standard delegation explicitly includes a fallback to
Pubkey::default()(marked as temporary via TODO), but the compressed delegation does not. Both use identicalPubkeyauthority fields from their respective record types.This could be intentional (compressed delegation program doesn't support default authority), or an oversight. Please verify:
- Does the compressed delegation program support
Pubkey::default()as a valid authority value?- If yes, should the fallback be added for consistency?
- If no, should a comment document why the compressed path omits this check?
468-497: The review comment is based on incorrect assumptions about the subscription data flow.The code at lines 468-497 is not only reachable but essential. When an account delegated to the validator arrives as a subscription update with owner
compressed_delegation_client::id(), it arrives with empty data (as confirmed by test line 228:Account::default()). Thetry_from_sliceat line 471 necessarily fails because there is noCompressedDelegationRecordin the subscription payload.The photon fallback at lines 477-495 is not redundant—it is the intended design. Subscription updates carry the empty wrapper account, while the actual delegation record data resides in photon. The fallback is essential and will succeed because photon has the properly formatted account data.
The review incorrectly assumes that "subscription update should already contain the latest data." In the compressed delegation flow, subscriptions deliberately carry only the wrapper account (with owner marker), and the actual delegation data must be fetched separately from photon.
Likely an incorrect or invalid review comment.
| let undelegated_acc = account_shared_with_owner_and_slot( | ||
| &acc.account, | ||
| program_pubkey, | ||
| rpc_client.get_slot(), |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Use the slot variable for consistency.
Line 192 calls rpc_client.get_slot() to retrieve the slot, but the slot variable was just updated at line 181 and should contain the same value. For consistency with the rest of the test (which uses the slot variable at lines 194, 220, 225, and 230), use the variable directly.
Apply this diff:
let undelegated_acc = account_shared_with_owner_and_slot(
&acc.account,
program_pubkey,
- rpc_client.get_slot(),
+ slot,
);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let undelegated_acc = account_shared_with_owner_and_slot( | |
| &acc.account, | |
| program_pubkey, | |
| rpc_client.get_slot(), | |
| let undelegated_acc = account_shared_with_owner_and_slot( | |
| &acc.account, | |
| program_pubkey, | |
| slot, | |
| ); |
🤖 Prompt for AI Agents
In magicblock-chainlink/tests/06_redeleg_us_separate_slots.rs around lines 189
to 192, the call uses rpc_client.get_slot() to fetch the current slot even
though the slot variable was updated at line 181 and is used elsewhere; replace
the rpc_client.get_slot() call with the existing slot variable to keep
consistency and avoid an extra RPC call.
| fn set_compressed_data(&mut self, compressed_data: CompressedData) { | ||
| match &mut self.task_type { | ||
| ArgsTaskType::CompressedCommit(value) => { | ||
| value.compressed_data = compressed_data; | ||
| } | ||
| ArgsTaskType::CompressedFinalize(value) => { | ||
| value.compressed_data = compressed_data; | ||
| } | ||
| ArgsTaskType::CompressedUndelegate(value) => { | ||
| value.compressed_data = compressed_data; | ||
| } | ||
| _ => {} | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider adding a debug assertion for non-compressed variants.
The _ => {} arm silently ignores calls on non-compressed tasks, which could mask caller bugs. A debug assertion or trace log would help catch misuse during development.
- _ => {}
+ _ => {
+ debug_assert!(
+ false,
+ "set_compressed_data called on non-compressed task variant"
+ );
+ }🤖 Prompt for AI Agents
In magicblock-committor-service/src/tasks/args_task.rs around lines 257 to 270,
the set_compressed_data method currently ignores non-compressed task variants
with `_ => {}`, which can hide misuse; change that arm to assert in debug builds
(e.g., use debug_assert!(matches!(self.task_type,
ArgsTaskType::CompressedCommit(_) | ArgsTaskType::CompressedFinalize(_) |
ArgsTaskType::CompressedUndelegate(_))) or call unreachable!() behind
cfg!(debug_assertions)), or alternatively log a trace/error, so calls on
non-compressed variants are detected during development.
| PreparationTask::Compressed => { | ||
| // Trying to fetch fresh data from the indexer | ||
| let photon_config = commit_slot.map(|slot| IndexerRpcConfig { | ||
| slot, | ||
| ..Default::default() | ||
| }); | ||
|
|
||
| let delegated_account = task | ||
| .delegated_account() | ||
| .ok_or(InternalError::DelegatedAccountNotFound)?; | ||
| let photon_client = photon_client | ||
| .as_ref() | ||
| .ok_or(InternalError::PhotonClientNotFound)?; | ||
|
|
||
| let compressed_data = get_compressed_data( | ||
| &delegated_account, | ||
| photon_client, | ||
| photon_config, | ||
| ) | ||
| .await?; | ||
| task.set_compressed_data(compressed_data); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Consider: Add validation that the indexer has reached the required slot.
When commit_slot is provided, the code creates an IndexerRpcConfig to constrain the fetch, but doesn't verify that the Photon indexer has actually indexed up to that slot. If the indexer lags behind, get_compressed_data may:
- Return stale data from an earlier slot
- Fail with an indexer-specific error
- Return successfully but with inconsistent state
Consider adding a slot-readiness check or documenting the expected behavior when the indexer hasn't reached commit_slot.
Based on learnings, compressed delegation flows require the indexer to be up-to-date for correctness.
🏁 Script executed:
#!/bin/bash
# Description: Check if there's slot validation logic in PhotonIndexer or get_compressed_data
# Search for slot validation in get_compressed_data and PhotonIndexer methods
rg -n -C3 'slot.*valid|min.*slot|indexer.*slot|slot.*check' --type rustLength of output: 87506
🏁 Script executed:
#!/bin/bash
# Find get_compressed_data function definition
rg -n -A10 'async fn get_compressed_data|fn get_compressed_data' --type rustLength of output: 3943
🏁 Script executed:
#!/bin/bash
# Search for get_compressed_data calls to see patterns
rg -n -B2 'get_compressed_data\(' --type rust | head -100Length of output: 3238
Add validation that the Photon indexer returns data from the requested slot or later.
The code passes commit_slot via IndexerRpcConfig to hint at the desired slot, but doesn't verify the response. Unlike the remote_account_provider path (which validates response.context.slot >= min_context_slot), the compressed data fetch accepts whatever the Photon indexer returns. If the indexer lags, get_compressed_data may return stale account state from an earlier slot.
Consider adding an explicit check after line 190 to validate the returned compressed data's slot meets the requirement, or document why this validation is not needed.
| // Refund account | ||
| invoke( | ||
| &system_instruction::transfer( | ||
| payer.key, | ||
| delegated_account.key, | ||
| args.delegation_record | ||
| .lamports | ||
| .checked_sub(delegated_account.lamports()) | ||
| .ok_or(ProgramError::ArithmeticOverflow)?, | ||
| ), |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Handle “overfunded” delegated accounts more explicitly in process_external_undelegate_compressed
The top‑up amount is computed via:
args.delegation_record
.lamports
.checked_sub(delegated_account.lamports())
.ok_or(ProgramError::ArithmeticOverflow)?If delegated_account.lamports() ends up greater than args.delegation_record.lamports (e.g., a test transfers extra lamports in while delegated), this produces ProgramError::ArithmeticOverflow, which doesn’t clearly describe the state and prevents a more intentional choice (either “no‑op top‑up” or a targeted error).
You could make the invariant explicit and pick clearer semantics, for example:
- invoke(
- &system_instruction::transfer(
- payer.key,
- delegated_account.key,
- args.delegation_record
- .lamports
- .checked_sub(delegated_account.lamports())
- .ok_or(ProgramError::ArithmeticOverflow)?,
- ),
- &[payer.clone(), delegated_account.clone()],
- )?;
+ let target = args.delegation_record.lamports;
+ let current = delegated_account.lamports();
+ if current > target {
+ msg!(
+ "Delegated account has {} lamports, expected at most {} from delegation record",
+ current,
+ target
+ );
+ // Either treat as invalid state …
+ return Err(ProgramError::InvalidAccountData);
+ // … or, if you prefer, skip the transfer entirely instead of erroring:
+ // return Ok(());
+ }
+
+ let top_up = target
+ .checked_sub(current)
+ .ok_or(ProgramError::ArithmeticOverflow)?;
+ if top_up > 0 {
+ invoke(
+ &system_instruction::transfer(payer.key, delegated_account.key, top_up),
+ &[payer.clone(), delegated_account.clone()],
+ )?;
+ }This makes the post‑delegation lamport invariant explicit and gives tests a clearer failure mode instead of a generic arithmetic overflow.
🤖 Prompt for AI Agents
In test-integration/programs/flexi-counter/src/processor.rs around lines
659-668, the code uses checked_sub which yields ProgramError::ArithmeticOverflow
when delegated_account.lamports() > args.delegation_record.lamports, producing
an unclear error; replace that expression with an explicit branch: first compare
delegated_account.lamports() and args.delegation_record.lamports(), and if
delegated_account.lamports() >= args.delegation_record.lamports() either return
a clear program error (add or use a descriptive custom error variant like
DelegatedAccountOverfunded) or treat it as a no-op by setting the
transfer/top-up amount to zero and skipping the invoke, and otherwise compute
the positive top-up via subtraction and proceed with the transfer; ensure the
chosen behavior is documented in comments and tests updated accordingly.
| let (pubkey, mut account) = match mode { | ||
| CommitAccountMode::Commit | CommitAccountMode::CommitAndUndelegate => { | ||
| init_and_delegate_account_on_chain( | ||
| &counter_auth, | ||
| bytes as u64, | ||
| None, | ||
| ) | ||
| .await | ||
| } | ||
| _ => { | ||
| let (pubkey, _hash, account) = | ||
| init_and_delegate_compressed_account_on_chain(&counter_auth) | ||
| .await; | ||
| (pubkey, account) | ||
| } | ||
| }; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider explicit matching for compressed variants.
The wildcard _ pattern works correctly here but makes it implicit which modes use compressed initialization. Consider explicit matching for clarity and to catch future enum additions at compile time:
- _ => {
+ CommitAccountMode::CompressedCommit
+ | CommitAccountMode::CompressedCommitAndUndelegate => {
let (pubkey, _hash, account) =
init_and_delegate_compressed_account_on_chain(&counter_auth)
.await;
(pubkey, account)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let (pubkey, mut account) = match mode { | |
| CommitAccountMode::Commit | CommitAccountMode::CommitAndUndelegate => { | |
| init_and_delegate_account_on_chain( | |
| &counter_auth, | |
| bytes as u64, | |
| None, | |
| ) | |
| .await | |
| } | |
| _ => { | |
| let (pubkey, _hash, account) = | |
| init_and_delegate_compressed_account_on_chain(&counter_auth) | |
| .await; | |
| (pubkey, account) | |
| } | |
| }; | |
| let (pubkey, mut account) = match mode { | |
| CommitAccountMode::Commit | CommitAccountMode::CommitAndUndelegate => { | |
| init_and_delegate_account_on_chain( | |
| &counter_auth, | |
| bytes as u64, | |
| None, | |
| ) | |
| .await | |
| } | |
| CommitAccountMode::CompressedCommit | |
| | CommitAccountMode::CompressedCommitAndUndelegate => { | |
| let (pubkey, _hash, account) = | |
| init_and_delegate_compressed_account_on_chain(&counter_auth) | |
| .await; | |
| (pubkey, account) | |
| } | |
| }; |
🤖 Prompt for AI Agents
In test-integration/test-committor-service/tests/test_ix_commit_local.rs around
lines 161 to 176, the match uses a wildcard `_` to select the compressed
initialization branch which hides exactly which CommitAccountMode variants use
compressed setup; replace the wildcard with explicit matches for the compressed
variants (e.g., CommitAccountMode::Compressed and
CommitAccountMode::CompressedAndSomething) so each enum variant is handled
explicitly, ensuring clarity and that future enum additions produce a
compile-time match error; update the match arms to list all current compressed
variants explicitly and keep the Commit/CommitAndUndelegate arm as-is.
| #[tokio::test] | ||
| async fn test_commit_5_compressed_accounts_100bytes_bundle_size_2_undelegate_all( | ||
| ) { | ||
| commit_5_compressed_accounts_100bytes( | ||
| 2, | ||
| expect_strategies(&[(CommitStrategy::Args, 2)]), | ||
| CommitAccountMode::CompressedCommitAndUndelegate, | ||
| ) | ||
| .await; | ||
| } |
There was a problem hiding this comment.
Incorrect expected strategy count - test will likely fail.
This test creates 5 accounts but expects only 2 commits in the strategies assertion. Comparing with the non-compressed equivalent test_commit_5_accounts_1kb_bundle_size_3_undelegate_all (lines 320-332) which correctly expects 5 total commits (3+2), this should also expect 5:
async fn test_commit_5_compressed_accounts_100bytes_bundle_size_2_undelegate_all(
) {
commit_5_compressed_accounts_100bytes(
2,
- expect_strategies(&[(CommitStrategy::Args, 2)]),
+ expect_strategies(&[(CommitStrategy::Args, 5)]),
CommitAccountMode::CompressedCommitAndUndelegate,
)
.await;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #[tokio::test] | |
| async fn test_commit_5_compressed_accounts_100bytes_bundle_size_2_undelegate_all( | |
| ) { | |
| commit_5_compressed_accounts_100bytes( | |
| 2, | |
| expect_strategies(&[(CommitStrategy::Args, 2)]), | |
| CommitAccountMode::CompressedCommitAndUndelegate, | |
| ) | |
| .await; | |
| } | |
| #[tokio::test] | |
| async fn test_commit_5_compressed_accounts_100bytes_bundle_size_2_undelegate_all( | |
| ) { | |
| commit_5_compressed_accounts_100bytes( | |
| 2, | |
| expect_strategies(&[(CommitStrategy::Args, 5)]), | |
| CommitAccountMode::CompressedCommitAndUndelegate, | |
| ) | |
| .await; | |
| } |
🤖 Prompt for AI Agents
In test-integration/test-committor-service/tests/test_ix_commit_local.rs around
lines 535 to 544, the test creates 5 accounts but the expected strategies
assertion only expects 2 commits; update the expectation to match the
non-compressed equivalent so the total expected commits are 5 by changing the
expect_strategies call to include both strategy counts (3 and 2) so the
assertion expects 5 commits in total.
| if !matches_undelegation { | ||
| trace!( | ||
| "Compressed account ({}) is {} but should be. Owner {} != {}", | ||
| account_pubkey, | ||
| if is_undelegate { | ||
| "not undelegated" | ||
| } else { | ||
| "undelegated" | ||
| }, | ||
| acc.owner, | ||
| expected_owner, | ||
| ); | ||
| } |
There was a problem hiding this comment.
Inconsistent owner logged vs owner compared.
Similar to the lamports fix applied earlier, the trace message logs acc.owner (line 1068) but the comparison at line 1045 uses data.owner. Log the value actually being compared:
if !matches_undelegation {
trace!(
"Compressed account ({}) is {} but should be. Owner {} != {}",
account_pubkey,
if is_undelegate {
"not undelegated"
} else {
"undelegated"
},
- acc.owner,
+ data.owner,
expected_owner,
);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if !matches_undelegation { | |
| trace!( | |
| "Compressed account ({}) is {} but should be. Owner {} != {}", | |
| account_pubkey, | |
| if is_undelegate { | |
| "not undelegated" | |
| } else { | |
| "undelegated" | |
| }, | |
| acc.owner, | |
| expected_owner, | |
| ); | |
| } | |
| if !matches_undelegation { | |
| trace!( | |
| "Compressed account ({}) is {} but should be. Owner {} != {}", | |
| account_pubkey, | |
| if is_undelegate { | |
| "not undelegated" | |
| } else { | |
| "undelegated" | |
| }, | |
| data.owner, | |
| expected_owner, | |
| ); | |
| } |
🤖 Prompt for AI Agents
In test-integration/test-committor-service/tests/test_ix_commit_local.rs around
lines 1059 to 1071, the trace message logs acc.owner but the comparison uses
data.owner; update the trace to log data.owner (the value actually compared) so
the logged output matches the comparison, keeping the rest of the message and
variables unchanged.
| let acc = $photon_client | ||
| .get_compressed_account($address, None) | ||
| .await | ||
| .ok() | ||
| .and_then(|acc| Some(acc.value.clone())); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Use .map() instead of .and_then() for clarity.
Line 87 uses .and_then(|acc| Some(acc.value.clone())), but since the closure always returns Some(...), this should be .map() instead. The and_then combinator is designed for operations that might return None, while map is for straightforward transformations.
Apply this diff:
let acc = $photon_client
.get_compressed_account($address, None)
.await
.ok()
- .and_then(|acc| Some(acc.value.clone()));
+ .map(|acc| acc.value.clone());📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let acc = $photon_client | |
| .get_compressed_account($address, None) | |
| .await | |
| .ok() | |
| .and_then(|acc| Some(acc.value.clone())); | |
| let acc = $photon_client | |
| .get_compressed_account($address, None) | |
| .await | |
| .ok() | |
| .map(|acc| acc.value.clone()); |
🤖 Prompt for AI Agents
In test-integration/test-committor-service/tests/utils/transactions.rs around
lines 83 to 87, replace the use of .and_then(|acc| Some(acc.value.clone())) with
.map(|acc| acc.value.clone()) so the closure is a simple transformation (map)
rather than an option-producing combinator; keep the surrounding .ok() and await
as-is and ensure the resulting expression compiles with the changed combinator.
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.