Problem: iterator on deeply nested cache contexts is extremely slow

[yihuang]

Closes: #616

Solution:

• flatten cache contexts before doing GetTxLogsTransient

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)

[codecov]

Codecov Report

Merging #617 (c96abc2) into main (cda968b) will decrease coverage by 0.04%.
The diff coverage is 37.50%.

❗ Current head c96abc2 differs from pull request most recent head d6e6d38. Consider uploading reports for the commit d6e6d38 to get more accurate results

@@            Coverage Diff             @@
##             main     #617      +/-   ##
==========================================
- Coverage   51.76%   51.72%   -0.05%     
==========================================
  Files          65       65              
  Lines        5461     5477      +16     
==========================================
+ Hits         2827     2833       +6     
- Misses       2474     2482       +8     
- Partials      160      162       +2     

Impacted Files	Coverage Δ
x/evm/keeper/context_stack.go	66.66% <28.57%> (-14.42%)	⬇️
x/evm/keeper/state_transition.go	68.60% <100.00%> (+0.20%)	⬆️

[fedekunze]

Please provide more context for this change (comments, docs, etc). This PR should also include a benchmark test to check the effect of this change with regard to performance.

[fedekunze]

please add a unit test for this function

[fedekunze]

how does this relate to k.CommitCachedContexts()? why is this called here instead of inside the !res.Failed condition? I need more context to understand the rationale of this change

[thomas-nguy]

For more context, we discovered a bug in our testnet which make the network to halt.

Some uniswap call triggers a lot of internal call. And trying to replay the tx will end up in an infinite loop there
https://github.com/tharsis/ethermint/blob/main/x/evm/keeper/keeper.go#L181
https://github.com/cosmos/cosmos-sdk/blob/master/store/cachekv/mergeiterator.go#L206

[fedekunze]

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

[fedekunze]

Please add relevant comments and tests based on the questions above

[leejw51crypto]

Keeper, ApplyTransaction,
logs := k.GetTxLogsTransient(txHash)

iter := store.Iterator(txHash.Bytes(), end) <- exact point
defer iter.Close()

blocking point
it hangs forever here,

// Commit commits all the cached contexts from top to bottom in order and clears the stack by setting an empty slice of cache contexts.
func (cs *ContextStack) Commit() {
	// commit in order from top to bottom
	for i := len(cs.cachedContexts) - 1; i >= 0; i-- {
		// keep all the cosmos events
		cs.initialCtx.EventManager().EmitEvents(cs.cachedContexts[i].ctx.EventManager().Events())
		if cs.cachedContexts[i].commit == nil {
			panic(fmt.Sprintf("commit function at index %d should not be nil", i))
		} else {
			cs.cachedContexts[i].commit()
		}
	}
	cs.cachedContexts = []cachedContext{}
}

Commit is recursive function

[leejw51crypto]

So in this pr, before calling Commit, calls cached contexts Commit first linearly.

[fedekunze]

@yihuang @thomas-nguy I think this code is not very clear on the fix and the bug itself and seems more like a short-term patch. The issue should ultimately be resolved on the SDK. As for this repo, we should consider using other longer-term alternatives that can address these issues.

I'm happy to merge it but you should address these on your comments or follow up issues and PRs

[fedekunze]

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

[fedekunze]

Please add relevant comments and tests based on the questions above

[fedekunze]

@yihuang please open an issue and follow up on the questions from above

[fedekunze]

@yihuang can you fix the tests?