Repository navigation
[CI] Improve lazy-loading test - #3811
Merged
Merged
Conversation
arturgontijo
added
B0-silent
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughLazy-loading now supports configurable startup delay and persistent fork-state caching. RPC requests use cached immutable results and adaptive retries. Backend fork checkpoints can be reused and persisted, while development tests add polling and transaction-settlement synchronization. ChangesLazy-loading cache and runtime controls
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant RunCmd
participant SubstrateBackend
participant StateCache
participant RPC
RunCmd->>SubstrateBackend: provide lazy-loading options
SubstrateBackend->>StateCache: load pinned fork block
SubstrateBackend->>RPC: create cached RPC client
RPC->>StateCache: read cached storage response
RPC->>SubstrateBackend: return cached or fetched response
SubstrateBackend->>StateCache: persist resolved fork block
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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: 1
🧹 Nitpick comments (2)
test/suites/dev/moonbase/test-eth-fee/test-eth-fee-history.ts (1)
60-84: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider reusing the existing
sleephelper instead of inlinesetTimeout.
requestFeeHistoryusesnew Promise((resolve) => setTimeout(resolve, 100))which is identical to thesleep(100)helper already exported fromtest/helpers/common.ts. Since this file already imports from the helpers barrel (sealUntilTxPoolEmptyat line 35), importingsleepwould eliminate the duplication.♻️ Optional refactor
async function requestFeeHistory( blockCount: string | number, reward_percentiles: number[], expectedBaseFeeLength: number ): Promise<FeeHistory> { let result = (await customDevRpcRequest("eth_feeHistory", [ blockCount, "latest", reward_percentiles, ])) as FeeHistory; for (let i = 0; i < 50 && result.baseFeePerGas.length < expectedBaseFeeLength; i++) { - await new Promise((resolve) => setTimeout(resolve, 100)); + await sleep(100); result = (await customDevRpcRequest("eth_feeHistory", [ blockCount, "latest", reward_percentiles, ])) as FeeHistory; } return result; }🤖 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/suites/dev/moonbase/test-eth-fee/test-eth-fee-history.ts` around lines 60 - 84, Replace the inline setTimeout promise in requestFeeHistory with the existing sleep helper. Add sleep to the helpers-barrel import alongside sealUntilTxPoolEmpty, then await sleep(100) during polling.node/service/src/lazy_loading/state_cache.rs (1)
106-124: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valuePinned-fork-block write is not atomic, unlike
put.
set_pinned_fork_blockwrites directly viafs::write, whileputuses a temp-file + atomic rename to avoid partial reads from concurrent writers. A crash or concurrent access mid-write here could leave a truncated file;pinned_fork_blockdegrades gracefully (returnsNoneon any parse failure) so this isn't data-corrupting, but it's inconsistent with the atomicity guarantee the rest of this cache provides.Separately: this file persists the caller-supplied
fingerprintstring verbatim and unencrypted to disk — see related comment insubstrate_backend.rsregarding what value is passed asfingerprintthere.♻️ Align with the atomic write pattern used in `put`
pub fn set_pinned_fork_block(&self, fingerprint: &str, hash_bytes: &[u8]) { - let _ = fs::write( - self.pinned_fork_block_path(), - format!("{}\n0x{}\n", fingerprint, hex::encode(hash_bytes)), - ); + let path = self.pinned_fork_block_path(); + let tmp = path.with_extension(format!("tmp-{}", std::process::id())); + let contents = format!("{}\n0x{}\n", fingerprint, hex::encode(hash_bytes)); + if fs::write(&tmp, contents).is_ok() { + if fs::rename(&tmp, &path).is_err() { + let _ = fs::remove_file(&tmp); + } + } else { + let _ = fs::remove_file(&tmp); + } }🤖 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 `@node/service/src/lazy_loading/state_cache.rs` around lines 106 - 124, Update set_pinned_fork_block to use the same temporary-file-and-atomic-rename strategy as put instead of writing directly with fs::write. Preserve the existing serialized fingerprint and hash format, handle temporary-file creation/write/rename failures consistently with put, and ensure incomplete writes cannot be observed by pinned_fork_block.
🤖 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 `@node/service/src/lazy_loading/substrate_backend.rs`:
- Around line 1515-1570: Replace the raw
`lazy_loading_config.state_rpc.as_str()` used by `cache_fingerprint` with a
deterministic sanitized fingerprint that cannot contain credentials or API keys,
and use it consistently for `pinned_fork_block`, `set_pinned_fork_block`, and
the stale-pin warning in the checkpoint-resolution logic. Update the
lazy-loading startup disclaimer in `lazy_loading::` (around the relevant startup
logging code) to redact the displayed RPC endpoint as well, using existing
URL-redaction utilities where available.
---
Nitpick comments:
In `@node/service/src/lazy_loading/state_cache.rs`:
- Around line 106-124: Update set_pinned_fork_block to use the same
temporary-file-and-atomic-rename strategy as put instead of writing directly
with fs::write. Preserve the existing serialized fingerprint and hash format,
handle temporary-file creation/write/rename failures consistently with put, and
ensure incomplete writes cannot be observed by pinned_fork_block.
In `@test/suites/dev/moonbase/test-eth-fee/test-eth-fee-history.ts`:
- Around line 60-84: Replace the inline setTimeout promise in requestFeeHistory
with the existing sleep helper. Add sleep to the helpers-barrel import alongside
sealUntilTxPoolEmpty, then await sleep(100) during polling.
🪄 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: CHILL
Plan: Pro
Run ID: ec841cf8-554a-4948-8e8e-6e9d5a3efd2d
📒 Files selected for processing (12)
node/cli-opt/src/lib.rsnode/cli/src/cli.rsnode/cli/src/command.rsnode/service/src/lazy_loading/mod.rsnode/service/src/lazy_loading/rpc_client.rsnode/service/src/lazy_loading/state_cache.rsnode/service/src/lazy_loading/substrate_backend.rstest/helpers/common.tstest/moonwall.config.jsontest/suites/dev/common/test-block/test-block-1.tstest/suites/dev/moonbase/test-balance/test-balance-existential.tstest/suites/dev/moonbase/test-eth-fee/test-eth-fee-history.ts
arturgontijo
dismissed stale reviews from manuelmauro and coderabbitai[bot]
via
July 13, 2026 14:24
078a0f4
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
2 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.
What does it do?
Improves CI reliability for the lazy-loading (fork) test environment and de-flakes three
devtest suites. These changes are infrastructure/test-only — there are no runtime or consensus changes and nothing here affects on-chain behavior.Lazy-loading resilience (
node/service/src/lazy_loading/**,node/cli*)state_cache.rs). State reads (storage,storage_hash,storage_keys_paged) are keyed by a blake2 hash of the request and served from disk on subsequent runs, so a warm cache turns network-bound startup into local reads.latest.--lazy-loading-startup-delay(default preserved) and--lazy-loading-cache-path.Moonwall config (
test/moonwall.config.json)Dev-test de-flakes
test-block-1(D010101): wait for the eth RPC view to catch up to the freshly sealed block before asserting the block number, via a new reusablewaitForpolling helper intest/helpers/common.ts.test-balance-existential(D020301): drain the tx pool (sealUntilTxPoolEmpty) so submitted transfers are included before balance assertions.test-eth-fee-history(D020901): polleth_feeHistoryuntil the asynchronously-populatedbaseFeePerGascache is complete before asserting on it.What important points should reviewers know?
latestwith a warning rather than failing.waitForis bounded (default ~5s) and does not throw on timeout, so the underlying assertions still fail loudly on a genuine regression rather than being masked.Is there something left for follow-up PRs?
devsuites still hand-rollsetTimeoutpolling loops; they could be migrated to the newwaitForhelper.What alternative implementations were considered?
Are there relevant PRs or issues in other repositories (Substrate, Polkadot, Frontier, Cumulus)?
None.
What value does it bring to the blockchain users?
No direct user-facing change. It makes CI faster and far less flaky, which shortens the feedback loop and reduces spurious failures for contributors.
Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.