From 8d2920bbc4cd427495eaa2e775421a0375615938 Mon Sep 17 00:00:00 2001 From: Pepper Lebeck-Jobe Date: Thu, 6 Mar 2025 11:54:50 +0100 Subject: [PATCH 1/6] Avoid a race in the SimulatedBeacon Stop call Fixes: #31327 --- eth/catalyst/simulated_beacon_api.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/eth/catalyst/simulated_beacon_api.go b/eth/catalyst/simulated_beacon_api.go index 668780531501..515908f86ca5 100644 --- a/eth/catalyst/simulated_beacon_api.go +++ b/eth/catalyst/simulated_beacon_api.go @@ -71,6 +71,10 @@ func (a *simulatedBeaconAPI) loop() { break } a.sim.Commit() + // Avoid a race condition where tx pool is terminated during Commit. + if err := a.sim.eth.TxPool().Sync(); err != nil { + break + } } } }() From 7fab62da4d78ca9dc8e000d1c22563e489f58747 Mon Sep 17 00:00:00 2001 From: Pepper Lebeck-Jobe Date: Fri, 7 Mar 2025 13:13:48 +0100 Subject: [PATCH 2/6] Introduce fallibleCommit which can error This allows the inner loop in the case of "on-demand" commits to bail out if the txPool has been terminated before the doCommit channel is closed. This has been tested with a known-flaky test which, before this commit was spinlooping and logging unfathomable reams of warning messages. Now, when the race occurs, only a single instance of the warning is logged. --- eth/catalyst/simulated_beacon.go | 9 ++++++++- eth/catalyst/simulated_beacon_api.go | 12 ++++++++---- 2 files changed, 16 insertions(+), 5 deletions(-) diff --git a/eth/catalyst/simulated_beacon.go b/eth/catalyst/simulated_beacon.go index c71add93bc99..286757054e36 100644 --- a/eth/catalyst/simulated_beacon.go +++ b/eth/catalyst/simulated_beacon.go @@ -302,11 +302,18 @@ func (c *SimulatedBeacon) setCurrentState(headHash, finalizedHash common.Hash) { // Commit seals a block on demand. func (c *SimulatedBeacon) Commit() common.Hash { + hash, _ := c.fallibleCommit() + return hash +} + +// fallibleCommit attempts to seal a block on demand, but may return an error. +func (c *SimulatedBeacon) fallibleCommit() (common.Hash, error) { withdrawals := c.withdrawals.pop(10) if err := c.sealBlock(withdrawals, uint64(time.Now().Unix())); err != nil { log.Warn("Error performing sealing work", "err", err) + return common.Hash{}, err } - return c.eth.BlockChain().CurrentBlock().Hash() + return c.eth.BlockChain().CurrentBlock().Hash(), nil } // Rollback un-sends previously added transactions. diff --git a/eth/catalyst/simulated_beacon_api.go b/eth/catalyst/simulated_beacon_api.go index 515908f86ca5..5955b033bdb2 100644 --- a/eth/catalyst/simulated_beacon_api.go +++ b/eth/catalyst/simulated_beacon_api.go @@ -70,11 +70,15 @@ func (a *simulatedBeaconAPI) loop() { if executable, _ := a.sim.eth.TxPool().Stats(); executable == 0 { break } - a.sim.Commit() - // Avoid a race condition where tx pool is terminated during Commit. - if err := a.sim.eth.TxPool().Sync(); err != nil { - break + // Avoid a race condition where doCommit is closed while in this loop. + select { + case _, ok := <-doCommit: + if !ok { + break + } + default: } + a.sim.Commit() } } }() From f6d5b76b2cc6430af502c7933f3a9cc374e6f0b1 Mon Sep 17 00:00:00 2001 From: Pepper Lebeck-Jobe Date: Fri, 7 Mar 2025 13:18:01 +0100 Subject: [PATCH 3/6] Also commit the call to fallibleCommit --- eth/catalyst/simulated_beacon_api.go | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-) diff --git a/eth/catalyst/simulated_beacon_api.go b/eth/catalyst/simulated_beacon_api.go index 5955b033bdb2..9e5888dfa713 100644 --- a/eth/catalyst/simulated_beacon_api.go +++ b/eth/catalyst/simulated_beacon_api.go @@ -70,15 +70,10 @@ func (a *simulatedBeaconAPI) loop() { if executable, _ := a.sim.eth.TxPool().Stats(); executable == 0 { break } - // Avoid a race condition where doCommit is closed while in this loop. - select { - case _, ok := <-doCommit: - if !ok { - break - } - default: + // Avoids spinlooping if the txPool is alredy terminated. + if _, err := a.sim.fallibleCommit(); err != nil { + break } - a.sim.Commit() } } }() From 71d44419818334e3e7e9456cc8dfdf6c38aaf456 Mon Sep 17 00:00:00 2001 From: Pepper Lebeck-Jobe Date: Fri, 7 Mar 2025 17:48:23 +0100 Subject: [PATCH 4/6] Try using a custom error type This way, we only break the spinloop when it is definitely because the transaction pool has already been terminated. --- eth/catalyst/simulated_beacon.go | 17 ++++++++++++++--- eth/catalyst/simulated_beacon_api.go | 8 ++++++-- 2 files changed, 20 insertions(+), 5 deletions(-) diff --git a/eth/catalyst/simulated_beacon.go b/eth/catalyst/simulated_beacon.go index 286757054e36..df8963b1ed4d 100644 --- a/eth/catalyst/simulated_beacon.go +++ b/eth/catalyst/simulated_beacon.go @@ -80,6 +80,17 @@ func (w *withdrawalQueue) subscribe(ch chan<- newWithdrawalsEvent) event.Subscri return w.subs.Track(sub) } +// txPoolTerminatedError is returned when the txpool is already terminated when +// trying to seal a block. +type txPoolTerminatedError struct { + error +} + +// Error returns the message from the wrapped error. +func (t *txPoolTerminatedError) Error() string { + return t.error.Error() +} + // SimulatedBeacon drives an Ethereum instance as if it were a real beacon // client. It can run in period mode where it mines a new block every period // (seconds) or on every transaction via Commit, Fork and AdjustTime. @@ -181,7 +192,7 @@ func (c *SimulatedBeacon) sealBlock(withdrawals []*types.Withdrawal, timestamp u // behavior, the pool will be explicitly blocked on its reset before // continuing to the block production below. if err := c.eth.APIBackend.TxPool().Sync(); err != nil { - return fmt.Errorf("failed to sync txpool: %w", err) + return &txPoolTerminatedError{fmt.Errorf("failed to sync txpool: %w", err)} } version := payloadVersion(c.eth.BlockChain().Config(), timestamp) @@ -302,12 +313,12 @@ func (c *SimulatedBeacon) setCurrentState(headHash, finalizedHash common.Hash) { // Commit seals a block on demand. func (c *SimulatedBeacon) Commit() common.Hash { - hash, _ := c.fallibleCommit() + hash, _ := c.commit() return hash } // fallibleCommit attempts to seal a block on demand, but may return an error. -func (c *SimulatedBeacon) fallibleCommit() (common.Hash, error) { +func (c *SimulatedBeacon) commit() (common.Hash, error) { withdrawals := c.withdrawals.pop(10) if err := c.sealBlock(withdrawals, uint64(time.Now().Unix())); err != nil { log.Warn("Error performing sealing work", "err", err) diff --git a/eth/catalyst/simulated_beacon_api.go b/eth/catalyst/simulated_beacon_api.go index 9e5888dfa713..ea490e6df534 100644 --- a/eth/catalyst/simulated_beacon_api.go +++ b/eth/catalyst/simulated_beacon_api.go @@ -18,6 +18,7 @@ package catalyst import ( "context" + "errors" "github.com/ethereum/go-ethereum/common" "github.com/ethereum/go-ethereum/core" @@ -71,8 +72,11 @@ func (a *simulatedBeaconAPI) loop() { break } // Avoids spinlooping if the txPool is alredy terminated. - if _, err := a.sim.fallibleCommit(); err != nil { - break + if _, err := a.sim.commit(); err != nil { + var txpTermErr *txPoolTerminatedError + if errors.As(err, &txpTermErr) { + break + } } } } From 45dce9d3e1846a6c59c44a825d59cef9f4a1fdc6 Mon Sep 17 00:00:00 2001 From: Pepper Lebeck-Jobe Date: Mon, 10 Mar 2025 10:19:12 +0100 Subject: [PATCH 5/6] Rename errTxPoolTerminated and fix comment This addresses some review feedback. --- eth/catalyst/simulated_beacon.go | 10 +++++----- eth/catalyst/simulated_beacon_api.go | 2 +- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/eth/catalyst/simulated_beacon.go b/eth/catalyst/simulated_beacon.go index df8963b1ed4d..7aa45914a720 100644 --- a/eth/catalyst/simulated_beacon.go +++ b/eth/catalyst/simulated_beacon.go @@ -80,14 +80,14 @@ func (w *withdrawalQueue) subscribe(ch chan<- newWithdrawalsEvent) event.Subscri return w.subs.Track(sub) } -// txPoolTerminatedError is returned when the txpool is already terminated when +// errTxPoolTerminated is returned when the txpool is already terminated when // trying to seal a block. -type txPoolTerminatedError struct { +type errTxPoolTerminated struct { error } // Error returns the message from the wrapped error. -func (t *txPoolTerminatedError) Error() string { +func (t *errTxPoolTerminated) Error() string { return t.error.Error() } @@ -192,7 +192,7 @@ func (c *SimulatedBeacon) sealBlock(withdrawals []*types.Withdrawal, timestamp u // behavior, the pool will be explicitly blocked on its reset before // continuing to the block production below. if err := c.eth.APIBackend.TxPool().Sync(); err != nil { - return &txPoolTerminatedError{fmt.Errorf("failed to sync txpool: %w", err)} + return &errTxPoolTerminated{fmt.Errorf("failed to sync txpool: %w", err)} } version := payloadVersion(c.eth.BlockChain().Config(), timestamp) @@ -317,7 +317,7 @@ func (c *SimulatedBeacon) Commit() common.Hash { return hash } -// fallibleCommit attempts to seal a block on demand, but may return an error. +// commit attempts to seal a block on demand, but may return an error. func (c *SimulatedBeacon) commit() (common.Hash, error) { withdrawals := c.withdrawals.pop(10) if err := c.sealBlock(withdrawals, uint64(time.Now().Unix())); err != nil { diff --git a/eth/catalyst/simulated_beacon_api.go b/eth/catalyst/simulated_beacon_api.go index ea490e6df534..bc8d1cb159d7 100644 --- a/eth/catalyst/simulated_beacon_api.go +++ b/eth/catalyst/simulated_beacon_api.go @@ -73,7 +73,7 @@ func (a *simulatedBeaconAPI) loop() { } // Avoids spinlooping if the txPool is alredy terminated. if _, err := a.sim.commit(); err != nil { - var txpTermErr *txPoolTerminatedError + var txpTermErr *errTxPoolTerminated if errors.As(err, &txpTermErr) { break } From e191e9f66d81d214aafc204cee9271560455e2c7 Mon Sep 17 00:00:00 2001 From: Pepper Lebeck-Jobe Date: Mon, 10 Mar 2025 12:48:32 +0100 Subject: [PATCH 6/6] Address more review feedback Use `errors.Is` instead of `errors.As` since we don't need to access special fields in the custom error type. Rather than generating a new error to wrap, just capture the exisitng error. --- eth/catalyst/simulated_beacon.go | 4 ++-- eth/catalyst/simulated_beacon_api.go | 3 +-- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/eth/catalyst/simulated_beacon.go b/eth/catalyst/simulated_beacon.go index 7aa45914a720..e4e2ca774607 100644 --- a/eth/catalyst/simulated_beacon.go +++ b/eth/catalyst/simulated_beacon.go @@ -88,7 +88,7 @@ type errTxPoolTerminated struct { // Error returns the message from the wrapped error. func (t *errTxPoolTerminated) Error() string { - return t.error.Error() + return fmt.Errorf("failed to sync txpool: %w", t.error).Error() } // SimulatedBeacon drives an Ethereum instance as if it were a real beacon @@ -192,7 +192,7 @@ func (c *SimulatedBeacon) sealBlock(withdrawals []*types.Withdrawal, timestamp u // behavior, the pool will be explicitly blocked on its reset before // continuing to the block production below. if err := c.eth.APIBackend.TxPool().Sync(); err != nil { - return &errTxPoolTerminated{fmt.Errorf("failed to sync txpool: %w", err)} + return &errTxPoolTerminated{err} } version := payloadVersion(c.eth.BlockChain().Config(), timestamp) diff --git a/eth/catalyst/simulated_beacon_api.go b/eth/catalyst/simulated_beacon_api.go index bc8d1cb159d7..34047a964c80 100644 --- a/eth/catalyst/simulated_beacon_api.go +++ b/eth/catalyst/simulated_beacon_api.go @@ -73,8 +73,7 @@ func (a *simulatedBeaconAPI) loop() { } // Avoids spinlooping if the txPool is alredy terminated. if _, err := a.sim.commit(); err != nil { - var txpTermErr *errTxPoolTerminated - if errors.As(err, &txpTermErr) { + if errors.Is(err, &errTxPoolTerminated{}) { break } }