From 5af2a8e573a53187338faa205a82b5b9f2495bf7 Mon Sep 17 00:00:00 2001 From: Nodar Ambroladze Date: Mon, 4 Dec 2023 17:41:38 +0100 Subject: [PATCH 1/6] Fix Validator when using external signer to use correct From address, don't require wallet for validator if external signer is enabled --- arbnode/dataposter/data_poster.go | 34 ++++++++++++++++++++----------- arbnode/node.go | 4 ++-- cmd/nitro/nitro.go | 5 +++-- contracts | 2 +- staker/validatorwallet/eoa.go | 4 ++-- system_tests/staker_test.go | 34 ++++++++++++++++++++++++------- 6 files changed, 57 insertions(+), 26 deletions(-) diff --git a/arbnode/dataposter/data_poster.go b/arbnode/dataposter/data_poster.go index 266131a6b9..50d8bfcc99 100644 --- a/arbnode/dataposter/data_poster.go +++ b/arbnode/dataposter/data_poster.go @@ -52,7 +52,7 @@ type DataPoster struct { stopwaiter.StopWaiter headerReader *headerreader.HeaderReader client arbutil.L1Interface - sender common.Address + auth *bind.TransactOpts signer signerFn redisLock AttemptLocker config ConfigFetcher @@ -155,7 +155,7 @@ func NewDataPoster(ctx context.Context, opts *DataPosterOpts) (*DataPoster, erro dp := &DataPoster{ headerReader: opts.HeaderReader, client: opts.HeaderReader.Client(), - sender: opts.Auth.From, + auth: opts.Auth, signer: func(_ context.Context, addr common.Address, tx *types.Transaction) (*types.Transaction, error) { return opts.Auth.Signer(addr, tx) }, @@ -172,7 +172,13 @@ func NewDataPoster(ctx context.Context, opts *DataPosterOpts) (*DataPoster, erro if err != nil { return nil, err } - dp.signer, dp.sender = signer, sender + dp.signer = signer + dp.auth = &bind.TransactOpts{ + From: sender, + Signer: func(address common.Address, tx *types.Transaction) (*types.Transaction, error) { + return signer(context.TODO(), address, tx) + }, + } } return dp, nil } @@ -251,8 +257,12 @@ func externalSigner(ctx context.Context, opts *ExternalSignerCfg) (signerFn, com }, sender, nil } +func (p *DataPoster) Auth() *bind.TransactOpts { + return p.auth +} + func (p *DataPoster) Sender() common.Address { - return p.sender + return p.auth.From } func (p *DataPoster) MaxMempoolTransactions() uint64 { @@ -279,7 +289,7 @@ func (p *DataPoster) canPostWithNonce(ctx context.Context, nextNonce uint64) err // Check that posting a new transaction won't exceed maximum pending // transactions in mempool. if cfg.MaxMempoolTransactions > 0 { - unconfirmedNonce, err := p.client.NonceAt(ctx, p.sender, nil) + unconfirmedNonce, err := p.client.NonceAt(ctx, p.Sender(), nil) if err != nil { return fmt.Errorf("getting nonce of a dataposter sender: %w", err) } @@ -322,7 +332,7 @@ func (p *DataPoster) getNextNonceAndMaybeMeta(ctx context.Context) (uint64, []by // Fall back to using a recent block to get the nonce. This is safe because there's nothing in the queue. nonceQueryBlock := arbmath.UintToBig(arbmath.SaturatingUSub(blockNum, 1)) log.Warn("failed to update nonce with queue empty; falling back to using a recent block", "recentBlock", nonceQueryBlock, "err", err) - nonce, err := p.client.NonceAt(ctx, p.sender, nonceQueryBlock) + nonce, err := p.client.NonceAt(ctx, p.Sender(), nonceQueryBlock) if err != nil { return 0, nil, false, fmt.Errorf("failed to get nonce at block %v: %w", nonceQueryBlock, err) } @@ -360,7 +370,7 @@ func (p *DataPoster) feeAndTipCaps(ctx context.Context, nonce uint64, gasLimit u return nil, nil, fmt.Errorf("latest parent chain block %v missing BaseFee (either the parent chain does not have EIP-1559 or the parent chain node is not synced)", latestHeader.Number) } softConfBlock := arbmath.BigSubByUint(latestHeader.Number, config.NonceRbfSoftConfs) - softConfNonce, err := p.client.NonceAt(ctx, p.sender, softConfBlock) + softConfNonce, err := p.client.NonceAt(ctx, p.Sender(), softConfBlock) if err != nil { return nil, nil, fmt.Errorf("failed to get latest nonce %v blocks ago (block %v): %w", config.NonceRbfSoftConfs, softConfBlock, err) } @@ -476,7 +486,7 @@ func (p *DataPoster) PostTransaction(ctx context.Context, dataCreatedAt time.Tim Data: calldata, AccessList: accessList, } - fullTx, err := p.signer(ctx, p.sender, types.NewTx(&inner)) + fullTx, err := p.signer(ctx, p.Sender(), types.NewTx(&inner)) if err != nil { return nil, fmt.Errorf("signing transaction: %w", err) } @@ -555,7 +565,7 @@ func (p *DataPoster) replaceTx(ctx context.Context, prevTx *storage.QueuedTransa newTx.Sent = false newTx.Data.GasFeeCap = newFeeCap newTx.Data.GasTipCap = newTipCap - newTx.FullTx, err = p.signer(ctx, p.sender, types.NewTx(&newTx.Data)) + newTx.FullTx, err = p.signer(ctx, p.Sender(), types.NewTx(&newTx.Data)) if err != nil { return err } @@ -578,7 +588,7 @@ func (p *DataPoster) updateNonce(ctx context.Context) error { if p.lastBlock != nil && arbmath.BigEquals(p.lastBlock, header.Number) { return nil } - nonce, err := p.client.NonceAt(ctx, p.sender, header.Number) + nonce, err := p.client.NonceAt(ctx, p.Sender(), header.Number) if err != nil { if p.lastBlock != nil { log.Warn("Failed to get current nonce", "lastBlock", p.lastBlock, "newBlock", header.Number, "err", err) @@ -616,7 +626,7 @@ func (p *DataPoster) updateNonce(ctx context.Context) error { func (p *DataPoster) updateBalance(ctx context.Context) error { // Use the pending (representated as -1) balance because we're looking at batches we'd post, // so we want to see how much gas we could afford with our pending state. - balance, err := p.client.BalanceAt(ctx, p.sender, big.NewInt(-1)) + balance, err := p.client.BalanceAt(ctx, p.Sender(), big.NewInt(-1)) if err != nil { return err } @@ -671,7 +681,7 @@ func (p *DataPoster) Start(ctxIn context.Context) { if maxTxsToRbf == 0 { maxTxsToRbf = 512 } - unconfirmedNonce, err := p.client.NonceAt(ctx, p.sender, nil) + unconfirmedNonce, err := p.client.NonceAt(ctx, p.Sender(), nil) if err != nil { log.Warn("Failed to get latest nonce", "err", err) return minWait diff --git a/arbnode/node.go b/arbnode/node.go index c6e117e700..87afcd7a2b 100644 --- a/arbnode/node.go +++ b/arbnode/node.go @@ -580,7 +580,7 @@ func createNodeImpl( // creation into multiple helpers. var wallet staker.ValidatorWalletInterface = validatorwallet.NewNoOp(l1client, deployInfo.Rollup) if !strings.EqualFold(config.Staker.Strategy, "watchtower") { - if config.Staker.UseSmartContractWallet || txOptsValidator == nil { + if config.Staker.UseSmartContractWallet || (txOptsValidator == nil && config.Staker.DataPoster.ExternalSigner.URL == "") { var existingWalletAddress *common.Address if len(config.Staker.ContractWalletAddress) > 0 { if !common.IsHexAddress(config.Staker.ContractWalletAddress) { @@ -598,7 +598,7 @@ func createNodeImpl( if len(config.Staker.ContractWalletAddress) > 0 { return nil, errors.New("validator contract wallet specified but flag to use a smart contract wallet was not specified") } - wallet, err = validatorwallet.NewEOA(dp, deployInfo.Rollup, l1client, txOptsValidator, getExtraGas) + wallet, err = validatorwallet.NewEOA(dp, deployInfo.Rollup, l1client, getExtraGas) if err != nil { return nil, err } diff --git a/cmd/nitro/nitro.go b/cmd/nitro/nitro.go index ddb18beeb7..82dfa894f3 100644 --- a/cmd/nitro/nitro.go +++ b/cmd/nitro/nitro.go @@ -235,8 +235,9 @@ func mainImpl() int { // If sequencer and signing is enabled or batchposter is enabled without // external signing sequencer will need a key. sequencerNeedsKey := (nodeConfig.Node.Sequencer && !nodeConfig.Node.Feed.Output.DisableSigning) || - (nodeConfig.Node.BatchPoster.Enable && nodeConfig.Node.BatchPoster.DataPoster.ExternalSigner.URL != "") - validatorNeedsKey := nodeConfig.Node.Staker.OnlyCreateWalletContract || nodeConfig.Node.Staker.Enable && !strings.EqualFold(nodeConfig.Node.Staker.Strategy, "watchtower") + (nodeConfig.Node.BatchPoster.Enable && nodeConfig.Node.BatchPoster.DataPoster.ExternalSigner.URL == "") + validatorNeedsKey := nodeConfig.Node.Staker.OnlyCreateWalletContract || + (nodeConfig.Node.Staker.Enable && !strings.EqualFold(nodeConfig.Node.Staker.Strategy, "watchtower") && nodeConfig.Node.Staker.DataPoster.ExternalSigner.URL == "") l1Wallet.ResolveDirectoryNames(nodeConfig.Persistent.Chain) defaultL1WalletConfig := conf.DefaultL1WalletConfig diff --git a/contracts b/contracts index 4184926b5e..0a149d2af9 160000 --- a/contracts +++ b/contracts @@ -1 +1 @@ -Subproject commit 4184926b5ea855365fb60b13a4bd52e59b9136ca +Subproject commit 0a149d2af9aee566c4abf493479ec15e5fc32d98 diff --git a/staker/validatorwallet/eoa.go b/staker/validatorwallet/eoa.go index d86181f42f..44af5e2b60 100644 --- a/staker/validatorwallet/eoa.go +++ b/staker/validatorwallet/eoa.go @@ -28,9 +28,9 @@ type EOA struct { getExtraGas func() uint64 } -func NewEOA(dataPoster *dataposter.DataPoster, rollupAddress common.Address, l1Client arbutil.L1Interface, auth *bind.TransactOpts, getExtraGas func() uint64) (*EOA, error) { +func NewEOA(dataPoster *dataposter.DataPoster, rollupAddress common.Address, l1Client arbutil.L1Interface, getExtraGas func() uint64) (*EOA, error) { return &EOA{ - auth: auth, + auth: dataPoster.Auth(), client: l1Client, rollupAddress: rollupAddress, dataPoster: dataPoster, diff --git a/system_tests/staker_test.go b/system_tests/staker_test.go index 6ac8dfddcb..ce726dcf83 100644 --- a/system_tests/staker_test.go +++ b/system_tests/staker_test.go @@ -12,6 +12,7 @@ import ( "errors" "fmt" "math/big" + "net/http" "strings" "testing" "time" @@ -59,6 +60,19 @@ func stakerTestImpl(t *testing.T, faultyStaker bool, honestStakerInactive bool) t.Parallel() ctx, cancelCtx := context.WithCancel(context.Background()) defer cancelCtx() + httpSrv, srv := newServer(ctx, t) + t.Cleanup(func() { + if err := httpSrv.Shutdown(ctx); err != nil { + t.Fatalf("Error shutting down http server: %v", err) + } + }) + go func() { + log.Debug("Server is listening on port 1234...") + if err := httpSrv.ListenAndServeTLS(signerServerCert, signerServerKey); err != nil && err != http.ErrServerClosed { + log.Debug("ListenAndServeTLS() failed", "error", err) + return + } + }() var transferGas = util.NormalizeL2GasForL1GasInitial(800_000, params.GWei) // include room for aggregator L1 costs builder := NewNodeBuilder(ctx).DefaultConfig(t, true) @@ -72,6 +86,11 @@ func stakerTestImpl(t *testing.T, faultyStaker bool, honestStakerInactive bool) cleanupA := builder.Build(t) defer cleanupA() + addNewBatchPoster(ctx, t, builder, srv.address) + + builder.L1.SendWaitTestTransactions(t, []*types.Transaction{ + builder.L1Info.PrepareTxTo("Faucet", &srv.address, 30000, big.NewInt(1).Mul(big.NewInt(1e18), big.NewInt(1e18)), nil)}) + l2nodeA := builder.L2.ConsensusNode execNodeA := builder.L2.ExecNode @@ -133,7 +152,7 @@ func stakerTestImpl(t *testing.T, faultyStaker bool, honestStakerInactive bool) rollupABI, err := abi.JSON(strings.NewReader(rollupgen.RollupAdminLogicABI)) Require(t, err, "unable to parse rollup ABI") - setValidatorCalldata, err := rollupABI.Pack("setValidator", []common.Address{valWalletAddrA, l1authB.From}, []bool{true, true}) + setValidatorCalldata, err := rollupABI.Pack("setValidator", []common.Address{valWalletAddrA, l1authB.From, srv.address}, []bool{true, true, true}) Require(t, err, "unable to generate setValidator calldata") tx, err := upgradeExecutor.ExecuteCall(&deployAuth, l2nodeA.DeployInfo.Rollup, setValidatorCalldata) Require(t, err, "unable to set validators") @@ -199,12 +218,13 @@ func stakerTestImpl(t *testing.T, faultyStaker bool, honestStakerInactive bool) Require(t, err) } Require(t, err) - - dpB, err := arbnode.StakerDataposter(ctx, rawdb.NewTable(l2nodeB.ArbDB, storage.StakerPrefix), l2nodeB.L1Reader, &l1authB, NewFetcherFromConfig(arbnode.ConfigDefaultL1NonSequencerTest()), nil) + cfg := arbnode.ConfigDefaultL1NonSequencerTest() + cfg.Staker.DataPoster.ExternalSigner = *externalSignerTestCfg(srv.address) + dpB, err := arbnode.StakerDataposter(ctx, rawdb.NewTable(l2nodeB.ArbDB, storage.StakerPrefix), l2nodeB.L1Reader, &l1authB, NewFetcherFromConfig(cfg), nil) if err != nil { t.Fatalf("Error creating validator dataposter: %v", err) } - valWalletB, err := validatorwallet.NewEOA(dpB, l2nodeB.DeployInfo.Rollup, l2nodeB.L1Reader.Client(), &l1authB, func() uint64 { return 0 }) + valWalletB, err := validatorwallet.NewEOA(dpB, l2nodeB.DeployInfo.Rollup, l2nodeB.L1Reader.Client(), func() uint64 { return 0 }) Require(t, err) valConfig.Strategy = "MakeNodes" statelessB, err := staker.NewStatelessBlockValidator( @@ -365,14 +385,14 @@ func stakerTestImpl(t *testing.T, faultyStaker bool, honestStakerInactive bool) Require(t, err, "EnsureTxSucceeded failed for staker", stakerName, "tx") } if faultyStaker { - conflictInfo, err := validatorUtils.FindStakerConflict(&bind.CallOpts{}, l2nodeA.DeployInfo.Rollup, l1authA.From, l1authB.From, big.NewInt(1024)) + conflictInfo, err := validatorUtils.FindStakerConflict(&bind.CallOpts{}, l2nodeA.DeployInfo.Rollup, l1authA.From, srv.address, big.NewInt(1024)) Require(t, err) if staker.ConflictType(conflictInfo.Ty) == staker.CONFLICT_TYPE_FOUND { cancelBackgroundTxs() } } if faultyStaker && !sawStakerZombie { - sawStakerZombie, err = rollup.IsZombie(&bind.CallOpts{}, l1authB.From) + sawStakerZombie, err = rollup.IsZombie(&bind.CallOpts{}, srv.address) Require(t, err) } isHonestZombie, err := rollup.IsZombie(&bind.CallOpts{}, valWalletAddrA) @@ -393,7 +413,7 @@ func stakerTestImpl(t *testing.T, faultyStaker bool, honestStakerInactive bool) Require(t, err) } if !stakerBWasStaked { - stakerBWasStaked, err = rollup.IsStaked(&bind.CallOpts{}, l1authB.From) + stakerBWasStaked, err = rollup.IsStaked(&bind.CallOpts{}, srv.address) Require(t, err) } for j := 0; j < 5; j++ { From 781f0f9dca14f3d2277c28807f6ac132bc7bc170 Mon Sep 17 00:00:00 2001 From: Nodar Ambroladze Date: Tue, 5 Dec 2023 18:08:35 +0100 Subject: [PATCH 2/6] Take external signer into account when creating staker dataposter --- arbnode/node.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/arbnode/node.go b/arbnode/node.go index 87afcd7a2b..fc415c1171 100644 --- a/arbnode/node.go +++ b/arbnode/node.go @@ -302,10 +302,10 @@ func StakerDataposter( ctx context.Context, db ethdb.Database, l1Reader *headerreader.HeaderReader, transactOpts *bind.TransactOpts, cfgFetcher ConfigFetcher, syncMonitor *SyncMonitor, ) (*dataposter.DataPoster, error) { - if transactOpts == nil { + cfg := cfgFetcher.Get() + if transactOpts == nil && cfg.BatchPoster.DataPoster.ExternalSigner.URL == "" { return nil, nil } - cfg := cfgFetcher.Get() mdRetriever := func(ctx context.Context, blockNum *big.Int) ([]byte, error) { return nil, nil } From 7264943cad6d70659dc922d0169aad6b42544be5 Mon Sep 17 00:00:00 2001 From: Nodar Ambroladze Date: Tue, 5 Dec 2023 18:20:13 +0100 Subject: [PATCH 3/6] Fix external signer check (use correct config flag) when creating dataposter for staker --- arbnode/node.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/arbnode/node.go b/arbnode/node.go index fc415c1171..4a08c9d718 100644 --- a/arbnode/node.go +++ b/arbnode/node.go @@ -303,7 +303,7 @@ func StakerDataposter( transactOpts *bind.TransactOpts, cfgFetcher ConfigFetcher, syncMonitor *SyncMonitor, ) (*dataposter.DataPoster, error) { cfg := cfgFetcher.Get() - if transactOpts == nil && cfg.BatchPoster.DataPoster.ExternalSigner.URL == "" { + if transactOpts == nil && cfg.Staker.DataPoster.ExternalSigner.URL == "" { return nil, nil } mdRetriever := func(ctx context.Context, blockNum *big.Int) ([]byte, error) { From 9ad1980a6b630dec5018d9ee14213626ba1a1f54 Mon Sep 17 00:00:00 2001 From: Nodar Ambroladze Date: Wed, 6 Dec 2023 13:23:52 +0100 Subject: [PATCH 4/6] Use external signer address for redis key, log correct tx sender address --- arbnode/node.go | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/arbnode/node.go b/arbnode/node.go index 4a08c9d718..593079feb3 100644 --- a/arbnode/node.go +++ b/arbnode/node.go @@ -323,6 +323,12 @@ func StakerDataposter( dpCfg := func() *dataposter.DataPosterConfig { return &cfg.Staker.DataPoster } + var sender string + if transactOpts != nil { + sender = transactOpts.From.String() + } else { + sender = cfg.Staker.DataPoster.ExternalSigner.Address + } return dataposter.NewDataPoster(ctx, &dataposter.DataPosterOpts{ Database: db, @@ -332,8 +338,7 @@ func StakerDataposter( RedisLock: redisLock, Config: dpCfg, MetadataRetriever: mdRetriever, - // transactOpts is non-nil, it's checked at the beginning. - RedisKey: transactOpts.From.String() + ".staker-data-poster.queue", + RedisKey: sender + ".staker-data-poster.queue", }) } @@ -618,15 +623,17 @@ func createNodeImpl( if err := wallet.Initialize(ctx); err != nil { return nil, err } - var txValidatorSenderPtr *common.Address + var validatorAddr string if txOptsValidator != nil { - txValidatorSenderPtr = &txOptsValidator.From + validatorAddr = txOptsValidator.From.String() + } else { + validatorAddr = config.Staker.DataPoster.ExternalSigner.Address } whitelisted, err := stakerObj.IsWhitelisted(ctx) if err != nil { return nil, err } - log.Info("running as validator", "txSender", txValidatorSenderPtr, "actingAsWallet", wallet.Address(), "whitelisted", whitelisted, "strategy", config.Staker.Strategy) + log.Info("running as validator", "txSender", validatorAddr, "actingAsWallet", wallet.Address(), "whitelisted", whitelisted, "strategy", config.Staker.Strategy) } var batchPoster *BatchPoster From b83126e39deee092dca63aad8f2ea9e40d9ec407 Mon Sep 17 00:00:00 2001 From: Nodar Ambroladze Date: Thu, 7 Dec 2023 16:49:10 +0100 Subject: [PATCH 5/6] Don't error out if batchposter txopts aren'ts specified but external signer is enabled --- arbnode/node.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/arbnode/node.go b/arbnode/node.go index 593079feb3..f2ed2941fa 100644 --- a/arbnode/node.go +++ b/arbnode/node.go @@ -639,7 +639,7 @@ func createNodeImpl( var batchPoster *BatchPoster var delayedSequencer *DelayedSequencer if config.BatchPoster.Enable { - if txOptsBatchPoster == nil { + if txOptsBatchPoster == nil && config.BatchPoster.DataPoster.ExternalSigner.URL == "" { return nil, errors.New("batchposter, but no TxOpts") } batchPoster, err = NewBatchPoster(ctx, &BatchPosterOpts{ From 02295212355c195556661c776f2c730eba42ff7a Mon Sep 17 00:00:00 2001 From: Nodar Ambroladze Date: Thu, 7 Dec 2023 18:50:12 +0100 Subject: [PATCH 6/6] Don't run batch_poster system tests in parallel --- system_tests/batch_poster_test.go | 1 - 1 file changed, 1 deletion(-) diff --git a/system_tests/batch_poster_test.go b/system_tests/batch_poster_test.go index f0c115e2f8..873a87c6f9 100644 --- a/system_tests/batch_poster_test.go +++ b/system_tests/batch_poster_test.go @@ -61,7 +61,6 @@ func addNewBatchPoster(ctx context.Context, t *testing.T, builder *NodeBuilder, } func testBatchPosterParallel(t *testing.T, useRedis bool) { - t.Parallel() ctx, cancel := context.WithCancel(context.Background()) defer cancel() httpSrv, srv := newServer(ctx, t)