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

Store tx hash index separately to get rid of full tendermint tx indexes #1075

Closed
yihuang opened this issue May 4, 2022 · 11 comments · Fixed by #1121
Closed

Store tx hash index separately to get rid of full tendermint tx indexes #1075

yihuang opened this issue May 4, 2022 · 11 comments · Fixed by #1121

Comments

@yihuang
Copy link
Contributor

yihuang commented May 4, 2022

Proposal: Store tx hash index separately to get rid of full tendermint tx indexes

Current behavior:

rely on full nodes to turn on tendermint tx indexer, cost extra storage space, and slow down consensus state machine.

Desired behavior:

  • Use a standalone kv db to store eth tx hash -> cosmos tx hash.
  • The eth tx hash index will include all the eth msgs in the block, no matter the execution result.
  • Support the unlucky tx that exceeds block gas limit into the json-rpc apis, since user fee is deducted in this case, it should leave a trace in json-rpc responses.

Use case:

@yihuang
Copy link
Contributor Author

yihuang commented May 4, 2022

This will also solve the failed tx hash not found issue, we don't need to manipulate the events anymore: #1062

@fedekunze
Copy link
Contributor

fedekunze commented May 4, 2022

I feel that we should we try to prioritize the modular hasher ADR: tendermint/tendermint#6539. Any other solution will involve additional maintenance and overhead

@yihuang
Copy link
Contributor Author

yihuang commented May 4, 2022

I feel that we should we try to prioritize the modular hasher ADR: tendermint/tendermint#6539. Any other solution will involve additional maintenance and overhead

Can modular hash handle multiples messages(eth tx) in one tm tx?

@JayT106
Copy link
Contributor

JayT106 commented May 4, 2022

I feel that we should we try to prioritize the modular hasher ADR: tendermint/tendermint#6539. Any other solution will involve additional maintenance and overhead

Can modular hash handle multiples messages(eth tx) in one tm tx?

From the Tendermint dev's decision, they would prefer to do it in ABCI++ during prepareproposal .

https://github.com/tendermint/tendermint/pull/8033/files#diff-d4c2afdd9920a19ba459ab66bb2ffa4e732447831618f6e8777e811314ef76e7R155

@yihuang
Copy link
Contributor Author

yihuang commented May 5, 2022

crypto-org-chain/cronos#451 (comment)

BTW, there's an option to fine turn what events to index, we can use that to shrink the tx indexer db size, so db size is less of a motivation for this proposal.

The other potential motivations are:

  • to cover more tx than tm tx indexer, this may also be solved by emitting an eth tx hash event at the very beginning of ante handlers for eth tx, so the failed tx can be indexed too.
  • I've seen the /tx_search call shown as hotspot in json-rpc api profiling, there's an expression parser there, so using a simply kv db query will improve performance.

@tomtau
Copy link
Contributor

tomtau commented May 11, 2022

One more motivation is not having the the indexer affected by unrelated tm issues that may take some time to diagnose and fix: tendermint/tendermint#5281

@yihuang
Copy link
Contributor Author

yihuang commented May 11, 2022

I think we can implement this extra indexer as an optional feature, like if the extra indexer is not initialized, the json-rpc can fallback to current logic.
and the indexing process can run asynchronously, monitoring the chain progress with the tm client api.
it'll workaround two known issues:

  • tx is missing when it exceeds block gas limit
  • tendermint includes duplicated txs which overrides the tx result

@JayT106
Copy link
Contributor

JayT106 commented May 11, 2022

Found the discussion from the slack TM sharing channel, this is a workaround for the duplicate tx indexing issue
terra-money/tendermint#76

@yihuang
Copy link
Contributor Author

yihuang commented May 12, 2022

By doing a reindexing for the old blocks, we can deprecate some old json-rpc fallback logic which is ugly and inefficient:
https://github.com/tharsis/ethermint/blob/main/rpc/namespaces/ethereum/eth/api.go#L904

@github-actions
Copy link

This issue is stale because it has been open 45 days with no activity. Remove Status: Stale label or comment or this will be closed in 7 days.

@yihuang
Copy link
Contributor Author

yihuang commented Jun 27, 2022

unstale.

yihuang added a commit to yihuang/ethermint that referenced this issue Aug 10, 2022
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 <[email protected]>

improve comments

share code between GetTxByEthHash and GetTxByIndex

fix unit test

Update server/indexer.go

Co-authored-by: Freddy Caceres <[email protected]>
danburck pushed a commit that referenced this issue Aug 11, 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 <[email protected]>

improve comments

share code between GetTxByEthHash and GetTxByIndex

fix unit test

Update server/indexer.go

Co-authored-by: Freddy Caceres <[email protected]>

* 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 <[email protected]>
Co-authored-by: Federico Kunze Küllmer <[email protected]>
facs95 added a commit that referenced this issue 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 <[email protected]>

improve comments

share code between GetTxByEthHash and GetTxByIndex

fix unit test

Update server/indexer.go

Co-authored-by: Freddy Caceres <[email protected]>

* 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 <[email protected]>
Co-authored-by: Federico Kunze Küllmer <[email protected]>
hoanguyenkh pushed a commit to AstraProtocol/ethermint that referenced this issue 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 <[email protected]>

improve comments

share code between GetTxByEthHash and GetTxByIndex

fix unit test

Update server/indexer.go

Co-authored-by: Freddy Caceres <[email protected]>

* 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 <[email protected]>
Co-authored-by: Federico Kunze Küllmer <[email protected]>
devon-chain pushed a commit to PundiAI/ethermint that referenced this issue 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 <[email protected]>

improve comments

share code between GetTxByEthHash and GetTxByIndex

fix unit test

Update server/indexer.go

Co-authored-by: Freddy Caceres <[email protected]>

* 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 <[email protected]>
Co-authored-by: Federico Kunze Küllmer <[email protected]>
(cherry picked from commit a8723c5)
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.

5 participants