Merge pull request #1986 from OffchainLabs/batch-poster-normal-estima…

…tion

arbnode/batch_poster.go

@@ -27,7 +27,6 @@ import (
 	"github.com/ethereum/go-ethereum/log"
 	"github.com/ethereum/go-ethereum/metrics"
 	"github.com/ethereum/go-ethereum/rlp"
-	"github.com/ethereum/go-ethereum/rpc"
 
 	"github.com/offchainlabs/nitro/arbnode/dataposter"
 	"github.com/offchainlabs/nitro/arbnode/redislock"
@@ -757,32 +756,30 @@ func (b *BatchPoster) encodeAddBatch(seqNum *big.Int, prevMsgNum arbutil.Message
 	return fullData, nil
 }
 
-func (b *BatchPoster) estimateGas(ctx context.Context, sequencerMessage []byte, delayedMessages uint64) (uint64, error) {
+func (b *BatchPoster) estimateGas(ctx context.Context, sequencerMessage []byte, delayedMessages uint64, realData []byte, realNonce uint64) (uint64, error) {
 	config := b.config()
-	callOpts := &bind.CallOpts{
-		Context: ctx,
-	}
-	if config.DataPoster.WaitForL1Finality {
-		callOpts.BlockNumber = big.NewInt(int64(rpc.SafeBlockNumber))
-	}
-	safeDelayedMessagesBig, err := b.bridge.DelayedMessageCount(callOpts)
-	if err != nil {
-		return 0, fmt.Errorf("failed to get the confirmed delayed message count: %w", err)
-	}
-	if !safeDelayedMessagesBig.IsUint64() {
-		return 0, fmt.Errorf("calling delayedMessageCount() on the bridge returned a non-uint64 result %v", safeDelayedMessagesBig)
-	}
-	safeDelayedMessages := safeDelayedMessagesBig.Uint64()
-	if safeDelayedMessages > delayedMessages {
-		// On restart, we may be trying to estimate gas for a batch whose successor has
-		// already made it into pending state, if not latest state.
-		// In that case, we might get a revert with `DelayedBackwards()`.
-		// To avoid that, we artificially increase the delayed messages to `safeDelayedMessages`.
-		// In theory, this might reduce gas usage, but only by a factor that's already
-		// accounted for in `config.ExtraBatchGas`, as that same factor can appear if a user
-		// posts a new delayed message that we didn't see while gas estimating.
-		delayedMessages = safeDelayedMessages
+	useNormalEstimation := b.dataPoster.MaxMempoolTransactions() == 1
+	if !useNormalEstimation {
+		// Check if we can use normal estimation anyways because we're at the latest nonce
+		latestNonce, err := b.l1Reader.Client().NonceAt(ctx, b.dataPoster.Sender(), nil)
+		if err != nil {
+			return 0, err
+		}
+		useNormalEstimation = latestNonce == realNonce
+	}
+	if useNormalEstimation {
+		// If we're at the latest nonce, we can skip the special future tx estimate stuff
+		gas, err := b.l1Reader.Client().EstimateGas(ctx, ethereum.CallMsg{
+			From: b.dataPoster.Sender(),
+			To:   &b.seqInboxAddr,
+			Data: realData,
+		})
+		if err != nil {
+			return 0, err
+		}
+		return gas + config.ExtraBatchGas, nil
 	}
+
 	// Here we set seqNum to MaxUint256, and prevMsgNum to 0, because it disables the smart contracts' consistency checks.
 	// However, we set nextMsgNum to 1 because it is necessary for a correct estimation for the final to be non-zero.
 	// Because we're likely estimating against older state, this might not be the actual next message,
@@ -805,7 +802,6 @@ func (b *BatchPoster) estimateGas(ctx context.Context, sequencerMessage []byte,
 			"error estimating gas for batch",
 			"err", err,
 			"delayedMessages", delayedMessages,
-			"safeDelayedMessages", safeDelayedMessages,
 			"sequencerMessageHeader", hex.EncodeToString(sequencerMessageHeader),
 			"sequencerMessageLen", len(sequencerMessage),
 		)
@@ -858,6 +854,11 @@ func (b *BatchPoster) maybePostSequencerBatch(ctx context.Context) (bool, error)
 	}
 	firstMsgTime := time.Unix(int64(firstMsg.Message.Header.Timestamp), 0)
 
+	lastPotentialMsg, err := b.streamer.GetMessage(msgCount - 1)
+	if err != nil {
+		return false, err
+	}
+
 	config := b.config()
 	forcePostBatch := time.Since(firstMsgTime) >= config.MaxDelay
 
@@ -1000,11 +1001,18 @@ func (b *BatchPoster) maybePostSequencerBatch(ctx context.Context) (bool, error)
 		}
 	}
 
-	gasLimit, err := b.estimateGas(ctx, sequencerMsg, b.building.segments.delayedMsg)
+	data, err := b.encodeAddBatch(new(big.Int).SetUint64(batchPosition.NextSeqNum), batchPosition.MessageCount, b.building.msgCount, sequencerMsg, b.building.segments.delayedMsg)
 	if err != nil {
 		return false, err
 	}
-	data, err := b.encodeAddBatch(new(big.Int).SetUint64(batchPosition.NextSeqNum), batchPosition.MessageCount, b.building.msgCount, sequencerMsg, b.building.segments.delayedMsg)
+	// On restart, we may be trying to estimate gas for a batch whose successor has
+	// already made it into pending state, if not latest state.
+	// In that case, we might get a revert with `DelayedBackwards()`.
+	// To avoid that, we artificially increase the delayed messages to `lastPotentialMsg.DelayedMessagesRead`.
+	// In theory, this might reduce gas usage, but only by a factor that's already
+	// accounted for in `config.ExtraBatchGas`, as that same factor can appear if a user
+	// posts a new delayed message that we didn't see while gas estimating.
+	gasLimit, err := b.estimateGas(ctx, sequencerMsg, lastPotentialMsg.DelayedMessagesRead, data, nonce)
 	if err != nil {
 		return false, err
 	}

arbnode/dataposter/data_poster.go

@@ -56,6 +56,7 @@ type DataPoster struct {
 	signer            signerFn
 	redisLock         AttemptLocker
 	config            ConfigFetcher
+	usingNoOpStorage  bool
 	replacementTimes  []time.Duration
 	metadataRetriever func(ctx context.Context, blockNum *big.Int) ([]byte, error)
 
@@ -119,8 +120,9 @@ func NewDataPoster(ctx context.Context, opts *DataPosterOpts) (*DataPoster, erro
 	if err != nil {
 		return nil, err
 	}
+	useNoOpStorage := cfg.UseNoOpStorage
 	if opts.HeaderReader.IsParentChainArbitrum() && !cfg.UseNoOpStorage {
-		cfg.UseNoOpStorage = true
+		useNoOpStorage = true
 		log.Info("Disabling data poster storage, as parent chain appears to be an Arbitrum chain without a mempool")
 	}
 	encF := func() storage.EncoderDecoderInterface {
@@ -131,7 +133,7 @@ func NewDataPoster(ctx context.Context, opts *DataPosterOpts) (*DataPoster, erro
 	}
 	var queue QueueStorage
 	switch {
-	case cfg.UseNoOpStorage:
+	case useNoOpStorage:
 		queue = &noop.Storage{}
 	case opts.RedisClient != nil:
 		var err error
@@ -158,6 +160,7 @@ func NewDataPoster(ctx context.Context, opts *DataPosterOpts) (*DataPoster, erro
 			return opts.Auth.Signer(addr, tx)
 		},
 		config:            opts.Config,
+		usingNoOpStorage:  useNoOpStorage,
 		replacementTimes:  replacementTimes,
 		metadataRetriever: opts.MetadataRetriever,
 		queue:             queue,
@@ -252,6 +255,13 @@ func (p *DataPoster) Sender() common.Address {
 	return p.sender
 }
 
+func (p *DataPoster) MaxMempoolTransactions() uint64 {
+	if p.usingNoOpStorage {
+		return 1
+	}
+	return p.config().MaxMempoolTransactions
+}
+
 // Does basic check whether posting transaction with specified nonce would
 // result in exceeding maximum queue length or maximum transactions in mempool.
 func (p *DataPoster) canPostWithNonce(ctx context.Context, nextNonce uint64) error {
@@ -398,7 +408,7 @@ func (p *DataPoster) feeAndTipCaps(ctx context.Context, nonce uint64, gasLimit u
 
 	latestBalance := p.balance
 	balanceForTx := new(big.Int).Set(latestBalance)
-	if config.AllocateMempoolBalance && !config.UseNoOpStorage {
+	if config.AllocateMempoolBalance && !p.usingNoOpStorage {
 		// We reserve half the balance for the first transaction, and then split the remaining balance for all after that.
 		// With noop storage, we don't try to replace-by-fee, so we don't need to worry about this.
 		balanceForTx.Div(balanceForTx, common.Big2)
@@ -500,12 +510,12 @@ func (p *DataPoster) sendTx(ctx context.Context, prevTx *storage.QueuedTransacti
 	}
 	if err := p.client.SendTransaction(ctx, newTx.FullTx); err != nil {
 		if !strings.Contains(err.Error(), "already known") && !strings.Contains(err.Error(), "nonce too low") {
-			log.Warn("DataPoster failed to send transaction", "err", err, "nonce", newTx.FullTx.Nonce(), "feeCap", newTx.FullTx.GasFeeCap(), "tipCap", newTx.FullTx.GasTipCap())
+			log.Warn("DataPoster failed to send transaction", "err", err, "nonce", newTx.FullTx.Nonce(), "feeCap", newTx.FullTx.GasFeeCap(), "tipCap", newTx.FullTx.GasTipCap(), "gas", newTx.FullTx.Gas())
 			return err
 		}
 		log.Info("DataPoster transaction already known", "err", err, "nonce", newTx.FullTx.Nonce(), "hash", newTx.FullTx.Hash())
 	} else {
-		log.Info("DataPoster sent transaction", "nonce", newTx.FullTx.Nonce(), "hash", newTx.FullTx.Hash(), "feeCap", newTx.FullTx.GasFeeCap())
+		log.Info("DataPoster sent transaction", "nonce", newTx.FullTx.Nonce(), "hash", newTx.FullTx.Hash(), "feeCap", newTx.FullTx.GasFeeCap(), "tipCap", newTx.FullTx.GasTipCap(), "gas", newTx.FullTx.Gas())
 	}
 	newerTx := *newTx
 	newerTx.Sent = true