-
Notifications
You must be signed in to change notification settings - Fork 161
fix handler csdb usage, fixes consensus error again #516
Conversation
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
araskachoi
left a comment
There was a problem hiding this 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
fedekunze
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Minor comment
| // TODO: block hash | ||
| k.CommitStateDB.Prepare(ethHash, common.Hash{}, k.TxCount) | ||
| k.TxCount++ | ||
| if !st.Simulate { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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!
| require.NoError(t, err, string(rpcRes.Result)) | ||
|
|
||
| require.Equal(t, "0x1cab2", gas.String()) | ||
| require.Equal(t, "0x1c2c4", gas.String()) |
There was a problem hiding this comment.
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
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:
update truffle-config.js to have this as the rpc network:
will likely need to install correct truffle version:
npm i -g [email protected]finally:
truffle test test/contracts/apps/app_acl.js --network rpcFor contributor use:
docs/) or specification (x/<module>/spec/)godoccomments.Unreleasedsection inCHANGELOG.mdFiles changedin the Github PR explorerFor admin use:
WIP,R4R,docs, etc)