Reduce RPC hot-path requests#1259
Conversation
|
Warning Review limit reached
More reviews will be available in 52 minutes and 13 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR adds per-client in-memory caching for latest blockhash and slot (with TTLs, guarded refresh, and invalidate API), changes the blockhash RPC call to RpcRequest::GetLatestBlockhash with JSON params, threads an optional shared Arc chain-slot through validator/chainlink/remote-provider/committor wiring, refactors remote-table wait loop logging to a time-based rate limiter, and propagates cache invalidation calls into intent executors and delivery preparator retry paths. Suggested reviewers
✨ 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 22db84c8e2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 01e4190d9c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if let Some(slot) = self.cached_slot().await { | ||
| return Ok(slot); |
There was a problem hiding this comment.
When a slot was cached shortly before a transaction (including via get_latest_blockhash() caching the response context), this branch can return the pre-transaction slot for up to 400ms. LookupTableRc::deactivate immediately stores rpc_client.get_slot() as the table's deactivation_slot, and is_deactivated_on_chain derives the close threshold from that value; on the repo's 50ms local validator this can make table GC/close() run several slots early and fail until the real deactivation window catches up. Keep get_slot() fresh, or split out a cached accessor only for polling paths that tolerate staleness.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
How was this resolved? Which commit?
There was a problem hiding this comment.
Fixed in 2151ff9. get_slot() now bypasses the cache and fetches from RPC every time, then updates the cache opportunistically.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@magicblock-rpc-client/src/lib.rs`:
- Around line 346-349: clear_cached_blockhash currently only writes to
self.cache.blockhash, allowing a concurrent get_latest_blockhash refresh to
repopulate an older value immediately after clear; fix by acquiring the same
refresh mutex used by get_latest_blockhash (the cache refresh lock) before
setting *cached = None so the clear is serialized with refreshes—update
clear_cached_blockhash to lock that refresh mutex, then write None to
self.cache.blockhash, and finally release the mutex.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9724d224-449a-4a20-ab1c-eb77c492b7c0
📒 Files selected for processing (5)
magicblock-committor-service/src/intent_executor/intent_execution_client.rsmagicblock-committor-service/src/intent_executor/single_stage_executor.rsmagicblock-committor-service/src/intent_executor/two_stage_executor.rsmagicblock-committor-service/src/transaction_preparator/delivery_preparator.rsmagicblock-rpc-client/src/lib.rs
thlorenz
left a comment
thlorenz
left a comment
There was a problem hiding this comment.
Overall looks good, but some things seem to be unclean and we should evaluate if they can be improved.
| } | ||
|
|
||
| let _guard = self.cache.blockhash_refresh.lock().await; | ||
| if let Some(blockhash) = self.cached_blockhash().await { |
There was a problem hiding this comment.
This seems to repeat the check from above except above we don't guard the access.
Also needing to take a guard of one resource to access another resource is not good practice. Instead if both resources are that dependent then maybe they should be kept inside a single wrapping resource which will be guarded to access the inner pieces.
There was a problem hiding this comment.
The second check is intentional double-checked locking: another task may have refreshed the cache while this task was waiting on the refresh mutex. Agree that looks unclean, will refactor
| return Ok(blockhash); | ||
| } | ||
|
|
||
| let resp: Response<RpcBlockhash> = self |
There was a problem hiding this comment.
Why not keep self.client.get_latest_blockhash() here instead of manually repeating what's in that method?
There was a problem hiding this comment.
Solana's Rust RPC client only exposes the parsed blockhash from get_latest_blockhash(), while the RPC response also includes context.slot. This is dumb, but afaik there is not way to get both without manually implementing it
| )) | ||
| })?; | ||
|
|
||
| self.cache_slot(resp.context.slot).await; |
There was a problem hiding this comment.
Seems like slot and blockhash are very coupled here, maybe keep them together in a wrapper as well?
There was a problem hiding this comment.
Resolved in d9be235: cached blockhash now carries the context slot and records it into the shared observed slot.
| if let Some(slot) = self.cached_slot().await { | ||
| return Ok(slot); |
There was a problem hiding this comment.
How was this resolved? Which commit?
| return Ok(slot); | ||
| } | ||
|
|
||
| let _guard = self.cache.slot_refresh.lock().await; |
There was a problem hiding this comment.
ditto separate guard (cache) for other resource (cached_slot) which seems like a code smell.
There was a problem hiding this comment.
Resolved in d9be235: slot cache now uses a single mutex, so the separate refresh guard is gone.
|
|
||
| pub async fn clear_cached_blockhash(&self) { | ||
| let _guard = self.cache.blockhash_refresh.lock().await; | ||
| let mut cached = self.cache.blockhash.write().await; |
There was a problem hiding this comment.
Resolved in d9be235: same cleanup, no separate refresh guard remains here.
| "bytemuck", | ||
| ] } | ||
| futures-util = { workspace = true } | ||
| serde_json = { workspace = true } |
There was a problem hiding this comment.
Afaik this is only needed since we reimplemented code that's already in the solana rpc client crate.
There was a problem hiding this comment.
See this: #1259 (comment)
Agree on the ugliness, but afaik there is not way to cleanly do it and I want to avoid 2 RPC calls when the slot information is already in the response
| debug!("Still waiting for remote tables"); | ||
| } | ||
| last_slot = slot; | ||
| sleep(Duration::from_millis(400)).await; |
There was a problem hiding this comment.
If I understand correctly this is to remove polling the RPC over and over for the next slot?
So here we just assume that after 400ms the next slot was reached?
In that case we should take slot jitter into account and wait for at least 450ms (slots don't update exactly after 400ms)
There was a problem hiding this comment.
Another option would be to wire the chain slot subscription that chainlink already has to avoid polling and still be able to continue exactly when the next slot was reached.
To me that seems the more correct solution, but may require a lot more changes.
There was a problem hiding this comment.
Resolved in d9be235: TableMania now waits on the shared Chainlink-observed slot (added an AtomicU64 to wire chain slot subscription from chainlink), with a 450ms fallback.
| .map_err(|e| MagicBlockRpcClientError::GetSlot(Box::new(e))) | ||
| } | ||
|
|
||
| pub async fn clear_cached_blockhash(&self) { |
There was a problem hiding this comment.
Why does one have to call this manually? I would imagine simple flow like this
get_latest_blockhash()
- Check cache
- Missing -> fetch, store, return
- Present -> continue
- Is cached value still fresh?
- Yes -> return cached value
- No -> fetch new value, overwrite cache, return
clear_cached_blockhash seems like an extra piece which is also error prone as one could forget to trigger that.
There was a problem hiding this comment.
The normal freshness path is already TTL-based. This explicit invalidation is for immediate retry paths after cleanup/patching, where we do not want to sign the next prepared transaction with the same cached hash even if it is still TTL-fresh. Will clarify the names
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7f63d87f15
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
test-integration/test-chainlink/src/ixtest_context.rs (1)
111-117: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winAdd one integration path that passes a real shared
Arc<AtomicU64>.Every updated helper still passes
None, so the new shared-slot wiring is untested here. A regression in the injected-slot path would still compile and these tests would keep passing because the provider falls back to its own default slot.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test-integration/test-chainlink/src/ixtest_context.rs` around lines 111 - 117, The call to RemoteAccountProvider::try_from_urls_and_config is always passing None for the injected slot, leaving the new shared-slot wiring untested; create a real shared Arc<AtomicU64> (e.g. Arc::new(AtomicU64::new(0))) and pass Some(shared_slot.clone()) into the function call where currently None is supplied (keep references to endpoints, commitment, tx, and config.remote_account_provider unchanged), and add one integration path/test that uses this variant so the injected-slot code path is exercised.test-integration/test-committor-service/tests/test_ix_commit_local.rs (1)
248-254: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winExercise
CommittorService::try_start(..., Some(chain_slot))in at least one integration test.All updated helpers still pass
None, so the new shared-slot branch inCommittorProcessor::try_newnever runs in CI. That leaves the cross-service slot-sharing path effectively unvalidated.Also applies to: 331-337, 805-811, 875-881
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test-integration/test-committor-service/tests/test_ix_commit_local.rs` around lines 248 - 254, Update at least one integration test to call CommittorService::try_start with Some(chain_slot) instead of None so the shared-slot branch in CommittorProcessor::try_new is exercised; create a ChainSlot/value (or reuse an existing slot-producing helper) before the try_start call and pass it as the fourth argument, ensuring any other services in the test that should share the slot get the same ChainSlot so the cross-service slot-sharing path runs; update one of the occurrences (e.g., the call with validator_auth.insecure_clone()) and adjust setup/teardown as needed so the test compiles and asserts behavior when a shared slot is provided.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@test-integration/test-chainlink/src/ixtest_context.rs`:
- Around line 111-117: The call to
RemoteAccountProvider::try_from_urls_and_config is always passing None for the
injected slot, leaving the new shared-slot wiring untested; create a real shared
Arc<AtomicU64> (e.g. Arc::new(AtomicU64::new(0))) and pass
Some(shared_slot.clone()) into the function call where currently None is
supplied (keep references to endpoints, commitment, tx, and
config.remote_account_provider unchanged), and add one integration path/test
that uses this variant so the injected-slot code path is exercised.
In `@test-integration/test-committor-service/tests/test_ix_commit_local.rs`:
- Around line 248-254: Update at least one integration test to call
CommittorService::try_start with Some(chain_slot) instead of None so the
shared-slot branch in CommittorProcessor::try_new is exercised; create a
ChainSlot/value (or reuse an existing slot-producing helper) before the
try_start call and pass it as the fourth argument, ensuring any other services
in the test that should share the slot get the same ChainSlot so the
cross-service slot-sharing path runs; update one of the occurrences (e.g., the
call with validator_auth.insecure_clone()) and adjust setup/teardown as needed
so the test compiles and asserts behavior when a shared slot is provided.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4dbae04e-0533-453e-80d8-5f66339ed63d
📒 Files selected for processing (14)
magicblock-api/src/magic_validator.rsmagicblock-chainlink/src/chainlink/mod.rsmagicblock-chainlink/src/remote_account_provider/mod.rsmagicblock-committor-service/src/committor_processor.rsmagicblock-committor-service/src/intent_executor/intent_execution_client.rsmagicblock-committor-service/src/intent_executor/single_stage_executor.rsmagicblock-committor-service/src/intent_executor/two_stage_executor.rsmagicblock-committor-service/src/service.rsmagicblock-committor-service/src/transaction_preparator/delivery_preparator.rsmagicblock-rpc-client/src/lib.rsmagicblock-table-mania/src/manager.rstest-integration/test-chainlink/src/ixtest_context.rstest-integration/test-chainlink/tests/ix_remote_account_provider.rstest-integration/test-committor-service/tests/test_ix_commit_local.rs
thlorenz
left a comment
thlorenz
left a comment
There was a problem hiding this comment.
LGTM now. Thanks for addressing all points I raised.
3 participants
Summary
MagicblockRpcClientto avoid repeated hot-path RPC calls.getLatestBlockhashresponses and serialize cache refreshes to avoid request stampedes.Validation
cargo check -p magicblock-rpc-client -p magicblock-table-maniacargo check -p magicblock-committor-servicecargo test -p magicblock-rpc-client -p magicblock-table-maniaSummary by CodeRabbit
Performance Improvements
Reliability
Logging