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

feat!: Store eth tx index separately #1121

Merged
merged 14 commits into from
Aug 11, 2022
Merged

feat!: Store eth tx index separately #1121

merged 14 commits into from
Aug 11, 2022

Conversation

yihuang
Copy link
Contributor

@yihuang yihuang commented Jun 9, 2022

Closes: #1075

Description

  • run a optional indexer service

  • adapt the json-rpc to the more efficient query

  • a cli command to index block range manually.


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
Copy link

codecov bot commented Jun 9, 2022

Codecov Report

Merging #1121 (d927def) into main (8f932dd) will decrease coverage by 0.14%.
The diff coverage is 48.07%.

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

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1121      +/-   ##
==========================================
- Coverage   61.74%   61.59%   -0.15%     
==========================================
  Files          89       88       -1     
  Lines        7303     7296       -7     
==========================================
- Hits         4509     4494      -15     
- Misses       2570     2581      +11     
+ Partials      224      221       -3     
Impacted Files Coverage Δ
rpc/types/events.go 73.01% <48.00%> (-17.59%) ⬇️
server/config/config.go 21.68% <50.00%> (+0.34%) ⬆️
x/evm/types/params.go 100.00% <0.00%> (ø)
x/evm/types/tx_data.go 97.91% <0.00%> (ø)
x/evm/migrations/v2/migrate.go
x/evm/module.go 53.73% <0.00%> (+0.87%) ⬆️
app/ante/eth.go 82.67% <0.00%> (+2.05%) ⬆️
x/evm/keeper/migrations.go 100.00% <0.00%> (+33.33%) ⬆️

@yihuang yihuang marked this pull request as draft June 9, 2022 10:07
@yihuang yihuang force-pushed the indexer branch 3 times, most recently from 4a0217d to 81ff827 Compare June 10, 2022 09:17
@yihuang yihuang changed the title Store eth tx index separately feat!: Store eth tx index separately Jun 10, 2022
@yihuang yihuang marked this pull request as ready for review June 13, 2022 08:26
rpc/backend/evm_backend.go Outdated Show resolved Hide resolved
server/config/toml.go Outdated Show resolved Hide resolved
server/flags/flags.go Outdated Show resolved Hide resolved
@yihuang yihuang force-pushed the indexer branch 3 times, most recently from 1bf47b8 to ecb633e Compare June 13, 2022 09:46
Copy link
Contributor

@tomtau tomtau left a comment

Choose a reason for hiding this comment

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

could there be a test for the indexer?

rpc/backend/evm_backend.go Outdated Show resolved Hide resolved
rpc/namespaces/ethereum/debug/api.go Outdated Show resolved Hide resolved
rpc/namespaces/ethereum/eth/api.go Outdated Show resolved Hide resolved
rpc/namespaces/ethereum/eth/api.go Outdated Show resolved Hide resolved
rpc/types/events.go Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Jun 29, 2022

Codecov Report

Merging #1121 (a6fc34a) into main (737c1de) will increase coverage by 0.02%.
The diff coverage is 40.20%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1121      +/-   ##
==========================================
+ Coverage   52.27%   52.29%   +0.02%     
==========================================
  Files         104      105       +1     
  Lines        9383     9519     +136     
==========================================
+ Hits         4905     4978      +73     
- Misses       4227     4274      +47     
- Partials      251      267      +16     
Impacted Files Coverage Δ
rpc/backend/tracing.go 0.00% <0.00%> (ø)
rpc/backend/tx_info.go 0.00% <0.00%> (ø)
rpc/backend/utils.go 1.29% <ø> (-2.50%) ⬇️
rpc/types/utils.go 0.00% <0.00%> (ø)
rpc/types/events.go 71.85% <50.00%> (-16.25%) ⬇️
server/config/config.go 22.48% <50.00%> (+0.32%) ⬆️
indexer/kv_indexer.go 64.96% <64.96%> (ø)
rpc/backend/backend.go 51.72% <100.00%> (+1.72%) ⬆️
rpc/backend/blocks_info.go 44.41% <100.00%> (ø)
... and 1 more

@lgtm-com
Copy link

lgtm-com bot commented Jun 29, 2022

