Skip to content
This repository has been archived by the owner on Nov 30, 2021. It is now read-only.

fix handler csdb usage, fixes consensus error again #516

Merged
merged 4 commits into from
Sep 17, 2020
Merged

fix handler csdb usage, fixes consensus error again #516

merged 4 commits into from
Sep 17, 2020

Conversation

noot
Copy link
Contributor

@noot noot commented Sep 16, 2020 •
edited
Loading

Closes: #XXX

Description

fix usage of csdb in handler

to test, start 3 docker nodes as per https://github.com/ChainSafe/ethermint/pull/513

then:

git clone [email protected]:ChainSafe/aragonOS
cd aragonOS

update truffle-config.js to have this as the rpc network:

        rpc: {
            network_id: 17,
            host: '172.17.0.2',
            port: 8545,
            gas: 6.9e6,
            gasPrice: 1,
        },

will likely need to install correct truffle version: npm i -g [email protected]

finally: truffle test test/contracts/apps/app_acl.js --network rpc

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)

@noot noot requested a review from fedekunze as a code owner September 16, 2020 22:46
@noot noot self-assigned this Sep 16, 2020
@codecov
Copy link

codecov bot commented Sep 16, 2020 •
edited
Loading

Codecov Report

Merging #516 into development will increase coverage by 0.04%.
The diff coverage is 75.00%.

Impacted file tree graph

@@               Coverage Diff               @@
##           development     #516      +/-   ##
===============================================
+ Coverage        72.04%   72.08%   +0.04%     
===============================================
  Files               41       41              
  Lines             2719     2723       +4     
===============================================
+ Hits              1959     1963       +4     
  Misses             615      615              
  Partials           145      145
Impacted Files Coverage Δ
x/evm/handler.go 91.73% <75.00%> (+0.28%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 244d836...5728eb4. Read the comment docs.

araskachoi
araskachoi approved these changes Sep 17, 2020
View reviewed changes
Copy link
Contributor

@araskachoi araskachoi left a comment

Choose a reason for hiding this comment

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

no more consensus err

@noot noot merged commit 73d6c41 into development Sep 17, 2020
@noot noot deleted the noot/fix2 branch September 17, 2020 00:29
fedekunze
fedekunze reviewed Sep 22, 2020
View reviewed changes
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.

LGTM. Minor comment

x/evm/handler.go
@@ -62,8 +62,10 @@ func handleMsgEthereumTx(ctx sdk.Context, k Keeper, msg types.MsgEthereumTx) (*s

// Prepare db for logs
// TODO: block hash
k.CommitStateDB.Prepare(ethHash, common.Hash{}, k.TxCount)
k.TxCount++
if !st.Simulate {
Copy link
Contributor

Choose a reason for hiding this comment

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

why is this the case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the txCount is used in the stateDB, and since a simulated tx is run only on the node it's submitted to, then this will cause the txCount/stateDB of the node that ran the simulated tx to be different than the other nodes, causing the consensus error

Copy link
Contributor

Choose a reason for hiding this comment

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

ahh great catch! I can we document this on the comments?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure I'll open a follow up with comments!

tests/rpc_test.go
@@ -777,7 +777,7 @@ func TestEth_EstimateGas_ContractDeployment(t *testing.T) {
err := json.Unmarshal(rpcRes.Result, &gas)
require.NoError(t, err, string(rpcRes.Result))

require.Equal(t, "0x1cab2", gas.String())
require.Equal(t, "0x1c2c4", gas.String())
Copy link
Contributor

Choose a reason for hiding this comment

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

lol, we def need to fix this

This was referenced Sep 22, 2020
Closed
Merged
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@fedekunze fedekunze fedekunze left review comments

@araskachoi araskachoi araskachoi approved these changes

@J-Thompson12 J-Thompson12 Awaiting requested review from J-Thompson12

Assignees

@noot noot

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@noot @araskachoi @fedekunze