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.

Sign up for GitHub

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

Jump to bottom

add market gas price to simulate duplicate nonce behavior #7

Merged
merged 4 commits into from
Sep 9, 2021
Merged

add market gas price to simulate duplicate nonce behavior #7

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
62079ae
add market gas price to simulate duplicate nonce behavior
susannapaxos Aug 28, 2021
16470f5
Merge branch 'paxosglobal:master' into nonce
susannapaxos Sep 3, 2021
8cc712d
combine two loops into one to add txes to block in simulated backend …
susannapaxos Sep 3, 2021
3647e5e
add test case coverage for simulated nonce test with dynamic tx type
susannapaxos Sep 5, 2021
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
64 changes: 57 additions & 7 deletions accounts/abi/bind/backends/simulated.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,13 +63,16 @@ type SimulatedBackend struct {
database ethdb.Database // In memory database to store our testing data
blockchain *core.BlockChain // Ethereum blockchain to handle the consensus

mu sync.Mutex
pendingBlock *types.Block // Currently pending block that will be imported on request
pendingState *state.StateDB // Currently pending state that will be the active on request
mu sync.Mutex
stuckTransactions types.Transactions // holds onto all txes that don't go into the pending block due to low gas
pendingBlock *types.Block // Currently pending block that will be imported on request
pendingState *state.StateDB // Currently pending state that will be the active on request

events *filters.EventSystem // Event system for filtering log events live

config *params.ChainConfig

marketGasPrice *big.Int // minimum gas price it needs to have the block mined in simulated blockchain
}

// NewSimulatedBackendWithDatabase creates a new binding backend based on the given database
Expand Down Expand Up @@ -109,12 +112,37 @@ func (b *SimulatedBackend) Commit() {
b.mu.Lock()
defer b.mu.Unlock()

if _, err := b.blockchain.InsertChain([]*types.Block{b.pendingBlock}); err != nil {
// Get the last block
parentBlock := b.blockchain.GetBlockByHash(b.pendingBlock.ParentHash())
var remainingTx types.Transactions

blocks, _ := core.GenerateChain(b.config, parentBlock, ethash.NewFaker(), b.database, 1,
func(number int, block *core.BlockGen) {

for _, tx := range b.pendingBlock.Transactions() {
if b.marketGasPrice == nil || b.marketGasPrice.Cmp(tx.GasPrice()) <= 0 {
block.AddTxWithChain(b.blockchain, tx)
} else {
remainingTx = append(remainingTx, tx)
}
}

for _, tx := range b.stuckTransactions {
if b.marketGasPrice == nil || b.marketGasPrice.Cmp(tx.GasPrice()) <= 0 {
block.AddTxWithChain(b.blockchain, tx)
} else {
remainingTx = append(remainingTx, tx)
}
}
Copy link
Member

Choose a reason for hiding this comment

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

nit:

                        for _, tx := range append(b.pendingBlock.Transactions(), b.stuckTransactions...) {
				if b.marketGasPrice == nil || b.marketGasPrice.Cmp(tx.GasPrice()) <= 0 {
					block.AddTxWithChain(b.blockchain, tx)
				} else {
					remainingTx = append(remainingTx, tx)
				}
			}

Can this work with a single loop? Not saving anything just less lines of code

Copy link
Author

Choose a reason for hiding this comment

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

thanks for the suggestion, its neat!

})

if _, err := b.blockchain.InsertChain([]*types.Block{blocks[0]}); err != nil {
panic(err) // This cannot happen unless the simulator is wrong, fail in that case
}
// Using the last inserted block here makes it possible to build on a side
// chain after a fork.
b.rollback(b.pendingBlock)
b.stuckTransactions = remainingTx
Copy link

@lsharir lsharir Sep 3, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

can we instead just put them back on the pending block?
similarly to how SendTransaction recreates the pendingBlock

Copy link
Author

Choose a reason for hiding this comment

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

we cannot put them to the pending block, the txes in the pending block determine whats the next nonce available. if they are stuck, they should not be in pending block. because that means we cannot send the transaction with the same nonce. it will be invalid/nonce too low.

b.rollback(blocks[0])
}

// Rollback aborts all pending transactions, reverting to the last committed state.
Expand All @@ -126,8 +154,8 @@ func (b *SimulatedBackend) Rollback() {
}

func (b *SimulatedBackend) rollback(parent *types.Block) {
blocks, _ := core.GenerateChain(b.config, parent, ethash.NewFaker(), b.database, 1, func(int, *core.BlockGen) {})

blocks, _ := core.GenerateChain(b.config, parent, ethash.NewFaker(), b.database, 1, func(int, *core.BlockGen) {})
susannapaxos marked this conversation as resolved.
Show resolved Hide resolved
b.pendingBlock = blocks[0]
b.pendingState, _ = state.New(b.pendingBlock.Root(), b.blockchain.StateCache(), nil)
}
Expand Down Expand Up @@ -210,6 +238,15 @@ func (b *SimulatedBackend) NonceAt(ctx context.Context, contract common.Address,
return stateDB.GetNonce(contract), nil
}

// SetMarketGasPrice sets the simulated blockchain's market gas price, which is
// the minimum price point for the transaction to be committed to the simulated chain.
func (b *SimulatedBackend) SetMarketGasPrice(gasPrice int64) {
gitteri marked this conversation as resolved.
Show resolved Hide resolved
b.mu.Lock()
defer b.mu.Unlock()

b.marketGasPrice = big.NewInt(gasPrice)
}

// StorageAt returns the value of key in the storage of an account in the blockchain.
func (b *SimulatedBackend) StorageAt(ctx context.Context, contract common.Address, key common.Hash, blockNumber *big.Int) ([]byte, error) {
b.mu.Lock()
Expand Down Expand Up @@ -642,17 +679,30 @@ func (b *SimulatedBackend) SendTransaction(ctx context.Context, tx *types.Transa
if tx.Nonce() != nonce {
panic(fmt.Errorf("invalid transaction nonce: got %d, want %d", tx.Nonce(), nonce))
}
var remainingTxes types.Transactions
// Include tx in chain
blocks, _ := core.GenerateChain(b.config, block, ethash.NewFaker(), b.database, 1, func(number int, block *core.BlockGen) {
for _, tx := range b.pendingBlock.Transactions() {
// if market gas price is not set or tx gas price is lower than the set market gas price
if b.marketGasPrice == nil || b.marketGasPrice.Cmp(tx.GasPrice()) <= 0 {
block.AddTxWithChain(b.blockchain, tx)
} else {
remainingTxes = append(remainingTxes, tx)
}
}
if b.marketGasPrice == nil || b.marketGasPrice.Cmp(tx.GasPrice()) <= 0 {
block.AddTxWithChain(b.blockchain, tx)
} else {
remainingTxes = append(remainingTxes, tx)
}
block.AddTxWithChain(b.blockchain, tx)
})

stateDB, _ := b.blockchain.State()

b.pendingBlock = blocks[0]
b.pendingState, _ = state.New(b.pendingBlock.Root(), stateDB.Database(), nil)
b.stuckTransactions = remainingTxes

return nil
}

Expand Down
129 changes: 129 additions & 0 deletions accounts/abi/bind/backends/simulated_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,9 @@ import (
"testing"
"time"

"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"

"github.com/ethereum/go-ethereum"
"github.com/ethereum/go-ethereum/accounts/abi"
"github.com/ethereum/go-ethereum/accounts/abi/bind"
Expand Down Expand Up @@ -1313,3 +1316,129 @@ func TestForkResendTx(t *testing.T) {
t.Errorf("TX included in wrong block: %d", h)
}
}

// TestLowGasTxNotBeingMined checks that the lower gas fee tx with same nonce does not get mined
// Steps:
// 1. Send a tx lower than the set market gas price
// 2. Commit to chain, check nonce stays the same
// 3. Send a tx meets the market gas price with same nonce.
// 4. Commit to chain
// 5. Check tx2 is not in pending state and nonce has increased
func TestLowGasTxNotBeingMined(t *testing.T) {
testAddr := crypto.PubkeyToAddress(testKey.PublicKey)
var gas uint64 = 21000

sim := simTestBackend(testAddr)
defer sim.Close()

bgCtx := context.Background()

// Create tx and send
head, _ := sim.HeaderByNumber(context.Background(), nil) // Should be child's, good enough
gasPrice := new(big.Int).Add(head.BaseFee, big.NewInt(1))
sim.SetMarketGasPrice(int64(gasPrice.Uint64()))

lowGasFeeTx := types.NewTx(&types.LegacyTx{
Copy link

Choose a reason for hiding this comment

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

nit: let's try both legacy and dynamic fee txs in this test

Copy link
Author

Choose a reason for hiding this comment

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

added both legacy and dynamic fee txs in both test cases!

Nonce: uint64(0),
To: &testAddr,
Value: big.NewInt(1),
GasPrice: new(big.Int).Sub(gasPrice, big.NewInt(1)),
Gas: gas,
})
signedTx, err := types.SignTx(lowGasFeeTx, types.HomesteadSigner{}, testKey)
require.NoError(t, err)

// send tx to simulated backend
err = sim.SendTransaction(bgCtx, signedTx)
require.NoError(t, err)
sim.Commit()

// expect nonce be the same because low gas fee tx will not be mined
nonce, err := sim.PendingNonceAt(bgCtx, testAddr)
require.NoError(t, err)
assert.Equal(t, uint64(0), nonce)

// send tx with higher gas fee
sufficientGasFeeTx := types.NewTx(&types.LegacyTx{
Nonce: uint64(0),
To: &testAddr,
Value: big.NewInt(1),
GasPrice: gasPrice,
Gas: gas,
})

signedSufficientTx, err := types.SignTx(sufficientGasFeeTx, types.HomesteadSigner{}, testKey)
require.NoError(t, err)

err = sim.SendTransaction(bgCtx, signedSufficientTx)
require.NoError(t, err)
sim.Commit()

// the higher gas transaction should have been mined
_, isPending, err := sim.TransactionByHash(bgCtx, signedSufficientTx.Hash())
require.NoError(t, err)
assert.False(t, isPending)

// expect nonce has increased
nonce, err = sim.PendingNonceAt(bgCtx, testAddr)
require.NoError(t, err)
assert.Equal(t, uint64(1), nonce)
}

// TestLowGasTxGetMinedOnceGasFeeDropped checks that lower gas fee txes still stay in mem pool
// and get mined once gas fee requirement has been met
// Steps:
// 1. Send a tx lower than the set market gas price
// 2. Commit to chain, tx does not get mined
// 3. Gas fee drops, commit again
// 4. Check tx get mined
func TestLowGasTxGetMinedOnceGasFeeDropped(t *testing.T) {
testAddr := crypto.PubkeyToAddress(testKey.PublicKey)
var gas uint64 = 21000

sim := simTestBackend(testAddr)
defer sim.Close()

bgCtx := context.Background()

// Create tx and send
head, _ := sim.HeaderByNumber(context.Background(), nil) // Should be child's, good enough
normalGasPrice := new(big.Int).Add(head.BaseFee, big.NewInt(1))
highGasPrice := new(big.Int).Add(head.BaseFee, big.NewInt(5))

sim.SetMarketGasPrice(int64(highGasPrice.Uint64()))

normalGasFeeTx := types.NewTx(&types.LegacyTx{
Nonce: uint64(0),
To: &testAddr,
Value: big.NewInt(1),
GasPrice: normalGasPrice,
Gas: gas,
})
signedTx, err := types.SignTx(normalGasFeeTx, types.HomesteadSigner{}, testKey)
require.NoError(t, err)

// send tx to simulated backend
err = sim.SendTransaction(bgCtx, signedTx)
require.NoError(t, err)
sim.Commit()

// check that the nonce stays the same because nothing has been mined
pendingNonce, err := sim.PendingNonceAt(bgCtx, testAddr)
require.NoError(t, err)
assert.Equal(t, uint64(0), pendingNonce)
Copy link

Choose a reason for hiding this comment

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

let's do another commit and nonce check after this one? so we go through the cycle of passing forward the stuckTransactions twice.

Copy link
Author

Choose a reason for hiding this comment

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

this test set up is the following
(1). Send a tx lower than the set market gas price >> tx ends up in stuck transactions
(2). call commit to chain >> tx stills in stuck transactions
(3). drop gas fee, call commit to chain >> stuck transactions txes get mined.

hmmm, are you suggesting after (3) we should commit again even though we dont have new tx and nothing in stuck transactions?


// gas fee has dropped
sim.SetMarketGasPrice(int64(normalGasPrice.Uint64()))
sim.Commit()

// nonce has increased
pendingNonce, err = sim.PendingNonceAt(bgCtx, testAddr)
require.NoError(t, err)
assert.Equal(t, uint64(1), pendingNonce)

// the transaction should have been mined
_, isPending, err := sim.TransactionByHash(bgCtx, signedTx.Hash())
require.NoError(t, err)
assert.False(t, isPending)
}