filter non eth txs in block rpc response #741

crypto-facs · 2021-11-11T01:30:01Z

Closes: #705

Description

Right now there is an inconsistency between eth_getBlockByNumber and other RPC endpoints such us eth_getTransactionReceipt or eth_transactionByHashAndIndex.

Get block is returning non EVM txs that can not be found on other endpoints causing web3 tools to experience unexpected failures. This PR ensures there is consistency between the endpoints.

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)

tomtau

looks good -- it may be worth checking other Web3 methods though.
e.g. eth_getBlockTransactionCountByNumber is still returning an incorrect number of Ethereum txs (thanks @calvinaco for testing this)

fedekunze

utACK. Pending @tomtau comments to be resolved

crypto-facs · 2021-11-11T13:23:57Z

Took @thomas-nguy additions on crypto's branch to cover the remaining endpoitns. Should be good now

fedekunze

I think we need to define the assumptions for the index queries, are we assuming the index always corresponds to an ethereum tx? or does it include the other cosmos txs in the block and we should return an error if the tx is not an Ethereum one?

rpc/ethereum/namespaces/eth/api.go

fedekunze · 2021-11-11T13:57:30Z

rpc/ethereum/namespaces/eth/api.go

@@ -721,7 +714,8 @@ func (e *PublicAPI) GetTransactionByBlockNumberAndIndex(blockNum rpctypes.BlockN
 	}

 	i := int(idx)
-	if i >= len(resBlock.Block.Txs) {
+	ethMsgs := e.getEthereumMsgFromTendermintBlock(resBlock)
+	if i >= len(ethMsgs) {


L723 (txBz := resBlock.Block.Txs[i] ) is inconsistent with the L685. See comment about assuming the index corresponds to an ethereum tx

line 723-730 needs to be removed @crypto-facs

Good catch! If we agree this #741 (comment) is the right approach to handle index across the endpoints I will add the filter here too.

ok please remove

fedekunze

see prev comments

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

…nto filter-block-txs

crypto-facs · 2021-11-11T18:00:14Z

Hey guys so I am seeing another issue. At the moment the index returned by the transactionReceipt is out of sync with the other indexes. for example if txIndex on transactionReceipt is 0x3 that does not guarantee that if you query eth_getTransactionByBlockHashAndIndex that transaction index is going to be found.

It might be wise to work on the integratoin tests and have them check this scenarios before we move forward with this. Because there are many scenarios to cover.

tomtau

looks all right -- backend.go just needs to be formatted with gofumpt

tomtau · 2021-11-16T01:43:09Z

I assume the solidity test failing is unrelated: https://github.com/tharsis/ethermint/runs/4219178563?check_suite_focus=true

rpc/ethereum/backend/backend.go

fedekunze

LGTM, pending minor comments and lint fixes

rpc/ethereum/backend/backend.go

rpc/ethereum/namespaces/eth/api.go

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

…nto filter-block-txs

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

filter non eth txs in block rpc response

fa947fe

crypto-facs requested review from fedekunze, jolube and khoslaventures as code owners November 11, 2021 01:30

update changelog

f6270f3

tomtau mentioned this pull request Nov 11, 2021

Pending transactions cannot be retrieved crypto-org-chain/cronos#208

Closed

tomtau suggested changes Nov 11, 2021

View reviewed changes

fedekunze approved these changes Nov 11, 2021

View reviewed changes

fix remaining endpoints

9a8ef55

fedekunze reviewed Nov 11, 2021

View reviewed changes

fedekunze suggested changes Nov 11, 2021

View reviewed changes

crypto-facs and others added 4 commits November 11, 2021 09:30

Update rpc/ethereum/namespaces/eth/api.go

59f554c

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

fix GetTransactionByBlockNumberAndIndex

686521a

Merge branch 'filter-block-txs' of github.com:crypto-facs/ethermint i…

b07323a

…nto filter-block-txs

add comment

d08d70b

crypto-facs marked this pull request as draft November 11, 2021 17:57

fedekunze and others added 5 commits November 12, 2021 09:35

Merge branch 'main' into filter-block-txs

48b04da

Merge branch 'main' into filter-block-txs

fc42fbf

fix tx index

4405f0e

filter txs by block height

e77a505

fix linter

1596cd3

crypto-facs marked this pull request as ready for review November 16, 2021 01:03

tomtau approved these changes Nov 16, 2021

View reviewed changes

thomas-nguy mentioned this pull request Nov 16, 2021

Commits needed for release v0.7.3 #752

Closed

fedekunze reviewed Nov 16, 2021

View reviewed changes

rpc/ethereum/backend/backend.go Outdated Show resolved Hide resolved

fedekunze approved these changes Nov 16, 2021

View reviewed changes

rpc/ethereum/backend/backend.go Outdated Show resolved Hide resolved

rpc/ethereum/namespaces/eth/api.go Outdated Show resolved Hide resolved

Merge branch 'main' into filter-block-txs

04dc770

crypto-facs and others added 4 commits November 16, 2021 06:47

Update rpc/ethereum/backend/backend.go

fd7fc28

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

remove unnecesary hex calls

2d7df9c

Merge branch 'filter-block-txs' of github.com:crypto-facs/ethermint i…

ebd7451

…nto filter-block-txs

fix linter

1a2b266

fedekunze merged commit fdabf69 into evmos:main Nov 16, 2021

thomas-nguy referenced this pull request in crypto-org-chain/ethermint Nov 17, 2021

cherry-pick (tharsis#741)

f2ad4af

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

coderabbitai bot mentioned this pull request Apr 19, 2024

Problem: panic on invalid elasticity_multiplier crypto-org-chain/cronos#1397

Merged

13 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

filter non eth txs in block rpc response #741

filter non eth txs in block rpc response #741

crypto-facs commented Nov 11, 2021

tomtau left a comment •

edited

Loading

fedekunze left a comment

crypto-facs commented Nov 11, 2021

fedekunze left a comment

fedekunze Nov 11, 2021

thomas-nguy Nov 11, 2021

crypto-facs Nov 11, 2021

fedekunze Nov 11, 2021

fedekunze left a comment

crypto-facs commented Nov 11, 2021

tomtau left a comment

tomtau commented Nov 16, 2021

fedekunze left a comment

filter non eth txs in block rpc response #741

filter non eth txs in block rpc response #741

Conversation

crypto-facs commented Nov 11, 2021

Description

tomtau left a comment • edited Loading

Choose a reason for hiding this comment

fedekunze left a comment

Choose a reason for hiding this comment

crypto-facs commented Nov 11, 2021

fedekunze left a comment

Choose a reason for hiding this comment

fedekunze Nov 11, 2021

Choose a reason for hiding this comment

thomas-nguy Nov 11, 2021

Choose a reason for hiding this comment

crypto-facs Nov 11, 2021

Choose a reason for hiding this comment

fedekunze Nov 11, 2021

Choose a reason for hiding this comment

fedekunze left a comment

Choose a reason for hiding this comment

crypto-facs commented Nov 11, 2021

tomtau left a comment

Choose a reason for hiding this comment

tomtau commented Nov 16, 2021

fedekunze left a comment

Choose a reason for hiding this comment

tomtau left a comment •

edited

Loading