Problem: contract call which does lots of message calls is slow to execute #729

yihuang · 2021-11-08T11:52:43Z

This PR is huge, added some guide for review in the comment section below.

Description

Closes: #710

As profiled in the issue page, the bottleneck is the slowness of deep context stack, there's no easy way around it other than refactor the StateDB to use journal logs like what go-ethereum does, thus this big code refactoring.

What this PR does

Implement a standalone statedb module in x/evm/statedb which implements vm.StateDB interface with the support of several keeper methods, the keeper interface is captured in statedb.Keeper.
The StateDB implementation keep all the dirty states in memory, at the end of tx execution, commit all the dirty changes to keeper at once
record all the modify operations in a list of journal logs, snapshot revert is done by undo part of the logs.
x/evm/statedb/{journal.go, access_list.go, state_object.go} are directly adapted from go-ethereum itself.

Benchmark result

BenchmarkMessageCall (improved 310X)

Before

BenchmarkMessageCall-16    	       1	25846269634 ns/op
BenchmarkMessageCall-16    	       1	26773165908 ns/op
BenchmarkMessageCall-16    	       1	25786298762 ns/op

After

BenchmarkMessageCall-16    	      12	  91367037 ns/op
BenchmarkMessageCall-16    	      13	  82284757 ns/op
BenchmarkMessageCall-16    	      14	  83061467 ns/op

BenchmarkEmitLogs (improved 2.5X)

Before

BenchmarkEmitLogs-16    	      98	  12034965 ns/op
BenchmarkEmitLogs-16    	      98	  12146658 ns/op
BenchmarkEmitLogs-16    	      96	  12313053 ns/op

After

BenchmarkEmitLogs-16    	     225	   5288751 ns/op
BenchmarkEmitLogs-16    	     232	   5189175 ns/op
BenchmarkEmitLogs-16    	     222	   5191943 ns/op

For contributor use:

Targeted PR against correct branch (see CONTRIBUTING.md)
Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
Code follows the module structure standards.
Wrote unit and integration tests
Updated relevant documentation (docs/) or specification (x/<module>/spec/)
Added relevant godoc comments.
Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
Re-reviewed Files changed in the Github PR explorer

For admin use:

Added appropriate labels to PR (ex. WIP, R4R, docs, etc)
Reviewers assigned
Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)

codecov · 2021-11-08T12:05:00Z

Codecov Report

Merging #729 (02e701a) into main (03768c2) will decrease coverage by 1.04%.
The diff coverage is 66.96%.

❗ Current head 02e701a differs from pull request most recent head 33d9d29. Consider uploading reports for the commit 33d9d29 to get more accurate results

@@            Coverage Diff             @@
##             main     #729      +/-   ##
==========================================
- Coverage   57.79%   56.74%   -1.05%     
==========================================
  Files          74       78       +4     
  Lines        6205     6238      +33     
==========================================
- Hits         3586     3540      -46     
- Misses       2408     2499      +91     
+ Partials      211      199      -12

Impacted Files	Coverage Δ
x/evm/keeper/abci.go	`20.00% <0.00%> (-1.43%)`	⬇️
x/evm/types/key.go	`0.00% <ø> (ø)`
x/evm/statedb/access_list.go	`5.79% <5.79%> (ø)`
x/evm/genesis.go	`53.57% <50.00%> (+1.90%)`	⬆️
x/evm/statedb/config.go	`50.00% <50.00%> (ø)`
app/ante/eth.go	`81.23% <53.84%> (-1.87%)`	⬇️
x/evm/statedb/statedb.go	`67.64% <67.64%> (ø)`
x/evm/statedb/journal.go	`71.11% <71.11%> (ø)`
x/evm/keeper/statedb.go	`77.39% <73.33%> (+4.19%)`	⬆️
x/evm/keeper/grpc_query.go	`66.66% <73.58%> (-1.22%)`	⬇️
... and 11 more