This pull request introduces 1 alert when merging 0f5cc5b2c4b37a1838985a4678ba675202318c4b into de21773 - view on LGTM.com

new alerts:

  • 1 for Inconsistent direction of for loop

@yihuang
Copy link
Contributor Author

yihuang commented Jun 30, 2022

The db size after a test run on a cronos mainnet node (block height: 3104880):

$ du -sh tx_index.db/ evmindexer.db/
290G	tx_index.db/
3.2G	evmindexer.db/

@yihuang yihuang marked this pull request as ready for review July 1, 2022 07:49
@yihuang
Copy link
Contributor Author

yihuang commented Jul 1, 2022

could there be a test for the indexer?

added some unit tests for the kv indexer.

rpc/types/events.go Show resolved Hide resolved
server/indexer.go Outdated Show resolved Hide resolved
Copy link
Contributor

@facs95 facs95 left a comment

Choose a reason for hiding this comment

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

Gave it another pass after conflicts were solved! LGTM 🚀

@yihuang yihuang requested a review from fedekunze August 10, 2022 13:17
Copy link
Contributor

@fedekunze fedekunze left a comment

Choose a reason for hiding this comment

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

Final pass. @danburck can you also review?

proto/ethermint/types/v1/indexer.proto Outdated Show resolved Hide resolved
rpc/backend/backend.go Outdated Show resolved Hide resolved
rpc/backend/backend_suite_test.go Outdated Show resolved Hide resolved
rpc/backend/tx_info.go Outdated Show resolved Hide resolved
rpc/backend/tx_info.go Show resolved Hide resolved
rpc/backend/tx_info.go Outdated Show resolved Hide resolved
server/indexer_cmd.go Show resolved Hide resolved
server/start.go Show resolved Hide resolved
server/indexer.go Outdated Show resolved Hide resolved
server/indexer.go Outdated Show resolved Hide resolved
- test indexer in backend unit test
- add comments
@yihuang yihuang requested a review from fedekunze August 11, 2022 12:19
Copy link
Contributor

@fedekunze fedekunze left a comment

Choose a reason for hiding this comment

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

ACK. I think this PR could be refactored further tho. Let's work on a document for how to enable this

server/indexer_service.go Outdated Show resolved Hide resolved
server/indexer_cmd.go Show resolved Hide resolved
Copy link
Contributor

@danburck danburck left a comment

Choose a reason for hiding this comment

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

LGTM, aggree that we need documentation for this. @yihuang can you add some after the merge?

@danburck danburck enabled auto-merge (squash) August 11, 2022 20:45
@danburck danburck merged commit 77ed4aa into evmos:main Aug 11, 2022
@yihuang yihuang deleted the indexer branch August 12, 2022 00:01
@yihuang
Copy link
Contributor Author

yihuang commented Aug 12, 2022

LGTM, aggree that we need documentation for this. @yihuang can you add some after the merge?

sure #1235

facs95 added a commit that referenced this pull request Aug 12, 2022
* Store eth tx index separately

Closes: #1075
Solution:
- run a optional indexer service
- adapt the json-rpc to the more efficient query

changelog

changelog

fix lint

fix backward compatibility

fix lint

timeout

better strconv

fix linter

fix package name

add cli command to index old tx

fix for loop

indexer cmd don't have access to local rpc

workaround exceed block gas limit situation

add unit tests for indexer

refactor

polish the indexer module

Update server/config/toml.go

Co-authored-by: Federico Kunze Küllmer <31522760+fedekunze@users.noreply.github.com>

improve comments

share code between GetTxByEthHash and GetTxByIndex

fix unit test

Update server/indexer.go

Co-authored-by: Freddy Caceres <facs95@gmail.com>

* Apply suggestions from code review

* test enable-indexer in integration test

* fix go lint

* address review suggestions

* fix linter

* address review suggestions

- test indexer in backend unit test
- add comments

* fix build

* fix test

* service name

Co-authored-by: Freddy Caceres <facs95@gmail.com>
Co-authored-by: Federico Kunze Küllmer <31522760+fedekunze@users.noreply.github.com>
hoanguyenkh pushed a commit to AstraProtocol/ethermint that referenced this pull request Aug 17, 2022
* Store eth tx index separately

