Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fixes for performing the overlay transition #203

Merged
merged 7 commits into from
Jun 7, 2023
Merged

fixes for performing the overlay transition #203

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
4c4205b
fixes for performing the overlay transition
gballet May 5, 2023
426ae7c
fixes for the full replay
gballet May 26, 2023
95ccc05
fix: deletion-and-recreation of EoA
gballet Jun 1, 2023
b10b176
fixes to replay 2M+ blocks
gballet Jun 2, 2023
249cd18
upgrade to go-verkle@master
gballet Jun 2, 2023
0d2bd5d
fix: proper number of chunk evals
gballet Jun 6, 2023
f75311a
rewrite conversion loop to fix known issues
gballet Jun 7, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions core/blockchain.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1725,6 +1725,11 @@ func (bc *BlockChain) insertChain(chain types.Blocks, verifySeals, setHead bool)
atomic.StoreUint32(&followupInterrupt, 1)
return it.index, err
}
if fdb, ok := statedb.Database().(*state.ForkingDB); ok {
if fdb.InTransition() {
bc.AddRootTranslation(block.Root(), statedb.IntermediateRoot(false))
}
}

// Update the metrics touched during block processing
accountReadTimer.Update(statedb.AccountReads) // Account reads are complete, we can mark them
Expand Down Expand Up @@ -2457,3 +2462,7 @@ func (bc *BlockChain) SetBlockValidatorAndProcessorForTesting(v Validator, p Pro
func (bc *BlockChain) StartVerkleTransition(originalRoot, translatedRoot common.Hash) {
bc.stateCache.(*state.ForkingDB).StartTransition(originalRoot, translatedRoot)
}

func (bc *BlockChain) AddRootTranslation(originalRoot, translatedRoot common.Hash) {
bc.stateCache.(*state.ForkingDB).AddTranslation(originalRoot, translatedRoot)
}
49 changes: 35 additions & 14 deletions core/state/database.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ package state
import (
"errors"
"fmt"
"sync"

"github.com/VictoriaMetrics/fastcache"
"github.com/ethereum/go-ethereum/common"
Expand Down Expand Up @@ -162,11 +163,18 @@ type ForkingDB struct {
*VerkleDB

// TODO ensure that this info is in the DB
started, ended bool
translatedRoots map[common.Hash]common.Hash // hash of the translated root, for opening
baseRoot common.Hash // hash of the read-only base tree
LastAccHash common.Hash // hash of the last translated account address
LastSlotHash common.Hash // hash of the last translated storage slot address
started, ended bool
translatedRoots map[common.Hash]common.Hash // hash of the translated root, for opening
translatedRootsLock sync.RWMutex

baseRoot common.Hash // hash of the read-only base tree
CurrentAccountHash common.Hash // hash of the last translated account
CurrentSlotHash common.Hash // hash of the last translated storage slot

// Mark whether the storage for an account has been processed. This is useful if the
// maximum number of leaves of the conversion is reached before the whole storage is
// processed.
StorageProcessed bool
}

// ContractCode implements Database
Expand All @@ -193,7 +201,7 @@ func (fdb *ForkingDB) CopyTrie(t Trie) Trie {

if fdb.started {
overlay := fdb.VerkleDB.CopyTrie(t)
return trie.NewTransitionTree(mpt.(*trie.SecureTrie), overlay.(*trie.VerkleTrie))
return trie.NewTransitionTree(mpt.(*trie.SecureTrie), overlay.(*trie.VerkleTrie), false)
}

return mpt
Expand All @@ -205,11 +213,13 @@ func (fdb *ForkingDB) OpenStorageTrie(stateRoot, addrHash, root common.Hash, sel
if fdb.started && err == nil {
// Return a "storage trie" that is an adapter between the storge MPT
// and the unique verkle tree.
vkt, err := fdb.VerkleDB.OpenStorageTrie(stateRoot, addrHash, fdb.translatedRoots[root], self)
fdb.translatedRootsLock.RLock()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need a mutex?
Is this related to the prefetcher? (but that shouldn't be "on" in VKT mode?)
Maybe it will become apparent later when I continue with the review.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we need a mutex because the map is accessed from the processor and the state prefetcher, that is on (not the trie prefetecher, that is off).

vkt, err := fdb.VerkleDB.OpenStorageTrie(stateRoot, addrHash, fdb.translatedRoots[root], self.(*trie.TransitionTrie).Overlay())
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had to add an extra parameter to OpenStorageTrie so that the single verkle tree can be reused, see implementation of OpenStorageTrie

fdb.translatedRootsLock.RUnlock()
if err != nil {
return nil, err
}
return trie.NewTransitionTree(mpt.(*trie.SecureTrie), vkt.(*trie.VerkleTrie)), nil
return trie.NewTransitionTree(mpt.(*trie.SecureTrie), vkt.(*trie.VerkleTrie), true), nil
}

return mpt, err
Expand All @@ -221,21 +231,24 @@ func (fdb *ForkingDB) OpenTrie(root common.Hash) (Trie, error) {
mpt Trie
err error
)

if fdb.started {
mpt, err = fdb.cachingDB.OpenTrie(fdb.baseRoot)
if err != nil {
return nil, err
}
fdb.translatedRootsLock.RLock()
vkt, err := fdb.VerkleDB.OpenTrie(fdb.translatedRoots[root])
fdb.translatedRootsLock.RUnlock()
if err != nil {
return nil, err
}
return trie.NewTransitionTree(mpt.(*trie.SecureTrie), vkt.(*trie.VerkleTrie), false), nil
} else {
mpt, err = fdb.cachingDB.OpenTrie(root)
if err != nil {
return nil, err
} else {
mpt, err = fdb.cachingDB.OpenTrie(root)
if err != nil {
return nil, err
}
}
return trie.NewTransitionTree(mpt.(*trie.SecureTrie), vkt.(*trie.VerkleTrie)), nil
}

return mpt, nil
Expand Down Expand Up @@ -263,6 +276,10 @@ func (fdg *ForkingDB) InTransition() bool {
return fdg.started && !fdg.ended
}

func (fdg *ForkingDB) Transitionned() bool {
return fdg.ended
}

// Fork implements the fork
func (fdb *ForkingDB) StartTransition(originalRoot, translatedRoot common.Hash) {
fmt.Println(`
Expand All @@ -275,6 +292,8 @@ func (fdb *ForkingDB) StartTransition(originalRoot, translatedRoot common.Hash)
fdb.started = true
fdb.translatedRoots = map[common.Hash]common.Hash{originalRoot: translatedRoot}
fdb.baseRoot = originalRoot
// initialize so that the first storage-less accounts are processed
fdb.StorageProcessed = true
}

func (fdb *ForkingDB) EndTransition() {
Expand All @@ -290,7 +309,9 @@ func (fdb *ForkingDB) EndTransition() {

func (fdb *ForkingDB) AddTranslation(orig, trans common.Hash) {
// TODO make this persistent
fdb.translatedRootsLock.Lock()
fdb.translatedRoots[orig] = trans
fdb.translatedRootsLock.Unlock()
}

type cachingDB struct {
Expand Down
24 changes: 12 additions & 12 deletions core/state/state_object.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -200,8 +200,9 @@ func (s *stateObject) GetCommittedState(db Database, key common.Hash) common.Has
}
// If no live objects are available, attempt to use snapshots
var (
enc []byte
err error
enc []byte
err error
value common.Hash
)
if s.db.snap != nil {
// If the object was destructed in *this* block (and potentially resurrected),
Expand All @@ -221,11 +222,15 @@ func (s *stateObject) GetCommittedState(db Database, key common.Hash) common.Has
}
// If the snapshot is unavailable or reading from it fails, load from the database.
if s.db.snap == nil || err != nil {
var tr = s.getTrie(db)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we move this line inside the if at L227?

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yup

start := time.Now()
if s.db.GetTrie().IsVerkle() {
panic("verkle trees use the snapshot")
var v []byte
v, err = tr.TryGet(s.address[:], key.Bytes())
copy(value[:], v)
} else {
enc, err = s.getTrie(db).TryGet(s.address[:], key.Bytes())
Comment on lines +229 to +232
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, despite this looks correct it's a bit weird-ish at first sight, because:

  • L230: We do the copy(value[:], v) even if err != nil
  • L232: Doesn't have a symmetrical copy to value.
  • L237: is actually capturing the err != nil for both cases.
  • L242: is decoding if len(enc)>0 which is detecting we entered the if-branch in L232 or we come from the snapshot, setting the value that we want.

To be clear, the logic is correct and it's trying to be a bit smart into "merging" the potential RLP decoding in a single place, considering you can come from different places. And eagerly doing that L230 copy to "accommodate" into that.

Probably we can come up with a better way to describe this logic that doesn't look that weird. Feels like this code is waiting to be a bug the next time someone adds some extra line of code around here.

To be honest, I don't have a good suggestion now. We can think about it when cleaning up all the work maybe.
</rant :) >

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this temporary code, I have a geth PR awaiting a merge that should make all this simpler

}
start := time.Now()
enc, err = s.getTrie(db).TryGet(s.address[:], key.Bytes())
if metrics.EnabledExpensive {
s.db.StorageReads += time.Since(start)
}
Expand All @@ -234,7 +239,6 @@ func (s *stateObject) GetCommittedState(db Database, key common.Hash) common.Has
return common.Hash{}
}
}
var value common.Hash
if len(enc) > 0 {
_, content, _, err := rlp.Split(enc)
if err != nil {
Expand Down Expand Up @@ -324,11 +328,7 @@ func (s *stateObject) updateTrie(db Database) Trie {
var storage map[common.Hash][]byte
// Insert all the pending updates into the trie
var tr Trie
if s.db.trie.IsVerkle() {
tr = s.db.trie
} else {
tr = s.getTrie(db)
}
tr = s.getTrie(db)
Comment on lines 330 to +331
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: merge both L330 and L331 in tr := s.getTrie(db).

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes it should be simplified like this

hasher := s.db.hasher

usedStorage := make([][]byte, 0, len(s.pendingStorage))
Expand Down Expand Up @@ -373,7 +373,7 @@ func (s *stateObject) updateTrie(db Database) Trie {
if len(s.pendingStorage) > 0 {
s.pendingStorage = make(Storage)
}
return tr
return s.trie
}

// UpdateRoot sets the trie root to the current root hash of
Expand Down
110 changes: 78 additions & 32 deletions core/state_processor.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ import (
"github.com/ethereum/go-ethereum/crypto"
"github.com/ethereum/go-ethereum/log"
"github.com/ethereum/go-ethereum/params"
"github.com/ethereum/go-ethereum/rlp"
"github.com/ethereum/go-ethereum/trie"
tutils "github.com/ethereum/go-ethereum/trie/utils"
"github.com/gballet/go-verkle"
Expand Down Expand Up @@ -102,42 +103,89 @@ func (p *StateProcessor) Process(block *types.Block, statedb *state.StateDB, cfg
if fdb, ok := statedb.Database().(*state.ForkingDB); ok {
if fdb.InTransition() {
now := time.Now()
// XXX overkill, just save the parent root in the forking db
tt := statedb.GetTrie().(*trie.TransitionTrie)
mpt := tt.Base()

accIt, err := statedb.Snaps().AccountIterator(mpt.Hash(), fdb.LastAccHash)
if err != nil {
return nil, nil, 0, err
}
stIt, err := statedb.Snaps().StorageIterator(mpt.Hash(), fdb.LastAccHash, fdb.LastSlotHash)
if err != nil {
return nil, nil, 0, err
}
defer accIt.Release()
accIt.Next()

const maxMovedCount = 500
const maxMovedCount = 1000
Comment on lines -118 to +116
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Q: Not sure if makes sense to switch to 10k.

// mkv will be assiting in the collection of up to maxMovedCount key values to be migrated to the VKT.
// It has internal caches to do efficient MPT->VKT key calculations, which will be discarded after
// this function.
mkv := &keyValueMigrator{vktLeafData: make(map[string]*verkle.BatchNewLeafNodeData)}
// move maxCount accounts into the verkle tree, starting with the
// slots from the previous account.
count := 0
addr := rawdb.ReadPreimage(statedb.Database().DiskDB(), accIt.Hash())
for ; stIt.Next() && count < maxMovedCount; count++ {
slotnr := rawdb.ReadPreimage(statedb.Database().DiskDB(), stIt.Hash())
mkv.addStorageSlot(addr, slotnr, stIt.Slot())
}

// if less than maxCount slots were moved, move to the next account
for count < maxMovedCount {
if accIt.Next() {
acc, err := snapshot.FullAccount(accIt.Account())
fdb.CurrentAccountHash = accIt.Hash()

acc, err := snapshot.FullAccount(accIt.Account())
if err != nil {
log.Error("Invalid account encountered during traversal", "error", err)
return nil, nil, 0, err
}
addr := rawdb.ReadPreimage(statedb.Database().DiskDB(), accIt.Hash())
if len(addr) == 0 {
panic(fmt.Sprintf("%x %x %v", addr, accIt.Hash(), acc))
}

// Start with processing the storage, because once the account is
// converted, the `stateRoot` field loses its meaning. Which means
// that it opens the door to a situation in which the storage isn't
// converted, but it can not be found since the account was and so
// there is no way to find the MPT storage from the information found
// in the verkle account.
// Note that this issue can still occur if the account gets written
// to during normal block execution. A mitigation strategy has been
// introduced with the `*StorageRootConversion` fields in VerkleDB.
if acc.HasStorage() {
stIt, err := statedb.Snaps().StorageIterator(mpt.Hash(), accIt.Hash(), fdb.CurrentSlotHash)
if err != nil {
log.Error("Invalid account encountered during traversal", "error", err)
return nil, nil, 0, err
}
addr := rawdb.ReadPreimage(statedb.Database().DiskDB(), accIt.Hash())
stIt.Next()

// fdb.StorageProcessed will be initialized to `true` if the
// entire storage for an account was not entirely processed
// by the previous block. This is used as a signal to resume
// processing the storage for that account where we left off.
// If the entire storage was processed, then the iterator was
// created in vain, but it's ok as this will not happen often.
for ; !fdb.StorageProcessed && count < maxMovedCount; count++ {
var (
value []byte // slot value after RLP decoding
safeValue [32]byte // 32-byte aligned value
)
if err := rlp.DecodeBytes(stIt.Slot(), &value); err != nil {
Copy link
Owner Author

@gballet gballet May 5, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in the MPT, the data is serialized using RLP. It must be deserialized before it is inserted.

return nil, nil, 0, fmt.Errorf("error decoding bytes %x: %w", stIt.Slot(), err)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we stIt.Release() here before returning?

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

called defer like you suggested

}
copy(safeValue[32-len(value):], value)
slotnr := rawdb.ReadPreimage(statedb.Database().DiskDB(), stIt.Hash())

mkv.addStorageSlot(addr, slotnr, safeValue[:])

// advance the storage iterator
fdb.StorageProcessed = !stIt.Next()
if !fdb.StorageProcessed {
fdb.CurrentSlotHash = stIt.Hash()
}
}
stIt.Release()
}

// If the maximum number of leaves hasn't been reached, then
// it means that the storage has finished processing (or none
// was available for this account) and that the account itself
// can be processed.
if count < maxMovedCount {
count++ // count increase for the account itself

mkv.addAccount(addr, acc)

Expand All @@ -149,26 +197,24 @@ func (p *StateProcessor) Process(block *types.Block, statedb *state.StateDB, cfg
mkv.addAccountCode(addr, uint64(len(code)), chunks)
}

if acc.HasStorage() {
for ; stIt.Next() && count < maxMovedCount; count++ {
slotnr := rawdb.ReadPreimage(statedb.Database().DiskDB(), stIt.Hash())

mkv.addStorageSlot(addr, slotnr, stIt.Slot())
}
// reset storage iterator marker for next account
fdb.StorageProcessed = false
fdb.CurrentSlotHash = common.Hash{}

// Move to the next account, if available - or end
// the transition otherwise.
if accIt.Next() {
fdb.CurrentAccountHash = accIt.Hash()
} else {
// case when the account iterator has
// reached the end but count < maxCount
fdb.EndTransition()
break
}
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there's a corner case that isn't correctly handled here; let me explain to double-check with you.

  • Let's suppose we're at L206 and fdb.StorageProcessed == true and count < maxMovedCount. OK, we finished with the storage slots, and we also have room to migrate the account. We migrate the account so count++.
  • We'll jump again to the main loop at L125 doing for accIt.Next() && count < maxMovedCount, but suppose that count < maxMovedCount == false. i.e the last account that we fully migrated fitted perfectly into the limit.

In this situation, fdb.LastAccHash is still that fully migrated account... so in the next block looks like we'll migrate it again. This sounds like a similar situation that (I think) you wanted to solve for storage-slots.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

right, initially I was doing like you said: not checking for count < maxMovedCount. I even have a comment to that effect. So you are correct that we need something similar, and I'm starting to see the wisdom in what you said before: revert and add a TODO.


// If the iterators have reached the end, mark the
// transition as complete.
if !accIt.Next() && !stIt.Next() {
fdb.EndTransition()
} else {
// Update the pointers in the forking db
fdb.LastAccHash = accIt.Hash()
fdb.LastSlotHash = stIt.Hash()
}
log.Info("Collected and prepared key values from base tree", "count", count, "duration", time.Since(now))
log.Info("Collected and prepared key values from base tree", "count", count, "duration", time.Since(now), "last account", fdb.CurrentAccountHash)

now = time.Now()
if err := mkv.migrateCollectedKeyValues(tt.Overlay()); err != nil {
Expand Down Expand Up @@ -337,7 +383,7 @@ func (kvm *keyValueMigrator) getOrInitLeafNodeData(stem []byte) *verkle.BatchNew
stemStr := string(stem)
if _, ok := kvm.vktLeafData[stemStr]; !ok {
kvm.vktLeafData[stemStr] = &verkle.BatchNewLeafNodeData{
Stem: stem,
Stem: stem[:verkle.StemSize],
Values: make(map[byte][]byte),
}
}
Expand Down
13 changes: 7 additions & 6 deletions core/vm/interpreter.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import (
"github.com/ethereum/go-ethereum/log"
"github.com/ethereum/go-ethereum/trie"
"github.com/ethereum/go-ethereum/trie/utils"
"github.com/gballet/go-verkle"
"github.com/holiman/uint256"
)

Expand Down Expand Up @@ -177,15 +178,15 @@ func (in *EVMInterpreter) Run(contract *Contract, input []byte, readOnly bool) (
if in.evm.ChainConfig().IsCancun(in.evm.Context.BlockNumber) && !contract.IsDeployment {
contract.Chunks = trie.ChunkifyCode(contract.Code)

totalEvals := len(contract.Code) / 31 / 256
if len(contract.Code)%(256*31) != 0 {
totalEvals += 1
}
// number of extra stems to evaluate after the header stem
extraEvals := (len(contract.Chunks) + 127) / verkle.NodeWidth

chunkEvals = make([][]byte, totalEvals)
for i := 0; i < totalEvals; i++ {
chunkEvals = make([][]byte, extraEvals+1)
for i := 1; i < extraEvals+1; i++ {
chunkEvals[i] = utils.GetTreeKeyCodeChunkWithEvaluatedAddress(contract.AddressPoint(), uint256.NewInt(uint64(i)*256))
}
// Header account is already known, it's the header account
chunkEvals[0] = utils.GetTreeKeyVersionWithEvaluatedAddress(contract.AddressPoint())
}

// The Interpreter main run loop (contextual). This loop runs until either an
Expand Down
Loading