odeke-em · 2021-11-08T12:19:13Z

@yihuang could I kindly ask if we've considered that things might be much faster using smt inside of cosmos-sdk instead of cosmos/iavl?

yihuang · 2021-11-08T12:22:49Z

@yihuang could I kindly ask if we've considered that things might be much faster using smt inside of cosmos-sdk instead of cosmos/iavl?

Does that mean we need to reimplement state-sync too?

tomtau · 2021-11-09T02:56:43Z

@yihuang could I kindly ask if we've considered that things might be much faster using smt inside of cosmos-sdk instead of cosmos/iavl?

@odeke-em SMT is orthogonal to it AFAIK: one of the core issues here is e.g. nested cache contexts (cosmos/cosmos-sdk#10310) #710 (comment)

tomtau · 2021-11-16T08:20:37Z

the importer test runs faster now:

processed block: 97000 (time so far: 3m3.791587401s)
--- PASS: TestImporterTestSuite (186.34s)
    --- PASS: TestImporterTestSuite/TestImportBlocks (186.34s)

vs on main:

processed block: 97000 (time so far: 6m53.091837321s)
--- PASS: TestImporterTestSuite (416.45s)
    --- PASS: TestImporterTestSuite/TestImportBlocks (416.45s)

yihuang · 2021-11-17T02:21:45Z

Review Guide

Abstract

The problem:

Deep context stack is slow to do commit, and it's hard to work around this issue.
Have to call WithContext on EvmKeeper is error-prone.

The basic idea is to adopt the design in go-ethereum:

Cache the intermediate states during contract execution in memory, flush the dirty states to storage at the end of tx execution.
Move the vm.StateDB implementation out of EvmKeeper, then the EvmKeeper become an idiomatic keeper, mostly providing stateless operations over the storage, the ctx parameter is passed around explicitly.
snapshot and revert is done by recording and undoing the journal logs.

`x/evm/statedb` package

All the statedb logic is implemented in this package, the access to underlying storage is provided by statedb.Keeper interface, which is implemented by EvmKeeper.

Most of the StateDB implementations are directly adapted from go-ethereum codebase itself.

Transient Storage Simplification

The transient storage is greatly simplified since many temporary states are moved to memory, the left ones are:

KeyPrefixTransientBloom store current block bloom
KeyPrefixTransientTxIndex store current tx index
KeyPrefixTransientLogSize store current log index

Persistent Storage

It doesn't change the format of persistent storage, so no migration is needed when upgrading. But because of different storage writing orders, the app hash would be different.

Unit Test Changes

They are all mechanically changes to adapt to the new API, the test logic didn't change.

app/ante/eth.go

tomtau

thanks for the review guide! I think moving out statedb makes sense. At least removing the need for the WithContext calls makes it a bit less error-prone.
I left a few comments, but it's mostly random parts I came across and they were probably there before:

For the upgrade migrations, it seems some of the storage keys aren't used anymore -- should that be cleaned up in migrations?

In any case, at least from the importer test, this looks like a big win. It'd need a bit more testing (especially on the web3 RPC API) and pairs of eyes to look at this PR. But overall, it looks promising (GPL v3 re-licensing is a side-issue and there are already some code portions from go-ethereum before in the current codebase, e.g.: https://github.com/tharsis/ethermint/blob/main/x/evm/types/tx_args.go#L19 so Ethermint should be re-licensed anyway)

app/ante/eth.go

x/evm/handler_test.go

x/evm/keeper/grpc_query.go

x/evm/statedb/access_list.go

x/evm/statedb/journal.go

x/evm/statedb/statedb.go

x/evm/keeper/keeper.go

yihuang · 2021-11-19T02:16:21Z

For the upgrade migrations, it seems some of the storage keys aren't used anymore -- should that be cleaned up in migrations?

I think the removed keys are all transient storage, so no migration is needed.

app/ante/eth.go

fedekunze

First pass

x/evm/genesis.go

app/ante/utils_test.go

x/evm/handler_test.go

x/evm/keeper/grpc_query.go

app/ante/eth.go

fedekunze

Second pass

app/ante/eth.go

fedekunze · 2022-01-04T13:17:35Z

app/ante/eth.go

 		}

