Add WriteStateSnapshot #15
i-norden
telackey
Labels
No Label
Copied from Github
Kind/Breaking
Kind/Bug
Kind/Documentation
Kind/Enhancement
Kind/Feature
Kind/Security
Kind/Testing
Priority
Critical
Priority
High
Priority
Low
Priority
Medium
Reviewed
Confirmed
Reviewed
Duplicate
Reviewed
Invalid
Reviewed
Won't Fix
Status
Abandoned
Status
Blocked
Status
Need More Info
No Milestone
No project
No Assignees
2 Participants
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: cerc-io/plugeth-statediff#15
Loading…
Reference in New Issue
Block a user
No description provided.
Delete Branch "with-iterator-tracker"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Adds a method to perform full-state snapshots by diffing against an empty state trie.
This replicates the functionality of
ipld-eth-state-snapshot, so that code can use this as a library; see: cerc-io/ipld-eth-state-snapshot#1Note that due to how incremental diffs are processed (updates are processed after the
trie has been traversed) the iterator state doesn't fully capture the progress of the diff, so it's not currently feasible to track them this way. Full snapshots don't have to worry about updated accounts, so we can support them.
c2160fac83to5ed22c03d2WIP: Add WriteStateDiffTracked to support tracking & recovery of a statediffto Add WriteStateDiffTracked to support tracking & recovery of a statediff5ed22c03d2to77e00278e4For such a massive PR, I really don't have a ton of comments or questions. I was befuddled on a couple of points though which I highlighted.
@ -93,4 +93,2 @@}t = time.Now()blockTx := &BatchTx{Cleaning this part of the code up is a big win.
@ -48,7 +47,7 @@ func SetupTestData(t *testing.T, ind interfaces.StateDiffIndexer) {t.Fatal(err)}defer func() {Do you recall why we passed err in to Submit() before? I have a vague memory buzzing in the back of my mind and I want to double check that there is no longer any reason.
Basically it was due to handling rollbacks and commit in the submit function, so it needed the error to check whether to rollback. By factoring the rollback out, we avoid that, but now the caller is responsible for it.
Which reminds me that I need to defer a rollback call from the scope of
service.writeStateDiff- since the tx is returned fromPushBlock.If you've added the rollback, feel free to resolve this conversation.
@ -36,0 +21,4 @@// SymmDiffAux exposes state specific to symmetric difference iteration, which is not accessible// from the NodeIterator interface. This includes the number of nodes seen, whether the current key// is common to both A and B, and whether the current node is sourced from A or B.type SymmDiffAux struct {What is the advantage/disadvantage of doing this by a sort of "sidecar" type vs extending the interface or type.
Eg, something more traditional like:
Just as a rule, having one type representing the internal state of another type, and passing them both around, strikes me as odd, but there may be a compelling reason to do it that way.
Definitely, it's ugly. I didn't want to do this, but the problem comes when we wrap the iterators for tracking. We need to use the wrapped iterator in order for tracking to work, but we need the state exposed by the underlying one. We could extract the base with casts, but that's not much better and leaks the abstraction.
Other workarounds would be to
tracker.Restorealong with the wrapped ones, ortracker.Iteratorinterface which exposes the base somehow.2 means new types and extra state that's mostly not used. 1 is okay, though from this end the result looks about the same (need to pass an extra object).
I've done option 1, but kept the separate state type, so it's clear that only a subset of the symm-diff iterator state is actually used in that context - rather than just passing the same object as a different type.
100c106ee5to367349b85aAdd WriteStateDiffTracked to support tracking & recovery of a statediffto Add WriteStateSnapshotLooks great! So much cleaner.
@ -163,0 +174,4 @@return fmt.Errorf("error opening new state trie: %w", err)}subiters, _, err := tracker.Restore(tree.NodeIterator)I didn't see a test case where the iterators are actually restored. Is there one? Perhaps it exists in ipld-eth-state-snapshot?
It happens in the
TestBuilderSnapshottests, viatest_helpers.RunStateSnapshot. Although there's no explicit check for the file in these tests, I do see them getting restored.Added a check for the file
Great! I see it now. I read that test, but I didn't put 2 and 2 together regarding the interrupt.
Yeah, a check for the file is even better, but you are right, practically speaking even the previous version of the test couldn't have succeeded if the file were not created and used.
@ -236,0 +169,4 @@func (sdi *StateDiffIndexer) PushHeader(batch interfaces.Batch, header *types.Header, reward, td *big.Int) (string, error) {tx, ok := batch.(*BatchTx)if !ok {return "", fmt.Errorf("sql: batch is expected to be of type %T, got %T", &BatchTx{}, batch)Whoa, does this happen? Why?
Haha, it shouldn't and hasn't, but it's technically possible for someone to create an indexer of a different type and pass the wrong batch in. That would just panic otherwise, so the error is more to document the fact that this batch type is covariant with the indexer.
Oh good. Absolutely best always to check, I was just shocked for a moment to think it might be something that really happens.