fix(rpc): cache back access list within same block #1585

mmsqe · 2023-01-04T15:37:07Z

Closes: #1583

Description

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)

VictorTrustyDev · 2023-01-04T15:56:32Z

Hi @mmsqe, so what is the root cause?

mmsqe · 2023-01-04T16:03:19Z

Hi @mmsqe, so what is the root cause?

some old tnx make use of cached warmAccess within same block before this fix which use less gas, might need a config for patch height here.

VictorTrustyDev · 2023-01-04T16:30:55Z

@mmsqe I see, totally understood, ty for your detailed explanation

codecov · 2023-01-05T02:08:40Z

Codecov Report

Merging #1585 (2f54334) into main (c5fdad7) will increase coverage by 0.04%.
The diff coverage is 86.74%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1585      +/-   ##
==========================================
+ Coverage   68.35%   68.40%   +0.04%     
==========================================
  Files         110      110              
  Lines       10181    10225      +44     
==========================================
+ Hits         6959     6994      +35     
- Misses       2819     2827       +8     
- Partials      403      404       +1

Impacted Files	Coverage Δ
server/config/config.go	`22.72% <0.00%> (-0.13%)`	⬇️
x/evm/statedb/statedb.go	`97.66% <0.00%> (-1.55%)`	⬇️
x/evm/keeper/grpc_query.go	`86.03% <89.47%> (+0.13%)`	⬆️
rpc/backend/tracing.go	`64.22% <100.00%> (+0.59%)`	⬆️
x/evm/keeper/state_transition.go	`78.62% <100.00%> (+0.33%)`	⬆️

adisaran64

Hey @mmsqe ! Left some comments, was curious about your approach to solving the problem described in the ticket. There are also several CI issues, most importantly that many of the integration tests are failing (most likely because of your changes in state_transition.go and grpc_query.go?) It also might be a good idea to add your own tests for this functionality. This PR probably belongs in draft mode for now.

adisaran64 · 2023-01-06T19:10:20Z

proto/ethermint/evm/v1/query.proto