-		if err := evmkeeper.CheckSenderBalance(ctx, avd.bankKeeper, from, txData, evmDenom); err != nil {
+		if err := evmkeeper.CheckSenderBalance(sdk.NewIntFromBigInt(acct.Balance), txData); err != nil {


check if acct.Balance is not nil

sdk.NewIntFromBigInt treat nil as an empty int, so looks ok?

fedekunze · 2022-01-04T13:18:43Z

x/evm/keeper/grpc_query.go

-		CodeHash: k.GetCodeHash(addr).Hex(),
-		Nonce:    k.GetNonce(addr),
+		Balance:  acct.Balance.String(),
+		CodeHash: hexutil.Encode(acct.CodeHash),


use common library instead

fedekunze · 2022-01-04T13:23:20Z

x/evm/statedb/access_list.go

+// operations.
+func (al *accessList) DeleteSlot(address common.Address, slot common.Hash) {
+	idx, addrOk := al.addresses[address]
+	// There are two ways this can fail


There are two ways this can fail

... which ones? can you add a comment

actually, I'm not sure, it's blindly copied from go-ethereum, by reading the code, it seems impossible to fail here, probably the comment is out of date here. let me remove it.

x/evm/statedb/journal.go

fedekunze

third pass, would be great to add tests for journal

x/evm/statedb/journal.go

x/evm/statedb/state_object.go

fedekunze · 2022-01-04T14:31:58Z

x/evm/statedb/state_object.go

+	// DB error.
+	// State objects are used by the consensus core and VM which are
+	// unable to deal with database-level errors. Any error that occurs
+	// during a database read is memoized here and will eventually be returned
+	// by StateDB.Commit.


no dbErr?

removed, since our account keeper API doesn't return database errors, so the field is useless here.

x/evm/statedb/state_object.go

fedekunze · 2022-01-04T14:34:50Z

x/evm/statedb/statedb_test.go

+	return nil
+}
+
+func TestAccounts(t *testing.T) {


create a suite for this?

fedekunze · 2022-01-04T14:35:00Z

x/evm/statedb/statedb_test.go

+		{
+			"success,empty account",
+			func(t *testing.T, db *statedb.StateDB) {
+				require.Equal(t, true, db.Empty(addr2))


use suite.Require()

fedekunze · 2022-01-04T14:35:27Z

x/evm/statedb/statedb_test.go

+
+var _ statedb.Keeper = &MockKeeper{}
+
+type MockKeeper struct {


move to mock_test.go

fedekunze · 2022-01-04T14:35:55Z

x/evm/statedb/statedb_test.go

+				require.Equal(t, big.NewInt(1), db.GetBalance(addr2))
+			},
+		},
+		// TODO access list, ForEachStorage


address this TODO or create issue for follow up

linked the #876 here.

fedekunze

Final rounds of comments. Mostly adding more code and godoc comments.
Can you also add a state machine breaking change changelog?
Also can you open a PR to update the spec with the new design?

x/evm/statedb/statedb.go

x/evm/keeper/keeper.go

unit tests unit tests keeper implementation extract TxConfig remove unused code

Co-authored-by: Federico Kunze Küllmer <[email protected]>

fedekunze

ACK! 💯 @yihuang can you add the relevant changelog entries?

yihuang · 2022-01-05T07:27:38Z

Final rounds of comments. Mostly adding more code and godoc comments.

for the unit tests and coding style of statedb package, I suggest postponing that to another PR (#876), because:

this PR is big enough
there's are several modules under x/evm/statedb that are copied from go-ethereum, the diffs are small, it's easy to track what have we changed on top of go-ethereum version.

Can you also add a state machine breaking change changelog?

done

Also can you open a PR to update the spec with the new design?

#879

This reverts commit ade8431.

yihuang requested review from fedekunze, jolube and khoslaventures as code owners November 8, 2021 11:52

yihuang marked this pull request as draft November 8, 2021 11:52

yihuang force-pushed the statedb-refactor branch from 03e0e97 to f680d4a Compare November 8, 2021 12:00

yihuang force-pushed the statedb-refactor branch 4 times, most recently from 99f643c to 69d4309 Compare November 15, 2021 10:46

yihuang marked this pull request as ready for review November 16, 2021 01:59

tomtau requested review from JayT106, crypto-facs, leejw51crypto and thomas-nguy November 16, 2021 08:15

yihuang mentioned this pull request Nov 16, 2021

Problem: EVM keeper keeps intermediate states inside #663

Closed

11 tasks

yihuang commented Nov 17, 2021

View reviewed changes

app/ante/eth.go Outdated Show resolved Hide resolved

tomtau reviewed Nov 17, 2021

View reviewed changes

yihuang force-pushed the statedb-refactor branch from 660903c to 521369b Compare November 17, 2021 10:33

JayT106 reviewed Nov 19, 2021

View reviewed changes

x/evm/keeper/keeper.go Outdated Show resolved Hide resolved

yihuang force-pushed the statedb-refactor branch from 521369b to b07b2cc Compare November 19, 2021 05:23

leejw51crypto reviewed Nov 30, 2021

View reviewed changes

app/ante/eth.go Show resolved Hide resolved

leejw51crypto reviewed Nov 30, 2021

View reviewed changes

app/ante/eth.go Show resolved Hide resolved

leejw51crypto reviewed Nov 30, 2021

View reviewed changes

app/ante/eth.go Show resolved Hide resolved

yihuang mentioned this pull request Dec 31, 2021

a contract suicide test case fail on ethermint #864

Closed

yihuang force-pushed the statedb-refactor branch from c68fa89 to 42fb5e0 Compare January 4, 2022 04:18

yihuang mentioned this pull request Jan 4, 2022

Problem: recent ethermint and cosmos-sdk fixes not included crypto-org-chain/cronos#289

Merged

13 tasks

fedekunze reviewed Jan 4, 2022

View reviewed changes

yihuang force-pushed the statedb-refactor branch 2 times, most recently from ef58960 to b9168a5 Compare January 4, 2022 13:09

fedekunze reviewed Jan 4, 2022

View reviewed changes

yihuang force-pushed the statedb-refactor branch from fce1c53 to d229bb4 Compare January 4, 2022 14:18

fedekunze reviewed Jan 4, 2022

View reviewed changes

yihuang force-pushed the statedb-refactor branch from d229bb4 to ed0daa2 Compare January 4, 2022 17:45

fedekunze reviewed Jan 4, 2022

View reviewed changes

yihuang force-pushed the statedb-refactor branch 3 times, most recently from 976e39a to 3c26f7f Compare January 5, 2022 03:42

yihuang and others added 4 commits January 5, 2022 12:03

initial statedb module

6217c25

unit tests unit tests keeper implementation extract TxConfig remove unused code

keeper integration

2f05748

fix unit tests

db4d41f

Apply suggestions from code review

646537b

Co-authored-by: Federico Kunze Küllmer <[email protected]>

yihuang force-pushed the statedb-refactor branch from 3c26f7f to 646537b Compare January 5, 2022 04:05

fixup! initial statedb module

a129af2

yihuang force-pushed the statedb-refactor branch from ff0d692 to a129af2 Compare January 5, 2022 04:25

fedekunze approved these changes Jan 5, 2022

View reviewed changes

yihuang and others added 2 commits January 5, 2022 15:24

changelog

02e701a

Merge branch 'main' into statedb-refactor

33d9d29

fedekunze merged commit ade8431 into evmos:main Jan 5, 2022

yihuang deleted the statedb-refactor branch January 5, 2022 07:33

allthatjazzleo mentioned this pull request Jan 14, 2022

Problem: cronosd high disk io consumption crypto-org-chain/cronos#291

Closed

mmsqe added a commit to mmsqe/ethermint that referenced this pull request Dec 21, 2022

Revert "evm: refactor statedb implementation (evmos#729)"

73041c1

This reverts commit ade8431.

mmsqe added a commit to mmsqe/ethermint that referenced this pull request Dec 26, 2022

Revert "evm: refactor statedb implementation (evmos#729)"

eb8a7b8

This reverts commit ade8431.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Problem: contract call which does lots of message calls is slow to execute #729

Problem: contract call which does lots of message calls is slow to execute #729

yihuang commented Nov 8, 2021 •

edited

Loading

codecov bot commented Nov 8, 2021 •

edited

Loading

odeke-em commented Nov 8, 2021

yihuang commented Nov 8, 2021 •

edited

Loading

tomtau commented Nov 9, 2021

tomtau commented Nov 16, 2021

yihuang commented Nov 17, 2021 •

edited

Loading

tomtau left a comment

yihuang commented Nov 19, 2021 •

edited

Loading

fedekunze left a comment

fedekunze left a comment

fedekunze Jan 4, 2022 •

edited

Loading

yihuang Jan 5, 2022

fedekunze Jan 4, 2022

yihuang Jan 5, 2022

fedekunze Jan 4, 2022

yihuang Jan 5, 2022 •

edited

Loading

fedekunze left a comment

fedekunze Jan 4, 2022

yihuang Jan 5, 2022

fedekunze Jan 4, 2022

yihuang Jan 5, 2022

fedekunze Jan 4, 2022

yihuang Jan 5, 2022

yihuang Jan 5, 2022

fedekunze Jan 4, 2022

yihuang Jan 5, 2022

fedekunze Jan 4, 2022

yihuang Jan 5, 2022

fedekunze left a comment

fedekunze left a comment

yihuang commented Jan 5, 2022


		var _ statedb.Keeper = &MockKeeper{}

		type MockKeeper struct {

Problem: contract call which does lots of message calls is slow to execute #729

Problem: contract call which does lots of message calls is slow to execute #729

Conversation

yihuang commented Nov 8, 2021 • edited Loading

Description

What this PR does

Benchmark result

BenchmarkMessageCall (improved 310X)

Before

After

BenchmarkEmitLogs (improved 2.5X)

Before

After

codecov bot commented Nov 8, 2021 • edited Loading

Codecov Report

odeke-em commented Nov 8, 2021

yihuang commented Nov 8, 2021 • edited Loading

tomtau commented Nov 9, 2021

tomtau commented Nov 16, 2021

yihuang commented Nov 17, 2021 • edited Loading

Review Guide

Abstract

x/evm/statedb package

Transient Storage Simplification

Persistent Storage

Unit Test Changes

tomtau left a comment

Choose a reason for hiding this comment

yihuang commented Nov 19, 2021 • edited Loading

fedekunze left a comment

Choose a reason for hiding this comment

fedekunze left a comment

Choose a reason for hiding this comment

fedekunze Jan 4, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yihuang Jan 5, 2022 • edited Loading

Choose a reason for hiding this comment

fedekunze left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fedekunze left a comment

Choose a reason for hiding this comment

fedekunze left a comment

Choose a reason for hiding this comment

yihuang commented Jan 5, 2022

yihuang commented Nov 8, 2021 •

edited

Loading

codecov bot commented Nov 8, 2021 •

edited

Loading

yihuang commented Nov 8, 2021 •

edited

Loading

yihuang commented Nov 17, 2021 •

edited

Loading

`x/evm/statedb` package

yihuang commented Nov 19, 2021 •

edited

Loading

fedekunze Jan 4, 2022 •

edited

Loading

yihuang Jan 5, 2022 •

edited

Loading