From 1f14c9e2374c2ca4d3f81e4287cc8f21448948bf Mon Sep 17 00:00:00 2001 From: Jared Wasinger Date: Thu, 25 Apr 2024 17:39:10 -0700 Subject: [PATCH 1/6] eth/catalyst: add test case for on-demand dev mode that spams a lot of transactions/withdrawals --- eth/catalyst/simulated_beacon_test.go | 83 ++++++++++++++++++++++++++- 1 file changed, 80 insertions(+), 3 deletions(-) diff --git a/eth/catalyst/simulated_beacon_test.go b/eth/catalyst/simulated_beacon_test.go index bb10938c359d..16ff7bb7972c 100644 --- a/eth/catalyst/simulated_beacon_test.go +++ b/eth/catalyst/simulated_beacon_test.go @@ -18,7 +18,9 @@ package catalyst import ( "context" + "github.com/ethereum/go-ethereum/log" "math/big" + "os" "testing" "time" @@ -35,7 +37,7 @@ import ( "github.com/ethereum/go-ethereum/params" ) -func startSimulatedBeaconEthService(t *testing.T, genesis *core.Genesis) (*node.Node, *eth.Ethereum, *SimulatedBeacon) { +func startSimulatedBeaconEthService(t *testing.T, genesis *core.Genesis, period uint64) (*node.Node, *eth.Ethereum, *SimulatedBeacon) { t.Helper() n, err := node.New(&node.Config{ @@ -55,7 +57,7 @@ func startSimulatedBeaconEthService(t *testing.T, genesis *core.Genesis) (*node. t.Fatal("can't create eth service:", err) } - simBeacon, err := NewSimulatedBeacon(1, ethservice) + simBeacon, err := NewSimulatedBeacon(period, ethservice) if err != nil { t.Fatal("can't create simulated beacon:", err) } @@ -87,7 +89,7 @@ func TestSimulatedBeaconSendWithdrawals(t *testing.T) { // short period (1 second) for testing purposes var gasLimit uint64 = 10_000_000 genesis := core.DeveloperGenesisBlock(gasLimit, &testAddr) - node, ethService, mock := startSimulatedBeaconEthService(t, genesis) + node, ethService, mock := startSimulatedBeaconEthService(t, genesis, 1) _ = mock defer node.Close() @@ -140,3 +142,78 @@ func TestSimulatedBeaconSendWithdrawals(t *testing.T) { } } } + +// Tests that zero-period dev mode can handle a lot of simultaneous +// transactions/withdrawals +func TestOnDemandSpam(t *testing.T) { + var withdrawals []types.Withdrawal + txs := make(map[common.Hash]*types.Transaction) + + var ( + // testKey is a private key to use for funding a tester account. + testKey, _ = crypto.HexToECDSA("b71c71a67e1177ad4e901695e1b4b9ee17ae16c6668d313eac2f96dbcda3f291") + + // testAddr is the Ethereum address of the tester account. + testAddr = crypto.PubkeyToAddress(testKey.PublicKey) + ) + + // short period (1 second) for testing purposes + var gasLimit uint64 = 10_000_000 + genesis := core.DeveloperGenesisBlock(gasLimit, &testAddr) + node, ethService, mock := startSimulatedBeaconEthService(t, genesis, 0) + _ = mock + defer node.Close() + + api := &api{mock} + go api.loop() + + chainHeadCh := make(chan core.ChainHeadEvent, 10) + subscription := ethService.BlockChain().SubscribeChainHeadEvent(chainHeadCh) + defer subscription.Unsubscribe() + + // generate some withdrawals + for i := 0; i < 0; i++ { + withdrawals = append(withdrawals, types.Withdrawal{Index: uint64(i)}) + if err := mock.withdrawals.add(&withdrawals[i]); err != nil { + t.Fatal("addWithdrawal failed", err) + } + } + + log.SetDefault(log.NewLogger(log.NewTerminalHandlerWithLevel(os.Stdout, log.LevelDebug, true))) + // generate a bunch of transactions + signer := types.NewEIP155Signer(ethService.BlockChain().Config().ChainID) + for i := 0; i < 20000; i++ { + tx, err := types.SignTx(types.NewTransaction(uint64(i), common.Address{}, big.NewInt(1000), params.TxGas, big.NewInt(params.InitialBaseFee), nil), signer, testKey) + if err != nil { + t.Fatalf("error signing transaction, err=%v", err) + } + txs[tx.Hash()] = tx + + if err := ethService.APIBackend.SendTx(context.Background(), tx); err != nil { + t.Fatal("SendTx failed", err) + } + } + + includedTxs := make(map[common.Hash]struct{}) + var includedWithdrawals []uint64 + + timer := time.NewTimer(12 * time.Second) + for { + select { + case evt := <-chainHeadCh: + for _, includedTx := range evt.Block.Transactions() { + includedTxs[includedTx.Hash()] = struct{}{} + } + for _, includedWithdrawal := range evt.Block.Withdrawals() { + includedWithdrawals = append(includedWithdrawals, includedWithdrawal.Index) + } + + // ensure all withdrawals/txs included. this will take two blocks b/c number of withdrawals > 10 + if len(includedTxs) == len(txs) && len(includedWithdrawals) == len(withdrawals) { + return + } + case <-timer.C: + t.Fatal("timed out without including all withdrawals/txs") + } + } +} From d4acec33101c6021f232b858b190c8ea114025cd Mon Sep 17 00:00:00 2001 From: Jared Wasinger Date: Thu, 25 Apr 2024 17:44:30 -0700 Subject: [PATCH 2/6] eth/catalyst: fix deadlock by committing blocks in separate go-routines --- eth/catalyst/simulated_beacon_api.go | 26 +++++++++++++++++++------- 1 file changed, 19 insertions(+), 7 deletions(-) diff --git a/eth/catalyst/simulated_beacon_api.go b/eth/catalyst/simulated_beacon_api.go index 73d0a5921d83..393ec7377264 100644 --- a/eth/catalyst/simulated_beacon_api.go +++ b/eth/catalyst/simulated_beacon_api.go @@ -18,6 +18,7 @@ package catalyst import ( "context" + "sync" "time" "github.com/ethereum/go-ethereum/common" @@ -32,8 +33,9 @@ type api struct { func (a *api) loop() { var ( - newTxs = make(chan core.NewTxsEvent) - sub = a.sim.eth.TxPool().SubscribeTransactions(newTxs, true) + newTxs = make(chan core.NewTxsEvent) + sub = a.sim.eth.TxPool().SubscribeTransactions(newTxs, true) + commitMu = sync.Mutex{} ) defer sub.Unsubscribe() @@ -42,12 +44,22 @@ func (a *api) loop() { case <-a.sim.shutdownCh: return case w := <-a.sim.withdrawals.pending: - withdrawals := append(a.sim.withdrawals.gatherPending(9), w) - if err := a.sim.sealBlock(withdrawals, uint64(time.Now().Unix())); err != nil { - log.Warn("Error performing sealing work", "err", err) - } + go func() { + commitMu.Lock() + defer commitMu.Unlock() + + withdrawals := append(a.sim.withdrawals.gatherPending(9), w) + if err := a.sim.sealBlock(withdrawals, uint64(time.Now().Unix())); err != nil { + log.Warn("Error performing sealing work", "err", err) + } + }() case <-newTxs: - a.sim.Commit() + go func() { + commitMu.Lock() + defer commitMu.Unlock() + + a.sim.Commit() + }() } } } From 77bea8a694f4e4940c38e0ab6af8d6a32e5ef977 Mon Sep 17 00:00:00 2001 From: Jared Wasinger Date: Thu, 25 Apr 2024 18:42:38 -0700 Subject: [PATCH 3/6] remove log statement --- eth/catalyst/simulated_beacon_test.go | 3 --- 1 file changed, 3 deletions(-) diff --git a/eth/catalyst/simulated_beacon_test.go b/eth/catalyst/simulated_beacon_test.go index 16ff7bb7972c..37fc6b21ec60 100644 --- a/eth/catalyst/simulated_beacon_test.go +++ b/eth/catalyst/simulated_beacon_test.go @@ -18,9 +18,7 @@ package catalyst import ( "context" - "github.com/ethereum/go-ethereum/log" "math/big" - "os" "testing" "time" @@ -179,7 +177,6 @@ func TestOnDemandSpam(t *testing.T) { } } - log.SetDefault(log.NewLogger(log.NewTerminalHandlerWithLevel(os.Stdout, log.LevelDebug, true))) // generate a bunch of transactions signer := types.NewEIP155Signer(ethService.BlockChain().Config().ChainID) for i := 0; i < 20000; i++ { From bd397091e6fb460c40b91dae0c23b98d4125af8d Mon Sep 17 00:00:00 2001 From: Jared Wasinger Date: Thu, 25 Apr 2024 21:07:04 -0700 Subject: [PATCH 4/6] up test timeout to see if it passes remote CI --- eth/catalyst/simulated_beacon_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/eth/catalyst/simulated_beacon_test.go b/eth/catalyst/simulated_beacon_test.go index 37fc6b21ec60..0c085f137518 100644 --- a/eth/catalyst/simulated_beacon_test.go +++ b/eth/catalyst/simulated_beacon_test.go @@ -194,7 +194,7 @@ func TestOnDemandSpam(t *testing.T) { includedTxs := make(map[common.Hash]struct{}) var includedWithdrawals []uint64 - timer := time.NewTimer(12 * time.Second) + timer := time.NewTimer(20 * time.Second) for { select { case evt := <-chainHeadCh: From a52979dc012405d784829d61523b39c8b2c46b6c Mon Sep 17 00:00:00 2001 From: Jared Wasinger Date: Thu, 27 Jun 2024 19:14:30 -0700 Subject: [PATCH 5/6] eth/catalyst: move manual txpool.Sync invocation into zero-period block sealing loop, and out of fcu --- eth/catalyst/api.go | 22 +++++----------------- eth/catalyst/simulated_beacon.go | 2 +- eth/catalyst/simulated_beacon_api.go | 16 +++++++++++++++- 3 files changed, 21 insertions(+), 19 deletions(-) diff --git a/eth/catalyst/api.go b/eth/catalyst/api.go index e279d168fe19..126daaad5edc 100644 --- a/eth/catalyst/api.go +++ b/eth/catalyst/api.go @@ -184,7 +184,7 @@ func (api *ConsensusAPI) ForkchoiceUpdatedV1(update engine.ForkchoiceStateV1, pa return engine.STATUS_INVALID, engine.InvalidParams.With(errors.New("forkChoiceUpdateV1 called post-shanghai")) } } - return api.forkchoiceUpdated(update, payloadAttributes, engine.PayloadV1, false) + return api.forkchoiceUpdated(update, payloadAttributes, engine.PayloadV1) } // ForkchoiceUpdatedV2 is equivalent to V1 with the addition of withdrawals in the payload @@ -207,7 +207,7 @@ func (api *ConsensusAPI) ForkchoiceUpdatedV2(update engine.ForkchoiceStateV1, pa return engine.STATUS_INVALID, engine.UnsupportedFork.With(errors.New("forkchoiceUpdatedV2 must only be called with paris and shanghai payloads")) } } - return api.forkchoiceUpdated(update, params, engine.PayloadV2, false) + return api.forkchoiceUpdated(update, params, engine.PayloadV2) } // ForkchoiceUpdatedV3 is equivalent to V2 with the addition of parent beacon block root @@ -228,10 +228,10 @@ func (api *ConsensusAPI) ForkchoiceUpdatedV3(update engine.ForkchoiceStateV1, pa // hash, even if params are wrong. To do this we need to split up // forkchoiceUpdate into a function that only updates the head and then a // function that kicks off block construction. - return api.forkchoiceUpdated(update, params, engine.PayloadV3, false) + return api.forkchoiceUpdated(update, params, engine.PayloadV3) } -func (api *ConsensusAPI) forkchoiceUpdated(update engine.ForkchoiceStateV1, payloadAttributes *engine.PayloadAttributes, payloadVersion engine.PayloadVersion, simulatorMode bool) (engine.ForkChoiceResponse, error) { +func (api *ConsensusAPI) forkchoiceUpdated(update engine.ForkchoiceStateV1, payloadAttributes *engine.PayloadAttributes, payloadVersion engine.PayloadVersion) (engine.ForkChoiceResponse, error) { api.forkchoiceLock.Lock() defer api.forkchoiceLock.Unlock() @@ -374,19 +374,7 @@ func (api *ConsensusAPI) forkchoiceUpdated(update engine.ForkchoiceStateV1, payl if api.localBlocks.has(id) { return valid(&id), nil } - // If the beacon chain is ran by a simulator, then transaction insertion, - // block insertion and block production will happen without any timing - // delay between them. This will cause flaky simulator executions due to - // the transaction pool running its internal reset operation on a back- - // ground thread. To avoid the racey behavior - in simulator mode - the - // pool will be explicitly blocked on its reset before continuing to the - // block production below. - if simulatorMode { - if err := api.eth.TxPool().Sync(); err != nil { - log.Error("Failed to sync transaction pool", "err", err) - return valid(nil), engine.InvalidPayloadAttributes.With(err) - } - } + payload, err := api.eth.Miner().BuildPayload(args) if err != nil { log.Error("Failed to build payload", "err", err) diff --git a/eth/catalyst/simulated_beacon.go b/eth/catalyst/simulated_beacon.go index fecd83f2762c..389e5223961c 100644 --- a/eth/catalyst/simulated_beacon.go +++ b/eth/catalyst/simulated_beacon.go @@ -164,7 +164,7 @@ func (c *SimulatedBeacon) sealBlock(withdrawals []*types.Withdrawal, timestamp u Withdrawals: withdrawals, Random: random, BeaconRoot: &common.Hash{}, - }, engine.PayloadV3, true) + }, engine.PayloadV3) if err != nil { return err } diff --git a/eth/catalyst/simulated_beacon_api.go b/eth/catalyst/simulated_beacon_api.go index 393ec7377264..414077d474b4 100644 --- a/eth/catalyst/simulated_beacon_api.go +++ b/eth/catalyst/simulated_beacon_api.go @@ -47,7 +47,17 @@ func (a *api) loop() { go func() { commitMu.Lock() defer commitMu.Unlock() - + // When the beacon chain is ran by a simulator, then transaction insertion, + // block insertion and block production will happen without any timing + // delay between them. This will cause flaky simulator executions due to + // the transaction pool running its internal reset operation on a back- + // ground thread. To avoid the racey behavior - in simulator mode - the + // pool will be explicitly blocked on its reset before continuing to the + // block production below. + if err := a.sim.eth.TxPool().Sync(); err != nil { + log.Error("Failed to sync transaction pool", "err", err) + return + } withdrawals := append(a.sim.withdrawals.gatherPending(9), w) if err := a.sim.sealBlock(withdrawals, uint64(time.Now().Unix())); err != nil { log.Warn("Error performing sealing work", "err", err) @@ -58,6 +68,10 @@ func (a *api) loop() { commitMu.Lock() defer commitMu.Unlock() + if err := a.sim.eth.TxPool().Sync(); err != nil { + log.Error("Failed to sync transaction pool", "err", err) + return + } a.sim.Commit() }() } From d0e94166038d79135c7ddd8325284fa9c442c9ef Mon Sep 17 00:00:00 2001 From: Felix Lange Date: Fri, 28 Jun 2024 14:36:04 +0200 Subject: [PATCH 6/6] Update simulated_beacon_test.go --- eth/catalyst/simulated_beacon_test.go | 1 + 1 file changed, 1 insertion(+) diff --git a/eth/catalyst/simulated_beacon_test.go b/eth/catalyst/simulated_beacon_test.go index 0c085f137518..0d447bfd76fb 100644 --- a/eth/catalyst/simulated_beacon_test.go +++ b/eth/catalyst/simulated_beacon_test.go @@ -195,6 +195,7 @@ func TestOnDemandSpam(t *testing.T) { var includedWithdrawals []uint64 timer := time.NewTimer(20 * time.Second) + defer timer.Stop() for { select { case evt := <-chainHeadCh: