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 tendermint store to get Tx hashes instead of storing explicitly #1913

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

codchen
Copy link
Collaborator

@codchen codchen commented Oct 30, 2024

Describe your changes and provide context

EVM Tx hashes are used for certain EVM RPC queries. Storing EVM Tx hashes explicitly as app state has been causing state bloat. This information is actually already stored in Tendermint block store. This PR changes relevant RPC endpoints to read from Tendermint instead of hashes in app state. Once we are confident that the change is not interruptive to current use cases out there, we can remove the tx hash writing logic and delete the app state.

Testing performed to validate your change

existing unit test should still work

Copy link
Contributor

@philipsu522 philipsu522 left a comment

Choose a reason for hiding this comment

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

what are some rpc calls that test this?

  1. a little worried about performance impact
  2. i'd also like to vierfy it on a loadtest cluster later today

evmrpc/block.go Outdated
Comment on lines 158 to 174
for i, tx := range block.Block.Data.Txs {
sdkTx, err := a.txConfig.TxDecoder()(tx)
if err != nil {
fmt.Printf("error decoding tx %d in block %d, skipping\n", i, height)
continue
}
if len(sdkTx.GetMsgs()) == 0 {
continue
}
if evmTx, ok := sdkTx.GetMsgs()[0].(*types.MsgEVMTransaction); ok {
if evmTx.IsAssociateTx() {
continue
}
ethtx, _ := evmTx.AsTransaction()
txHashes = append(txHashes, ethtx.Hash())
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

nit - shoudl we consider moving this to a helper function so it can be reused in filter.go?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

updated

@philipsu522 philipsu522 enabled auto-merge (squash) October 30, 2024 16:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants