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): proper nonce management in statedb #2130

Merged
merged 7 commits into from
Jan 8, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -51,9 +51,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
Remove unnecessary argument in the `VerifyFee` function, which returns the token
payment required based on the effective fee from the tx data. Improve
documentation.
- [#2127](https://github.com/NibiruChain/nibiru/pull/2127) - fix(vesting): disabled built in auth/vesting module functionality
- [#2125](https://github.com/NibiruChain/nibiru/pull/2125) - feat(evm-precompile):Emit EVM events created to reflect the ABCI events that occur outside the EVM to make sure that block explorers and indexers can find indexed ABCI event information.
- [#2127](https://github.com/NibiruChain/nibiru/pull/2127) - fix(vesting): disabled built in auth/vesting module functionality
- [#2129](https://github.com/NibiruChain/nibiru/pull/2129) - fix(evm): issue with infinite recursion in erc20 funtoken contracts
- [#2130](https://github.com/NibiruChain/nibiru/pull/2130) - fix(evm): proper nonce management in statedb
- [#2134](https://github.com/NibiruChain/nibiru/pull/2134) - fix(evm): query of NIBI should use bank state, not the StateDB
- [#2139](https://github.com/NibiruChain/nibiru/pull/2139) - fix(evm): erc20 born funtoken: properly burn bank coins after converting coin back to erc20
- [#2140](https://github.com/NibiruChain/nibiru/pull/2140) - fix(bank): bank keeper extension now charges gas for the bank operations
Expand Down
82 changes: 76 additions & 6 deletions eth/rpc/backend/backend_suite_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import (
"testing"
"time"

"github.com/cosmos/cosmos-sdk/client/flags"
sdk "github.com/cosmos/cosmos-sdk/types"
gethcommon "github.com/ethereum/go-ethereum/common"
gethcore "github.com/ethereum/go-ethereum/core/types"
Expand Down Expand Up @@ -115,13 +116,12 @@ func (s *BackendSuite) SendNibiViaEthTransfer(
amount *big.Int,
waitForNextBlock bool,
) gethcommon.Hash {
nonce, err := s.backend.GetTransactionCount(s.fundedAccEthAddr, rpc.EthPendingBlockNumber)
s.Require().NoError(err)
nonce := s.getCurrentNonce(s.fundedAccEthAddr)
return SendTransaction(
s,
&gethcore.LegacyTx{
To: &to,
Nonce: uint64(*nonce),
Nonce: uint64(nonce),
Value: amount,
Gas: params.TxGas,
GasPrice: big.NewInt(1),
Expand All @@ -135,20 +135,20 @@ func (s *BackendSuite) DeployTestContract(waitForNextBlock bool) (gethcommon.Has
packedArgs, err := embeds.SmartContract_TestERC20.ABI.Pack("")
s.Require().NoError(err)
bytecodeForCall := append(embeds.SmartContract_TestERC20.Bytecode, packedArgs...)
nonce, err := s.backend.GetTransactionCount(s.fundedAccEthAddr, rpc.EthPendingBlockNumber)
nonce := s.getCurrentNonce(s.fundedAccEthAddr)
s.Require().NoError(err)

txHash := SendTransaction(
s,
&gethcore.LegacyTx{
Nonce: uint64(*nonce),
Nonce: uint64(nonce),
Data: bytecodeForCall,
Gas: 1500_000,
GasPrice: big.NewInt(1),
},
waitForNextBlock,
)
contractAddr := crypto.CreateAddress(s.fundedAccEthAddr, (uint64)(*nonce))
contractAddr := crypto.CreateAddress(s.fundedAccEthAddr, nonce)
return txHash, contractAddr
}

Expand Down Expand Up @@ -188,3 +188,73 @@ func WaitForReceipt(s *BackendSuite, txHash gethcommon.Hash) (*big.Int, *gethcom
}
}
}

// getCurrentNonce returns the current nonce of the funded account
func (s *BackendSuite) getCurrentNonce(address gethcommon.Address) uint64 {
bn, err := s.backend.BlockNumber()
s.Require().NoError(err)
currentHeight := rpc.BlockNumber(bn)

nonce, err := s.backend.GetTransactionCount(address, currentHeight)
s.Require().NoError(err)

return uint64(*nonce)
}

// broadcastSDKTx broadcasts the given SDK transaction and returns the response
func (s *BackendSuite) broadcastSDKTx(sdkTx sdk.Tx) *sdk.TxResponse {
txBytes, err := s.backend.ClientCtx().TxConfig.TxEncoder()(sdkTx)
s.Require().NoError(err)

syncCtx := s.backend.ClientCtx().WithBroadcastMode(flags.BroadcastSync)
rsp, err := syncCtx.BroadcastTx(txBytes)
s.Require().NoError(err)
return rsp
}

// buildContractCreationTx builds a contract creation transaction
func (s *BackendSuite) buildContractCreationTx(nonce uint64) gethcore.Transaction {
packedArgs, err := embeds.SmartContract_TestERC20.ABI.Pack("")
s.Require().NoError(err)
bytecodeForCall := append(embeds.SmartContract_TestERC20.Bytecode, packedArgs...)

creationTx := &gethcore.LegacyTx{
Nonce: nonce,
Data: bytecodeForCall,
Gas: 1_500_000,
GasPrice: big.NewInt(1),
}

signer := gethcore.LatestSignerForChainID(s.ethChainID)
signedTx, err := gethcore.SignNewTx(s.fundedAccPrivateKey, signer, creationTx)
s.Require().NoError(err)

return *signedTx
}

// buildContractCallTx builds a contract call transaction
func (s *BackendSuite) buildContractCallTx(nonce uint64, contractAddr gethcommon.Address) gethcore.Transaction {
//recipient := crypto.CreateAddress(s.fundedAccEthAddr, 29381)
transferAmount := big.NewInt(100)

packedArgs, err := embeds.SmartContract_TestERC20.ABI.Pack(
"transfer",
recipient,
transferAmount,
)
s.Require().NoError(err)

transferTx := &gethcore.LegacyTx{
Nonce: nonce,
Data: packedArgs,
Gas: 100_000,
GasPrice: big.NewInt(1),
To: &contractAddr,
}

signer := gethcore.LatestSignerForChainID(s.ethChainID)
signedTx, err := gethcore.SignNewTx(s.fundedAccPrivateKey, signer, transferTx)
s.Require().NoError(err)

return *signedTx
}
5 changes: 5 additions & 0 deletions eth/rpc/backend/call_tx.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import (
"math/big"

errorsmod "cosmossdk.io/errors"
"github.com/cosmos/cosmos-sdk/client"
"github.com/cosmos/cosmos-sdk/client/flags"
sdk "github.com/cosmos/cosmos-sdk/types"
"github.com/ethereum/go-ethereum/common"
Expand Down Expand Up @@ -338,3 +339,7 @@ func (b *Backend) GasPrice() (*hexutil.Big, error) {

return (*hexutil.Big)(result), nil
}

func (b *Backend) ClientCtx() client.Context {
return b.clientCtx
}
75 changes: 75 additions & 0 deletions eth/rpc/backend/nonce_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,75 @@
package backend_test

import (
sdkmath "cosmossdk.io/math"
codectypes "github.com/cosmos/cosmos-sdk/codec/types"
sdk "github.com/cosmos/cosmos-sdk/types"
authtx "github.com/cosmos/cosmos-sdk/x/auth/tx"
gethcore "github.com/ethereum/go-ethereum/core/types"

"github.com/NibiruChain/nibiru/v2/x/evm"
)

// TestNonceIncrementWithMultipleMsgsTx tests that the nonce is incremented correctly
// when multiple messages are included in a single transaction.
func (s *BackendSuite) TestNonceIncrementWithMultipleMsgsTx() {
nonce := s.getCurrentNonce(s.fundedAccEthAddr)

// Create series of 3 tx messages. Expecting nonce to be incremented by 3
creationTx := s.buildContractCreationTx(nonce)
firstTransferTx := s.buildContractCallTx(nonce+1, testContractAddress)
secondTransferTx := s.buildContractCallTx(nonce+2, testContractAddress)

// Create and broadcast SDK transaction
sdkTx := s.buildSDKTxWithEVMMessages(
creationTx,
firstTransferTx,
secondTransferTx,
)

// Broadcast transaction
rsp := s.broadcastSDKTx(sdkTx)
s.Assert().NotEqual(rsp.Code, 0)
s.Require().NoError(s.network.WaitForNextBlock())
Comment on lines +32 to +33
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Assertion should check for success, not failure

The assertion is checking for non-zero response code, which typically indicates failure. For a successful transaction, the response code should be zero.

-    s.Assert().NotEqual(rsp.Code, 0)
+    s.Assert().Equal(rsp.Code, 0)
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
s.Assert().NotEqual(rsp.Code, 0)
s.Require().NoError(s.network.WaitForNextBlock())
s.Assert().Equal(rsp.Code, 0)
s.Require().NoError(s.network.WaitForNextBlock())


// Expected nonce should be incremented by 3
currentNonce := s.getCurrentNonce(s.fundedAccEthAddr)
s.Assert().Equal(nonce+3, currentNonce)

// Assert all transactions included in block
for _, tx := range []gethcore.Transaction{creationTx, firstTransferTx, secondTransferTx} {
blockNum, blockHash := WaitForReceipt(s, tx.Hash())
s.Require().NotNil(blockNum)
s.Require().NotNil(blockHash)
}
}

// buildSDKTxWithEVMMessages creates an SDK transaction with EVM messages
func (s *BackendSuite) buildSDKTxWithEVMMessages(txs ...gethcore.Transaction) sdk.Tx {
msgs := make([]sdk.Msg, len(txs))
for i, tx := range txs {
msg := &evm.MsgEthereumTx{}
err := msg.FromEthereumTx(&tx)
s.Require().NoError(err)
msgs[i] = msg
}

option, err := codectypes.NewAnyWithValue(&evm.ExtensionOptionsEthereumTx{})
s.Require().NoError(err)

txBuilder, _ := s.backend.ClientCtx().TxConfig.NewTxBuilder().(authtx.ExtensionOptionsTxBuilder)
txBuilder.SetExtensionOptions(option)
err = txBuilder.SetMsgs(msgs...)
s.Require().NoError(err)

// Set fees for all messages
totalGas := uint64(0)
for _, tx := range txs {
totalGas += tx.Gas()
}
fees := sdk.NewCoins(sdk.NewCoin("unibi", sdkmath.NewIntFromUint64(totalGas)))
txBuilder.SetFeeAmount(fees)
txBuilder.SetGasLimit(totalGas)

return txBuilder.GetTx()
}
11 changes: 6 additions & 5 deletions x/evm/keeper/msg_server.go
Original file line number Diff line number Diff line change
Expand Up @@ -335,18 +335,17 @@ func (k *Keeper) ApplyEvmMsg(ctx sdk.Context,
return nil, evmObj, errors.Wrapf(err, "ApplyEvmMsg: invalid wei amount %s", msg.Value())
}

// take over the nonce management from evm:
// - reset sender's nonce to msg.Nonce() before calling evm.
// - increase sender's nonce by one no matter the result.
stateDB.SetNonce(sender.Address(), msg.Nonce())
if contractCreation {
// take over the nonce management from evm:
// - reset sender's nonce to msg.Nonce() before calling evm.
// - increase sender's nonce by one no matter the result.
stateDB.SetNonce(sender.Address(), msg.Nonce())
ret, _, leftoverGas, vmErr = evmObj.Create(
sender,
msg.Data(),
leftoverGas,
msgWei,
)
stateDB.SetNonce(sender.Address(), msg.Nonce()+1)
} else {
ret, leftoverGas, vmErr = evmObj.Call(
sender,
Expand All @@ -356,6 +355,8 @@ func (k *Keeper) ApplyEvmMsg(ctx sdk.Context,
msgWei,
)
}
// Increment nonce after processing the message
stateDB.SetNonce(sender.Address(), msg.Nonce()+1)

// EVM execution error needs to be available for the JSON-RPC client
var vmError string
Expand Down
6 changes: 3 additions & 3 deletions x/evm/statedb/journal_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -156,10 +156,10 @@ func (s *Suite) TestComplexJournalChanges() {
stateDB, ok = evmObj.StateDB.(*statedb.StateDB)
s.Require().True(ok, "error retrieving StateDB from the EVM")

s.T().Log("Expect exactly 0 dirty journal entry for the precompile snapshot")
if stateDB.DebugDirtiesCount() != 0 {
s.T().Log("Expect exactly 1 dirty journal entry for the precompile snapshot")
if stateDB.DebugDirtiesCount() != 1 {
debugDirtiesCountMismatch(stateDB, s.T())
s.FailNow("expected 0 dirty journal changes")
s.FailNow("expected 1 dirty journal change")
}

s.T().Log("Expect no change since the StateDB has not been committed")
Expand Down
Loading