-
Notifications
You must be signed in to change notification settings - Fork 193
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): ensure only one copy of StateDB when executing Ethereum txs #2165
Changes from 33 commits
3b0387a
d64647e
8507e06
bd6e0d5
f44ce1e
837a6a0
ae354d6
e9ab63d
84e4bf6
4cb566c
3b420a3
aec26d0
1962805
4f9d897
6d082cc
4d92d5b
edf959e
e94abc7
e95f809
396afb4
39fdac9
8445c92
b4f3ac8
ff27306
0d26abc
ab913ba
ecfd2fe
4117272
997a946
cfc3bbe
b775d16
a8dd8a6
eb47f0a
138dd63
d35274a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -102,7 +102,7 @@ func (s *BackendSuite) SetupSuite() { | |
|
||
// Send Transfer TX and use the results in the tests | ||
s.Require().NoError(err) | ||
transferTxHash = s.SendNibiViaEthTransfer(recipient, amountToSend, true) | ||
transferTxHash = s.SendNibiViaEthTransfer(recipient, amountToSend, true /*waitForNextBlock*/) | ||
blockNumber, blockHash, _ := WaitForReceipt(s, transferTxHash) | ||
s.Require().NotNil(blockNumber) | ||
s.Require().NotNil(blockHash) | ||
|
@@ -151,7 +151,7 @@ func (s *BackendSuite) DeployTestContract(waitForNextBlock bool) (gethcommon.Has | |
&gethcore.LegacyTx{ | ||
Nonce: uint64(nonce), | ||
Data: bytecodeForCall, | ||
Gas: 1500_000, | ||
Gas: 1_500_000, | ||
GasPrice: big.NewInt(1), | ||
}, | ||
waitForNextBlock, | ||
|
@@ -177,7 +177,7 @@ func SendTransaction(s *BackendSuite, tx *gethcore.LegacyTx, waitForNextBlock bo | |
|
||
// WaitForReceipt waits for a transaction to be included in a block, returns block number, block hash and receipt | ||
func WaitForReceipt(s *BackendSuite, txHash gethcommon.Hash) (*big.Int, *gethcommon.Hash, *backend.TransactionReceipt) { | ||
ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) | ||
ctx, cancel := context.WithTimeout(context.Background(), 5*time.Minute) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I increased the wait time because a 5 second timeout was causing issues when I stepped through the code with a debugger (which takes longer than 5 seconds). |
||
defer cancel() | ||
|
||
for { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -154,10 +154,6 @@ func (s *BackendSuite) TestGasUsedFunTokens() { | |
s.Require().NotNil(blockNumber2) | ||
s.Require().NotNil(blockNumber3) | ||
|
||
// TXs should have been included in the same block | ||
s.Require().Equal(blockNumber1, blockNumber2) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Unnecessary to have the txs in the same block. |
||
s.Require().Equal(blockNumber2, blockNumber3) | ||
|
||
// 1 and 3 should pass and 2 should fail | ||
s.Require().Equal(gethcore.ReceiptStatusSuccessful, receipt1.Status) | ||
s.Require().Equal(gethcore.ReceiptStatusFailed, receipt2.Status) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -494,7 +494,7 @@ func (n *Network) LatestHeight() (int64, error) { | |
// committed after a given block. If that height is not reached within a timeout, | ||
// an error is returned. Regardless, the latest height queried is returned. | ||
func (n *Network) WaitForHeight(h int64) (int64, error) { | ||
return n.WaitForHeightWithTimeout(h, 40*time.Second) | ||
return n.WaitForHeightWithTimeout(h, 5*time.Minute) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same issue with the debugger timing out. |
||
} | ||
|
||
// WaitForHeightWithTimeout is the same as WaitForHeight except the caller can | ||
|
Original file line number | Diff line number | Diff line change | ||||||
---|---|---|---|---|---|---|---|---|
|
@@ -23,8 +23,7 @@ type Suite struct { | |||||||
|
||||||||
// TestKeeperSuite: Runs all the tests in the suite. | ||||||||
func TestKeeperSuite(t *testing.T) { | ||||||||
s := new(Suite) | ||||||||
suite.Run(t, s) | ||||||||
suite.Run(t, new(Suite)) | ||||||||
} | ||||||||
|
||||||||
// TestExportInitGenesis | ||||||||
|
@@ -48,21 +47,23 @@ func (s *Suite) TestExportInitGenesis() { | |||||||
deployResp, err := evmtest.DeployContract(&deps, erc20Contract) | ||||||||
s.Require().NoError(err) | ||||||||
erc20Addr := deployResp.ContractAddr | ||||||||
|
||||||||
evmObj, _ := deps.NewEVM() | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Handle error returned by The error returned by Apply this diff to handle the error: - evmObj, _ := deps.NewEVM()
+ evmObj, err := deps.NewEVM()
+ s.Require().NoError(err) 📝 Committable suggestion
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @k-yang You are correct. Looking at the source code, 🔗 Analysis chain@k-yang You are correct. I apologize for my mistake. The 🏁 Scripts executedThe following scripts were executed for the analysis: Script:
Length of output: 59 Script:
Length of output: 39790 ✏️ Learnings added
|
||||||||
totalSupply, err := deps.EvmKeeper.ERC20().LoadERC20BigInt( | ||||||||
deps.Ctx, erc20Contract.ABI, erc20Addr, "totalSupply", | ||||||||
deps.Ctx, evmObj, erc20Contract.ABI, erc20Addr, "totalSupply", | ||||||||
) | ||||||||
s.Require().NoError(err) | ||||||||
|
||||||||
// Transfer ERC-20 tokens to user A | ||||||||
_, _, err = deps.EvmKeeper.ERC20().Transfer(erc20Addr, fromUser, toUserA, amountToSendA, deps.Ctx) | ||||||||
_, _, err = deps.EvmKeeper.ERC20().Transfer(erc20Addr, fromUser, toUserA, amountToSendA, deps.Ctx, evmObj) | ||||||||
s.Require().NoError(err) | ||||||||
|
||||||||
// Transfer ERC-20 tokens to user B | ||||||||
_, _, err = deps.EvmKeeper.ERC20().Transfer(erc20Addr, fromUser, toUserB, amountToSendB, deps.Ctx) | ||||||||
_, _, err = deps.EvmKeeper.ERC20().Transfer(erc20Addr, fromUser, toUserB, amountToSendB, deps.Ctx, evmObj) | ||||||||
s.Require().NoError(err) | ||||||||
|
||||||||
// Create fungible token from bank coin | ||||||||
funToken := evmtest.CreateFunTokenForBankCoin(&deps, "unibi", &s.Suite) | ||||||||
funToken := evmtest.CreateFunTokenForBankCoin(deps, "unibi", &s.Suite) | ||||||||
s.Require().NoError(err) | ||||||||
funTokenAddr := funToken.Erc20Addr.Address | ||||||||
|
||||||||
|
@@ -98,15 +99,15 @@ func (s *Suite) TestExportInitGenesis() { | |||||||
evmmodule.InitGenesis(deps.Ctx, deps.EvmKeeper, deps.App.AccountKeeper, *evmGenesisState) | ||||||||
|
||||||||
// Verify erc20 balances for users A, B and sender | ||||||||
balance, err := deps.EvmKeeper.ERC20().BalanceOf(erc20Addr, toUserA, deps.Ctx) | ||||||||
balance, err := deps.EvmKeeper.ERC20().BalanceOf(erc20Addr, toUserA, deps.Ctx, evmObj) | ||||||||
s.Require().NoError(err) | ||||||||
s.Require().Equal(amountToSendA, balance) | ||||||||
|
||||||||
balance, err = deps.EvmKeeper.ERC20().BalanceOf(erc20Addr, toUserB, deps.Ctx) | ||||||||
balance, err = deps.EvmKeeper.ERC20().BalanceOf(erc20Addr, toUserB, deps.Ctx, evmObj) | ||||||||
s.Require().NoError(err) | ||||||||
s.Require().Equal(amountToSendB, balance) | ||||||||
|
||||||||
balance, err = deps.EvmKeeper.ERC20().BalanceOf(erc20Addr, fromUser, deps.Ctx) | ||||||||
balance, err = deps.EvmKeeper.ERC20().BalanceOf(erc20Addr, fromUser, deps.Ctx, evmObj) | ||||||||
s.Require().NoError(err) | ||||||||
s.Require().Equal( | ||||||||
new(big.Int).Sub(totalSupply, big.NewInt(amountToSendA.Int64()+amountToSendB.Int64())), | ||||||||
|
@@ -122,7 +123,7 @@ func (s *Suite) TestExportInitGenesis() { | |||||||
s.Require().True(funTokens[0].IsMadeFromCoin) | ||||||||
|
||||||||
// Check that fungible token balance of user C is correct | ||||||||
balance, err = deps.EvmKeeper.ERC20().BalanceOf(funTokenAddr, toUserC, deps.Ctx) | ||||||||
balance, err = deps.EvmKeeper.ERC20().BalanceOf(funTokenAddr, toUserC, deps.Ctx, evmObj) | ||||||||
s.Require().NoError(err) | ||||||||
s.Require().Equal(amountToSendC, balance) | ||||||||
} |
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.
Creating a new StateDB here uses the anteCtx, which gets tossed when the tx runs. That means whenever StateDB commits, it's committing to a out of scope ctx.
It's better to just read the bank balances directly via the Cosmos ctx (which is what core.CanTransfer does anyways).