Skip to content
This repository has been archived by the owner on Apr 4, 2024. It is now read-only.

Follow up tracking issue on deep context stack efficiency #626

Closed
yihuang opened this issue Oct 6, 2021 · 1 comment · Fixed by #627
Closed

Follow up tracking issue on deep context stack efficiency #626

yihuang opened this issue Oct 6, 2021 · 1 comment · Fixed by #627

Comments

@yihuang
Copy link
Contributor

yihuang commented Oct 6, 2021

The change here doesn't provide context on the following questions:

  • How does flattening improve efficiency? Do you have benchmark results?
  • Is it just to commit changes and avoid the infinite loop?
  • What's the threshold (i.e max number) of internal calls that the EVM supports without crashing?
  • Can we add a test to check the max number of internal calls supported?
  • Do we still need to call the CommitCachedContexts() if the changes are being committed regardless if it fails or not? Can this logic commit an invalid result or affect the revert logic as well?

Since the underlying issue is from the SDK, you should open an issue there to get it fixed in the long term

Originally posted by @fedekunze in #617 (comment)

yihuang added a commit to yihuang/ethermint that referenced this issue Oct 6, 2021
Closes: evmos#626

Solution:
- add a benchmark to demonstrate an extremely inefficiency in deep
  context stack
@yihuang
Copy link
Contributor Author

yihuang commented Oct 6, 2021

The change here doesn't provide context on the following questions:

  • How does flattening improve efficiency? Do you have benchmark results?

benchmark was done in the PR

  • Is it just to commit changes and avoid the infinite loop?

not an infinite loop, just a steep computational complexity curve.

  • What's the threshold (i.e max number) of internal calls that the EVM supports without crashing?

it's demonstrated in the benchmark, 13 layers will take seconds to simply create an iterator.

  • Can we add a test to check the max number of internal calls supported?

the benchmark should have show this.

  • Do we still need to call the CommitCachedContexts() if the changes are being committed regardless if it fails or not? Can this logic commit an invalid result or affect the revert logic as well?

CommitToRevision doesn't commit all the cache contexts, only the ones after the target revision. And after the EVM contract is executed, we don't need those intermediate contexts anymore.

Since the underlying issue is from the SDK, you should open an issue there to get it fixed in the long term

cosmos/cosmos-sdk#10310

Originally posted by @fedekunze in #617 (comment)

fedekunze added a commit that referenced this issue Oct 6, 2021
* Problem: deep context stack efficienty is not benchmarked

Closes: #626

Solution:
- add a benchmark to demonstrate an extremely inefficiency in deep
  context stack

* Update x/evm/keeper/benchmark_test.go

* prefix storage is irrelevant

* add comment to state_transition.go

Co-authored-by: Federico Kunze Küllmer <[email protected]>
fedekunze added a commit that referenced this issue Oct 7, 2021
* Problem: deep context stack efficienty is not benchmarked

Closes: #626

Solution:
- add a benchmark to demonstrate an extremely inefficiency in deep
  context stack

* Update x/evm/keeper/benchmark_test.go

* prefix storage is irrelevant

* add comment to state_transition.go

Co-authored-by: Federico Kunze Küllmer <[email protected]>
fedekunze added a commit that referenced this issue Oct 7, 2021
* docs: v0.6.0 changelog (#605) (#606)

* docs: v0.6.0 changelog

* update codeowners

* build(deps): bump github.com/cosmos/cosmos-sdk from 0.44.0 to 0.44.1 (#610)

* build(deps): bump github.com/cosmos/cosmos-sdk from 0.44.0 to 0.44.1

Bumps [github.com/cosmos/cosmos-sdk](https:/cosmos/cosmos-sdk) from 0.44.0 to 0.44.1.
- [Release notes](https:/cosmos/cosmos-sdk/releases)
- [Changelog](https:/cosmos/cosmos-sdk/blob/v0.44.1/CHANGELOG.md)
- [Commits](cosmos/cosmos-sdk@v0.44.0...v0.44.1)

---
updated-dependencies:
- dependency-name: github.com/cosmos/cosmos-sdk
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <[email protected]>

* changelog

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Federico Kunze <[email protected]>

* cmd: use config on genaccounts (#483)

* cmd: use config on genaccounts

* update

* c++

* rpc: fix panic (#611)

* rpc: fix panic

* fix

* c++

* rpc: restructure JSON-RPC directory and rename server config (#612)

* Restructure ethermint/rpc repo structure and change import statements

* Add #400 to changelog

* fix filepath in util and json_rpc

* Move #400  to unreleased section

* evm: refactor `traceTx` (#613)

* DNM: debug traceTx

* c++

* deps: bump IBC-go (#621)

* deps: bump IBC-go

* changelog

* evm, rpc:  fix tx log attribute value is not parsable by some client (#615)

* Problem: tx log attribute value not parsable by some client

Closes: #614

Solution:
- encode the value to json string rather than bytes

Apply suggestions from code review

* rm cdc and changelog

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

* evm: fix iterator on deeply nested cache contexts (#617)

* Problem: iterator on deeply nested cache contexts is extremely slow

Closes: #616

Solution:
- flatten cache contexts before doing `GetTxLogsTransient`

* Update x/evm/keeper/context_stack.go

* changelog

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

* evm: add benchmark for deep context stack (#627)

* Problem: deep context stack efficienty is not benchmarked

Closes: #626

Solution:
- add a benchmark to demonstrate an extremely inefficiency in deep
  context stack

* Update x/evm/keeper/benchmark_test.go

* prefix storage is irrelevant

* add comment to state_transition.go

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

* rpc: support personal apis with different keyring backends (#591)

* UPDATE Unlock keyring on start

* ADD comment

* ADD validation

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

* conflicts

* changelog

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Daniel Burckhardt <[email protected]>
Co-authored-by: yihuang <[email protected]>
Co-authored-by: davcrypto <[email protected]>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant