feat(sync/customrawdb): migrate customrawdb package from coreth #4387
Conversation
- Move `plugin/evm/customrawdb` in `coreth` to `vms/evm/sync` in `avalanchego` as part of state sync code migration effort. resolves #4386 Signed-off-by: Tsvetan Dimitrov (tsvetan.dimitrov23@gmail.com)
powerslider
force-pushed
the
powerslider/4386-migrate-customrawdb-coreth
branch
from
296bb75 to
fff1df9
Compare
|
|
||
| // InspectDatabase traverses the entire database and checks the size | ||
| // of all different categories of data. | ||
| func InspectDatabase(db ethdb.Database, keyPrefix, keyStart []byte) error { |
There was a problem hiding this comment.
This looks like dead code, why do we have this?
There was a problem hiding this comment.
It is used only in database_ext_test.go, so it is a bit unnecessary to be present here. I will check though historically why it was added in the first place since I am not the original author.
There was a problem hiding this comment.
Why do we have this file? This contains dead code/a function that isn't actually a test.
There was a problem hiding this comment.
Yes, I agree with this. I will check why this was historically added in the first place, but we can definitely remove it from here.
There was a problem hiding this comment.
What does the _ext suffix on these files mean? Should we remove it?
| } | ||
|
|
||
| // IterateAccountSnapshots returns an iterator for walking all of the accounts in the snapshot | ||
| func IterateAccountSnapshots(db ethdb.Iteratee) ethdb.Iterator { |
There was a problem hiding this comment.
nit: Maybe name this NewAccountSnapshotsIterator? IterateAccountSnapshots makes it sound like this function is iterating over the database when it is just a constructor for an iterator.
Referencing rawdb - I can't tell what the intended convention is because they also do something similar by mixing the two naming styles (ref).
| } | ||
|
|
||
| // AddCodeToFetch adds a marker that we need to fetch the code for `hash`. | ||
| func AddCodeToFetch(db ethdb.KeyValueWriter, hash common.Hash) { |
There was a problem hiding this comment.
Naming in this package is inconsistent - we're calling this Add but the other apis are all Write* for db writes.
| } | ||
|
|
||
| // packSyncSegmentKey packs root and account into a key for storage in db. | ||
| func packSyncSegmentKey(root common.Hash, start common.Hash) []byte { |
There was a problem hiding this comment.
Why do we refactor this into a separate function if it's only used in one place, and the call-site is basically a direct pass through into this?
| } | ||
|
|
||
| // packSyncStorageTrieKey packs root and account into a key for storage in db. | ||
| func packSyncStorageTrieKey(root common.Hash, account common.Hash) []byte { |
There was a problem hiding this comment.
Similar comment w.r.t refactored function
| "github.com/stretchr/testify/require" | ||
| ) | ||
|
|
||
| func TestClearPrefix(t *testing.T) { |
There was a problem hiding this comment.
It looks like ClearAllSyncSegments is the only function tested (and that this test name is wrong) - could we add coverage on this package api?
| "github.com/ava-labs/avalanchego/utils/wrappers" | ||
| ) | ||
|
|
||
| var ( |
There was a problem hiding this comment.
Could we group all package-level global vars under a single block? If we want to group them by intent, we can add in-line comments
| var FirewoodScheme = "firewood" | ||
|
|
||
| // upgradeConfigKey = upgradeConfigPrefix + hash | ||
| func upgradeConfigKey(hash common.Hash) []byte { |
There was a problem hiding this comment.
This is a weird place for this function to live - it's defined in a file that's not close to where it's used. I think the file structure of this package more generally speaking could be improved - Go code usually will place code into files where there are clear boundaries between abstractions (big files with multiple types are fine). It's not clear to me what this file actually owns/represents.
Why this should be merged
Check #4386
How this works
plugin/evm/customrawdbincorethtovms/evm/syncinavalanchegoas part of state sync code migration effort.How this was tested
Existing UT that come with the migrated code.
Need to be documented in RELEASES.md?
no
resolves #4386
Signed-off-by: Tsvetan Dimitrov (tsvetan.dimitrov23@gmail.com)