@@ -255,6 +255,9 @@ message QueryTraceTxRequest {
 bytes proposer_address = 8 [(gogoproto.casttype) = "github.com/cosmos/cosmos-sdk/types.ConsAddress"];
 // chain_id is the the eip155 chain id parsed from the requested block header
 int64 chain_id = 9;
+ // fix_clear_access_list_height defines the upgrade height for fix clear access list before processing each
+ // transaction
+ int64 fix_clear_access_list_height = 10;


Why does this constant have to be included in the .proto file for this request? Why can't it be included in the code elsewhere?

adisaran64 · 2023-01-06T19:10:23Z

proto/ethermint/evm/v1/query.proto

@@ -279,6 +282,9 @@ message QueryTraceBlockRequest {
 bytes proposer_address = 8 [(gogoproto.casttype) = "github.com/cosmos/cosmos-sdk/types.ConsAddress"];
 // chain_id is the eip155 chain id parsed from the requested block header
 int64 chain_id = 9;
+ // fix_clear_access_list_height defines the upgrade height for fix clear access list before processing each
+ // transaction
+ int64 fix_clear_access_list_height = 10;


Same question as above.

adisaran64 · 2023-01-06T19:10:28Z

x/evm/keeper/grpc_query.go

- signer := ethtypes.MakeSigner(cfg.ChainConfig, big.NewInt(ctx.BlockHeight()))
-
+ height := ctx.BlockHeight()
+ patch := height < req.FixClearAccessListHeight


Here, I see that you are accessing the FixClearAccessListHeight from the request itself. Similar to the question above: why does the constant have to come from the request? Why can't it be stored elsewhere, ex. in the code for the evm keeper itself, to avoid changing the proto files and json-rpc configuration overall?

Cut off height config might be different based on chain, and not sure if env variable is better way than JSONRPCConfig for keeper to access?

x/evm/keeper/grpc_query.go

mmsqe · 2023-01-06T20:49:03Z

Hey @mmsqe ! Left some comments, was curious about your approach to solving the problem described in the ticket. There are also several CI issues, most importantly that many of the integration tests are failing (most likely because of your changes in state_transition.go and grpc_query.go?) It also might be a good idea to add your own tests for this functionality. This PR probably belongs in draft mode for now.

Most test fail due to duplicate register proto type when fixing, but I find the upgrade also have issue.

x/evm/keeper/grpc_query.go

- continue
- }
- txConfig.LogIndex += uint(len(rsp.Logs))
+ lastDB, _ = k.GetTxTraceResultForTx(ctx, tx, signer, cfg, txConfig, patch, lastDB)


MalteHerrmann

Thanks for your work on this! Left some more comments

MalteHerrmann · 2023-01-17T06:56:35Z

x/evm/keeper/grpc_query.go

+// GetTxTraceResultForBlock returns TxTraceResult and
+// statedb with cached address list when need patch and


Can you complete the sentence you have started here?

MalteHerrmann · 2023-01-17T07:05:40Z

x/evm/keeper/state_transition.go

+ return k.ApplyMessageWithStateDB(ctx, msg, tracer, commit, cfg, txConfig, nil)
+}
+
+func (k *Keeper) ApplyMessageWithStateDB(ctx sdk.Context,


Please add a docstring here

MalteHerrmann · 2023-01-17T07:07:45Z

x/evm/statedb/statedb.go

+}
+
+// SetAddressToAccessList overwrite with new access list
+func (s *StateDB) SetAddressToAccessList(accessList *accessList) {


IMO including "address" here can be confusing

Suggested change

func (s *StateDB) SetAddressToAccessList(accessList *accessList) {

func (s *StateDB) SetAccessList(accessList *accessList) {

MalteHerrmann · 2023-01-17T07:09:17Z

x/evm/statedb/statedb.go

+// GetAddressToAccessList return full access list
+func (s *StateDB) GetAddressToAccessList() *accessList {


Currently not sure on the implication of the unexported-return warning - this seems like bad practice. Is there maybe a better way to handle this?

Suggested change

// GetAddressToAccessList return full access list

func (s *StateDB) GetAddressToAccessList() *accessList {

// GetAccessList return full access list

func (s *StateDB) GetAccessList() *accessList {

MalteHerrmann · 2023-01-17T07:09:59Z

x/evm/statedb/statedb.go

@@ -396,6 +396,16 @@ func (s *StateDB) AddAddressToAccessList(addr common.Address) {
 }
 }

+// GetAddressToAccessList return full access list
+func (s *StateDB) GetAddressToAccessList() *accessList {
+ return s.accessList


This should be tested, especially with the golangci-lint warning

MalteHerrmann · 2023-01-17T07:42:18Z

x/evm/keeper/grpc_query.go

- continue
- }
- txConfig.LogIndex += uint(len(rsp.Logs))
+ lastDB, _ = k.GetTxTraceResultForTx(ctx, tx, signer, cfg, txConfig, patch, lastDB)


Suggested change

lastDB, _ = k.GetTxTraceResultForTx(ctx, tx, signer, cfg, txConfig, patch, lastDB)

//#nosec G703 -- if an error is returned, just continue with the next iteration

lastDB, _ = k.GetTxTraceResultForTx(ctx, tx, signer, cfg, txConfig, patch, lastDB)

MalteHerrmann · 2023-01-17T07:47:06Z

x/evm/keeper/grpc_query.go

+ if err != nil {
+ result.Error = err.Error()
+ } else {
+ txConfig.LogIndex = logIndex


Why assign txConfig.LogIndex here when it doesn't get returned or used in the rest of the function?

mmsqe · 2023-01-17T07:58:14Z

Thanks for your work on this! Left some more comments

Sorry I forgot to close this PR, since #1603 might be more generic to handle these kind of bug or migrate, thanks for the review.

cache back access list within same block

92ba99f

mmsqe added 2 commits January 5, 2023 09:59

update dep

83fdbe4

configurable fix-clear-access-list-height

78869d4

mmsqe added 2 commits January 5, 2023 10:28

add FixClearAccessListHeight in request

5b427f9

Merge branch 'main' into align_trace_tx

3172072

mmsqe marked this pull request as ready for review January 6, 2023 01:58

mmsqe requested a review from a team as a code owner January 6, 2023 01:58

mmsqe requested review from MalteHerrmann and adisaran64 and removed request for a team January 6, 2023 01:58

patch trace block as well

b8f6834

adisaran64 reviewed Jan 6, 2023

View reviewed changes

mmsqe added 2 commits January 7, 2023 09:37

add get tx trace res func

32705aa

add test

8c6664c

4rgon4ut reviewed Jan 7, 2023

View reviewed changes

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

mmsqe mentioned this pull request Jan 11, 2023

Allow multi gRPC query clients serve with old binary #1601

Closed

Merge branch 'main' into align_trace_tx

d2252b8

github-advanced-security bot found potential problems Jan 12, 2023

View reviewed changes

x/evm/keeper/grpc_query.go

continue

}

txConfig.LogIndex += uint(len(rsp.Logs))

lastDB, _ = k.GetTxTraceResultForTx(ctx, tx, signer, cfg, txConfig, patch, lastDB)

Check warning

Code scanning / gosec

Returned error is not propagated up the stack.

Returned error is not propagated up the stack.

mmsqe added 2 commits January 17, 2023 09:59

Merge branch 'main' into align_trace_tx

cfcde2a

update nix

2f54334

MalteHerrmann reviewed Jan 17, 2023

View reviewed changes

mmsqe closed this Jan 17, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(rpc): cache back access list within same block #1585

fix(rpc): cache back access list within same block #1585

mmsqe commented Jan 4, 2023

VictorTrustyDev commented Jan 4, 2023

mmsqe commented Jan 4, 2023

VictorTrustyDev commented Jan 4, 2023

codecov bot commented Jan 5, 2023 •

edited

Loading

adisaran64 left a comment

adisaran64 Jan 6, 2023

adisaran64 Jan 6, 2023

adisaran64 Jan 6, 2023

mmsqe Jan 6, 2023 •

edited

Loading

mmsqe commented Jan 6, 2023

MalteHerrmann left a comment

MalteHerrmann Jan 17, 2023

MalteHerrmann Jan 17, 2023

MalteHerrmann Jan 17, 2023

MalteHerrmann Jan 17, 2023

MalteHerrmann Jan 17, 2023

MalteHerrmann Jan 17, 2023

MalteHerrmann Jan 17, 2023

mmsqe commented Jan 17, 2023

		// GetTxTraceResultForBlock returns TxTraceResult and
		// statedb with cached address list when need patch and

	func (s StateDB) SetAddressToAccessList(accessList accessList) {
	func (s StateDB) SetAccessList(accessList accessList) {

		// GetAddressToAccessList return full access list
		func (s StateDB) GetAddressToAccessList() accessList {

	lastDB, _ = k.GetTxTraceResultForTx(ctx, tx, signer, cfg, txConfig, patch, lastDB)
	//#nosec G703 -- if an error is returned, just continue with the next iteration
	lastDB, _ = k.GetTxTraceResultForTx(ctx, tx, signer, cfg, txConfig, patch, lastDB)

fix(rpc): cache back access list within same block #1585

fix(rpc): cache back access list within same block #1585

Conversation

mmsqe commented Jan 4, 2023

Description

VictorTrustyDev commented Jan 4, 2023

mmsqe commented Jan 4, 2023

VictorTrustyDev commented Jan 4, 2023

codecov bot commented Jan 5, 2023 • edited Loading

Codecov Report

adisaran64 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

mmsqe Jan 6, 2023 • edited Loading

Choose a reason for hiding this comment

mmsqe commented Jan 6, 2023

MalteHerrmann 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

mmsqe commented Jan 17, 2023

codecov bot commented Jan 5, 2023 •

edited

Loading

mmsqe Jan 6, 2023 •

edited

Loading