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

Support rebuilding of wasm store on init #2618

Merged
merged 5 commits into from
Aug 29, 2024

Conversation

ganeshvanahalli
Copy link
Contributor

@ganeshvanahalli ganeshvanahalli commented Aug 28, 2024

Before, we did not have support for rebuilding of wasm store when --init.force flag was enabled or in cases when databases were downloaded via snapshot, users would have to quit the node and restart with --init.rebuild-local-wasm flag. With this PR in above mentioned scenarios force building of wasm store is carried out if --init.rebuild-local-wasm is not false.

@cla-bot cla-bot bot added the s Automatically added by the CLA bot if the creator of a PR is registered as having signed the CLA. label Aug 28, 2024
cmd/nitro/init.go Outdated Show resolved Hide resolved
@@ -795,6 +800,11 @@ func openInitializeChainDb(ctx context.Context, stack *node.Node, config *NodeCo
return chainDb, l2BlockChain, err
}

if config.Init.RebuildLocalWasm != "false" {
// In order to rebuild wasm store correctly we have to use force option
return rebuildLocalWasm(ctx, config, l2BlockChain, chainConfig, chainDb, wasmDb, "force")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should just use the Init.RebuildLocalWasm flag.
If we're downloading a database without WASM - when creating the new database there won't be a wasm rebuild and so we'll rebuild it here.
If we're downloading a database with wasm (which we had to specifically enable) the whole point is to avoid local rebuilding wasm phase.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in the first scenario you mentioned, consider this variation- if we had previously attempted rebuilding and the node was stopped we would have a non-empty and non-done rebuilding position and after we download a new database without wasm chaindb gets updated and now when rebuilding starts with old wasm but new chaindb we might miss some programs right? Hence to handle this I was always setting it to force.

But I agree that we could just let the user decide this

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

People are only expected to download a new database when they sync a new node.
For weird/unexpected cases - we always have fallback to compiling wasms on-demand

Copy link
Contributor

@tsahee tsahee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@joshuacolvin0 joshuacolvin0 merged commit 75f84db into master Aug 29, 2024
13 checks passed
@joshuacolvin0 joshuacolvin0 deleted the support-rebuildlocalwasm-initwithsnapshot branch August 29, 2024 23:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
design-approved s Automatically added by the CLA bot if the creator of a PR is registered as having signed the CLA.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants