From 8b5f9f44c81dc4ec86593e37d859fbd26dfebb60 Mon Sep 17 00:00:00 2001 From: Piers Powlesland Date: Fri, 1 Nov 2024 13:25:29 +0000 Subject: [PATCH 1/3] Add test to verify lack of race conditions in txpool When handling fee currency transactions. --- e2e_test/e2e_test.go | 65 +++++++++++++++++++++++++++++++++++++++++- ethclient/ethclient.go | 12 +++++++- test/account.go | 12 ++++---- test/node.go | 12 +++++--- 4 files changed, 90 insertions(+), 11 deletions(-) diff --git a/e2e_test/e2e_test.go b/e2e_test/e2e_test.go index 0fb57c4aa6..e945914eed 100644 --- a/e2e_test/e2e_test.go +++ b/e2e_test/e2e_test.go @@ -732,7 +732,7 @@ func testRPCDynamicTxGasPriceWithoutState(t *testing.T, afterGingerbread, altern gasTipCap := big.NewInt(0).Mul(suggestedGasPrice, big.NewInt(2)) // Send one celo from external account 0 to 1 via node 0. - tx, err := accounts[0].SendValueWithDynamicFee(ctx, accounts[1].Address, 1, feeCurrency, gasFeeCap, gasTipCap, network[0]) + tx, err := accounts[0].SendValueWithDynamicFee(ctx, accounts[1].Address, 1, feeCurrency, gasFeeCap, gasTipCap, network[0], 0) require.NoError(t, err) // Wait for the whole network to process the transaction. @@ -1044,3 +1044,66 @@ func TestGetFinalizedBlock(t *testing.T) { // Check latest and finalzed block are the same require.Equal(t, h.Hash(), h2.Hash()) } + +// TestManyFeeCurrencyTransactions is intended to test that we don't have race conditions in the tx pool when handling +// fee currency transactions. It does this by submitting many fee currency transactions from 3 different goroutines over +// a period of roughly 5 seconds which with the configured block time of 1 second means that the transactions should +// span multiple block boundaries with the goal of ensuring that both runReorg and AddRemotes are being called +// concurrently in the txPool. This issue https://github.com/celo-org/celo-blockchain/issues/2318 is occurring somewhat +// randomly and could be the result of some race condition in tx pool handling for fee currency transactions. However +// this test seems to run fairly reliably with the race flag enabled, which seems to indicate that the problem is not a +// result of racy behavior in the tx pool. +func TestManyFeeCurrencyTransactions(t *testing.T) { + ac := test.AccountConfig(3, 3) + gingerbreadBlock := common.Big0 + gc, ec, err := test.BuildConfig(ac, gingerbreadBlock, nil) + require.NoError(t, err) + ec.Istanbul.BlockPeriod = 1 + network, shutdown, err := test.NewNetwork(ac, gc, ec) + require.NoError(t, err) + defer shutdown() + ctx, cancel := context.WithTimeout(context.Background(), time.Second*200) + defer cancel() + + cUSD := common.HexToAddress("0x000000000000000000000000000000000000d008") + cEUR := common.HexToAddress("0x000000000000000000000000000000000000D024") + cREAL := common.HexToAddress("0x000000000000000000000000000000000000d026") + + accounts := test.Accounts(ac.DeveloperAccounts(), gc.ChainConfig()) + + time.Sleep(2 * time.Second) + txsChan := make(chan []*types.Transaction, 3) + for nodeIndex := 0; nodeIndex < len(network); nodeIndex++ { + go func(nodeIndex int) { + txs := make([]*types.Transaction, 0, 3000) + for i := 0; i < 100; i++ { + for _, feeCurrency := range []*common.Address{&cUSD, &cEUR, &cREAL} { + baseFee, err := network[nodeIndex].WsClient.SuggestGasPriceInCurrency(ctx, feeCurrency) + require.NoError(t, err) + tip, err := network[nodeIndex].WsClient.SuggestGasTipCapInCurrency(ctx, feeCurrency) + require.NoError(t, err) + + // Send one celo from external account 0 to 1 via node 0. + tx, err := accounts[nodeIndex].SendValueWithDynamicFee(ctx, accounts[nodeIndex].Address, 1, feeCurrency, baseFee.Add(baseFee, tip), tip, network[nodeIndex], 71000) + require.NoError(t, err) + txs = append(txs, tx) + time.Sleep(10 * time.Millisecond) + } + } + txsChan <- txs + }(nodeIndex) + } + + allTxs := make([]*types.Transaction, 0, 3000*len(network)) + count := 0 + for txs := range txsChan { + allTxs = append(allTxs, txs...) + count++ + if count == len(network) { + break + } + } + + err = network.AwaitTransactions(ctx, allTxs...) + require.NoError(t, err) +} diff --git a/ethclient/ethclient.go b/ethclient/ethclient.go index 8f8b7ae0e6..c99dc74eb8 100644 --- a/ethclient/ethclient.go +++ b/ethclient/ethclient.go @@ -546,7 +546,7 @@ func (ec *Client) SuggestGasPrice(ctx context.Context) (*big.Int, error) { return (*big.Int)(&hex), nil } -// SuggestGasPrice retrieves the currently suggested gas price to allow a timely +// SuggestGasPriceInCurrency retrieves the currently suggested gas price to allow a timely // execution of a transaction. func (ec *Client) SuggestGasPriceInCurrency(ctx context.Context, feeCurrency *common.Address) (*big.Int, error) { var hex hexutil.Big @@ -566,6 +566,16 @@ func (ec *Client) SuggestGasTipCap(ctx context.Context) (*big.Int, error) { return (*big.Int)(&hex), nil } +// SuggestGasTipCapInCurrency retrieves the currently suggested gas tip cap after 1559 to +// allow a timely execution of a transaction. +func (ec *Client) SuggestGasTipCapInCurrency(ctx context.Context, feeCurrency *common.Address) (*big.Int, error) { + var hex hexutil.Big + if err := ec.c.CallContext(ctx, &hex, "eth_maxPriorityFeePerGas", feeCurrency); err != nil { + return nil, err + } + return (*big.Int)(&hex), nil +} + // EstimateGas tries to estimate the gas needed to execute a specific transaction based on // the current pending state of the backend blockchain. There is no guarantee that this is // the true gas limit requirement as other transactions may be added or removed by miners, diff --git a/test/account.go b/test/account.go index 710510c59e..ecc4f81fee 100644 --- a/test/account.go +++ b/test/account.go @@ -70,12 +70,13 @@ func (a *Account) SendCelo(ctx context.Context, recipient common.Address, value // SendCeloWithDynamicFee submits a value transfer transaction via the provided Node to send // celo to the recipient. The submitted transaction is returned. func (a *Account) SendCeloWithDynamicFee(ctx context.Context, recipient common.Address, value int64, gasFeeCap *big.Int, gasTipCap *big.Int, node *Node) (*types.Transaction, error) { - return a.SendValueWithDynamicFee(ctx, recipient, value, nil, gasFeeCap, gasTipCap, node) + return a.SendValueWithDynamicFee(ctx, recipient, value, nil, gasFeeCap, gasTipCap, node, 0) } -// SendValueWithDynamicFee submits a value transfer transaction via the provided Node to send -// celo to the recipient. The submitted transaction is returned. -func (a *Account) SendValueWithDynamicFee(ctx context.Context, recipient common.Address, value int64, feeCurrency *common.Address, gasFeeCap, gasTipCap *big.Int, node *Node) (*types.Transaction, error) { +// SendValueWithDynamicFee submits a value transfer transaction via the provided Node to send celo to the recipient. The +// submitted transaction is returned. Note that gasLimit is optional and if 0 is provided the estimate gas will be +// called to determine the gas limit. +func (a *Account) SendValueWithDynamicFee(ctx context.Context, recipient common.Address, value int64, feeCurrency *common.Address, gasFeeCap, gasTipCap *big.Int, node *Node, gasLimit uint64) (*types.Transaction, error) { var err error // Lazy set nonce if a.Nonce == nil { @@ -101,7 +102,8 @@ func (a *Account) SendValueWithDynamicFee(ctx context.Context, recipient common. feeCurrency, gasFeeCap, gasTipCap, - signer) + signer, + gasLimit) if err != nil { return nil, err diff --git a/test/node.go b/test/node.go index 4b189a4983..18f83c7073 100644 --- a/test/node.go +++ b/test/node.go @@ -665,14 +665,18 @@ func ValueTransferTransactionWithDynamicFee( gasFeeCap *big.Int, gasTipCap *big.Int, signer types.Signer, + gasLimit uint64, ) (*types.Transaction, error) { ctx, cancel := context.WithTimeout(context.Background(), time.Second*2) defer cancel() - msg := ethereum.CallMsg{From: sender, To: &recipient, Value: value, FeeCurrency: feeCurrency} - gasLimit, err := client.EstimateGas(ctx, msg) - if err != nil { - return nil, fmt.Errorf("failed to estimate gas needed: %v", err) + if gasLimit == 0 { + msg := ethereum.CallMsg{From: sender, To: &recipient, Value: value, FeeCurrency: feeCurrency} + var err error + gasLimit, err = client.EstimateGas(ctx, msg) + if err != nil { + return nil, fmt.Errorf("failed to estimate gas needed: %v", err) + } } // Create the transaction and sign it rawTx := types.NewTx(&types.CeloDynamicFeeTx{ From 40b0de01e068476fc072dd63e71582b5e7624f79 Mon Sep 17 00:00:00 2001 From: Piers Powlesland Date: Fri, 1 Nov 2024 14:35:48 +0000 Subject: [PATCH 2/3] Fix race condition in istanbul core --- consensus/istanbul/core/handler.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/consensus/istanbul/core/handler.go b/consensus/istanbul/core/handler.go index a591fb217a..90d2a215da 100644 --- a/consensus/istanbul/core/handler.go +++ b/consensus/istanbul/core/handler.go @@ -36,7 +36,9 @@ func (c *core) Start() error { return err } + c.currentMu.Lock() c.current = roundState + c.currentMu.Unlock() c.roundChangeSetV2 = newRoundChangeSetV2(c.current.ValidatorSet()) // Reset the Round Change timer for the current round to timeout. From d2cc8ca420d0157bf85022b28a420b29ff3f97a2 Mon Sep 17 00:00:00 2001 From: Piers Powlesland Date: Fri, 1 Nov 2024 18:32:31 +0000 Subject: [PATCH 3/3] Reduce likelihood of exceeding AccountQueue Increased time between transactions sending so that accounts don't send more than 64 txs within the block period. --- e2e_test/e2e_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/e2e_test/e2e_test.go b/e2e_test/e2e_test.go index e945914eed..1cf13f0032 100644 --- a/e2e_test/e2e_test.go +++ b/e2e_test/e2e_test.go @@ -1087,7 +1087,7 @@ func TestManyFeeCurrencyTransactions(t *testing.T) { tx, err := accounts[nodeIndex].SendValueWithDynamicFee(ctx, accounts[nodeIndex].Address, 1, feeCurrency, baseFee.Add(baseFee, tip), tip, network[nodeIndex], 71000) require.NoError(t, err) txs = append(txs, tx) - time.Sleep(10 * time.Millisecond) + time.Sleep(16 * time.Millisecond) } } txsChan <- txs