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.

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

Use upstream statedb #669

Open
wants to merge 38 commits into
base: libevm
Choose a base branch
from
Open

Use upstream statedb #669

wants to merge 38 commits into from

Conversation

darioush
Copy link
Collaborator

@darioush darioush commented Oct 7, 2024

Uses upstream statedb

  • Uses upstream statedb
  • Passes snapshot as SnapshotTree interface
  • Uses libevm trie prefetcher construct

Import Path Updates:

  • Updated import paths from github.com/ava-labs/coreth/trie and github.com/ava-labs/coreth/triedb to github.com/ava-labs/libevm/trie and github.com/ava-labs/libevm/triedb in several files, deleting those packages.

State Management:

  • Moved GetLogData to the StateDB wrapper struct in core/extstate/statedb.go, in preparation for using upstream StateDB.
  • Modified the commitWithSnap function to handle state commits with snapshot updates in core/blockchain.go.
  • Updated writeBlockAndSetHead and writeBlockWithState functions to include parentRoot as a parameter in core/blockchain.go. [1] [2]
  • Removed unnecessary multicoin code from core/genesis.go. [1]

Error Handling Improvements (already added in master):

  • Added error handling for transaction signing and sending in the TestWaitDeployedCornerCases function in accounts/abi/bind/util_test.go.

@darioush darioush changed the title format: as subnet-evm [wip] changes to use upstream statedb Oct 8, 2024
Base automatically changed from use-libevm to libevm November 6, 2024 16:39
@darioush darioush changed the title [wip] changes to use upstream statedb Use upstream statedb Nov 26, 2024
@darioush darioush marked this pull request as ready for review November 26, 2024 18:37
@darioush darioush requested review from ceyonur and a team as code owners November 26, 2024 18:37
core/chain_makers.go Show resolved Hide resolved
@@ -258,11 +258,6 @@ func (g *Genesis) toBlock(db ethdb.Database, triedb *triedb.Database) *types.Blo
for key, value := range account.Storage {
statedb.SetState(addr, key, value)
}
if account.MCBalance != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

How are we now handling this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We're not, fuji and mainnet genesis don't use it so we will be getting rid of it for better alignment with upstream.

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 also get rid of

  • GenesisMultiCoinBalance in gen_genesis_account and from account.go
  • GetAssetBalance API

s.DumpToCollector(iterativeDump{output}, opts)
}
type (
// XXX: Handling IsMultiCoin / extras in dump
Copy link
Contributor

Choose a reason for hiding this comment

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

What does this comment mean? You use XXX as a reminder, don't you?

Copy link
Collaborator Author

@darioush darioush Nov 27, 2024

Choose a reason for hiding this comment

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

We could add support for properly dumping the extras in libevm or break api compatibility in that we just won't output IsMulticoin as I don't think anyone is really depending on it.
The XXX is here for us to make a decision

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 we can get rid of it.

Copy link
Contributor

@ARR4N ARR4N Nov 28, 2024

Choose a reason for hiding this comment

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

I'm happy either way and already have it in ava-labs/libevm#49 pending what you guys decide.

core/state/snapshot/conversion.go Show resolved Hide resolved
core/state/snapshot/snapshot.go Outdated Show resolved Hide resolved
@@ -515,15 +410,16 @@ func (test *snapshotTest) checkEqual(state, checkstate *StateDB) error {
checkeq("GetCode", state.GetCode(addr), checkstate.GetCode(addr))
checkeq("GetCodeHash", state.GetCodeHash(addr), checkstate.GetCodeHash(addr))
checkeq("GetCodeSize", state.GetCodeSize(addr), checkstate.GetCodeSize(addr))
// XXX: Can this be restored?
Copy link
Contributor

Choose a reason for hiding this comment

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

When and why was it commented out?

Copy link
Collaborator Author

@darioush darioush Nov 27, 2024

Choose a reason for hiding this comment

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

It's commented out since the forEachStorage requires iterating on storage and we don't have that ability over the exported interfaces as far as I can tell.

@@ -1170,12 +838,16 @@ func TestGenerateMultiCoinAccounts(t *testing.T) {

// Get latest snapshot and make sure it has the correct account and storage
snap := snaps.Snapshot(root)
snapAccount, err := snap.Account(addrHash)
snapAccount, err := snap.AccountRLP(addrHash)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why decode the RLP manually instead of just using Account()?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Since there are no accessors for Extra on the SlimAccount, the only way to access it is to get the RLP and convert it to a full account (StateAccount) which we can call types.IsMuliCoin on

Copy link
Contributor

Choose a reason for hiding this comment

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

ava-labs/libevm#79 adds the accessor to SlimAccount.

go.mod Show resolved Hide resolved
scripts/eth-allowed-packages.txt Outdated Show resolved Hide resolved
triedb/hashdb/database.go Show resolved Hide resolved
Copy link
Collaborator

@ceyonur ceyonur left a comment

Choose a reason for hiding this comment

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

Overall lgtm, I think we can get rid of few more multicoinbalance stuff.

s.DumpToCollector(iterativeDump{output}, opts)
}
type (
// XXX: Handling IsMultiCoin / extras in dump
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 we can get rid of it.

core/types/state_account.go Show resolved Hide resolved
@@ -258,11 +258,6 @@ func (g *Genesis) toBlock(db ethdb.Database, triedb *triedb.Database) *types.Blo
for key, value := range account.Storage {
statedb.SetState(addr, key, value)
}
if account.MCBalance != nil {
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 also get rid of

  • GenesisMultiCoinBalance in gen_genesis_account and from account.go
  • GetAssetBalance API

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants