From 531af196f34a47c1a2d0eabf41074973e826adf7 Mon Sep 17 00:00:00 2001 From: Samuel Stokes Date: Tue, 30 Jul 2024 15:53:14 -0400 Subject: [PATCH 1/5] op-proposer: add retries to output proposal --- op-e2e/actions/l2_proposer.go | 2 + op-e2e/actions/l2_proposer_test.go | 8 ++-- op-e2e/actions/user_test.go | 8 ++-- op-e2e/setup.go | 30 ++++++++------- op-proposer/flags/flags.go | 7 ++++ op-proposer/proposer/config.go | 4 ++ op-proposer/proposer/driver.go | 62 ++++++++++++++++++++++-------- op-proposer/proposer/service.go | 4 ++ 8 files changed, 90 insertions(+), 35 deletions(-) diff --git a/op-e2e/actions/l2_proposer.go b/op-e2e/actions/l2_proposer.go index 9d0260700512..3952f3de17f0 100644 --- a/op-e2e/actions/l2_proposer.go +++ b/op-e2e/actions/l2_proposer.go @@ -31,6 +31,7 @@ type ProposerCfg struct { OutputOracleAddr *common.Address DisputeGameFactoryAddr *common.Address ProposalInterval time.Duration + ProposalRetryInterval time.Duration DisputeGameType uint32 ProposerKey *ecdsa.PrivateKey AllowNonFinalized bool @@ -77,6 +78,7 @@ func NewL2Proposer(t Testing, log log.Logger, cfg *ProposerCfg, l1 *ethclient.Cl PollInterval: time.Second, NetworkTimeout: time.Second, ProposalInterval: cfg.ProposalInterval, + ProposalRetryInterval: cfg.ProposalRetryInterval, L2OutputOracleAddr: cfg.OutputOracleAddr, DisputeGameFactoryAddr: cfg.DisputeGameFactoryAddr, DisputeGameType: cfg.DisputeGameType, diff --git a/op-e2e/actions/l2_proposer_test.go b/op-e2e/actions/l2_proposer_test.go index ef7966eb8e43..bed138cdb3a7 100644 --- a/op-e2e/actions/l2_proposer_test.go +++ b/op-e2e/actions/l2_proposer_test.go @@ -64,15 +64,17 @@ func RunProposerTest(gt *testing.T, deltaTimeOffset *hexutil.Uint64) { proposer = NewL2Proposer(t, log, &ProposerCfg{ DisputeGameFactoryAddr: &sd.DeploymentsL1.DisputeGameFactoryProxy, ProposalInterval: 6 * time.Second, + ProposalRetryInterval: 3 * time.Second, DisputeGameType: respectedGameType, ProposerKey: dp.Secrets.Proposer, AllowNonFinalized: true, }, miner.EthClient(), rollupSeqCl) } else { proposer = NewL2Proposer(t, log, &ProposerCfg{ - OutputOracleAddr: &sd.DeploymentsL1.L2OutputOracleProxy, - ProposerKey: dp.Secrets.Proposer, - AllowNonFinalized: false, + OutputOracleAddr: &sd.DeploymentsL1.L2OutputOracleProxy, + ProposerKey: dp.Secrets.Proposer, + ProposalRetryInterval: 3 * time.Second, + AllowNonFinalized: false, }, miner.EthClient(), rollupSeqCl) } diff --git a/op-e2e/actions/user_test.go b/op-e2e/actions/user_test.go index c9692c91f0aa..3a0b079cb3e4 100644 --- a/op-e2e/actions/user_test.go +++ b/op-e2e/actions/user_test.go @@ -144,15 +144,17 @@ func runCrossLayerUserTest(gt *testing.T, test hardforkScheduledTest) { proposer = NewL2Proposer(t, log, &ProposerCfg{ DisputeGameFactoryAddr: &sd.DeploymentsL1.DisputeGameFactoryProxy, ProposalInterval: 6 * time.Second, + ProposalRetryInterval: 3 * time.Second, DisputeGameType: respectedGameType, ProposerKey: dp.Secrets.Proposer, AllowNonFinalized: true, }, miner.EthClient(), seq.RollupClient()) } else { proposer = NewL2Proposer(t, log, &ProposerCfg{ - OutputOracleAddr: &sd.DeploymentsL1.L2OutputOracleProxy, - ProposerKey: dp.Secrets.Proposer, - AllowNonFinalized: true, + OutputOracleAddr: &sd.DeploymentsL1.L2OutputOracleProxy, + ProposerKey: dp.Secrets.Proposer, + ProposalRetryInterval: 3 * time.Second, + AllowNonFinalized: true, }, miner.EthClient(), seq.RollupClient()) } diff --git a/op-e2e/setup.go b/op-e2e/setup.go index ce04f4374a33..c26a27bbee76 100644 --- a/op-e2e/setup.go +++ b/op-e2e/setup.go @@ -828,14 +828,15 @@ func (cfg SystemConfig) Start(t *testing.T, _opts ...SystemConfigOption) (*Syste var proposerCLIConfig *l2os.CLIConfig if e2eutils.UseFaultProofs() { proposerCLIConfig = &l2os.CLIConfig{ - L1EthRpc: sys.EthInstances[RoleL1].WSEndpoint(), - RollupRpc: sys.RollupNodes[RoleSeq].HTTPEndpoint(), - DGFAddress: config.L1Deployments.DisputeGameFactoryProxy.Hex(), - ProposalInterval: 6 * time.Second, - DisputeGameType: 254, // Fast game type - PollInterval: 50 * time.Millisecond, - TxMgrConfig: newTxMgrConfig(sys.EthInstances[RoleL1].WSEndpoint(), cfg.Secrets.Proposer), - AllowNonFinalized: cfg.NonFinalizedProposals, + L1EthRpc: sys.EthInstances[RoleL1].WSEndpoint(), + RollupRpc: sys.RollupNodes[RoleSeq].HTTPEndpoint(), + DGFAddress: config.L1Deployments.DisputeGameFactoryProxy.Hex(), + ProposalInterval: 6 * time.Second, + DisputeGameType: 254, // Fast game type + PollInterval: 50 * time.Millisecond, + ProposalRetryInterval: 10 * time.Millisecond, + TxMgrConfig: newTxMgrConfig(sys.EthInstances[RoleL1].WSEndpoint(), cfg.Secrets.Proposer), + AllowNonFinalized: cfg.NonFinalizedProposals, LogConfig: oplog.CLIConfig{ Level: log.LvlInfo, Format: oplog.FormatText, @@ -843,12 +844,13 @@ func (cfg SystemConfig) Start(t *testing.T, _opts ...SystemConfigOption) (*Syste } } else { proposerCLIConfig = &l2os.CLIConfig{ - L1EthRpc: sys.EthInstances[RoleL1].WSEndpoint(), - RollupRpc: sys.RollupNodes[RoleSeq].HTTPEndpoint(), - L2OOAddress: config.L1Deployments.L2OutputOracleProxy.Hex(), - PollInterval: 50 * time.Millisecond, - TxMgrConfig: newTxMgrConfig(sys.EthInstances[RoleL1].WSEndpoint(), cfg.Secrets.Proposer), - AllowNonFinalized: cfg.NonFinalizedProposals, + L1EthRpc: sys.EthInstances[RoleL1].WSEndpoint(), + RollupRpc: sys.RollupNodes[RoleSeq].HTTPEndpoint(), + L2OOAddress: config.L1Deployments.L2OutputOracleProxy.Hex(), + PollInterval: 50 * time.Millisecond, + ProposalRetryInterval: 10 * time.Millisecond, + TxMgrConfig: newTxMgrConfig(sys.EthInstances[RoleL1].WSEndpoint(), cfg.Secrets.Proposer), + AllowNonFinalized: cfg.NonFinalizedProposals, LogConfig: oplog.CLIConfig{ Level: log.LvlInfo, Format: oplog.FormatText, diff --git a/op-proposer/flags/flags.go b/op-proposer/flags/flags.go index 4321e000fe2f..432b645ddad5 100644 --- a/op-proposer/flags/flags.go +++ b/op-proposer/flags/flags.go @@ -60,6 +60,12 @@ var ( Usage: "Interval between submitting L2 output proposals when the dispute game factory address is set", EnvVars: prefixEnvVars("PROPOSAL_INTERVAL"), } + ProposalRetryIntervalFlag = &cli.DurationFlag{ + Name: "proposal-retry-interval", + Usage: "Interval between retrying output proposals if one fails", + Value: 120 * time.Second, + EnvVars: prefixEnvVars("PROPOSAL_RETRY_INTERVAL"), + } DisputeGameTypeFlag = &cli.UintFlag{ Name: "game-type", Usage: "Dispute game type to create via the configured DisputeGameFactory", @@ -95,6 +101,7 @@ var optionalFlags = []cli.Flag{ L2OutputHDPathFlag, DisputeGameFactoryAddressFlag, ProposalIntervalFlag, + ProposalRetryIntervalFlag, DisputeGameTypeFlag, ActiveSequencerCheckDurationFlag, WaitNodeSyncFlag, diff --git a/op-proposer/proposer/config.go b/op-proposer/proposer/config.go index 8786da0c0daa..d25b50181dae 100644 --- a/op-proposer/proposer/config.go +++ b/op-proposer/proposer/config.go @@ -53,6 +53,9 @@ type CLIConfig struct { // ProposalInterval is the delay between submitting L2 output proposals when the DGFAddress is set. ProposalInterval time.Duration + // ProposalRetryInterval is the delay between retrying L2 output proposals if one fails. + ProposalRetryInterval time.Duration + // DisputeGameType is the type of dispute game to create when submitting an output proposal. DisputeGameType uint32 @@ -110,6 +113,7 @@ func NewConfig(ctx *cli.Context) *CLIConfig { PprofConfig: oppprof.ReadCLIConfig(ctx), DGFAddress: ctx.String(flags.DisputeGameFactoryAddressFlag.Name), ProposalInterval: ctx.Duration(flags.ProposalIntervalFlag.Name), + ProposalRetryInterval: ctx.Duration(flags.ProposalRetryIntervalFlag.Name), DisputeGameType: uint32(ctx.Uint(flags.DisputeGameTypeFlag.Name)), ActiveSequencerCheckDuration: ctx.Duration(flags.ActiveSequencerCheckDurationFlag.Name), WaitNodeSync: ctx.Bool(flags.WaitNodeSyncFlag.Name), diff --git a/op-proposer/proposer/driver.go b/op-proposer/proposer/driver.go index 4138deda1bfd..5c0c34315baf 100644 --- a/op-proposer/proposer/driver.go +++ b/op-proposer/proposer/driver.go @@ -453,15 +453,30 @@ func (l *L2OutputSubmitter) waitNodeSync() error { func (l *L2OutputSubmitter) loopL2OO(ctx context.Context) { ticker := time.NewTicker(l.Cfg.PollInterval) defer ticker.Stop() + + retryOutputProposal := false + retryTicker := time.NewTicker(l.Cfg.ProposalRetryInterval) + defer retryTicker.Stop() + + proposeOutput := func() bool { + output, shouldPropose, err := l.FetchNextOutputInfo(ctx) + if err != nil || !shouldPropose { + retryTicker.Reset(l.Cfg.ProposalRetryInterval) + return true + } + l.proposeOutput(ctx, output) + return false + } + for { select { case <-ticker.C: - output, shouldPropose, err := l.FetchNextOutputInfo(ctx) - if err != nil || !shouldPropose { - break + retryOutputProposal = proposeOutput() + case <-retryTicker.C: + if retryOutputProposal { + l.Log.Info("retrying output proposal") + retryOutputProposal = proposeOutput() } - - l.proposeOutput(ctx, output) case <-l.done: return } @@ -471,20 +486,37 @@ func (l *L2OutputSubmitter) loopL2OO(ctx context.Context) { func (l *L2OutputSubmitter) loopDGF(ctx context.Context) { ticker := time.NewTicker(l.Cfg.ProposalInterval) defer ticker.Stop() + + retryOutputProposal := false + retryTicker := time.NewTicker(l.Cfg.ProposalRetryInterval) + defer retryTicker.Stop() + + proposeOutput := func() bool { + blockNumber, err := l.FetchCurrentBlockNumber(ctx) + if err != nil { + retryTicker.Reset(l.Cfg.ProposalRetryInterval) + return true + } + + output, shouldPropose, err := l.FetchOutput(ctx, blockNumber) + if err != nil || !shouldPropose { + retryTicker.Reset(l.Cfg.ProposalRetryInterval) + return true + } + + l.proposeOutput(ctx, output) + return false + } + for { select { case <-ticker.C: - blockNumber, err := l.FetchCurrentBlockNumber(ctx) - if err != nil { - break - } - - output, shouldPropose, err := l.FetchOutput(ctx, blockNumber) - if err != nil || !shouldPropose { - break + retryOutputProposal = proposeOutput() + case <-retryTicker.C: + if retryOutputProposal { + l.Log.Info("retrying output proposal") + retryOutputProposal = proposeOutput() } - - l.proposeOutput(ctx, output) case <-l.done: return } diff --git a/op-proposer/proposer/service.go b/op-proposer/proposer/service.go index f40fdf0b496a..493270ec88e7 100644 --- a/op-proposer/proposer/service.go +++ b/op-proposer/proposer/service.go @@ -32,6 +32,9 @@ type ProposerConfig struct { PollInterval time.Duration NetworkTimeout time.Duration + // How frequently to retry a proposal if one fails + ProposalRetryInterval time.Duration + // How frequently to post L2 outputs when the DisputeGameFactory is configured ProposalInterval time.Duration @@ -89,6 +92,7 @@ func (ps *ProposerService) initFromCLIConfig(ctx context.Context, version string ps.initMetrics(cfg) ps.PollInterval = cfg.PollInterval + ps.ProposalRetryInterval = cfg.ProposalRetryInterval ps.NetworkTimeout = cfg.TxMgrConfig.NetworkTimeout ps.AllowNonFinalized = cfg.AllowNonFinalized ps.WaitNodeSync = cfg.WaitNodeSync From fdbf8eec0dad310c230eb670391cc04d0f660937 Mon Sep 17 00:00:00 2001 From: Samuel Stokes Date: Tue, 30 Jul 2024 16:06:26 -0400 Subject: [PATCH 2/5] op-proposer: proposeOutput returns err to help trigger retry --- op-proposer/proposer/driver.go | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-) diff --git a/op-proposer/proposer/driver.go b/op-proposer/proposer/driver.go index 5c0c34315baf..c0ab8642f45a 100644 --- a/op-proposer/proposer/driver.go +++ b/op-proposer/proposer/driver.go @@ -464,7 +464,11 @@ func (l *L2OutputSubmitter) loopL2OO(ctx context.Context) { retryTicker.Reset(l.Cfg.ProposalRetryInterval) return true } - l.proposeOutput(ctx, output) + err = l.proposeOutput(ctx, output) + if err != nil { + retryTicker.Reset(l.Cfg.ProposalRetryInterval) + return true + } return false } @@ -504,7 +508,11 @@ func (l *L2OutputSubmitter) loopDGF(ctx context.Context) { return true } - l.proposeOutput(ctx, output) + err = l.proposeOutput(ctx, output) + if err != nil { + retryTicker.Reset(l.Cfg.ProposalRetryInterval) + return true + } return false } @@ -523,7 +531,7 @@ func (l *L2OutputSubmitter) loopDGF(ctx context.Context) { } } -func (l *L2OutputSubmitter) proposeOutput(ctx context.Context, output *eth.OutputResponse) { +func (l *L2OutputSubmitter) proposeOutput(ctx context.Context, output *eth.OutputResponse) error { cCtx, cancel := context.WithTimeout(ctx, 10*time.Minute) defer cancel() @@ -533,7 +541,8 @@ func (l *L2OutputSubmitter) proposeOutput(ctx context.Context, output *eth.Outpu "l1blocknum", output.Status.CurrentL1.Number, "l1blockhash", output.Status.CurrentL1.Hash, "l1head", output.Status.HeadL1.Number) - return + return err } l.Metr.RecordL2BlocksProposed(output.BlockRef) + return nil } From 4bf63c4a6faf2afd2c4bf4875de2885c4a3dbf54 Mon Sep 17 00:00:00 2001 From: Samuel Stokes Date: Fri, 2 Aug 2024 01:01:23 -0400 Subject: [PATCH 3/5] op-proposer: use retry.Do for FetchOutput, add unit tests --- op-e2e/actions/l2_proposer.go | 2 +- op-e2e/setup.go | 32 +++--- op-proposer/flags/flags.go | 12 +-- op-proposer/proposer/config.go | 6 +- op-proposer/proposer/driver.go | 93 +++++++---------- op-proposer/proposer/driver_test.go | 148 ++++++++++++++++++++++++++++ op-proposer/proposer/service.go | 6 +- 7 files changed, 211 insertions(+), 88 deletions(-) create mode 100644 op-proposer/proposer/driver_test.go diff --git a/op-e2e/actions/l2_proposer.go b/op-e2e/actions/l2_proposer.go index 3952f3de17f0..7f520ec1f88d 100644 --- a/op-e2e/actions/l2_proposer.go +++ b/op-e2e/actions/l2_proposer.go @@ -78,7 +78,7 @@ func NewL2Proposer(t Testing, log log.Logger, cfg *ProposerCfg, l1 *ethclient.Cl PollInterval: time.Second, NetworkTimeout: time.Second, ProposalInterval: cfg.ProposalInterval, - ProposalRetryInterval: cfg.ProposalRetryInterval, + OutputRetryInterval: cfg.ProposalRetryInterval, L2OutputOracleAddr: cfg.OutputOracleAddr, DisputeGameFactoryAddr: cfg.DisputeGameFactoryAddr, DisputeGameType: cfg.DisputeGameType, diff --git a/op-e2e/setup.go b/op-e2e/setup.go index c26a27bbee76..77128a48eb0c 100644 --- a/op-e2e/setup.go +++ b/op-e2e/setup.go @@ -828,15 +828,15 @@ func (cfg SystemConfig) Start(t *testing.T, _opts ...SystemConfigOption) (*Syste var proposerCLIConfig *l2os.CLIConfig if e2eutils.UseFaultProofs() { proposerCLIConfig = &l2os.CLIConfig{ - L1EthRpc: sys.EthInstances[RoleL1].WSEndpoint(), - RollupRpc: sys.RollupNodes[RoleSeq].HTTPEndpoint(), - DGFAddress: config.L1Deployments.DisputeGameFactoryProxy.Hex(), - ProposalInterval: 6 * time.Second, - DisputeGameType: 254, // Fast game type - PollInterval: 50 * time.Millisecond, - ProposalRetryInterval: 10 * time.Millisecond, - TxMgrConfig: newTxMgrConfig(sys.EthInstances[RoleL1].WSEndpoint(), cfg.Secrets.Proposer), - AllowNonFinalized: cfg.NonFinalizedProposals, + L1EthRpc: sys.EthInstances[RoleL1].WSEndpoint(), + RollupRpc: sys.RollupNodes[RoleSeq].HTTPEndpoint(), + DGFAddress: config.L1Deployments.DisputeGameFactoryProxy.Hex(), + ProposalInterval: 6 * time.Second, + DisputeGameType: 254, // Fast game type + PollInterval: 50 * time.Millisecond, + OutputRetryInterval: 10 * time.Millisecond, + TxMgrConfig: newTxMgrConfig(sys.EthInstances[RoleL1].WSEndpoint(), cfg.Secrets.Proposer), + AllowNonFinalized: cfg.NonFinalizedProposals, LogConfig: oplog.CLIConfig{ Level: log.LvlInfo, Format: oplog.FormatText, @@ -844,13 +844,13 @@ func (cfg SystemConfig) Start(t *testing.T, _opts ...SystemConfigOption) (*Syste } } else { proposerCLIConfig = &l2os.CLIConfig{ - L1EthRpc: sys.EthInstances[RoleL1].WSEndpoint(), - RollupRpc: sys.RollupNodes[RoleSeq].HTTPEndpoint(), - L2OOAddress: config.L1Deployments.L2OutputOracleProxy.Hex(), - PollInterval: 50 * time.Millisecond, - ProposalRetryInterval: 10 * time.Millisecond, - TxMgrConfig: newTxMgrConfig(sys.EthInstances[RoleL1].WSEndpoint(), cfg.Secrets.Proposer), - AllowNonFinalized: cfg.NonFinalizedProposals, + L1EthRpc: sys.EthInstances[RoleL1].WSEndpoint(), + RollupRpc: sys.RollupNodes[RoleSeq].HTTPEndpoint(), + L2OOAddress: config.L1Deployments.L2OutputOracleProxy.Hex(), + PollInterval: 50 * time.Millisecond, + OutputRetryInterval: 10 * time.Millisecond, + TxMgrConfig: newTxMgrConfig(sys.EthInstances[RoleL1].WSEndpoint(), cfg.Secrets.Proposer), + AllowNonFinalized: cfg.NonFinalizedProposals, LogConfig: oplog.CLIConfig{ Level: log.LvlInfo, Format: oplog.FormatText, diff --git a/op-proposer/flags/flags.go b/op-proposer/flags/flags.go index 432b645ddad5..850c7232ae93 100644 --- a/op-proposer/flags/flags.go +++ b/op-proposer/flags/flags.go @@ -60,11 +60,11 @@ var ( Usage: "Interval between submitting L2 output proposals when the dispute game factory address is set", EnvVars: prefixEnvVars("PROPOSAL_INTERVAL"), } - ProposalRetryIntervalFlag = &cli.DurationFlag{ - Name: "proposal-retry-interval", - Usage: "Interval between retrying output proposals if one fails", - Value: 120 * time.Second, - EnvVars: prefixEnvVars("PROPOSAL_RETRY_INTERVAL"), + OutputRetryIntervalFlag = &cli.DurationFlag{ + Name: "output-retry-interval", + Usage: "Interval between retrying output fetch if one fails", + Value: time.Minute, + EnvVars: prefixEnvVars("OUTPUT_RETRY_INTERVAL"), } DisputeGameTypeFlag = &cli.UintFlag{ Name: "game-type", @@ -101,7 +101,7 @@ var optionalFlags = []cli.Flag{ L2OutputHDPathFlag, DisputeGameFactoryAddressFlag, ProposalIntervalFlag, - ProposalRetryIntervalFlag, + OutputRetryIntervalFlag, DisputeGameTypeFlag, ActiveSequencerCheckDurationFlag, WaitNodeSyncFlag, diff --git a/op-proposer/proposer/config.go b/op-proposer/proposer/config.go index d25b50181dae..892d4686b3a9 100644 --- a/op-proposer/proposer/config.go +++ b/op-proposer/proposer/config.go @@ -53,8 +53,8 @@ type CLIConfig struct { // ProposalInterval is the delay between submitting L2 output proposals when the DGFAddress is set. ProposalInterval time.Duration - // ProposalRetryInterval is the delay between retrying L2 output proposals if one fails. - ProposalRetryInterval time.Duration + // OutputRetryInterval is the delay between retrying output fetch if one fails. + OutputRetryInterval time.Duration // DisputeGameType is the type of dispute game to create when submitting an output proposal. DisputeGameType uint32 @@ -113,7 +113,7 @@ func NewConfig(ctx *cli.Context) *CLIConfig { PprofConfig: oppprof.ReadCLIConfig(ctx), DGFAddress: ctx.String(flags.DisputeGameFactoryAddressFlag.Name), ProposalInterval: ctx.Duration(flags.ProposalIntervalFlag.Name), - ProposalRetryInterval: ctx.Duration(flags.ProposalRetryIntervalFlag.Name), + OutputRetryInterval: ctx.Duration(flags.OutputRetryIntervalFlag.Name), DisputeGameType: uint32(ctx.Uint(flags.DisputeGameTypeFlag.Name)), ActiveSequencerCheckDuration: ctx.Duration(flags.ActiveSequencerCheckDurationFlag.Name), WaitNodeSync: ctx.Bool(flags.WaitNodeSyncFlag.Name), diff --git a/op-proposer/proposer/driver.go b/op-proposer/proposer/driver.go index c0ab8642f45a..1730a216f9dd 100644 --- a/op-proposer/proposer/driver.go +++ b/op-proposer/proposer/driver.go @@ -20,6 +20,7 @@ import ( "github.com/ethereum-optimism/optimism/op-proposer/metrics" "github.com/ethereum-optimism/optimism/op-service/dial" "github.com/ethereum-optimism/optimism/op-service/eth" + "github.com/ethereum-optimism/optimism/op-service/retry" "github.com/ethereum-optimism/optimism/op-service/txmgr" ) @@ -39,6 +40,11 @@ type L1Client interface { CallContract(ctx context.Context, call ethereum.CallMsg, blockNumber *big.Int) ([]byte, error) } +type L2OOContract interface { + Version(*bind.CallOpts) (string, error) + NextBlockNumber(*bind.CallOpts) (*big.Int, error) +} + type RollupClient interface { SyncStatus(ctx context.Context) (*eth.SyncStatus, error) OutputAtBlock(ctx context.Context, blockNum uint64) (*eth.OutputResponse, error) @@ -68,7 +74,7 @@ type L2OutputSubmitter struct { mutex sync.Mutex running bool - l2ooContract *bindings.L2OutputOracleCaller + l2ooContract L2OOContract l2ooABI *abi.ABI dgfContract *bindings.DisputeGameFactoryCaller @@ -453,34 +459,19 @@ func (l *L2OutputSubmitter) waitNodeSync() error { func (l *L2OutputSubmitter) loopL2OO(ctx context.Context) { ticker := time.NewTicker(l.Cfg.PollInterval) defer ticker.Stop() - - retryOutputProposal := false - retryTicker := time.NewTicker(l.Cfg.ProposalRetryInterval) - defer retryTicker.Stop() - - proposeOutput := func() bool { - output, shouldPropose, err := l.FetchNextOutputInfo(ctx) - if err != nil || !shouldPropose { - retryTicker.Reset(l.Cfg.ProposalRetryInterval) - return true - } - err = l.proposeOutput(ctx, output) - if err != nil { - retryTicker.Reset(l.Cfg.ProposalRetryInterval) - return true - } - return false - } - for { select { case <-ticker.C: - retryOutputProposal = proposeOutput() - case <-retryTicker.C: - if retryOutputProposal { - l.Log.Info("retrying output proposal") - retryOutputProposal = proposeOutput() + output, shouldPropose, err := retry.Do2(ctx, 10, &retry.FixedStrategy{Dur: l.Cfg.OutputRetryInterval}, func() (*eth.OutputResponse, bool, error) { + ctx, cancel := context.WithTimeout(ctx, 3*time.Second) + defer cancel() + return l.FetchNextOutputInfo(ctx) + }) + if err != nil || !shouldPropose { + break } + + l.proposeOutput(ctx, output) case <-l.done: return } @@ -490,48 +481,33 @@ func (l *L2OutputSubmitter) loopL2OO(ctx context.Context) { func (l *L2OutputSubmitter) loopDGF(ctx context.Context) { ticker := time.NewTicker(l.Cfg.ProposalInterval) defer ticker.Stop() - - retryOutputProposal := false - retryTicker := time.NewTicker(l.Cfg.ProposalRetryInterval) - defer retryTicker.Stop() - - proposeOutput := func() bool { - blockNumber, err := l.FetchCurrentBlockNumber(ctx) - if err != nil { - retryTicker.Reset(l.Cfg.ProposalRetryInterval) - return true - } - - output, shouldPropose, err := l.FetchOutput(ctx, blockNumber) - if err != nil || !shouldPropose { - retryTicker.Reset(l.Cfg.ProposalRetryInterval) - return true - } - - err = l.proposeOutput(ctx, output) - if err != nil { - retryTicker.Reset(l.Cfg.ProposalRetryInterval) - return true - } - return false - } - for { select { case <-ticker.C: - retryOutputProposal = proposeOutput() - case <-retryTicker.C: - if retryOutputProposal { - l.Log.Info("retrying output proposal") - retryOutputProposal = proposeOutput() + blockNumber, err := retry.Do(ctx, 10, &retry.FixedStrategy{Dur: l.Cfg.OutputRetryInterval}, func() (*big.Int, error) { + return l.FetchCurrentBlockNumber(ctx) + }) + if err != nil { + break + } + + output, shouldPropose, err := retry.Do2(ctx, 10, &retry.FixedStrategy{Dur: l.Cfg.OutputRetryInterval}, func() (*eth.OutputResponse, bool, error) { + ctx, cancel := context.WithTimeout(ctx, 3*time.Second) + defer cancel() + return l.FetchOutput(ctx, blockNumber) + }) + if err != nil || !shouldPropose { + break } + + l.proposeOutput(ctx, output) case <-l.done: return } } } -func (l *L2OutputSubmitter) proposeOutput(ctx context.Context, output *eth.OutputResponse) error { +func (l *L2OutputSubmitter) proposeOutput(ctx context.Context, output *eth.OutputResponse) { cCtx, cancel := context.WithTimeout(ctx, 10*time.Minute) defer cancel() @@ -541,8 +517,7 @@ func (l *L2OutputSubmitter) proposeOutput(ctx context.Context, output *eth.Outpu "l1blocknum", output.Status.CurrentL1.Number, "l1blockhash", output.Status.CurrentL1.Hash, "l1head", output.Status.HeadL1.Number) - return err + return } l.Metr.RecordL2BlocksProposed(output.BlockRef) - return nil } diff --git a/op-proposer/proposer/driver_test.go b/op-proposer/proposer/driver_test.go new file mode 100644 index 000000000000..872313784013 --- /dev/null +++ b/op-proposer/proposer/driver_test.go @@ -0,0 +1,148 @@ +package proposer + +import ( + "context" + "fmt" + "math/big" + "testing" + "time" + + "github.com/ethereum-optimism/optimism/op-proposer/bindings" + "github.com/ethereum-optimism/optimism/op-proposer/metrics" + "github.com/ethereum-optimism/optimism/op-service/dial" + "github.com/ethereum-optimism/optimism/op-service/eth" + "github.com/ethereum-optimism/optimism/op-service/testlog" + "github.com/ethereum-optimism/optimism/op-service/testutils" + "github.com/ethereum-optimism/optimism/op-service/txmgr/mocks" + "github.com/ethereum/go-ethereum/accounts/abi/bind" + "github.com/ethereum/go-ethereum/common" + "github.com/ethereum/go-ethereum/core/types" + "github.com/ethereum/go-ethereum/log" + "github.com/stretchr/testify/mock" +) + +type MockL2OOContract struct { + mock.Mock +} + +func (m *MockL2OOContract) Version(opts *bind.CallOpts) (string, error) { + args := m.Called(opts) + return args.String(0), args.Error(1) +} + +func (m *MockL2OOContract) NextBlockNumber(opts *bind.CallOpts) (*big.Int, error) { + args := m.Called(opts) + return args.Get(0).(*big.Int), args.Error(1) +} + +type mockL2EndpointProvider struct { + ethClient *testutils.MockL2Client + ethClientErr error + rollupClient *testutils.MockRollupClient + rollupClientErr error +} + +func newEndpointProvider() *mockL2EndpointProvider { + return &mockL2EndpointProvider{ + ethClient: new(testutils.MockL2Client), + rollupClient: new(testutils.MockRollupClient), + } +} + +func (p *mockL2EndpointProvider) EthClient(context.Context) (dial.EthClientInterface, error) { + return p.ethClient, p.ethClientErr +} + +func (p *mockL2EndpointProvider) RollupClient(context.Context) (dial.RollupClientInterface, error) { + return p.rollupClient, p.rollupClientErr +} + +func (p *mockL2EndpointProvider) Close() {} + +func setup(t *testing.T) (*L2OutputSubmitter, *mockL2EndpointProvider, *MockL2OOContract) { + ep := newEndpointProvider() + + l2OutputOracleAddr := common.HexToAddress("0x3F8A862E63E759a77DA22d384027D21BF096bA9E") + + proposerConfig := ProposerConfig{ + PollInterval: 20 * time.Millisecond, + ProposalInterval: 20 * time.Millisecond, + OutputRetryInterval: 1 * time.Millisecond, + L2OutputOracleAddr: &l2OutputOracleAddr, + } + + txmgr := mocks.TxManager{} + txmgr.On("From").Return(common.Address{}) + txmgr.On("BlockNumber", mock.Anything).Return(uint64(100), nil) + txmgr.On("Send", mock.Anything, mock.Anything).Return(&types.Receipt{Status: uint64(1), TxHash: common.Hash{}}, nil) + + setup := DriverSetup{ + Log: testlog.Logger(t, log.LevelDebug), + Metr: metrics.NoopMetrics, + Cfg: proposerConfig, + Txmgr: &txmgr, + RollupProvider: ep, + } + + parsed, err := bindings.L2OutputOracleMetaData.GetAbi() + if err != nil { + panic(err) + } + + ctx, cancel := context.WithCancel(context.Background()) + l2ooContract := MockL2OOContract{} + l2OutputSubmitter := L2OutputSubmitter{ + DriverSetup: setup, + done: make(chan struct{}), + l2ooContract: &l2ooContract, + l2ooABI: parsed, + ctx: ctx, + cancel: cancel, + } + + return &l2OutputSubmitter, ep, &l2ooContract +} + +func TestL2OutputSubmitter_OutputRetry(t *testing.T) { + tests := []struct { + name string + }{ + {name: "loopL2OO"}, + {name: "loopDGF"}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + ps, ep, l2ooContract := setup(t) + + ep.rollupClient.On("SyncStatus").Return(ð.SyncStatus{FinalizedL2: eth.L2BlockRef{Number: 42}}, nil).Once() + ep.rollupClient.ExpectOutputAtBlock(42, nil, fmt.Errorf("failed to fetch output")) + ep.rollupClient.ExpectOutputAtBlock( + 42, + ð.OutputResponse{ + Version: supportedL2OutputVersion, + BlockRef: eth.L2BlockRef{Number: 42}, + Status: ð.SyncStatus{ + CurrentL1: eth.L1BlockRef{Hash: common.Hash{}}, + FinalizedL2: eth.L2BlockRef{Number: 42}, + }}, + nil, + ) + + if tt.name == "loopDGF" { + go ps.loopDGF(ps.ctx) + } else { + l2ooContract.On("NextBlockNumber", mock.AnythingOfType("*bind.CallOpts")).Return(big.NewInt(42), nil).Once() + l2ooContract.On("NextBlockNumber", mock.AnythingOfType("*bind.CallOpts")).Return(big.NewInt(42), nil).Once() + ep.rollupClient.On("SyncStatus").Return(ð.SyncStatus{FinalizedL2: eth.L2BlockRef{Number: 42}}, nil).Once() + go ps.loopL2OO(ps.ctx) + } + + time.Sleep(25 * time.Millisecond) + close(ps.done) + + ep.rollupClient.AssertExpectations(t) + l2ooContract.AssertExpectations(t) + }) + } +} diff --git a/op-proposer/proposer/service.go b/op-proposer/proposer/service.go index 493270ec88e7..8b3d3c553cce 100644 --- a/op-proposer/proposer/service.go +++ b/op-proposer/proposer/service.go @@ -32,8 +32,8 @@ type ProposerConfig struct { PollInterval time.Duration NetworkTimeout time.Duration - // How frequently to retry a proposal if one fails - ProposalRetryInterval time.Duration + // How frequently to retry fetching an output if one fails + OutputRetryInterval time.Duration // How frequently to post L2 outputs when the DisputeGameFactory is configured ProposalInterval time.Duration @@ -92,7 +92,7 @@ func (ps *ProposerService) initFromCLIConfig(ctx context.Context, version string ps.initMetrics(cfg) ps.PollInterval = cfg.PollInterval - ps.ProposalRetryInterval = cfg.ProposalRetryInterval + ps.OutputRetryInterval = cfg.OutputRetryInterval ps.NetworkTimeout = cfg.TxMgrConfig.NetworkTimeout ps.AllowNonFinalized = cfg.AllowNonFinalized ps.WaitNodeSync = cfg.WaitNodeSync From 03f30fef2be41ced6bbee25ddbb42f7a48813b34 Mon Sep 17 00:00:00 2001 From: Sebastian Stammler Date: Mon, 5 Aug 2024 21:50:32 +0200 Subject: [PATCH 4/5] op-proposer: improve output fetching retry impl --- op-e2e/actions/l2_proposer.go | 13 +- op-proposer/flags/flags.go | 8 +- op-proposer/proposer/driver.go | 164 ++++++++++++--------- op-proposer/proposer/driver_test.go | 90 +++++------ op-service/testutils/mock_rollup_client.go | 4 +- 5 files changed, 153 insertions(+), 126 deletions(-) diff --git a/op-e2e/actions/l2_proposer.go b/op-e2e/actions/l2_proposer.go index 7f520ec1f88d..6a9c2fe4c4b5 100644 --- a/op-e2e/actions/l2_proposer.go +++ b/op-e2e/actions/l2_proposer.go @@ -3,6 +3,7 @@ package actions import ( "context" "crypto/ecdsa" + "encoding/binary" "math/big" "time" @@ -208,18 +209,12 @@ func toCallArg(msg ethereum.CallMsg) interface{} { func (p *L2Proposer) fetchNextOutput(t Testing) (*eth.OutputResponse, bool, error) { if e2eutils.UseFaultProofs() { - blockNumber, err := p.driver.FetchCurrentBlockNumber(t.Ctx()) + output, err := p.driver.FetchDGFOutput(t.Ctx()) if err != nil { return nil, false, err } - - output, _, err := p.driver.FetchOutput(t.Ctx(), blockNumber) - if err != nil { - return nil, false, err - } - encodedBlockNumber := make([]byte, 32) - copy(encodedBlockNumber[32-len(blockNumber.Bytes()):], blockNumber.Bytes()) + binary.BigEndian.PutUint64(encodedBlockNumber[24:], output.BlockRef.Number) game, err := p.disputeGameFactory.Games(&bind.CallOpts{}, p.driver.Cfg.DisputeGameType, output.OutputRoot, encodedBlockNumber) if err != nil { return nil, false, err @@ -230,7 +225,7 @@ func (p *L2Proposer) fetchNextOutput(t Testing) (*eth.OutputResponse, bool, erro return output, true, nil } else { - return p.driver.FetchNextOutputInfo(t.Ctx()) + return p.driver.FetchL2OOOutput(t.Ctx()) } } diff --git a/op-proposer/flags/flags.go b/op-proposer/flags/flags.go index 850c7232ae93..d4ca19150c47 100644 --- a/op-proposer/flags/flags.go +++ b/op-proposer/flags/flags.go @@ -41,8 +41,8 @@ var ( } PollIntervalFlag = &cli.DurationFlag{ Name: "poll-interval", - Usage: "How frequently to poll L2 for new blocks", - Value: 6 * time.Second, + Usage: "How frequently to poll L2 for new blocks (legacy L2OO)", + Value: 12 * time.Second, EnvVars: prefixEnvVars("POLL_INTERVAL"), } AllowNonFinalizedFlag = &cli.BoolFlag{ @@ -62,8 +62,8 @@ var ( } OutputRetryIntervalFlag = &cli.DurationFlag{ Name: "output-retry-interval", - Usage: "Interval between retrying output fetch if one fails", - Value: time.Minute, + Usage: "Interval between retrying output fetching (DGF)", + Value: 12 * time.Second, EnvVars: prefixEnvVars("OUTPUT_RETRY_INTERVAL"), } DisputeGameTypeFlag = &cli.UintFlag{ diff --git a/op-proposer/proposer/driver.go b/op-proposer/proposer/driver.go index 1730a216f9dd..d4d56e20edb9 100644 --- a/op-proposer/proposer/driver.go +++ b/op-proposer/proposer/driver.go @@ -20,7 +20,6 @@ import ( "github.com/ethereum-optimism/optimism/op-proposer/metrics" "github.com/ethereum-optimism/optimism/op-service/dial" "github.com/ethereum-optimism/optimism/op-service/eth" - "github.com/ethereum-optimism/optimism/op-service/retry" "github.com/ethereum-optimism/optimism/op-service/txmgr" ) @@ -213,9 +212,12 @@ func (l *L2OutputSubmitter) StopL2OutputSubmitting() error { return nil } -// FetchNextOutputInfo gets the block number of the next proposal. -// It returns: the next block number, if the proposal should be made, error -func (l *L2OutputSubmitter) FetchNextOutputInfo(ctx context.Context) (*eth.OutputResponse, bool, error) { +// FetchL2OOOutput gets the next output proposal for the L2OO. +// It queries the L2OO for the earliest next block number that should be proposed. +// It returns the output to propose, and whether the proposal should be submitted at all. +// The passed context is expected to be a lifecycle context. A network timeout +// context will be derived from it. +func (l *L2OutputSubmitter) FetchL2OOOutput(ctx context.Context) (*eth.OutputResponse, bool, error) { if l.l2ooContract == nil { return nil, false, fmt.Errorf("L2OutputOracle contract not set, cannot fetch next output info") } @@ -226,11 +228,11 @@ func (l *L2OutputSubmitter) FetchNextOutputInfo(ctx context.Context) (*eth.Outpu From: l.Txmgr.From(), Context: cCtx, } - nextCheckpointBlock, err := l.l2ooContract.NextBlockNumber(callOpts) + nextCheckpointBlockBig, err := l.l2ooContract.NextBlockNumber(callOpts) if err != nil { - l.Log.Error("Proposer unable to get next block number", "err", err) - return nil, false, err + return nil, false, fmt.Errorf("querying next block number: %w", err) } + nextCheckpointBlock := nextCheckpointBlockBig.Uint64() // Fetch the current L2 heads currentBlockNumber, err := l.FetchCurrentBlockNumber(ctx) if err != nil { @@ -238,76 +240,79 @@ func (l *L2OutputSubmitter) FetchNextOutputInfo(ctx context.Context) (*eth.Outpu } // Ensure that we do not submit a block in the future - if currentBlockNumber.Cmp(nextCheckpointBlock) < 0 { + if currentBlockNumber < nextCheckpointBlock { l.Log.Debug("Proposer submission interval has not elapsed", "currentBlockNumber", currentBlockNumber, "nextBlockNumber", nextCheckpointBlock) return nil, false, nil } - return l.FetchOutput(ctx, nextCheckpointBlock) + output, err := l.FetchOutput(ctx, nextCheckpointBlock) + if err != nil { + return nil, false, fmt.Errorf("fetching output: %w", err) + } + + // Always propose if it's part of the Finalized L2 chain. Or if allowed, if it's part of the safe L2 chain. + if output.BlockRef.Number > output.Status.FinalizedL2.Number && (!l.Cfg.AllowNonFinalized || output.BlockRef.Number > output.Status.SafeL2.Number) { + l.Log.Debug("Not proposing yet, L2 block is not ready for proposal", + "l2_proposal", output.BlockRef, + "l2_safe", output.Status.SafeL2, + "l2_finalized", output.Status.FinalizedL2, + "allow_non_finalized", l.Cfg.AllowNonFinalized) + return output, false, nil + } + return output, true, nil +} + +// FetchDGFOutput gets the next output proposal for the DGF. +// The passed context is expected to be a lifecycle context. A network timeout +// context will be derived from it. +func (l *L2OutputSubmitter) FetchDGFOutput(ctx context.Context) (*eth.OutputResponse, error) { + ctx, cancel := context.WithTimeout(ctx, l.Cfg.NetworkTimeout) + defer cancel() + + blockNum, err := l.FetchCurrentBlockNumber(ctx) + if err != nil { + return nil, err + } + return l.FetchOutput(ctx, blockNum) } // FetchCurrentBlockNumber gets the current block number from the [L2OutputSubmitter]'s [RollupClient]. If the `AllowNonFinalized` configuration // option is set, it will return the safe head block number, and if not, it will return the finalized head block number. -func (l *L2OutputSubmitter) FetchCurrentBlockNumber(ctx context.Context) (*big.Int, error) { +func (l *L2OutputSubmitter) FetchCurrentBlockNumber(ctx context.Context) (uint64, error) { rollupClient, err := l.RollupProvider.RollupClient(ctx) if err != nil { - l.Log.Error("Proposer unable to get rollup client", "err", err) - return nil, err + return 0, fmt.Errorf("getting rollup client: %w", err) } - cCtx, cancel := context.WithTimeout(ctx, l.Cfg.NetworkTimeout) - defer cancel() - - status, err := rollupClient.SyncStatus(cCtx) + status, err := rollupClient.SyncStatus(ctx) if err != nil { - l.Log.Error("Proposer unable to get sync status", "err", err) - return nil, err + return 0, fmt.Errorf("getting sync status: %w", err) } // Use either the finalized or safe head depending on the config. Finalized head is default & safer. - var currentBlockNumber *big.Int if l.Cfg.AllowNonFinalized { - currentBlockNumber = new(big.Int).SetUint64(status.SafeL2.Number) - } else { - currentBlockNumber = new(big.Int).SetUint64(status.FinalizedL2.Number) + return status.SafeL2.Number, nil } - return currentBlockNumber, nil + return status.FinalizedL2.Number, nil } -func (l *L2OutputSubmitter) FetchOutput(ctx context.Context, block *big.Int) (*eth.OutputResponse, bool, error) { +func (l *L2OutputSubmitter) FetchOutput(ctx context.Context, block uint64) (*eth.OutputResponse, error) { rollupClient, err := l.RollupProvider.RollupClient(ctx) if err != nil { - l.Log.Error("Proposer unable to get rollup client", "err", err) - return nil, false, err + return nil, fmt.Errorf("getting rollup client: %w", err) } - cCtx, cancel := context.WithTimeout(ctx, l.Cfg.NetworkTimeout) - defer cancel() - - output, err := rollupClient.OutputAtBlock(cCtx, block.Uint64()) + output, err := rollupClient.OutputAtBlock(ctx, block) if err != nil { - l.Log.Error("Failed to fetch output at block", "block", block, "err", err) - return nil, false, err + return nil, fmt.Errorf("fetching output at block %d: %w", block, err) } if output.Version != supportedL2OutputVersion { - l.Log.Error("Unsupported l2 output version", "output_version", output.Version, "supported_version", supportedL2OutputVersion) - return nil, false, errors.New("unsupported l2 output version") + return nil, fmt.Errorf("unsupported l2 output version: %v, supported: %v", output.Version, supportedL2OutputVersion) } - if output.BlockRef.Number != block.Uint64() { // sanity check, e.g. in case of bad RPC caching - l.Log.Error("Invalid blockNumber", "next_block", block, "output_block", output.BlockRef.Number) - return nil, false, errors.New("invalid blockNumber") + if onum := output.BlockRef.Number; onum != block { // sanity check, e.g. in case of bad RPC caching + return nil, fmt.Errorf("output block number %d mismatches requested %d", output.BlockRef.Number, block) } - - // Always propose if it's part of the Finalized L2 chain. Or if allowed, if it's part of the safe L2 chain. - if output.BlockRef.Number > output.Status.FinalizedL2.Number && (!l.Cfg.AllowNonFinalized || output.BlockRef.Number > output.Status.SafeL2.Number) { - l.Log.Debug("Not proposing yet, L2 block is not ready for proposal", - "l2_proposal", output.BlockRef, - "l2_safe", output.Status.SafeL2, - "l2_finalized", output.Status.FinalizedL2, - "allow_non_finalized", l.Cfg.AllowNonFinalized) - return nil, false, nil - } - return output, true, nil + return output, nil } // ProposeL2OutputTxData creates the transaction data for the ProposeL2Output function @@ -456,19 +461,33 @@ func (l *L2OutputSubmitter) waitNodeSync() error { return dial.WaitRollupSync(l.ctx, l.Log, rollupClient, l1head, time.Second*12) } +// The loopL2OO regularly polls the L2OO for the next block to propose, +// and if the current finalized (or safe) block is past that next block, it +// proposes it. func (l *L2OutputSubmitter) loopL2OO(ctx context.Context) { + defer l.Log.Info("loopL2OO returning") ticker := time.NewTicker(l.Cfg.PollInterval) defer ticker.Stop() for { select { case <-ticker.C: - output, shouldPropose, err := retry.Do2(ctx, 10, &retry.FixedStrategy{Dur: l.Cfg.OutputRetryInterval}, func() (*eth.OutputResponse, bool, error) { - ctx, cancel := context.WithTimeout(ctx, 3*time.Second) - defer cancel() - return l.FetchNextOutputInfo(ctx) - }) - if err != nil || !shouldPropose { - break + // prioritize quit signal + select { + case <-l.done: + return + default: + } + + // A note on retrying: the outer ticker already runs on a short + // poll interval, which has a default value of 6 seconds. So no + // retry logic is needed around output fetching here. + output, shouldPropose, err := l.FetchL2OOOutput(ctx) + if err != nil { + l.Log.Warn("Error getting L2OO output", "err", err) + continue + } else if !shouldPropose { + // debug logging already in FetchL2OOOutput + continue } l.proposeOutput(ctx, output) @@ -478,26 +497,37 @@ func (l *L2OutputSubmitter) loopL2OO(ctx context.Context) { } } +// The loopDGF proposes a new output every proposal interval. It does _not_ query +// the DGF for when to next propose, as the DGF doesn't have the concept of a +// proposal interval, like in the L2OO case. For this reason, it has to keep track +// of the interval itself, for which it uses an internal ticker. func (l *L2OutputSubmitter) loopDGF(ctx context.Context) { + defer l.Log.Info("loopDGF returning") ticker := time.NewTicker(l.Cfg.ProposalInterval) defer ticker.Stop() for { select { case <-ticker.C: - blockNumber, err := retry.Do(ctx, 10, &retry.FixedStrategy{Dur: l.Cfg.OutputRetryInterval}, func() (*big.Int, error) { - return l.FetchCurrentBlockNumber(ctx) - }) - if err != nil { - break + // prioritize quit signal + select { + case <-l.done: + return + default: } - output, shouldPropose, err := retry.Do2(ctx, 10, &retry.FixedStrategy{Dur: l.Cfg.OutputRetryInterval}, func() (*eth.OutputResponse, bool, error) { - ctx, cancel := context.WithTimeout(ctx, 3*time.Second) - defer cancel() - return l.FetchOutput(ctx, blockNumber) - }) - if err != nil || !shouldPropose { - break + var ( + output *eth.OutputResponse + err error + ) + // A note on retrying: because the proposal interval is usually much + // larger than the interval at which to retry proposing on a failed attempt, + // we want to keep retrying getting the output proposal until we succeed. + for output == nil || err != nil { + output, err = l.FetchDGFOutput(ctx) + if err != nil { + l.Log.Warn("Error getting DGF output, retrying...", "err", err) + time.Sleep(l.Cfg.OutputRetryInterval) + } } l.proposeOutput(ctx, output) diff --git a/op-proposer/proposer/driver_test.go b/op-proposer/proposer/driver_test.go index 872313784013..c0dd4d3d3daf 100644 --- a/op-proposer/proposer/driver_test.go +++ b/op-proposer/proposer/driver_test.go @@ -13,12 +13,14 @@ import ( "github.com/ethereum-optimism/optimism/op-service/eth" "github.com/ethereum-optimism/optimism/op-service/testlog" "github.com/ethereum-optimism/optimism/op-service/testutils" - "github.com/ethereum-optimism/optimism/op-service/txmgr/mocks" + txmgrmocks "github.com/ethereum-optimism/optimism/op-service/txmgr/mocks" + "github.com/ethereum/go-ethereum/accounts/abi/bind" "github.com/ethereum/go-ethereum/common" "github.com/ethereum/go-ethereum/core/types" "github.com/ethereum/go-ethereum/log" "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" ) type MockL2OOContract struct { @@ -35,88 +37,88 @@ func (m *MockL2OOContract) NextBlockNumber(opts *bind.CallOpts) (*big.Int, error return args.Get(0).(*big.Int), args.Error(1) } -type mockL2EndpointProvider struct { - ethClient *testutils.MockL2Client - ethClientErr error +type mockRollupEndpointProvider struct { rollupClient *testutils.MockRollupClient rollupClientErr error } -func newEndpointProvider() *mockL2EndpointProvider { - return &mockL2EndpointProvider{ - ethClient: new(testutils.MockL2Client), +func newEndpointProvider() *mockRollupEndpointProvider { + return &mockRollupEndpointProvider{ rollupClient: new(testutils.MockRollupClient), } } -func (p *mockL2EndpointProvider) EthClient(context.Context) (dial.EthClientInterface, error) { - return p.ethClient, p.ethClientErr -} - -func (p *mockL2EndpointProvider) RollupClient(context.Context) (dial.RollupClientInterface, error) { +func (p *mockRollupEndpointProvider) RollupClient(context.Context) (dial.RollupClientInterface, error) { return p.rollupClient, p.rollupClientErr } -func (p *mockL2EndpointProvider) Close() {} +func (p *mockRollupEndpointProvider) Close() {} -func setup(t *testing.T) (*L2OutputSubmitter, *mockL2EndpointProvider, *MockL2OOContract) { +func setup(t *testing.T) (*L2OutputSubmitter, *mockRollupEndpointProvider, *MockL2OOContract, *txmgrmocks.TxManager, *testlog.CapturingHandler) { ep := newEndpointProvider() l2OutputOracleAddr := common.HexToAddress("0x3F8A862E63E759a77DA22d384027D21BF096bA9E") proposerConfig := ProposerConfig{ - PollInterval: 20 * time.Millisecond, - ProposalInterval: 20 * time.Millisecond, - OutputRetryInterval: 1 * time.Millisecond, + PollInterval: time.Microsecond, + ProposalInterval: time.Microsecond, + OutputRetryInterval: time.Microsecond, L2OutputOracleAddr: &l2OutputOracleAddr, } - txmgr := mocks.TxManager{} - txmgr.On("From").Return(common.Address{}) - txmgr.On("BlockNumber", mock.Anything).Return(uint64(100), nil) - txmgr.On("Send", mock.Anything, mock.Anything).Return(&types.Receipt{Status: uint64(1), TxHash: common.Hash{}}, nil) + txmgr := txmgrmocks.NewTxManager(t) + lgr, logs := testlog.CaptureLogger(t, log.LevelDebug) setup := DriverSetup{ - Log: testlog.Logger(t, log.LevelDebug), + Log: lgr, Metr: metrics.NoopMetrics, Cfg: proposerConfig, - Txmgr: &txmgr, + Txmgr: txmgr, RollupProvider: ep, } parsed, err := bindings.L2OutputOracleMetaData.GetAbi() - if err != nil { - panic(err) - } + require.NoError(t, err) ctx, cancel := context.WithCancel(context.Background()) - l2ooContract := MockL2OOContract{} + l2ooContract := new(MockL2OOContract) l2OutputSubmitter := L2OutputSubmitter{ DriverSetup: setup, done: make(chan struct{}), - l2ooContract: &l2ooContract, + l2ooContract: l2ooContract, l2ooABI: parsed, ctx: ctx, cancel: cancel, } - return &l2OutputSubmitter, ep, &l2ooContract + txmgr.On("BlockNumber", mock.Anything).Return(uint64(100), nil).Once() + txmgr.On("Send", mock.Anything, mock.Anything). + Return(&types.Receipt{Status: uint64(1), TxHash: common.Hash{}}, nil). + Once(). + Run(func(_ mock.Arguments) { + // let loops return after first Send call + t.Log("Closing proposer.") + close(l2OutputSubmitter.done) + }) + + return &l2OutputSubmitter, ep, l2ooContract, txmgr, logs } func TestL2OutputSubmitter_OutputRetry(t *testing.T) { tests := []struct { name string }{ - {name: "loopL2OO"}, - {name: "loopDGF"}, + {name: "L2OO"}, + {name: "DGF"}, } + const numFails = 3 for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - ps, ep, l2ooContract := setup(t) + ps, ep, l2ooContract, txmgr, logs := setup(t) - ep.rollupClient.On("SyncStatus").Return(ð.SyncStatus{FinalizedL2: eth.L2BlockRef{Number: 42}}, nil).Once() - ep.rollupClient.ExpectOutputAtBlock(42, nil, fmt.Errorf("failed to fetch output")) + ep.rollupClient.On("SyncStatus").Return(ð.SyncStatus{FinalizedL2: eth.L2BlockRef{Number: 42}}, nil).Times(numFails + 1) + ep.rollupClient.ExpectOutputAtBlock(42, nil, fmt.Errorf("TEST: failed to fetch output")).Times(numFails) ep.rollupClient.ExpectOutputAtBlock( 42, ð.OutputResponse{ @@ -125,24 +127,24 @@ func TestL2OutputSubmitter_OutputRetry(t *testing.T) { Status: ð.SyncStatus{ CurrentL1: eth.L1BlockRef{Hash: common.Hash{}}, FinalizedL2: eth.L2BlockRef{Number: 42}, - }}, + }, + }, nil, ) - if tt.name == "loopDGF" { - go ps.loopDGF(ps.ctx) + if tt.name == "DGF" { + ps.loopDGF(ps.ctx) } else { - l2ooContract.On("NextBlockNumber", mock.AnythingOfType("*bind.CallOpts")).Return(big.NewInt(42), nil).Once() - l2ooContract.On("NextBlockNumber", mock.AnythingOfType("*bind.CallOpts")).Return(big.NewInt(42), nil).Once() - ep.rollupClient.On("SyncStatus").Return(ð.SyncStatus{FinalizedL2: eth.L2BlockRef{Number: 42}}, nil).Once() - go ps.loopL2OO(ps.ctx) + txmgr.On("From").Return(common.Address{}).Times(numFails + 1) + l2ooContract.On("NextBlockNumber", mock.AnythingOfType("*bind.CallOpts")).Return(big.NewInt(42), nil).Times(numFails + 1) + ps.loopL2OO(ps.ctx) } - time.Sleep(25 * time.Millisecond) - close(ps.done) - ep.rollupClient.AssertExpectations(t) l2ooContract.AssertExpectations(t) + require.Len(t, logs.FindLogs(testlog.NewMessageContainsFilter("Error getting "+tt.name)), numFails) + require.NotNil(t, logs.FindLog(testlog.NewMessageFilter("Proposer tx successfully published"))) + require.NotNil(t, logs.FindLog(testlog.NewMessageFilter("loop"+tt.name+" returning"))) }) } } diff --git a/op-service/testutils/mock_rollup_client.go b/op-service/testutils/mock_rollup_client.go index a7a42e878481..2ed6d54a00d0 100644 --- a/op-service/testutils/mock_rollup_client.go +++ b/op-service/testutils/mock_rollup_client.go @@ -18,8 +18,8 @@ func (m *MockRollupClient) OutputAtBlock(ctx context.Context, blockNum uint64) ( return out.Get(0).(*eth.OutputResponse), out.Error(1) } -func (m *MockRollupClient) ExpectOutputAtBlock(blockNum uint64, response *eth.OutputResponse, err error) { - m.Mock.On("OutputAtBlock", blockNum).Once().Return(response, err) +func (m *MockRollupClient) ExpectOutputAtBlock(blockNum uint64, response *eth.OutputResponse, err error) *mock.Call { + return m.Mock.On("OutputAtBlock", blockNum).Once().Return(response, err) } func (m *MockRollupClient) SyncStatus(ctx context.Context) (*eth.SyncStatus, error) { From 9616b22c42927b2d418f1897c9c9c856c8bad0f1 Mon Sep 17 00:00:00 2001 From: Sebastian Stammler Date: Tue, 6 Aug 2024 13:35:41 +0200 Subject: [PATCH 5/5] op-proposer: move done signal check into inner loop --- op-proposer/proposer/driver.go | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/op-proposer/proposer/driver.go b/op-proposer/proposer/driver.go index d4d56e20edb9..7f8caac53f8b 100644 --- a/op-proposer/proposer/driver.go +++ b/op-proposer/proposer/driver.go @@ -508,13 +508,6 @@ func (l *L2OutputSubmitter) loopDGF(ctx context.Context) { for { select { case <-ticker.C: - // prioritize quit signal - select { - case <-l.done: - return - default: - } - var ( output *eth.OutputResponse err error @@ -523,6 +516,12 @@ func (l *L2OutputSubmitter) loopDGF(ctx context.Context) { // larger than the interval at which to retry proposing on a failed attempt, // we want to keep retrying getting the output proposal until we succeed. for output == nil || err != nil { + select { + case <-l.done: + return + default: + } + output, err = l.FetchDGFOutput(ctx) if err != nil { l.Log.Warn("Error getting DGF output, retrying...", "err", err)