-
Notifications
You must be signed in to change notification settings - Fork 440
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
[config change] Improve BlocksReExecutor implementation #2714
base: master
Are you sure you want to change the base?
Conversation
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.
LGTM
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.
looks good, but we should configure cleans cache + we potentially can simplify / clean up arbitrum.AdvanceStateUpToBlock
and follow the release func pattern
}, | ||
trieConfig := triedb.Config{ | ||
Preimages: false, | ||
HashDB: hashdb.Defaults, |
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.
hashdb.Defaults
have CleanCacheSize = 0
(cleans cache disabled). We should use cleans cache here to have better perfomance, similarly to:
nitro/cmd/staterecovery/staterecovery.go
Lines 39 to 44 in 7a3d6e6
hashConfig := *hashdb.Defaults | |
hashConfig.CleanCacheSize = cacheConfig.TrieCleanLimit * 1024 * 1024 | |
trieConfig := &triedb.Config{ | |
Preimages: false, | |
HashDB: &hashConfig, | |
} |
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.
Done. I've added a new config field trie-clean-limit
and have set the default value for this as 600 sourcing from here let me know if it should be adjusted
blocksReExecutor.stateFor = func(header *types.Header) (*state.StateDB, arbitrum.StateReleaseFunc, error) { | ||
sdb, err := state.NewDeterministic(header.Root, blocksReExecutor.db) | ||
if err == nil { | ||
_ = blocksReExecutor.db.TrieDB().Reference(header.Root, common.Hash{}) // Will be dereferenced later in advanceStateUpToBlock |
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.
how about following the release function pattern?
-
we could return
arbitrum.StateReleaseFunc
that will callDereference
. That can be something likefunc() {triedb.Dereference(root)}
) -
we would need to move the call to release func returned from
arbitrum.FindLastAvailableState
to the thread started with LaunchThread so as it would be called afterAdvanceStateUpToBlock
or even better - we could pass thestartStateRelease
toadvanceStateUpToBlock
and call it after we reference next state -
arbitrum.AdvanceStateUpToBlock was used only here, so we can modify it to commit statedb after each reexecuted block and check the hashes OR we can just remove
arbitrum.AdvanceStateUpToBlock
-
we can return release func also from
advanceStateUpToBlock
and have something like:
startState, startHeader, startStateRelease, err := arbitrum.FindLastAvailableState(ctx, s.blockchain, s.stateFor, s.blockchain.GetHeaderByNumber(start), nil, -1)
...
s.LaunchThread(func(ctx context.Context) {
...
_, release, err := advanceStateUpToBlock(..., startHeader, startStateRelease, ...)
...
defer 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.
note to startStateRelease
- we should make sure that it will be called also in case of an error
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.
Agreed, its a great idea to utilize StateReleaseFunc
s here!
Also removed AdvanceStateUpToBlock
from geth.
This PR changes the implementation of blocks re-execution mainly to use a separate stateDB during each block's re-execution and to verify that the resulting state successfully commits.
Config option
--blocks-reexecutor.blocks-per-thread
is renamed to--blocks-reexecutor.min-blocks-per-thread
.A new config option
trie-clean-limit
(memory allowance in MB to use for caching trie nodes in memory) is added.Pulls geth PR- OffchainLabs/go-ethereum#364
Resolves NIT-2810