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

fix(evm,rpc): coinbase should not be the current one in traceTransact… #1

Merged
merged 3 commits into from
Dec 8, 2023

Conversation

zsystm
Copy link
Collaborator

@zsystm zsystm commented Dec 8, 2023

Closes: #XXX

Before review

  • This PR is based on the releases/v0.19.x branch, which originates from Ethermint v0.19.3 currently utilized by Canto.
  • The primary objective of this PR is to introduce a hotfix to version v0.19.3.
  • Can't compile proto files in Ethermint v0.19.3. So I updated related scripts to a working version (v0.20.0).
    • It looks like there are a lot of changes in proto files, but they are mostly due to formatting. The real contents changes happened only in proto/evm/v1/query.proto.

Description

Problems

  • When querying the EVM state with a past block height, the error "failed to retrieve validator from block proposer address" occurs.
  • The validator address shown in this error varies with each query.

Error triggering point

  • Error occurs while retrieving Validator information during the execution of GetCoinbaseAddres.

Cause

  • It's observed that when performing a gRPC query for past block heights, the Context created contains the header of the latest committed block (ref: code link).
  • Only the Height is set to the past block height, while other fields like the proposer address use the values from the latest block header.
  • The Cache Multi Store used for queries is set based on height (ref: code link).
  • Consequently, querying the proposer address in the Cache store based on past height results in a situation where the validator state is absent.

Solution

How to test this PR

Reproduction Steps

  • Set up a local testnet with 2 validators using current Canto (ref: setup-local-testnet-with-2-validators.md)
  • The validator registered through genesis transaction is Validator A.
  • And validator created after using the create-validator is Validator B.
  • When creating B, the amount self-delegated was about nine times more than what A had done.
  • Most of the blocks after block H, where B joined, were created by B.
  • Attempt to reproduce by specifying a block number before H in eth_call.
  • Successfully reproduced as follows:
curl --location 'http://localhost:8545' \
--header 'Content-Type: application/json' \
--header 'X-API-KEY: 2dbba7c8fea26030dc532b2f' \
--data '{
  "jsonrpc": "2.0",
  "method": "eth_call",
  "params": [
    {
      "to": "0xEcf044C5B4b867CFda001101c617eCd347095B44",
      "data": "0xe66bb345"
    },
    "0x9"
  ],
  "id": 1
}'

{"jsonrpc":"2.0","id":1,"error":{"code":-32000,"message":"rpc error: code = Unknown desc = rpc error: code = Internal desc = failed to obtain coinbase address: failed to retrieve validator from block proposer address cantovalcons1zlxap2zx0u4zjxtxzqlf907t8qny90dpnld0el: validator does not exist: unknown request"}}

Steps to Confirm the Fix

  • Stop the nodes running for Validators A and B.
  • Build cantod with the hotfix-applied binary.
    • change ethermint dependency of Canto's go.mod to this branch.
  • Start the nodes for A and B again with the built version.
  • Attempt the same call.
  • The issue was confirmed to be resolved as follows:
curl --location 'http://localhost:8545' \
--header 'Content-Type: application/json' \
--header 'X-API-KEY: 2dbba7c8fea26030dc532b2f' \
--data '{
  "jsonrpc": "2.0",
  "method": "eth_call",
  "params": [
    {
      "to": "0xEcf044C5B4b867CFda001101c617eCd347095B44",
      "data": "0xe66bb345"
    },
    "0x9"
  ],
  "id": 1
}'
{"jsonrpc":"2.0","id":1,"result":"0x"}

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)

…ion execution (evmos#1392)

* add proposer address

* make proto-all

* update nix

* fix test

* keep default proposerAddress

* add change doc

* refine GetProposerAddress with test

* include ProposerAddress for trace api

* fix eth call req

* wrap proposerAddress for eth call

* allow proto translates to sdk.ConsAddress

* Update rpc/backend/call_tx.go

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 295a886)
@zsystm zsystm self-assigned this Dec 8, 2023
@zsystm zsystm removed the request for review from tkkwon1998 December 8, 2023 02:18
compile protobuf files in ethermint v0.19.3 does not work. so we should update related scripts to a working version.

this commit does not contain proto updates, it just linted by protoc and linter.
@zsystm zsystm marked this pull request as ready for review December 8, 2023 08:11
Copy link
Collaborator

@dongsam dongsam left a comment

Choose a reason for hiding this comment

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

LGTM

@tkkwon1998 tkkwon1998 merged commit db8920e into releases/v0.19.x Dec 8, 2023
zsystm added a commit to b-harvest/Canto that referenced this pull request Dec 11, 2023
change ethermint to Canto-Network/ethermint which includes Canto-Network/ethermint#1 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants