MultiWriterAppStore for EVM state migration (Phase 1)#1064
Conversation
enlight
left a comment
enlight
left a comment
There was a problem hiding this comment.
Need to update the store loading code as well, if the evm.db is one block ahead need to load the last height matching app.db
| if cachingStore, ok := (a.Store.(*store.CachingStore)); ok { | ||
|
|
||
| readOnlyStore := a.Store.GetSnapshot() | ||
| if cachingStore, ok := (readOnlyStore.(*store.CachingStore)); ok { |
There was a problem hiding this comment.
Hmm, so we basically lose the cache on all queries.
| if root == nil { | ||
| root = []byte("") | ||
| } | ||
| s.evmDB.Set(util.PrefixKey(vmPrefix, rootKey), root) |
There was a problem hiding this comment.
Really don't like this, this will write to the evm.db on disk when loading the store, which is counter-intuitive because a load should really only read from disk, not write to it.
There was a problem hiding this comment.
This is necessary. LoomEthDB will read the current root from vmvmroot. We have to reload the root of target version and set it to vmvmroot for LoomEthDB
There was a problem hiding this comment.
Yeah, I guess we'll have to refactor this later.
| if root == nil { | ||
| root = []byte("") | ||
| } | ||
| s.evmDB.Set(util.PrefixKey(vmPrefix, rootKey), root) |
There was a problem hiding this comment.
Yeah, I guess we'll have to refactor this later.
enlight
changed the title
|
Going to merge this as is shortly, but there are a couple of issues that will need to be addressed in follow up PRs before this can be shipped:
|
| if err != nil { | ||
| log.Error("failed to unprefix key", "key", x, "prefix", prefix, "err", err) | ||
| k = nil | ||
| for i, k := range keys { |
There was a problem hiding this comment.
OK, I think this is probably to change without a feature flag since all the current uses of Range seem to correctly use util.PrefixKey...
Before this change I take it UnprefixKey always stripped off len(prefix)+1 from the key even if it wasn't prefixed by prefix + 0?
There was a problem hiding this comment.
Yes, previously, UnprefixKey does not check if the key is really prefixed with prefix + 0. If we called Range([]byte("vm") it would return both []byte("vmroot") and util.PrefixKey([]byte("vm"), []byte("root")) keys. It was supposed to return only util.PrefixKey([]byte("vm"),[]byte("root")) key
|
Every block is setting a "vmevmroot" despite not changing, it just adds a lot of extra writes to the db . Negates a lot of performance games we had |
|
If you're talking about |
| } | ||
|
|
||
| func (s *MultiWriterAppStore) GetSnapshot() Snapshot { | ||
| // TODO: Need to ensure that the EvmStore and ImmutableTree are from the same height. |
There was a problem hiding this comment.
Umm, there is a small chance that the mismatched heights can happen as MultiWriterAppStore.SaveVersion is not atomic. We can prevent this by using a mutex but not sure how much it is going to hurt the performance.
The caching store doesn't work with the multi writer app store snapshots, so the snapshots bypass the cache, which means the QueryServer never hits the cache in the caching store, no point writing to the cache if nothing ever reads from it. Also simplified the Range implementation of the multi writer app store and the EVM store, we don't really use Range(nil) anywhere, and there was a lot of code to support that use case that just added a lot of additional complexity.
enlight
force-pushed
the
multi-writer-appstore
branch
from
971e1a3 to
ae0c80a
Compare
|
Remaining issues mentioned in |
4 participants
This is an alternative implementation for EVM state migration.
Ref: #1035
Issue: #849