Skip to content
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

Introduce API for directly importing the state into the database #5956

Draft
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

liuchengxu
Copy link
Contributor

This new API commits the state directly to the underlying database, reducing the risk of OOM issues when importing large states into the node. More background on this can be found at #5862

This PR is an internal implementation change, replacing the reset_storage() function with the new commit_trie_changes() API in Client. The entire state still stays in the memory, which will be worked on after merging this PR.

I’ve successfully tested fast sync locally, and it works as expected. However, the unit test I added for this API has been failing since 3372d31. I’m submitting this PR to get early feedback on the overall approach and insights into the test failure before diving deeper into troubleshooting.

cc @bkchr

@liuchengxu
Copy link
Contributor Author

liuchengxu commented Oct 8, 2024

The existing warp sync tests failed after the state sync was complete, still trying to figure out why this happens. Fixed in ecf9975

2024-10-08 18:04:21.206 TRACE tokio-runtime-worker sync::import-queue: Verifying 2(0x087b…4d08) from 12D3KooWJRRjUXjsS6h5AGPx9vBszRZExDAHBiD2iLxhQSKUF7ZQ failed: Api called for an unknown Block: State already discarded for 0x5c3c3bbda63d22c5faaa40a0b0653c21a8960352da6b81003b2ac73067e60db6
2024-10-08 18:04:21.206  WARN tokio-runtime-worker sync: 💔 Verification failed for block 0x087b1e0dd8bf3ba8a13f64e7b39ba6d99e5a3937d6f2b0ed82f858dd44f44d08 received from (12D3KooWJRRjUXjsS6h5AGPx9vBszRZExDAHBiD2iLxhQSKUF7ZQ): "Api called for an unknown Block: State already discarded for 0x5c3c3bbda63d22c5faaa40a0b0653c21a8960352da6b81003b2ac73067e60db6"

let import_block = verifier.verify(import_block).await.map_err(|msg| {

Two fixes:
- Workaround for using the proper hash of block before importing the target block's state.
- Set `commit_state` flag along with the `commit_trie_changes()`.
@liuchengxu
Copy link
Contributor Author

liuchengxu commented Oct 9, 2024

This should be ready for the first round of review, sc-network-test sync::syncs_state failed because of the TODO of child trie handling. @bkchr, could you please take a look? I’ll hold off on addressing the remaining TODOs until I get your initial feedback for efficiency.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant