-
Notifications
You must be signed in to change notification settings - Fork 39
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add maxBatchInterval to skip some empty batches #1599
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
Large diffs are not rendered by default.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -29,6 +29,8 @@ import ( | |
"github.com/obscuronet/go-obscuro/go/enclave/genesis" | ||
) | ||
|
||
var ErrNoTransactionsToProcess = fmt.Errorf("no transactions to process") | ||
|
||
// batchExecutor - the component responsible for executing batches | ||
type batchExecutor struct { | ||
storage storage.Storage | ||
|
@@ -128,7 +130,7 @@ func (executor *batchExecutor) refundL1Fees(stateDB *state.StateDB, context *Bat | |
} | ||
} | ||
|
||
func (executor *batchExecutor) ComputeBatch(context *BatchExecutionContext) (*ComputedBatch, error) { | ||
func (executor *batchExecutor) ComputeBatch(context *BatchExecutionContext, failForEmptyBatch bool) (*ComputedBatch, error) { //nolint:gocognit | ||
defer core.LogMethodDuration(executor.logger, measure.NewStopwatch(), "Batch context processed") | ||
|
||
// sanity check that the l1 block exists. We don't have to execute batches of forks. | ||
|
@@ -207,6 +209,15 @@ func (executor *batchExecutor) ComputeBatch(context *BatchExecutionContext) (*Co | |
} | ||
|
||
executor.populateHeader(©Batch, allReceipts(txReceipts, ccReceipts)) | ||
if failForEmptyBatch && | ||
len(txReceipts) == 0 && | ||
len(ccReceipts) == 0 && | ||
len(transactionsToProcess) == 0 && | ||
len(crossChainTransactions) == 0 && | ||
len(messages) == 0 && | ||
len(transfers) == 0 { | ||
return nil, ErrNoTransactionsToProcess | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The AI bunny is right to be concerned I think. I don't like adding the lint ignore above and this approach does feel messy. But by bailing out within this method it's easy to be sure there was nothing important going to be in the batch and it also avoid committing anything to the stateDB cache. Any ideas for how to do this more cleanly? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it's ok |
||
} | ||
Comment on lines
209
to
+220
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The new code checks if the batch is empty when + func (executor *batchExecutor) isBatchEmpty(txReceipts []*types.Receipt, ccReceipts []*types.Receipt, transactionsToProcess []*types.Transaction, crossChainTransactions []*types.Transaction, messages []*types.CrossChainMessage, transfers []*types.CrossChainTransfer) bool {
+ return len(txReceipts) == 0 &&
+ len(ccReceipts) == 0 &&
+ len(transactionsToProcess) == 0 &&
+ len(crossChainTransactions) == 0 &&
+ len(messages) == 0 &&
+ len(transfers) == 0
+ }
+
executor.populateHeader(©Batch, allReceipts(txReceipts, ccReceipts))
- if failForEmptyBatch &&
- len(txReceipts) == 0 &&
- len(ccReceipts) == 0 &&
- len(transactionsToProcess) == 0 &&
- len(crossChainTransactions) == 0 &&
- len(messages) == 0 &&
- len(transfers) == 0 {
+ if failForEmptyBatch && executor.isBatchEmpty(txReceipts, ccReceipts, transactionsToProcess, crossChainTransactions, messages, transfers) {
return nil, ErrNoTransactionsToProcess
} |
||
|
||
// the logs and receipts produced by the EVM have the wrong hash which must be adjusted | ||
for _, receipt := range txReceipts { | ||
|
@@ -250,7 +261,7 @@ func (executor *batchExecutor) ExecuteBatch(batch *core.Batch) (types.Receipts, | |
SequencerNo: batch.Header.SequencerOrderNo, | ||
Creator: batch.Header.Coinbase, | ||
BaseFee: batch.Header.BaseFee, | ||
}) | ||
}, false) // this execution is not used when first producing a batch, we never want to fail for empty batches | ||
if err != nil { | ||
return nil, fmt.Errorf("failed computing batch %s. Cause: %w", batch.Hash(), err) | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -98,7 +98,7 @@ func NewSequencer( | |
} | ||
} | ||
|
||
func (s *sequencer) CreateBatch() error { | ||
func (s *sequencer) CreateBatch(skipBatchIfEmpty bool) error { | ||
hasGenesis, err := s.batchRegistry.HasGenesisBatch() | ||
if err != nil { | ||
return fmt.Errorf("unknown genesis batch state. Cause: %w", err) | ||
|
@@ -114,7 +114,7 @@ func (s *sequencer) CreateBatch() error { | |
return s.initGenesis(l1HeadBlock) | ||
} | ||
|
||
return s.createNewHeadBatch(l1HeadBlock) | ||
return s.createNewHeadBatch(l1HeadBlock, skipBatchIfEmpty) | ||
} | ||
|
||
// TODO - This is iffy, the producer commits the stateDB. The producer | ||
|
@@ -149,7 +149,7 @@ func (s *sequencer) initGenesis(block *common.L1Block) error { | |
return nil | ||
} | ||
|
||
func (s *sequencer) createNewHeadBatch(l1HeadBlock *common.L1Block) error { | ||
func (s *sequencer) createNewHeadBatch(l1HeadBlock *common.L1Block, skipBatchIfEmpty bool) error { | ||
headBatchSeq := s.batchRegistry.HeadBatchSeq() | ||
if headBatchSeq == nil { | ||
headBatchSeq = big.NewInt(int64(common.L2GenesisSeqNo)) | ||
|
@@ -186,7 +186,12 @@ func (s *sequencer) createNewHeadBatch(l1HeadBlock *common.L1Block) error { | |
} | ||
|
||
// todo - time is set only here; take from l1 block? | ||
if _, err := s.produceBatch(sequencerNo.Add(sequencerNo, big.NewInt(1)), l1HeadBlock.Hash(), headBatch.Hash(), transactions, uint64(time.Now().Unix())); err != nil { | ||
if _, err := s.produceBatch(sequencerNo.Add(sequencerNo, big.NewInt(1)), l1HeadBlock.Hash(), headBatch.Hash(), transactions, uint64(time.Now().Unix()), skipBatchIfEmpty); err != nil { | ||
if errors.Is(err, components.ErrNoTransactionsToProcess) { | ||
// skip batch production when there are no transactions to process | ||
s.logger.Info("Skipping batch production, no transactions to execute") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we might want to eventually make this a DEBUG, as we'll see quite a lot of it There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah 100%, it will be implied by the silence I guess. But yeah I'll leave it there for now just to make sure it's behaving as expected until we're confident. |
||
return nil | ||
} | ||
return fmt.Errorf(" failed producing batch. Cause: %w", err) | ||
} | ||
|
||
|
@@ -197,7 +202,7 @@ func (s *sequencer) createNewHeadBatch(l1HeadBlock *common.L1Block) error { | |
return nil | ||
} | ||
|
||
func (s *sequencer) produceBatch(sequencerNo *big.Int, l1Hash common.L1BlockHash, headBatch common.L2BatchHash, transactions common.L2Transactions, batchTime uint64) (*core.Batch, error) { | ||
func (s *sequencer) produceBatch(sequencerNo *big.Int, l1Hash common.L1BlockHash, headBatch common.L2BatchHash, transactions common.L2Transactions, batchTime uint64, failForEmptyBatch bool) (*core.Batch, error) { | ||
cb, err := s.batchProducer.ComputeBatch(&components.BatchExecutionContext{ | ||
BlockPtr: l1Hash, | ||
ParentPtr: headBatch, | ||
|
@@ -207,7 +212,7 @@ func (s *sequencer) produceBatch(sequencerNo *big.Int, l1Hash common.L1BlockHash | |
BaseFee: s.settings.BaseFee, | ||
ChainConfig: s.chainConfig, | ||
SequencerNo: sequencerNo, | ||
}) | ||
}, failForEmptyBatch) | ||
if err != nil { | ||
return nil, fmt.Errorf("failed computing batch. Cause: %w", err) | ||
} | ||
|
@@ -317,8 +322,8 @@ func (s *sequencer) duplicateBatches(l1Head *types.Block, nonCanonicalL1Path []c | |
return fmt.Errorf("could not fetch sequencer no. Cause %w", err) | ||
} | ||
sequencerNo = sequencerNo.Add(sequencerNo, big.NewInt(1)) | ||
// create the duplicate and store/broadcast it | ||
b, err := s.produceBatch(sequencerNo, l1Head.ParentHash(), currentHead, orphanBatch.Transactions, orphanBatch.Header.Time) | ||
// create the duplicate and store/broadcast it, recreate batch even if it was empty | ||
b, err := s.produceBatch(sequencerNo, l1Head.ParentHash(), currentHead, orphanBatch.Transactions, orphanBatch.Header.Time, false) | ||
if err != nil { | ||
return fmt.Errorf("could not produce batch. Cause %w", err) | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This config key looks a bit different because it's using the new naming convention (got a PR in flight to update the rest).