-
Notifications
You must be signed in to change notification settings - Fork 337
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Lazy loading #2840
Lazy loading #2840
Conversation
Coverage Report@@ Coverage Diff @@
## master rq/lazy-loading +/- ##
===================================================
- Coverage 80.93% 78.65% -2.28%
+ Files 286 294 +8
+ Lines 81965 84225 +2260
===================================================
- Hits 66331 66244 -87
+ Misses 15634 17981 +2347
|
WASM runtime size check:Compared to target branchMoonbase runtime: 2188 KB (no changes) ✅ Moonbeam runtime: 2120 KB (no changes) ✅ Moonriver runtime: 2128 KB (no changes) ✅ Compared to latest release (runtime-3102)Moonbase runtime: 2188 KB (+248 KB compared to latest release) Moonbeam runtime: 2120 KB (+220 KB compared to latest release) Moonriver runtime: 2128 KB (+228 KB compared to latest release) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall it looks good to me. It mostly needs some documentation on the new feature and the tradeoffs of using this particular client for tests, but I know its coming in a future PR.
Also could use a script for setting up the binary as the test suite expects when running it locally. Wrote this one:
#!/bin/bash
# Build lazy-loading client
echo '😴 Building lazy-loading client...'
cargo build --release --features lazy-loading
# Rename
echo '✍️ Renaming lazy-loading client...'
mv target/release/moonbeam target/release/lazy-loading
echo '✅ Lazy loading client ready!'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The PR looks good, great changes 💯
Maybe we can add this parameter to the description of the PR because it might be needed for testing (I had to add it) —unsafe-force-node-key-generation
.
There's a lot of code implementing the substrate client api, I compared to the in_mem.rs
to see the actual changes. I paid more attention to backend.rs
and call_executor.rs.
It looks good to me!
Improved the PR description 👍 |
What does it do?
The objective of this PR is to provide a solution for allowing lazy downloading of the state of a live network instead of requiring its state to be downloaded in advance, which may take a long time for chains that have been running for a few years.
What important points reviewers should know?
Building with
lazy-loading
features enabled:New parameters:
Example state overrides files:
Running lazy loading:
Is there something left for follow-up PRs?
What alternative implementations were considered?
Are there relevant PRs or issues in other repositories (Substrate, Polkadot, Frontier, Cumulus)?
What value does it bring to the blockchain users?
This feature enables testing of client code against live chains, which is not possible with Chopsticks. Allowing testing both the client and runtime simultaneously, we can thoroughly verify client-specific components, such as RPCs.