core, triedb/pathdb: final integration (snapshot integration pt 5) #30661
Conversation
rjl493456442
force-pushed
the
snapshot-integration-p5
branch
from
d249035 to
133900e
Compare
holiman
changed the title
rjl493456442
force-pushed
the
snapshot-integration-p5
branch
4 times, most recently
from
22dcce6 to
b879386
Compare
rjl493456442
force-pushed
the
snapshot-integration-p5
branch
from
b879386 to
985a84e
Compare
| @@ -1997,15 +1999,21 @@ func testIssue23496(t *testing.T, scheme string) { | |||
| } | |||
| expHead := uint64(1) | |||
| if scheme == rawdb.PathScheme { | |||
| expHead = uint64(2) | |||
| expHead = uint64(3) | |||
There was a problem hiding this comment.
Maybe we could add a comment here, sth. like: "The pathdb database makes sure that snapshot and trie are consistent, so only the last block is reverted in case of a crash."
| // Reinsert B3-B4 | ||
| if _, err := chain.InsertChain(blocks[2:]); err != nil { | ||
| t.Fatalf("Failed to import canonical chain tail: %v", err) |
There was a problem hiding this comment.
| // Reinsert B3-B4 | |
| if _, err := chain.InsertChain(blocks[2:]); err != nil { | |
| t.Fatalf("Failed to import canonical chain tail: %v", err) | |
| // Reinsert B4 | |
| if _, err := chain.InsertChain(blocks[3:]); err != nil { | |
| t.Fatalf("Failed to import canonical chain tail: %v", err) |
In path mode only the last block is missing, reinsert that
| @@ -570,7 +574,7 @@ func TestHighCommitCrashWithNewSnapshot(t *testing.T) { | |||
| for _, scheme := range []string{rawdb.HashScheme, rawdb.PathScheme} { | |||
| expHead := uint64(0) | |||
| if scheme == rawdb.PathScheme { | |||
| expHead = uint64(4) | |||
| expHead = uint64(6) | |||
There was a problem hiding this comment.
Same here, would be good to have a comment that in path mode we expect the database to be consistent up to block 6, since that was the last comitted
| // the state reader with database. | ||
| // If standalone state snapshot is not available (path scheme | ||
| // or the state snapshot is explicitly disabled in hash mode), | ||
| // try to construct the state reader with database. | ||
| reader, err := db.triedb.StateReader(stateRoot) | ||
| if err == nil { |
There was a problem hiding this comment.
Afaict this only works because hashdb.StateReader returns an error, otherwise we would create two readers for a hashdb. Right now the logic (afaict) is:
if snapshot {
// create snapshot reader
} else {
if hashdb {
// do nothing
} else {
// create pathdb snapshot reader
}
}
Which seems a bit convoluted, why not make it like this:
if triedb.Scheme() == rawdb.HashScheme && db.snap != nil {
// construct legacy snap reader
}
if triedb.Scheme() == rawdb.PathScheme {
// construct triedb.StateReader
}
| @@ -979,7 +979,8 @@ func testMissingTrieNodes(t *testing.T, scheme string) { | |||
| ) | |||
| if scheme == rawdb.PathScheme { | |||
| tdb = triedb.NewDatabase(memDb, &triedb.Config{PathDB: &pathdb.Config{ | |||
| CleanCacheSize: 0, | |||
| TrieCleanSize: 0, | |||
| StateCleanSize: 0, | |||
| WriteBufferSize: 0, | |||
There was a problem hiding this comment.
Should we also disable the background generation (SnapshotNoBuild: true) here? Seems like disabling it does not impact the outcome of the test and we would save some resources
There was a problem hiding this comment.
Same with a few other places where we use pathdb.Defaults, disabling the background generation there, saved around 200ms in my (very rough) tests
There was a problem hiding this comment.
Okay nvm, the 200ms was just a fluke on my machine, seem to be almost equal. So probably not worth it
| if err := chain.Snapshots().Verify(chain.CurrentBlock().Root); err != nil { | ||
| return err | ||
| if chain.Snapshots() != nil { | ||
| if err := chain.Snapshots().Verify(chain.CurrentBlock().Root); err != nil { |
There was a problem hiding this comment.
We could, maybe in a follow-up PR, add something to TrieDB to also let us verify its integrity. This way we wouldn't loose this sanity check when running the block tests
| @@ -362,6 +430,7 @@ func (db *Database) Enable(root common.Hash) error { | |||
| // reset the persistent state id back to zero. | |||
| batch := db.diskdb.NewBatch() | |||
| rawdb.DeleteTrieJournal(batch) | |||
| rawdb.DeleteSnapshotRoot(batch) | |||
There was a problem hiding this comment.
Could you please explain why we need to delete the snapshot root here, I don't really understand
| } | ||
| } | ||
| } | ||
| for addrHash, storages := range storageData { |
There was a problem hiding this comment.
I'm wondering whether it makes a difference in the speed of flushing if we sort the keys before trying to insert them, iirc some databases handle ordered key insertions better than random keys. This way we could also break on the first key that exceeds the genMarker
|
|
||
| // removeStorageLeft deletes all storage entries which are located after | ||
| // the current iterator position. | ||
| func (ctx *generatorContext) removeStorageLeft() uint64 { |
There was a problem hiding this comment.
This naming is a bit misleading, it deletes all storages that is left, but this storage is "right" of the marker in my mental model. Maybe removeRemainingStorage or something?
| // into two parts. | ||
| func splitMarker(marker []byte) ([]byte, []byte) { | ||
| var accMarker []byte | ||
| if len(marker) > 0 { // []byte{} is the start, use nil for that |
There was a problem hiding this comment.
Do we need to check here that len(marker) >= HashLength?
|
Took a first look now and it looks pretty good so far. I added a few questions/things I didn't understand while reviewing. The biggest changes (generator, generator_test, context) are modified copies from the snapshot package |
No description provided.