[wip] combined coreth mods #3
Conversation
…27691) Co-authored-by: Sina Mahmoodi <itz.s1na@gmail.com>
| func (db *Database) Reader(blockRoot common.Hash) (Reader, error) { | ||
| switch b := db.backend.(type) { | ||
| case *hashdb.Database: | ||
| return b.Reader(blockRoot) | ||
| case *pathdb.Database: | ||
| return b.Reader(blockRoot) | ||
| } | ||
| return nil, errors.New("unknown backend") |
There was a problem hiding this comment.
why did we remove dual support for this?
There was a problem hiding this comment.
it is not needed to fork on the concrete type of the db backend since the interface supports it. I also applied this upstream.
There has also been some refactoring upstream here, which I partially applied to make things a bit more simple (provides a place for the interfaces)
| UpdateSameRoot(common.Hash) error | ||
| } | ||
| if u, ok := db.backend.(withUpdateSameRoot); ok { | ||
| return u.UpdateSameRoot(root) |
There was a problem hiding this comment.
I could not see where this was actually implemented in coreth. I guess we cannot handle this in atomic trie itself right?
There was a problem hiding this comment.
Probably a bug that it's not referenced from coreth, (seems I only referenced it in subnet-evm's hashdb).
We should review the path that ensures blocks with the same state root are processed correctly.
| // NewWithSnapshot creates a new state from a given trie with the specified [snap] | ||
| // If [snap] doesn't have the same root as [root], then NewWithSnapshot will return | ||
| // an error. | ||
| func NewWithSnapshot(root common.Hash, db Database, snaps SnapshotTree, snap snapshot.Snapshot) (*StateDB, error) { |
There was a problem hiding this comment.
I think we can either unexport or remove this. It seems we use this only here and we already check if the snap == nil we can also check if root != snap.Root().
There was a problem hiding this comment.
I'm not sure if this is possible due to differences in nil, but definitely if possible I agree to not add it.
| @@ -285,7 +300,7 @@ func (s *StateDB) GetBalance(addr common.Address) *big.Int { | |||
| if stateObject != nil { | |||
| return stateObject.Balance() | |||
| } | |||
| return common.Big0 | |||
| return new(big.Int).Set(common.Big0) | |||
There was a problem hiding this comment.
I wonder where/if we violate this. This is probably safer but again we should not ever modify returned bigint here.
There was a problem hiding this comment.
seems some diff we have from upstream for a long time, didn't notice anything failing without this patch
| if acc == nil { | ||
| if len(acc) == 0 { | ||
| return nil | ||
| } | ||
| data = &types.StateAccount{ | ||
| Nonce: acc.Nonce, | ||
| Balance: acc.Balance, | ||
| CodeHash: acc.CodeHash, | ||
| Root: common.BytesToHash(acc.Root), | ||
| data, err = types.FullAccount(acc) | ||
| if err != nil { | ||
| s.setError(fmt.Errorf("could not unmarshal account (%x) error: %w", addr.Bytes(), err)) | ||
| return nil | ||
| } |
There was a problem hiding this comment.
aren't these (before/after changes) doing essentialy the same thing? If we use FullAccount we don't need below data.CodeHash/data.Root checks, but again it looks like we don't need these changes at all.
There was a problem hiding this comment.
I use the rlp serialization here to avoid having to explicitly set the IsMulticoin field
| @@ -1274,7 +1289,7 @@ func (s *StateDB) Commit(block uint64, deleteEmptyObjects bool) (common.Hash, er | |||
| if origin == (common.Hash{}) { | |||
| origin = types.EmptyRootHash | |||
| } | |||
| if root != origin { | |||
There was a problem hiding this comment.
Due to some consecutive blocks having the same root, I agree that we can probably hoist this to the caller instead, and probably drop the UpdateSameRoot. (Instead, the caller (blockchain.go) should know how to handle it, not sure that makes it super cleaner)
There was a problem hiding this comment.
I wonder if we move the stateDB interface here from coreth
This pull request introduces several changes to the
core/statepackage, focusing on adding support for multi-coin accounts, enhancing snapshot functionality, and improving error handling and logging. The most significant changes include the addition of multi-coin account support, the introduction of new snapshot interfaces, and various improvements to theStateDBstruct and its methods.Multi-coin Account Support:
IsMultiCoinfield to theDumpAccountstruct and updated relevant functions to handle this new field. (core/state/dump.go) [1] [2] [3]state_object_coreth.goto manage multi-coin balances, includingAddBalanceMultiCoin,SubBalanceMultiCoin, andSetBalanceMultiCoin. (core/state/state_object_coreth.go)Snapshot Enhancements:
SnapshotTreeinterface to manage snapshots and updated theStateDBstruct to use this new interface. (core/state/interfaces.go,core/state/statedb.go) [1] [2] [3]snapshotinterface withAccountIteratorandStorageIteratormethods, and updated theTreestruct to includeUpdateWithBlockHashmethod. (core/state/snapshot/snapshot.go) [1] [2] [3]Error Handling and Logging:
StateDBmethods, such asSubRefund,GetBalance, andgetDeletedStateObject, to provide more informative logs and prevent panics. (core/state/statedb.go) [1] [2] [3]core/state/snapshot/snapshot_test.go) [1] [2] [3] [4]Additional Changes:
emptymethod instateObjectto account for theIsMultiCoinfield. (core/state/state_object.go)Commitmethod inStateDBto include a newCommitWithBlockHashmethod for better snapshot management. (core/state/statedb.go) [1] [2] [3]