Repository navigation
Picco/allow paying for rent - #645
Closed
GabrielePicco wants to merge 123 commits into
Closed
GabrielePicco wants to merge 123 commits into
GabrielePicco wants to merge 123 commits into
Conversation
* master: chore: dependency update in Cargo.lock file fix: set caps for bincode and serde dependencies fix: capping solana program version due to transit dependency issue
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
* master: feat: replace task context with thread-local storage (#614)
* master: feat: use per-branch cache keys for Integration Tests CI (#641)
Contributor
WalkthroughThis pull request introduces auto-airdrop lamports for feepayers, enhances subscription management with reconnection support and connection state tracking, refactors pubsub architecture with a PubSubConnection abstraction, adds undelegation workflow tracking with metrics, and improves account fetching with per-slot and delegation status checks across multiple modules. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client/Test
participant RemoteAP as RemoteAccountProvider
participant LRUCache as AccountsLruCache
participant ChainPubSub as ChainPubsubClient<br/>(via actor)
participant PubSubConn as PubSubConnection
participant UpdateTask as Background<br/>Updater Task
Client->>RemoteAP: subscribe(pubkey)
RemoteAP->>LRUCache: promote(pubkey)
RemoteAP->>ChainPubSub: subscribe(pubkey)
ChainPubSub->>PubSubConn: account_subscribe(pubkey)
PubSubConn-->>ChainPubSub: subscription established
ChainPubSub-->>RemoteAP: subscription confirmed
RemoteAP-->>Client: Ok
par UpdateTask polling
UpdateTask->>ChainPubSub: subscription_count()
ChainPubSub->>UpdateTask: current count
UpdateTask->>UpdateTask: set_monitored_accounts_count(count)
and Account update
PubSubConn->>ChainPubSub: account update received
ChainPubSub->>RemoteAP: forward update
RemoteAP->>LRUCache: is_watching(pubkey)
alt Account in cache
RemoteAP-->>Client: deliver update
end
end
Note over PubSubConn: Connection lost
PubSubConn->>ChainPubSub: EOF signal
ChainPubSub->>ChainPubSub: abort_and_signal_connection_issue()
ChainPubSub-->>RemoteAP: abort signal sent
RemoteAP->>RemoteAP: try_reconnect queued
RemoteAP->>PubSubConn: reconnect()
PubSubConn->>PubSubConn: swap client in ArcSwap
PubSubConn-->>RemoteAP: Ok
RemoteAP->>ChainPubSub: resub_multiple(pubkeys)
ChainPubSub->>PubSubConn: re-establish subscriptions
PubSubConn-->>ChainPubSub: subscriptions active
ChainPubSub-->>RemoteAP: resubscribe complete
sequenceDiagram
participant Executor as TransactionExecutor
participant Chainlink as Chainlink
participant Bank as AccountsBank
participant Account as AccountData
Executor->>Chainlink: ensure_transaction_accounts(feepayer, ...)
Chainlink->>Bank: fetch feepayer account
alt Account not found
Bank-->>Chainlink: None
alt auto_airdrop_lamports enabled
Chainlink->>Chainlink: airdrop_account_if_empty(feepayer, lamports)
Chainlink->>Bank: create feepayer
else
Chainlink->>Chainlink: mark_empty_if_not_found(feepayer)
end
else Account found
Bank-->>Chainlink: Some(account)
Chainlink->>Account: check undelegating status
end
Chainlink-->>Executor: account ready
Executor->>Executor: execute transaction
Executor->>Executor: check gasless mode (fee == 0)
alt Gasless mode && feepayer mutated
alt auto_airdrop disabled
Executor->>Executor: mark InvalidAccountForFee
else
Executor->>Executor: log potential airdrop failure
end
end
Executor->>Executor: persist accounts (non-zero lamports or privileged)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Areas requiring extra attention:
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 22
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
test-integration/test-cloning/tests/01_program-deploy.rs (1)
181-205: Bump calculation fix is correct; consider a simpler attempt counter for readabilityThe new
bumpexpression:let bump = MAX_RETRIES.saturating_sub(remaining_retries) + 1;avoids the previous risk of negative values and now yields a clean, monotonic 1‑based sequence tied to how many times the loop has run, which is appropriate for this log message. The retry behavior (max attempts and panic condition) stays unchanged.
If you want to make the intent a bit clearer, you could instead track an explicit attempt counter and keep
remaining_retriespurely as a guard, e.g.:let mut attempt = 1; let mut remaining_retries = MAX_RETRIES; loop { // ... let msg = format!("Hola Mundo {attempt}"); // ... if remaining_retries == 0 { panic!("Upgrade not effective after maximum retries {MAX_RETRIES}"); } remaining_retries -= 1; attempt += 1; }Not required, but this separates “how many times we tried” from “how many retries are left”, which makes the loop semantics a bit easier to follow at a glance.
test-integration/test-cloning/tests/04_escrow_transfer.rs (1)
101-106: Critical: Contradictory assertions on escrow_balance.Lines 102 and 104 contain mutually exclusive assertions on the same variable:
- Line 102:
assert!((0.4..=0.5).contains(&escrow_balance))— requires escrow to be 0.4–0.5 SOL- Line 104:
assert!(escrow_balance >= 1.0)— requires escrow to be ≥ 1.0 SOLThese conditions cannot both be true. One of these assertions is incorrect.
Based on the test logic (1 SOL escrowed, 0.5 SOL transferred + fees), line 102 appears correct. Line 104 may be checking the wrong variable or the comment "Airdropped 2 SOL - escrowed half" might refer to a different account balance.
Please clarify which assertion is correct or if line 104 should check a different variable (e.g., the payer account balance).
test-integration/test-schedule-intent/tests/test_schedule_intents.rs (1)
118-141: Consider also verifying undelegation after the second intent in this testIn
test_schedule_intent_undelegate_delegate_back_undelegate_again, you only callverify_undelegation_in_ephem_via_ownerafter the first undelegation (Line 128). Given the test name and flow (undelegate → delegate back → undelegate again via the secondschedule_intent), it would strengthen coverage to assert undelegation via owner after the second intent as well.For example, you could add a second verification after the final
assert_counters:@@ schedule_intent(&ctx, &[&payer], Some(vec![102])); assert_counters( &ctx, &[ExpectedCounter { pda: FlexiCounter::pda(&payer.pubkey()).0, expected: 103, }], true, ); + verify_undelegation_in_ephem_via_owner(&[payer.pubkey()], &ctx);This keeps the test aligned with its name and ensures the redelegate→undelegate-again path is also validated at the ephem level.
magicblock-chainlink/src/testing/chain_pubsub.rs (1)
19-28: Don’t drop the receiver for theSubscriptionUpdatechannel passed into the actorIn
setup_actor_and_client:let (tx, _) = mpsc::channel(10); let (actor, updates_rx) = ChainPubsubActor::new_from_url( PUBSUB_URL, tx, CommitmentConfig::confirmed(), ) …The receiver is immediately dropped. Given prior patterns in this codebase (e.g., keeping
_fwd_rxalive ininit_remote_account_provider()to avoid “receiver dropped” errors on the sender side), this is risky:
- Any
send()ontxinsideChainPubsubActorwill see a closed channel and likely error.- That can subtly break metrics or subscription-forwarding behavior even if tests don’t explicitly read from this channel.
Recommend keeping the receiver alive, even if unused, to mirror the established pattern:
- let (tx, _) = mpsc::channel(10); + let (tx, _subscription_updates_rx) = mpsc::channel(10);The new
reconnecthelper and use ofChainPubsubActorMessage::Reconnect { response }otherwise look consistent.Also applies to: 59-67
magicblock-metrics/src/metrics/mod.rs (1)
40-45: Metrics additions and helpers are wired correctly (minor text nits only)
Gauge/counter naming:
MONITORED_ACCOUNTS_GAUGEregistered as"monitored_accounts_gauge"withmbvprefix matches the newset_monitored_accounts_count()helper.EVICTED_ACCOUNTS_COUNTswitched to anIntCounterand is incremented from the LRU cache eviction path, which is appropriate for a monotonically increasing “total evicted” metric.New counters:
ACCOUNT_FETCHES_*,UNDELEGATION_*, and the various*_A_COUNTcounters are all registered inregister()and have simple, one-line helpers that eitherinc()orinc_by(count)as expected.- This lines up with their usage in other modules (remote account provider, task info fetcher, table-mania, etc.).
Functionally everything looks consistent. Only very minor nit: the help strings for the “A_COUNT” metrics contain a few typos (“mupltiple”); fix them only if you care about polished metric descriptions.
Example (optional):
- "Get mupltiple account count" + "Get multiple-account RPC call count"Also applies to: 103-123, 178-231, 290-343, 429-437, 471-513
magicblock-chainlink/src/remote_account_provider/mod.rs (1)
633-647: Refreshfetch_start_slotwhen reusing fetching_entries to avoid stale-slot comparisonsWhen populating
fetching_accountsintry_get_multi, theEntry::Occupiedbranch only pushes a new sender:match fetching.entry(pubkey) { Entry::Occupied(mut entry) => { entry.get_mut().1.push(sender); } Entry::Vacant(entry) => { entry.insert((fetch_start_slot, vec![sender])); } }If a previous attempt left an entry behind (e.g., due to a subscription setup error before
fetch()ran), the storedfetch_start_slotremains from that older attempt. Subsequent calls will:
- Use the new
min_context_slotfor the RPC fetch, but- Still compare subscription updates against the old
fetch_start_slotinlisten_for_account_updates.That can cause older subscription updates (newer than the stale slot but older than the current attempt’s slot) to be treated as sufficiently fresh.
You can fix this by updating the stored slot on
Entry::Occupied:- match fetching.entry(pubkey) { - Entry::Occupied(mut entry) => { - entry.get_mut().1.push(sender); - } - Entry::Vacant(entry) => { - entry.insert((fetch_start_slot, vec![sender])); - } - } + match fetching.entry(pubkey) { + Entry::Occupied(mut entry) => { + // Refresh fetch_start_slot for the new attempt and add the sender + let entry_ref = entry.get_mut(); + entry_ref.0 = fetch_start_slot; + entry_ref.1.push(sender); + } + Entry::Vacant(entry) => { + entry.insert((fetch_start_slot, vec![sender])); + } + }This keeps the “subscription update vs. fetch” race-resolution logic aligned with the most recent fetch attempt.
Also applies to: 664-683
magicblock-chainlink/src/chainlink/fetch_cloner.rs (1)
369-379: Centralized delegation record parsing improves safety and error reportingThe new
parse_delegation_recordhelper standardizes conversion failures intoChainlinkError::InvalidDelegationRecord, and its reuse both in subscription updates and multi‑fetch resolution keeps behavior consistent. The early cancel_subs +return Err(err)path on invalid records infetch_and_clone_accountspreserves the previous “fail fast, do not clone anything” semantics while making the error origin explicit.If you anticipate more delegation‑related helpers, consider moving them into a small internal module to avoid this file growing further.
Also applies to: 453-468, 837-864
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.locktest-integration/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (57)
Cargo.toml(3 hunks)magicblock-account-cloner/src/lib.rs(1 hunks)magicblock-accounts-db/src/lib.rs(2 hunks)magicblock-aperture/src/requests/http/mod.rs(1 hunks)magicblock-aperture/src/requests/http/send_transaction.rs(1 hunks)magicblock-aperture/src/requests/http/simulate_transaction.rs(1 hunks)magicblock-aperture/src/tests.rs(1 hunks)magicblock-aperture/tests/setup.rs(1 hunks)magicblock-api/src/magic_validator.rs(2 hunks)magicblock-api/src/tickers.rs(0 hunks)magicblock-chainlink/Cargo.toml(2 hunks)magicblock-chainlink/src/chainlink/blacklisted_accounts.rs(2 hunks)magicblock-chainlink/src/chainlink/errors.rs(1 hunks)magicblock-chainlink/src/chainlink/fetch_cloner.rs(12 hunks)magicblock-chainlink/src/chainlink/mod.rs(11 hunks)magicblock-chainlink/src/remote_account_provider/chain_pubsub_actor.rs(8 hunks)magicblock-chainlink/src/remote_account_provider/chain_pubsub_client.rs(11 hunks)magicblock-chainlink/src/remote_account_provider/config.rs(4 hunks)magicblock-chainlink/src/remote_account_provider/lru_cache.rs(4 hunks)magicblock-chainlink/src/remote_account_provider/mod.rs(27 hunks)magicblock-chainlink/src/remote_account_provider/program_account.rs(3 hunks)magicblock-chainlink/src/remote_account_provider/remote_account.rs(1 hunks)magicblock-chainlink/src/submux/mod.rs(20 hunks)magicblock-chainlink/src/testing/chain_pubsub.rs(2 hunks)magicblock-chainlink/src/testing/mod.rs(1 hunks)magicblock-chainlink/tests/utils/test_context.rs(1 hunks)magicblock-committor-service/src/intent_executor/task_info_fetcher.rs(2 hunks)magicblock-metrics/src/metrics/mod.rs(9 hunks)magicblock-metrics/src/service.rs(2 hunks)magicblock-processor/Cargo.toml(1 hunks)magicblock-processor/src/executor/mod.rs(2 hunks)magicblock-processor/src/executor/processing.rs(6 hunks)magicblock-processor/src/scheduler/state.rs(1 hunks)magicblock-processor/tests/fees.rs(2 hunks)magicblock-table-mania/Cargo.toml(1 hunks)magicblock-table-mania/src/lookup_table_rc.rs(2 hunks)magicblock-table-mania/src/manager.rs(2 hunks)programs/magicblock/src/schedule_transactions/process_schedule_base_intent.rs(1 hunks)test-integration/Cargo.toml(2 hunks)test-integration/test-chainlink/src/ixtest_context.rs(1 hunks)test-integration/test-chainlink/src/test_context.rs(2 hunks)test-integration/test-chainlink/tests/chain_pubsub_actor.rs(3 hunks)test-integration/test-chainlink/tests/chain_pubsub_client.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(3 hunks)test-integration/test-chainlink/tests/ix_exceed_capacity.rs(1 hunks)test-integration/test-chainlink/tests/ix_remote_account_provider.rs(6 hunks)test-integration/test-cloning/tests/01_program-deploy.rs(1 hunks)test-integration/test-cloning/tests/04_escrow_transfer.rs(3 hunks)test-integration/test-cloning/tests/05_parallel-cloning.rs(2 hunks)test-integration/test-cloning/tests/06_escrows.rs(1 hunks)test-integration/test-cloning/tests/07_subscription_limits.rs(1 hunks)test-integration/test-config/tests/auto_airdrop_feepayer.rs(0 hunks)test-integration/test-schedule-intent/tests/test_schedule_intents.rs(5 hunks)test-integration/test-tools/Cargo.toml(1 hunks)test-integration/test-tools/src/integration_test_context.rs(2 hunks)test-kit/src/lib.rs(2 hunks)
💤 Files with no reviewable changes (2)
- magicblock-api/src/tickers.rs
- test-integration/test-config/tests/auto_airdrop_feepayer.rs
🧰 Additional context used
🧠 Learnings (12)
📚 Learning: 2025-11-04T10:53:50.922Z
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 589
File: magicblock-processor/src/scheduler/locks.rs:110-122
Timestamp: 2025-11-04T10:53:50.922Z
Learning: In magicblock-processor, the TransactionScheduler runs in a single, dedicated thread and will always remain single-threaded. The `next_transaction_id()` function in scheduler/locks.rs uses `unsafe static mut` which is safe given this architectural guarantee.
Applied to files:
magicblock-processor/src/scheduler/state.rstest-kit/src/lib.rsmagicblock-processor/src/executor/mod.rs
📚 Learning: 2025-11-19T09:34:37.890Z
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.890Z
Learning: In test-integration/test-chainlink/tests/ix_remote_account_provider.rs and similar test files, the `_fwd_rx` receiver returned by `init_remote_account_provider()` is intentionally kept alive (but unused) to prevent "receiver dropped" errors on the sender side. The pattern `let (remote_account_provider, _fwd_rx) = init_remote_account_provider().await;` should NOT be changed to `let (remote_account_provider, _) = ...` because dropping the receiver would cause send() operations to fail.
Applied to files:
test-integration/test-cloning/tests/06_escrows.rstest-integration/test-chainlink/src/ixtest_context.rsmagicblock-aperture/tests/setup.rsmagicblock-chainlink/src/remote_account_provider/remote_account.rstest-integration/test-cloning/tests/07_subscription_limits.rsmagicblock-chainlink/src/remote_account_provider/program_account.rstest-integration/test-chainlink/tests/ix_exceed_capacity.rstest-integration/test-chainlink/tests/chain_pubsub_actor.rstest-integration/test-cloning/tests/05_parallel-cloning.rsmagicblock-chainlink/src/remote_account_provider/lru_cache.rsmagicblock-chainlink/src/remote_account_provider/config.rstest-integration/test-schedule-intent/tests/test_schedule_intents.rsmagicblock-chainlink/src/testing/mod.rstest-integration/test-chainlink/tests/chain_pubsub_client.rstest-integration/test-chainlink/tests/ix_07_redeleg_us_same_slot.rstest-integration/test-cloning/tests/04_escrow_transfer.rsmagicblock-chainlink/src/testing/chain_pubsub.rsmagicblock-chainlink/src/chainlink/mod.rstest-integration/test-chainlink/tests/ix_06_redeleg_us_separate_slots.rstest-integration/test-chainlink/src/test_context.rsmagicblock-chainlink/src/remote_account_provider/chain_pubsub_client.rsmagicblock-chainlink/src/remote_account_provider/chain_pubsub_actor.rsmagicblock-chainlink/src/submux/mod.rsmagicblock-chainlink/src/chainlink/fetch_cloner.rsmagicblock-chainlink/src/remote_account_provider/mod.rstest-integration/test-chainlink/tests/ix_remote_account_provider.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-cloning/tests/06_escrows.rsmagicblock-aperture/tests/setup.rsmagicblock-processor/tests/fees.rstest-integration/test-schedule-intent/tests/test_schedule_intents.rstest-integration/test-chainlink/tests/ix_07_redeleg_us_same_slot.rstest-integration/test-cloning/tests/04_escrow_transfer.rstest-integration/test-chainlink/tests/ix_06_redeleg_us_separate_slots.rstest-integration/test-chainlink/src/test_context.rs
📚 Learning: 2025-11-07T13:09:52.253Z
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 589
File: test-kit/src/lib.rs:275-0
Timestamp: 2025-11-07T13:09:52.253Z
Learning: In test-kit, the transaction scheduler in ExecutionTestEnv is not expected to shut down during tests. Therefore, using `.unwrap()` in test helper methods like `schedule_transaction` is acceptable and will not cause issues in the test environment.
Applied to files:
test-integration/test-cloning/tests/06_escrows.rstest-kit/src/lib.rsmagicblock-processor/tests/fees.rstest-integration/test-cloning/tests/05_parallel-cloning.rstest-integration/test-schedule-intent/tests/test_schedule_intents.rsmagicblock-chainlink/src/remote_account_provider/chain_pubsub_client.rsmagicblock-aperture/src/requests/http/send_transaction.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-table-mania/src/lookup_table_rc.rsmagicblock-chainlink/src/remote_account_provider/remote_account.rstest-integration/test-cloning/tests/07_subscription_limits.rstest-integration/test-chainlink/tests/ix_exceed_capacity.rstest-integration/test-chainlink/tests/chain_pubsub_actor.rsmagicblock-chainlink/src/remote_account_provider/lru_cache.rsmagicblock-chainlink/src/remote_account_provider/config.rstest-integration/test-chainlink/tests/chain_pubsub_client.rstest-integration/test-chainlink/tests/ix_07_redeleg_us_same_slot.rsmagicblock-chainlink/src/testing/chain_pubsub.rsmagicblock-chainlink/src/chainlink/mod.rstest-integration/test-chainlink/tests/ix_06_redeleg_us_separate_slots.rstest-integration/test-chainlink/src/test_context.rsmagicblock-chainlink/src/remote_account_provider/chain_pubsub_client.rsmagicblock-chainlink/src/remote_account_provider/chain_pubsub_actor.rsmagicblock-chainlink/src/submux/mod.rsmagicblock-chainlink/src/chainlink/fetch_cloner.rsmagicblock-chainlink/src/remote_account_provider/mod.rstest-integration/test-chainlink/tests/ix_remote_account_provider.rs
📚 Learning: 2025-11-18T08:47:39.681Z
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.681Z
Learning: In magicblock-chainlink tests involving compressed accounts, `set_remote_slot()` sets the slot of the `AccountSharedData`, while `compressed_account_shared_with_owner_and_slot()` sets the slot of the delegation record. These are two different fields and both calls are necessary.
Applied to files:
magicblock-chainlink/src/remote_account_provider/remote_account.rsmagicblock-chainlink/src/remote_account_provider/program_account.rsmagicblock-account-cloner/src/lib.rsmagicblock-chainlink/src/remote_account_provider/lru_cache.rsmagicblock-chainlink/src/testing/mod.rstest-integration/test-chainlink/tests/ix_07_redeleg_us_same_slot.rstest-integration/test-chainlink/tests/ix_06_redeleg_us_separate_slots.rs
📚 Learning: 2025-10-21T14:00:54.642Z
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 578
File: magicblock-aperture/src/requests/websocket/account_subscribe.rs:18-27
Timestamp: 2025-10-21T14:00:54.642Z
Learning: In magicblock-aperture account_subscribe handler (src/requests/websocket/account_subscribe.rs), the RpcAccountInfoConfig fields data_slice, commitment, and min_context_slot are currently ignored—only encoding is applied. This is tracked as technical debt in issue #579: https://github.com/magicblock-labs/magicblock-validator/issues/579
Applied to files:
magicblock-chainlink/src/remote_account_provider/remote_account.rstest-integration/test-cloning/tests/07_subscription_limits.rsmagicblock-chainlink/src/remote_account_provider/program_account.rstest-integration/test-chainlink/tests/ix_exceed_capacity.rsmagicblock-account-cloner/src/lib.rsmagicblock-processor/src/executor/processing.rsmagicblock-chainlink/src/remote_account_provider/config.rsmagicblock-api/src/magic_validator.rstest-integration/test-chainlink/tests/chain_pubsub_client.rstest-integration/test-chainlink/tests/ix_07_redeleg_us_same_slot.rsmagicblock-aperture/src/requests/http/mod.rsprograms/magicblock/src/schedule_transactions/process_schedule_base_intent.rsmagicblock-chainlink/src/chainlink/mod.rstest-integration/test-chainlink/tests/ix_06_redeleg_us_separate_slots.rstest-integration/test-chainlink/src/test_context.rsmagicblock-chainlink/src/remote_account_provider/chain_pubsub_client.rsmagicblock-chainlink/src/remote_account_provider/chain_pubsub_actor.rsmagicblock-chainlink/src/submux/mod.rsmagicblock-chainlink/src/chainlink/fetch_cloner.rsmagicblock-chainlink/src/remote_account_provider/mod.rstest-integration/test-chainlink/tests/ix_remote_account_provider.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-account-cloner/src/lib.rstest-kit/src/lib.rs
📚 Learning: 2025-11-13T09:38:43.804Z
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 589
File: magicblock-processor/src/scheduler/locks.rs:64-102
Timestamp: 2025-11-13T09:38:43.804Z
Learning: In magicblock-processor's TransactionScheduler (scheduler/mod.rs line 59), the executor count is clamped to MAX_SVM_EXECUTORS (63) at initialization time, and executor IDs are assigned sequentially from 0 to count-1. This architectural guarantee ensures that executor IDs used in the bitmask-based AccountLock (scheduler/locks.rs) will always be within valid bounds for bit shifting operations, making runtime bounds checks unnecessary.
Applied to files:
test-kit/src/lib.rsmagicblock-processor/src/executor/mod.rs
📚 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-processor/src/executor/mod.rs
📚 Learning: 2025-11-07T13:20:13.793Z
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 589
File: magicblock-processor/src/scheduler/coordinator.rs:227-238
Timestamp: 2025-11-07T13:20:13.793Z
Learning: In magicblock-processor's ExecutionCoordinator (scheduler/coordinator.rs), the `account_contention` HashMap intentionally does not call `shrink_to_fit()`. Maintaining slack capacity is beneficial for performance by avoiding frequent reallocations during high transaction throughput. As long as empty entries are removed from the map (which `clear_account_contention` does), the capacity overhead is acceptable.
Applied to files:
magicblock-processor/src/executor/processing.rsmagicblock-chainlink/src/remote_account_provider/lru_cache.rsmagicblock-accounts-db/src/lib.rsprograms/magicblock/src/schedule_transactions/process_schedule_base_intent.rsmagicblock-chainlink/src/chainlink/mod.rs
📚 Learning: 2025-10-26T16:53:29.820Z
Learnt from: thlorenz
Repo: magicblock-labs/magicblock-validator PR: 587
File: magicblock-chainlink/src/remote_account_provider/mod.rs:134-0
Timestamp: 2025-10-26T16:53:29.820Z
Learning: In magicblock-chainlink/src/remote_account_provider/mod.rs, the `Endpoint::separate_pubsub_url_and_api_key()` method uses `split_once("?api-key=")` because the api-key parameter is always the only query parameter right after `?`. No additional query parameter parsing is needed for this use case.
Applied to files:
test-integration/test-chainlink/tests/ix_remote_account_provider.rs
🧬 Code graph analysis (23)
magicblock-table-mania/src/lookup_table_rc.rs (1)
magicblock-metrics/src/metrics/mod.rs (1)
inc_table_mania_close_a_count(511-513)
magicblock-committor-service/src/intent_executor/task_info_fetcher.rs (1)
magicblock-metrics/src/metrics/mod.rs (1)
inc_task_info_fetcher_a_count(503-505)
magicblock-table-mania/src/manager.rs (1)
magicblock-metrics/src/metrics/mod.rs (1)
inc_table_mania_a_count(507-509)
test-integration/test-chainlink/tests/ix_exceed_capacity.rs (1)
magicblock-chainlink/src/remote_account_provider/config.rs (2)
try_new_with_metrics(27-42)subscribed_accounts_lru_capacity(55-57)
test-integration/test-chainlink/tests/chain_pubsub_actor.rs (2)
magicblock-chainlink/src/testing/chain_pubsub.rs (4)
reconnect(59-68)setup_actor_and_client(14-29)subscribe(31-43)unsubscribe(45-57)test-integration/test-tools/src/integration_test_context.rs (1)
airdrop(610-635)
magicblock-processor/tests/fees.rs (1)
test-kit/src/lib.rs (3)
new_with_fee(101-157)new(83-85)new_with_payer_and_fees(87-91)
test-integration/test-tools/src/integration_test_context.rs (1)
magicblock-chainlink/src/remote_account_provider/chain_pubsub_client.rs (1)
url(56-58)
magicblock-chainlink/src/remote_account_provider/lru_cache.rs (1)
magicblock-metrics/src/metrics/mod.rs (1)
inc_evicted_accounts_count(436-438)
test-integration/test-schedule-intent/tests/test_schedule_intents.rs (2)
test-integration/programs/flexi-counter/src/state.rs (1)
pda(32-35)test-integration/test-chainlink/src/ixtest_context.rs (1)
counter_pda(169-171)
magicblock-chainlink/src/testing/mod.rs (1)
magicblock-chainlink/src/remote_account_provider/remote_account.rs (2)
delegated(95-101)remote_slot(153-159)
test-integration/test-chainlink/tests/chain_pubsub_client.rs (1)
magicblock-chainlink/src/remote_account_provider/chain_pubsub_client.rs (1)
try_new_from_url(165-180)
test-integration/test-cloning/tests/04_escrow_transfer.rs (2)
magicblock-validator/src/main.rs (1)
init_logger(13-50)test-integration/test-chainlink/src/ixtest_context.rs (1)
counter_pda(169-171)
magicblock-chainlink/src/testing/chain_pubsub.rs (2)
magicblock-chainlink/src/remote_account_provider/chain_pubsub_actor.rs (1)
new_from_url(116-124)magicblock-chainlink/src/remote_account_provider/chain_pubsub_client.rs (1)
reconnect(81-111)
magicblock-chainlink/src/chainlink/mod.rs (4)
magicblock-chainlink/src/remote_account_provider/remote_account.rs (1)
delegated(95-101)magicblock-chainlink/src/chainlink/blacklisted_accounts.rs (1)
blacklisted_accounts(6-29)magicblock-processor/tests/fees.rs (1)
ephemeral_balance_pda_from_payer(20-26)magicblock-metrics/src/metrics/mod.rs (1)
inc_undelegation_requested(487-489)
test-integration/test-chainlink/tests/ix_06_redeleg_us_separate_slots.rs (2)
test-integration/test-chainlink/src/ixtest_context.rs (1)
counter_pda(169-171)magicblock-chainlink/src/remote_account_provider/remote_account.rs (1)
remote_slot(153-159)
test-integration/test-chainlink/src/test_context.rs (2)
magicblock-chainlink/src/remote_account_provider/config.rs (2)
try_new_with_metrics(27-42)lifecycle_mode(51-53)magicblock-chainlink/src/remote_account_provider/mod.rs (3)
try_from_clients_and_mode(182-201)rpc_client(1068-1070)rpc_client(1080-1082)
magicblock-chainlink/src/remote_account_provider/chain_pubsub_client.rs (3)
magicblock-chainlink/src/remote_account_provider/mod.rs (1)
new(278-333)magicblock-chainlink/src/remote_account_provider/chain_pubsub_actor.rs (5)
new(126-158)subscription_count(177-192)subscriptions(165-170)subscriptions(194-203)try_reconnect(457-495)magicblock-chainlink/src/submux/mod.rs (3)
new(147-158)subscription_count(620-636)subscriptions(641-643)
magicblock-chainlink/src/remote_account_provider/chain_pubsub_actor.rs (4)
magicblock-chainlink/src/remote_account_provider/mod.rs (4)
new(278-333)default(124-130)unsubscribe(797-832)update(438-438)magicblock-chainlink/src/remote_account_provider/chain_pubsub_client.rs (17)
new(46-54)new(311-322)url(56-58)subscription_count(135-138)subscription_count(237-248)subscription_count(414-429)subscriptions(140-140)subscriptions(250-252)subscriptions(431-434)try_reconnect(147-147)try_reconnect(257-266)try_reconnect(439-442)unsubscribe(123-126)unsubscribe(219-235)unsubscribe(402-410)send(335-344)mem(73-76)magicblock-chainlink/src/submux/mod.rs (4)
new(147-158)subscription_count(620-636)subscriptions(641-643)unsubscribe(587-595)magicblock-chainlink/src/testing/chain_pubsub.rs (1)
unsubscribe(45-57)
magicblock-chainlink/src/submux/mod.rs (3)
magicblock-chainlink/src/remote_account_provider/mod.rs (1)
new(278-333)magicblock-chainlink/src/remote_account_provider/chain_pubsub_actor.rs (4)
new(126-158)subscription_count(177-192)subscriptions(165-170)subscriptions(194-203)magicblock-chainlink/src/remote_account_provider/chain_pubsub_client.rs (8)
new(46-54)new(311-322)subscription_count(135-138)subscription_count(237-248)subscription_count(414-429)subscriptions(140-140)subscriptions(250-252)subscriptions(431-434)
magicblock-chainlink/src/chainlink/fetch_cloner.rs (3)
magicblock-chainlink/src/remote_account_provider/remote_account.rs (4)
account(188-210)data(71-77)slot(211-223)lamports(79-85)magicblock-metrics/src/metrics/mod.rs (1)
inc_undelegation_completed(491-493)magicblock-chainlink/src/remote_account_provider/config.rs (1)
try_new_with_metrics(27-42)
magicblock-chainlink/src/remote_account_provider/mod.rs (6)
magicblock-metrics/src/metrics/mod.rs (6)
inc_account_fetches_failed(475-477)inc_account_fetches_found(479-481)inc_account_fetches_not_found(483-485)inc_account_fetches_success(471-473)set_monitored_accounts_count(433-435)inc_remote_account_provider_a_count(499-501)magicblock-chainlink/src/remote_account_provider/lru_cache.rs (4)
new(26-35)pubkeys(39-42)pubkeys(135-141)len(119-125)magicblock-chainlink/src/remote_account_provider/chain_pubsub_client.rs (3)
new(46-54)new(311-322)try_new_from_url(165-180)magicblock-chainlink/src/submux/mod.rs (1)
new(147-158)magicblock-chainlink/src/remote_account_provider/config.rs (2)
default(65-72)try_new_with_metrics(27-42)magicblock-chainlink/src/remote_account_provider/remote_account.rs (1)
from_fresh_account(175-186)
magicblock-metrics/src/metrics/mod.rs (6)
magicblock-chainlink/src/chainlink/fetch_cloner.rs (1)
new(135-159)magicblock-table-mania/src/lookup_table_rc.rs (1)
new(48-55)magicblock-chainlink/src/remote_account_provider/mod.rs (1)
new(278-333)magicblock-committor-service/src/intent_executor/task_info_fetcher.rs (1)
new(52-60)magicblock-table-mania/src/manager.rs (1)
new(64-88)magicblock-aperture/src/state/subscriptions.rs (4)
new(342-355)new(410-413)count(335-337)count(370-372)
test-integration/test-chainlink/tests/ix_remote_account_provider.rs (3)
magicblock-chainlink/src/remote_account_provider/mod.rs (3)
try_new_from_urls(335-380)rpc_client(1068-1070)rpc_client(1080-1082)magicblock-chainlink/src/remote_account_provider/config.rs (1)
try_new_with_metrics(27-42)test-integration/test-tools/src/integration_test_context.rs (1)
airdrop(610-635)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: Integration Tests - schedule_intents
- GitHub Check: Integration Tests - task-scheduler
- GitHub Check: Integration Tests - table_mania
- GitHub Check: Integration Tests - config
- GitHub Check: Integration Tests - chainlink
- GitHub Check: Integration Tests - schedulecommit
- GitHub Check: Integration Tests - magicblock_api
- GitHub Check: Integration Tests - cloning
- GitHub Check: Integration Tests - restore_ledger
| // There is nothing we can do if fetching the account fails | ||
| // Log the error and return whatever is in the accounts db | ||
| warn!("Failed to ensure account {pubkey}: {e}"); | ||
| debug!("Failed to ensure account {pubkey}: {e}"); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
Verify inconsistent log levels for similar error conditions.
The log level for account ensure failure was downgraded from warn to debug, but Line 137 in read_accounts_with_ensure still uses warn! for the same type of error. Both functions have identical error handling patterns and comments ("There is nothing we can do if fetching the account fails").
Is this inconsistency intentional? If single-account failures warrant only debug-level logging, should multi-account failures also be downgraded? Alternatively, if ensure failures indicate operational issues (network problems, chain unavailability, etc.) that could lead to stale data being served, should both remain at warn level?
Please clarify:
- Why do these semantically equivalent errors have different log levels?
- Should they be consistent?
- Will downgrading to
debughide important operational issues that need monitoring?
🏁 Script executed:
#!/bin/bash
# Inspect the file to verify log levels and context
cat magicblock-aperture/src/requests/http/mod.rs | sed -n '100,150p'Length of output: 1968
Ensure consistent log levels for semantically equivalent errors.
Verification confirms the log level inconsistency between read_account_with_ensure (line 115: debug!) and read_accounts_with_ensure (line 137: warn!). Both functions have identical error handling logic and the same recovery comment ("There is nothing we can do if fetching the account(s) fails"), yet they log at different levels.
Either:
- Make both consistent—apply the same log level to both single and multi-account failures
- Document the intentional difference if single-account failures are genuinely less critical than multi-account failures
Without explicit documentation, this inconsistency suggests an oversight and could unintentionally suppress observability into chain/network issues.
🤖 Prompt for AI Agents
In magicblock-aperture/src/requests/http/mod.rs around lines 115 and 137, the
error logging for semantically equivalent failures is inconsistent (line 115
uses debug! while line 137 uses warn!); make them consistent by using the same
log level for both read_account_with_ensure and read_accounts_with_ensure
(preferably change the debug! at line 115 to warn! to match line 137), and keep
the existing error message and recovery comment unchanged (or add a brief
comment if you intentionally want a different level).
| @@ -1,4 +1,4 @@ | |||
| use log::{debug, trace}; | |||
| use log::*; | |||
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider explicit imports for clarity.
While wildcard imports (use log::*) are commonly accepted for the log crate, explicit imports (use log::{debug, trace}) improve code clarity by making dependencies visible. This is an optional style preference.
🤖 Prompt for AI Agents
In magicblock-aperture/src/requests/http/send_transaction.rs around line 1, the
file currently uses a wildcard import `use log::*`; replace this with explicit
imports for the logging macros actually used in the file (for example `use
log::{debug, trace, info, warn, error}` only including the ones referenced) so
dependencies are visible and clearer; update the use statement accordingly and
remove the wildcard import.
Comment on lines
+151
to
+199
| let delegated = AtomicU64::new(0); | ||
| let dlp_owned_not_delegated = AtomicU64::new(0); | ||
| let blacklisted = AtomicU64::new(0); | ||
| let remaining = AtomicU64::new(0); | ||
| let remaining_empty = AtomicU64::new(0); | ||
|
|
||
| let removed = self.accounts_bank.remove_where(|pubkey, account| { | ||
| (!account.delegated() | ||
| // This fixes the edge-case of accounts that were in the process of | ||
| // being undelegated but never completed while the validator was running | ||
| || account.owner().eq(&dlp::id())) | ||
| && !blacklisted_accounts.contains(pubkey) | ||
| if blacklisted_accounts.contains(pubkey) { | ||
| blacklisted.fetch_add(1, Ordering::Relaxed); | ||
| return false; | ||
| } | ||
| // TODO: this potentially looses data and is a temporary measure | ||
| if account.owner().eq(&dlp::id()) { | ||
| dlp_owned_not_delegated.fetch_add(1, Ordering::Relaxed); | ||
| return true; | ||
| } | ||
| if account.delegated() { | ||
| delegated.fetch_add(1, Ordering::Relaxed); | ||
| return false; | ||
| } | ||
| trace!( | ||
| "Removing non-delegated, non-DLP-owned account: {pubkey} {:#?}", | ||
| account | ||
| ); | ||
| remaining.fetch_add(1, Ordering::Relaxed); | ||
| if account.lamports() == 0 | ||
| && account.owner().ne(&solana_sdk::feature::id()) | ||
| { | ||
| remaining_empty.fetch_add(1, Ordering::Relaxed); | ||
| } | ||
| true | ||
| }); | ||
|
|
||
| debug!("Removed {removed} non-delegated accounts"); | ||
| let non_empty = remaining | ||
| .load(Ordering::Relaxed) | ||
| .saturating_sub(remaining_empty.load(Ordering::Relaxed)); | ||
|
|
||
| info!( | ||
| "Removed {removed} accounts from bank: | ||
| {} DLP-owned non-delegated | ||
| {} non-delegated non-blacklisted, no-feature non-empty. | ||
| {} non-delegated non-blacklisted empty | ||
| Kept: {} delegated, {} blacklisted", | ||
| dlp_owned_not_delegated.into_inner(), | ||
| non_empty, | ||
| remaining_empty.into_inner(), | ||
| delegated.into_inner(), | ||
| blacklisted.into_inner() | ||
| ); |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
reset_accounts_bank metrics & logging are sound but could be slightly cheaper
The per‑category AtomicU64 tracking and summary log are correct and use Relaxed ordering appropriately for one‑threaded accounting. If this remains single‑threaded, plain u64 counters with interior mutability (e.g. Cell) would be marginally cheaper, but the current implementation is fine and clear.
🤖 Prompt for AI Agents
In magicblock-chainlink/src/chainlink/mod.rs around lines 151 to 199, the review
notes that the per-category AtomicU64 counters (delegated,
dlp_owned_not_delegated, blacklisted, remaining, remaining_empty) are safe but
unnecessarily heavy if this code is single-threaded; replace the AtomicU64s with
cheaper plain counters using interior mutability (e.g., std::cell::Cell<u64>) or
simple mutable u64s depending on lexical mutability, update all uses
(fetch_add/load/into_inner) to the corresponding Cell methods
(set/get/get+increment) or direct increments/reads, and keep Ordering semantics
out (no atomics) while preserving the same increment and read points and the
info! summary formatting.
Comment on lines
+131
to
+132
| static CLIENT_ID: AtomicU16 = AtomicU16::new(0); | ||
|
|
Contributor
There was a problem hiding this comment.
Consider CLIENT_ID overflow risk.
The static CLIENT_ID counter uses AtomicU16, which will wrap after 65,535 increments. In long-running services that frequently create and destroy actors, this could lead to duplicate client IDs in logs, making debugging more difficult.
Consider using AtomicU64 instead:
- static CLIENT_ID: AtomicU16 = AtomicU16::new(0);
+ static CLIENT_ID: AtomicU64 = AtomicU64::new(0);And update the field type:
- client_id: u16,
+ client_id: u64,Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
magicblock-chainlink/src/remote_account_provider/chain_pubsub_actor.rs around
lines 131-132: the static CLIENT_ID is declared as AtomicU16 which can overflow
and wrap after 65,535; change it to AtomicU64 (replace AtomicU16 with AtomicU64
and initialize with AtomicU64::new(0)), update the CLIENT_ID field type usages
and any variables or function signatures that assume a u16 to use u64, and
ensure all atomic operations (fetch_add, load, store) keep the existing Ordering
and that any logging/formatting or serialization still accepts u64 values.
Comment on lines
389
to
398
| Err(err) => { | ||
| let _ = sub_response.send(Err(err.into())); | ||
| error!("[client_id={client_id}] Failed to subscribe to account {pubkey} {err:?}"); | ||
| Self::abort_and_signal_connection_issue( | ||
| client_id, | ||
| subs.clone(), | ||
| abort_sender, | ||
| is_connected.clone(), | ||
| ); | ||
|
|
||
| return; |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Verify that single subscription failure should abort all subscriptions.
When account_subscribe fails for a single account, abort_and_signal_connection_issue is called, which cancels ALL subscriptions and signals a connection issue. This may be too aggressive if the error is account-specific (e.g., invalid pubkey, permission denied) rather than a connection-level failure.
Consider distinguishing between connection-level errors (connection refused, timeout) and account-level errors (invalid account, permission denied). Only trigger the full abort for connection-level failures.
Would you like me to help refactor this to handle transient/account-specific errors more gracefully?
Send error response to requester before aborting.
When account_subscribe fails (line 384-389), the code calls abort_and_signal_connection_issue but returns early without sending a response to sub_response. This causes the requester to receive a RecvError instead of the actual subscription error, making debugging harder.
Apply this diff:
Ok(res) => res,
Err(err) => {
error!("[client_id={client_id}] Failed to subscribe to account {pubkey} {err:?}");
+ let _ = sub_response.send(Err(err.clone().into()));
Self::abort_and_signal_connection_issue(
client_id,
subs.clone(),
abort_sender,
is_connected.clone(),
);
return;
}📝 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.
Suggested change
| Err(err) => { | |
| let _ = sub_response.send(Err(err.into())); | |
| error!("[client_id={client_id}] Failed to subscribe to account {pubkey} {err:?}"); | |
| Self::abort_and_signal_connection_issue( | |
| client_id, | |
| subs.clone(), | |
| abort_sender, | |
| is_connected.clone(), | |
| ); | |
| return; | |
| Err(err) => { | |
| error!("[client_id={client_id}] Failed to subscribe to account {pubkey} {err:?}"); | |
| let _ = sub_response.send(Err(err.clone().into())); | |
| Self::abort_and_signal_connection_issue( | |
| client_id, | |
| subs.clone(), | |
| abort_sender, | |
| is_connected.clone(), | |
| ); | |
| return; | |
| } |
🤖 Prompt for AI Agents
In magicblock-chainlink/src/remote_account_provider/chain_pubsub_actor.rs around
lines 389 to 398, the code aborts and returns when account_subscribe fails but
never sends the subscription error back to sub_response; update the error path
to first send the subscription error to the requester (e.g., send an Err variant
or equivalent failure message via sub_response, handling a potential send error
with a debug/error log), then call Self::abort_and_signal_connection_issue(...)
and return; ensure the send happens before aborting so the requester receives
the real subscription failure instead of a RecvError.
Comment on lines
+12
to
+42
| fn log_accounts_balances( | ||
| ctx: &IntegrationTestContext, | ||
| stage: &str, | ||
| counter: &Pubkey, | ||
| payer: &Pubkey, | ||
| escrow: &Pubkey, | ||
| ) -> (u64, u64, u64) { | ||
| let accs = ctx | ||
| .fetch_ephem_multiple_accounts(&[*counter, *payer, *escrow]) | ||
| .unwrap(); | ||
| let [counter_acc, payer_acc, escrow_acc] = accs.as_slice() else { | ||
| panic!("Expected 3 accounts, got {:#?}", accs); | ||
| }; | ||
|
|
||
| let counter_balance = | ||
| counter_acc.as_ref().unwrap().lamports as f64 / LAMPORTS_PER_SOL as f64; | ||
| let payer_balance = | ||
| payer_acc.as_ref().unwrap().lamports as f64 / LAMPORTS_PER_SOL as f64; | ||
| let escrow_balance = | ||
| escrow_acc.as_ref().unwrap().lamports as f64 / LAMPORTS_PER_SOL as f64; | ||
| debug!("--- {stage} ---"); | ||
| debug!("Counter {counter}: {counter_balance} SOL"); | ||
| debug!("Payer {payer}: {payer_balance} SOL"); | ||
| debug!("Escrow {escrow} {escrow_balance} SOL"); | ||
|
|
||
| ( | ||
| counter_acc.as_ref().unwrap().lamports, | ||
| payer_acc.as_ref().unwrap().lamports, | ||
| escrow_acc.as_ref().unwrap().lamports, | ||
| ) | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider adding account existence checks.
The helper function unwraps account references multiple times (lines 27, 29, 31, 38-40) without first verifying the accounts exist. While this may be acceptable in test code where accounts are expected to exist, a panic here would produce unclear error messages.
Consider adding explicit checks or documenting the precondition:
) -> (u64, u64, u64) {
let accs = ctx
.fetch_ephem_multiple_accounts(&[*counter, *payer, *escrow])
.unwrap();
let [counter_acc, payer_acc, escrow_acc] = accs.as_slice() else {
panic!("Expected 3 accounts, got {:#?}", accs);
};
+
+ assert!(counter_acc.is_some(), "Counter account not found at {}", counter);
+ assert!(payer_acc.is_some(), "Payer account not found at {}", payer);
+ assert!(escrow_acc.is_some(), "Escrow account not found at {}", escrow);
let counter_balance =📝 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.
Suggested change
| fn log_accounts_balances( | |
| ctx: &IntegrationTestContext, | |
| stage: &str, | |
| counter: &Pubkey, | |
| payer: &Pubkey, | |
| escrow: &Pubkey, | |
| ) -> (u64, u64, u64) { | |
| let accs = ctx | |
| .fetch_ephem_multiple_accounts(&[*counter, *payer, *escrow]) | |
| .unwrap(); | |
| let [counter_acc, payer_acc, escrow_acc] = accs.as_slice() else { | |
| panic!("Expected 3 accounts, got {:#?}", accs); | |
| }; | |
| let counter_balance = | |
| counter_acc.as_ref().unwrap().lamports as f64 / LAMPORTS_PER_SOL as f64; | |
| let payer_balance = | |
| payer_acc.as_ref().unwrap().lamports as f64 / LAMPORTS_PER_SOL as f64; | |
| let escrow_balance = | |
| escrow_acc.as_ref().unwrap().lamports as f64 / LAMPORTS_PER_SOL as f64; | |
| debug!("--- {stage} ---"); | |
| debug!("Counter {counter}: {counter_balance} SOL"); | |
| debug!("Payer {payer}: {payer_balance} SOL"); | |
| debug!("Escrow {escrow} {escrow_balance} SOL"); | |
| ( | |
| counter_acc.as_ref().unwrap().lamports, | |
| payer_acc.as_ref().unwrap().lamports, | |
| escrow_acc.as_ref().unwrap().lamports, | |
| ) | |
| } | |
| fn log_accounts_balances( | |
| ctx: &IntegrationTestContext, | |
| stage: &str, | |
| counter: &Pubkey, | |
| payer: &Pubkey, | |
| escrow: &Pubkey, | |
| ) -> (u64, u64, u64) { | |
| let accs = ctx | |
| .fetch_ephem_multiple_accounts(&[*counter, *payer, *escrow]) | |
| .unwrap(); | |
| let [counter_acc, payer_acc, escrow_acc] = accs.as_slice() else { | |
| panic!("Expected 3 accounts, got {:#?}", accs); | |
| }; | |
| assert!(counter_acc.is_some(), "Counter account not found at {}", counter); | |
| assert!(payer_acc.is_some(), "Payer account not found at {}", payer); | |
| assert!(escrow_acc.is_some(), "Escrow account not found at {}", escrow); | |
| let counter_balance = | |
| counter_acc.as_ref().unwrap().lamports as f64 / LAMPORTS_PER_SOL as f64; | |
| let payer_balance = | |
| payer_acc.as_ref().unwrap().lamports as f64 / LAMPORTS_PER_SOL as f64; | |
| let escrow_balance = | |
| escrow_acc.as_ref().unwrap().lamports as f64 / LAMPORTS_PER_SOL as f64; | |
| debug!("--- {stage} ---"); | |
| debug!("Counter {counter}: {counter_balance} SOL"); | |
| debug!("Payer {payer}: {payer_balance} SOL"); | |
| debug!("Escrow {escrow} {escrow_balance} SOL"); | |
| ( | |
| counter_acc.as_ref().unwrap().lamports, | |
| payer_acc.as_ref().unwrap().lamports, | |
| escrow_acc.as_ref().unwrap().lamports, | |
| ) | |
| } |
🤖 Prompt for AI Agents
In test-integration/test-cloning/tests/04_escrow_transfer.rs around lines 12 to
42, the helper log_accounts_balances unwraps account Option values multiple
times without verifying presence, which can lead to unclear panics; update the
function to explicitly check that fetch_ephem_multiple_accounts returned three
accounts and that each account is Some before using lamports — either by
returning a Result and propagating a clear error, or by replacing the plain
unwraps with expect/unwrap_or_else that include descriptive messages (e.g.,
which pubkey was missing) so failures are explicit and easier to debug.
Comment on lines
+144
to
145
| #[ignore = "We are still evaluating escrow functionality that allows anything except just paying fees"] | ||
| #[tokio::test(flavor = "multi_thread", worker_threads = 2)] |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider adding GitHub issue reference to track ignored test re-enablement.
The test is now ignored pending escrow functionality evaluation. While the message clearly explains the reason, it would improve tracking and follow-up if the ignore attribute included a reference to a GitHub issue number or additional context for when/how this test should be re-enabled.
For example:
#[ignore = "We are still evaluating escrow functionality that allows anything except just paying fees (see issue #XYZ)"]This makes it easier for future maintainers to find the decision context and track the re-enablement task.
🤖 Prompt for AI Agents
In test-integration/test-cloning/tests/05_parallel-cloning.rs around lines 144
to 145, the #[ignore] attribute message lacks a GitHub issue reference to track
re-enablement; update the ignore attribute string to include the relevant issue
number (or create one and reference it) and optionally add a short inline
comment indicating the condition for re-enabling so future maintainers can find
the decision context easily.
Comment on lines
+53
to
+71
| join_set.spawn(async move { | ||
| if idx % 2 == 0 { | ||
| ctx.airdrop_chain_and_delegate( | ||
| &payer_chain, | ||
| &keypair, | ||
| rent_exempt_amount, | ||
| ) | ||
| .expect( | ||
| "failed to airdrop and delegate to on-chain account", | ||
| ); | ||
| } else { | ||
| ctx.airdrop_chain(&keypair.pubkey(), rent_exempt_amount) | ||
| .expect("failed to airdrop to on-chain account"); | ||
| } | ||
| }); | ||
| } | ||
| for _result in join_set.join_all().await { | ||
| // spawned task panicked or was cancelled - handled by join_all | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Spawned tasks lack error handling.
The spawned tasks in the JoinSet can panic, but the errors are silently discarded at line 69-71. Consider logging failures or propagating them to ensure test reliability.
- for _result in join_set.join_all().await {
- // spawned task panicked or was cancelled - handled by join_all
- }
+ for result in join_set.join_all().await {
+ if let Err(e) = result {
+ error!("Task failed during airdrop/delegation: {:?}", e);
+ }
+ }Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In test-integration/test-cloning/tests/07_subscription_limits.rs around lines 53
to 71, the JoinSet::join_all() results are ignored so panics in spawned tasks
are silently discarded; change the code to collect the join_all results and
handle each entry (e.g., iterate over the Vec returned by join_all().await and
call .expect(...) or assert!(result.is_ok(), ...) on each Result to surface
panics, or log the JoinError and then fail the test), ensuring any task panic or
cancellation causes the test to fail or is at least logged with context.
| EncodedConfirmedBlock, EncodedConfirmedTransactionWithStatusMeta, | ||
| UiTransactionEncoding, | ||
| }; | ||
| use url::Url; |
Contributor
There was a problem hiding this comment.
Metric name mismatch: parser looks for mbv_monitored_accounts but metric is mbv_monitored_accounts_gauge
get_monitored_accounts_count scans the /metrics output for lines starting with:
if line.starts_with("mbv_monitored_accounts ") {
…
}However, in magicblock-metrics/src/metrics/mod.rs the underlying gauge is registered as:
IntGauge::new("monitored_accounts_gauge", ...)with registry prefix "mbv", so the exported Prometheus metric name will be mbv_monitored_accounts_gauge, not mbv_monitored_accounts. This function will therefore never find the metric and will always return the "monitored_accounts metric not found" error.
Suggestion:
- for line in response.lines() {
- if line.starts_with("mbv_monitored_accounts ") {
+ for line in response.lines() {
+ if line.starts_with("mbv_monitored_accounts_gauge ") {The rest of the parsing logic (splitting on whitespace and parsing the second token) is fine.
Also, Url::parse(ephem_url) is slightly clearer than Url::parse(&ephem_url), but that’s purely cosmetic.
Also applies to: 1153-1204
Comment on lines
+87
to
+91
| pub fn new_with_payer_and_fees(payer: &Keypair, fee: u64) -> Self { | ||
| let mut ctx = Self::new_with_fee(fee); | ||
| ctx.payer = payer.insecure_clone(); | ||
| ctx | ||
| } |
Contributor
There was a problem hiding this comment.
new_with_payer_and_fees leaves payer without a backing funded account
new_with_payer_and_fees calls new_with_fee(fee) (which funds the original internal payer and inserts its account) and then overwrites ctx.payer with payer.insecure_clone() without:
- removing the old funded payer account from
accountsdb, or - inserting/funding an account for the new payer.
As a result:
ExecutionTestEnv::payerno longer corresponds to the funded account created innew_with_fee.get_payer()will panic for environments constructed vianew_with_payer_and_fees, because the new payer has no account inaccountsdb.
In this PR you only use this helper for the “non-existing fee payer in gasless mode” test, where this behavior is intentional, but the API name suggests general-purpose use and may surprise future callers.
Consider either:
- explicitly inserting an account for the new payer (preserving the usual invariant that
payerhas an on-ledger account), or - documenting/renaming this helper to make it clear that the provided payer is unfunded and may not exist in
accountsdb.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Bug Fixes
Tests