feat(snapshot): provide snapshot structure acceleration #324
Conversation
…e module scaffolding required by the snapshot feature
… stack_trie related cases for now.
…into feat/snap
core/blockchain_test.go
Outdated
| @@ -1747,8 +1748,8 @@ func TestInsertReceiptChainRollback(t *testing.T) { | |||
| // overtake the 'canon' chain until after it's passed canon by about 200 blocks. | |||
| // | |||
| // Details at: | |||
| // - https://github.com/scroll-tech/go-ethereum/issues/18977 | |||
| // - https://github.com/scroll-tech/go-ethereum/pull/18988 | |||
| // - https://github.com/scroll-tech/go-ethereum/issues/18977 | |||
There was a problem hiding this comment.
We should avoid this changes on the blanks to kept the comment style consistent.
I guess this unexpected updating is induced by golang 1.19/20 so in our project I advice to stick in 1.18
There was a problem hiding this comment.
Yes, I have try my best to resolve this comment style inconsistent problems just now, you may refresh and check
| @@ -570,6 +570,7 @@ func TestLowCommitCrashWithNewSnapshot(t *testing.T) { | |||
| // committed point so the chain should be rewound to genesis and the disk layer | |||
| // should be left for recovery. | |||
| func TestHighCommitCrashWithNewSnapshot(t *testing.T) { | |||
| skipForTrieDB(t) | |||
There was a problem hiding this comment.
Why do these tests have to be skipped explicitly now?
There was a problem hiding this comment.
This is because this test relies on the behavior of the GC in the trie.
For example, in this example, there would be a chain
G->C1->C2->C3->C4->C5->C6->C7->C8 (HEAD)
Among them, the snapshot is persisted at C4, and then the trie commit at C6. The test expects that the restart after the crash will start from G, because there is no trie information in the DB in the range of C1-4. The current trie db is currently written directly, so C4 also exists in the DB, so this test case cannot pass.
| @@ -653,7 +652,7 @@ func (bc *BlockChain) FastSyncCommitHead(hash common.Hash) error { | |||
| if block == nil { | |||
| return fmt.Errorf("non existent block [%x..]", hash[:4]) | |||
| } | |||
| if _, err := trie.NewSecure(block.Root(), bc.stateCache.TrieDB()); err != nil { | |||
| if _, err := zktrie.NewSecure(block.Root(), bc.stateCache.TrieDB()); err != nil { | |||
There was a problem hiding this comment.
So we totally drop any options for non-zktrie db within this PR?
There was a problem hiding this comment.
Yes, I understand that there is no reason for us to use the original trie for related operations involving state? However, in some places where block, transaction and receipt trie are calculated, the original trie is still used
… into feat/snap Delay Archimedes on Alpha by a week
| for leaf := range in { | ||
| t.TryUpdate(leaf.key[:], leaf.value) | ||
| t.TryUpdateWithKind(kind, leaf.key[:], leaf.value) |
There was a problem hiding this comment.
We can turn the "full Account" into state.Account and use the MarshalFields as trieV bytes. Then we would get a unified byte represents for both account and storage and we can avoid inducing the 'kind' argument here
There was a problem hiding this comment.
I had considered a similar solution, but the underlying zktrie update requires two parameters: vFlag and vPreimage. If we only have bytes, it might be difficult to deduce the value of vFlag backwards.
There was a problem hiding this comment.
vFlag should be part of the encoded bytes: put the first 4 bytes for vFlag and item number, then appending with the byte32 obtained from MarshalFields, then you can use these as value bytes for presistent. vPreimage is never a field for presistent.
There was a problem hiding this comment.
It sounds work, but it involves a lot of minor fixes corresponding to the encoding and we should re-test the whole process to ensure its correctness.
I will take note of this modification, and make the changes to the tests once we have reviewed everything together.
No description provided.