Closes: evmos#1075
Solution:
- run a optional indexer service
- adapt the json-rpc to the more efficient query

changelog

changelog

fix lint

fix backward compatibility

fix lint

timeout

better strconv

fix linter

fix package name

add cli command to index old tx

fix for loop

indexer cmd don't have access to local rpc

workaround exceed block gas limit situation

add unit tests for indexer

refactor

polish the indexer module

Update server/config/toml.go

Co-authored-by: Federico Kunze Küllmer <31522760+fedekunze@users.noreply.github.com>

improve comments

share code between GetTxByEthHash and GetTxByIndex

fix unit test

Update server/indexer.go

Co-authored-by: Freddy Caceres <facs95@gmail.com>

* Apply suggestions from code review

* test enable-indexer in integration test

* fix go lint

* address review suggestions

* fix linter

* address review suggestions

- test indexer in backend unit test
- add comments

* fix build

* fix test

* service name

Co-authored-by: Freddy Caceres <facs95@gmail.com>
Co-authored-by: Federico Kunze Küllmer <31522760+fedekunze@users.noreply.github.com>
Copy link

@onewhiskeypls onewhiskeypls left a comment

Choose a reason for hiding this comment

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

utACK: quick pass with a nit and thought

ethMsg := msg.(*evmtypes.MsgEthereumTx)
txHash := common.HexToHash(ethMsg.Hash)

txResult := ethermint.TxResult{

Choose a reason for hiding this comment

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

thought: For the following if-else block, could we set Failed here as true or would that be poor practice?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It can save a branch block if we init both GasUsed and Failed to the failed cases here.

"github.com/cosmos/cosmos-sdk/client"
"github.com/cosmos/cosmos-sdk/codec"
sdk "github.com/cosmos/cosmos-sdk/types"
sdkerrors "github.com/cosmos/cosmos-sdk/types/errors"

Choose a reason for hiding this comment

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

nitpick: This package was deprecated and moved to its own module here "cosmossdk.io/errors"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can migrate that globally sometime.

if err := idxer.IndexBlock(blk, resBlk.DeliverTxs); err != nil {
return err
}
fmt.Println(height)

Choose a reason for hiding this comment

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

Consider: A progress bar instead or remove it, this feels like a very vague way to keep track of the progress?

@@ -97,40 +83,38 @@ func ParseTxResult(result *abci.ResponseDeliverTx) (*ParsedTxs, error) {
TxHashes: make(map[common.Hash]int),
}
for _, event := range result.Events {

Choose a reason for hiding this comment

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

Consider: extracting the format check, then seperate each formats into its own functional block, this will also make it easier to purge deprecated format eventually

devon-chain pushed a commit to PundiAI/ethermint that referenced this pull request Nov 17, 2022
* Store eth tx index separately

Closes: evmos#1075
Solution:
- run a optional indexer service
- adapt the json-rpc to the more efficient query

changelog

changelog

fix lint

fix backward compatibility

fix lint

timeout

better strconv

fix linter

fix package name

add cli command to index old tx

fix for loop

indexer cmd don't have access to local rpc

workaround exceed block gas limit situation

add unit tests for indexer

refactor

polish the indexer module

Update server/config/toml.go

Co-authored-by: Federico Kunze Küllmer <31522760+fedekunze@users.noreply.github.com>

improve comments

share code between GetTxByEthHash and GetTxByIndex

fix unit test

Update server/indexer.go

Co-authored-by: Freddy Caceres <facs95@gmail.com>

* Apply suggestions from code review

* test enable-indexer in integration test

* fix go lint

* address review suggestions

* fix linter

* address review suggestions

- test indexer in backend unit test
- add comments

* fix build

* fix test

* service name

Co-authored-by: Freddy Caceres <facs95@gmail.com>
Co-authored-by: Federico Kunze Küllmer <31522760+fedekunze@users.noreply.github.com>
(cherry picked from commit a8723c5)
@danburck danburck mentioned this pull request Nov 30, 2022
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 this pull request may close these issues.

Store tx hash index separately to get rid of full tendermint tx indexes
8 participants