From 06269e0ec3d2efad07dc894a4e929baa0172ecc4 Mon Sep 17 00:00:00 2001 From: Lee Bousfield Date: Mon, 18 Dec 2023 09:31:37 -0700 Subject: [PATCH] Address PR review comments --- arbnode/batch_poster.go | 37 ++++++++++++++++++----------- util/arbmath/moving_average.go | 17 ++++++------- util/arbmath/moving_average_test.go | 24 ++++++++++++++----- 3 files changed, 50 insertions(+), 28 deletions(-) diff --git a/arbnode/batch_poster.go b/arbnode/batch_poster.go index 594eec40d9..a54336fd5a 100644 --- a/arbnode/batch_poster.go +++ b/arbnode/batch_poster.go @@ -259,20 +259,23 @@ func NewBatchPoster(ctx context.Context, opts *BatchPosterOpts) (*BatchPoster, e return nil, err } b := &BatchPoster{ - l1Reader: opts.L1Reader, - inbox: opts.Inbox, - streamer: opts.Streamer, - syncMonitor: opts.SyncMonitor, - config: opts.Config, - bridge: bridge, - seqInbox: seqInbox, - seqInboxABI: seqInboxABI, - seqInboxAddr: opts.DeployInfo.SequencerInbox, - gasRefunderAddr: opts.Config().gasRefunder, - bridgeAddr: opts.DeployInfo.Bridge, - daWriter: opts.DAWriter, - redisLock: redisLock, - messagesPerBatch: arbmath.NewMovingAverage[uint64](20), + l1Reader: opts.L1Reader, + inbox: opts.Inbox, + streamer: opts.Streamer, + syncMonitor: opts.SyncMonitor, + config: opts.Config, + bridge: bridge, + seqInbox: seqInbox, + seqInboxABI: seqInboxABI, + seqInboxAddr: opts.DeployInfo.SequencerInbox, + gasRefunderAddr: opts.Config().gasRefunder, + bridgeAddr: opts.DeployInfo.Bridge, + daWriter: opts.DAWriter, + redisLock: redisLock, + } + b.messagesPerBatch, err = arbmath.NewMovingAverage[uint64](20) + if err != nil { + return nil, err } dataPosterConfigFetcher := func() *dataposter.DataPosterConfig { return &(opts.Config().DataPoster) @@ -1070,6 +1073,12 @@ func (b *BatchPoster) maybePostSequencerBatch(ctx context.Context) (bool, error) if messagesPerBatch == 0 { // This should be impossible because we always post at least one message in a batch. // That said, better safe than sorry, as we would panic if this remained at 0. + log.Warn( + "messagesPerBatch is somehow zero", + "postedMessages", postedMessages, + "buildingFrom", batchPosition.MessageCount, + "buildingTo", b.building.msgCount, + ) messagesPerBatch = 1 } backlog := uint64(unpostedMessages) / messagesPerBatch diff --git a/util/arbmath/moving_average.go b/util/arbmath/moving_average.go index fe5a349f4c..fcd5f6e307 100644 --- a/util/arbmath/moving_average.go +++ b/util/arbmath/moving_average.go @@ -3,36 +3,37 @@ package arbmath +import "fmt" + // A simple moving average of a generic number type. type MovingAverage[T Number] struct { - period int buffer []T bufferPosition int sum T } -func NewMovingAverage[T Number](period int) *MovingAverage[T] { +func NewMovingAverage[T Number](period int) (*MovingAverage[T], error) { if period <= 0 { - panic("MovingAverage period must be positive") + return nil, fmt.Errorf("MovingAverage period specified as %v but it must be positive", period) } return &MovingAverage[T]{ - period: period, buffer: make([]T, 0, period), - } + }, nil } func (a *MovingAverage[T]) Update(value T) { - if a.period <= 0 { + period := cap(a.buffer) + if period == 0 { return } - if len(a.buffer) < a.period { + if len(a.buffer) < period { a.buffer = append(a.buffer, value) a.sum += value } else { a.sum += value a.sum -= a.buffer[a.bufferPosition] a.buffer[a.bufferPosition] = value - a.bufferPosition = (a.bufferPosition + 1) % a.period + a.bufferPosition = (a.bufferPosition + 1) % period } } diff --git a/util/arbmath/moving_average_test.go b/util/arbmath/moving_average_test.go index 249080717c..9013d3f487 100644 --- a/util/arbmath/moving_average_test.go +++ b/util/arbmath/moving_average_test.go @@ -6,30 +6,42 @@ package arbmath import "testing" func TestMovingAverage(t *testing.T) { - ma := NewMovingAverage[int](5) + _, err := NewMovingAverage[int](0) + if err == nil { + t.Error("Expected error when creating moving average of period 0") + } + _, err = NewMovingAverage[int](-1) + if err == nil { + t.Error("Expected error when creating moving average of period -1") + } + + ma, err := NewMovingAverage[int](5) + if err != nil { + t.Fatalf("Error creating moving average of period 5: %v", err) + } if ma.Average() != 0 { - t.Errorf("moving average should be 0 at start, got %v", ma.Average()) + t.Errorf("Average() = %v, want 0", ma.Average()) } ma.Update(2) if ma.Average() != 2 { - t.Errorf("moving average should be 2, got %v", ma.Average()) + t.Errorf("Average() = %v, want 2", ma.Average()) } ma.Update(4) if ma.Average() != 3 { - t.Errorf("moving average should be 3, got %v", ma.Average()) + t.Errorf("Average() = %v, want 3", ma.Average()) } for i := 0; i < 5; i++ { ma.Update(10) } if ma.Average() != 10 { - t.Errorf("moving average should be 10, got %v", ma.Average()) + t.Errorf("Average() = %v, want 10", ma.Average()) } for i := 0; i < 5; i++ { ma.Update(0) } if ma.Average() != 0 { - t.Errorf("moving average should be 0, got %v", ma.Average()) + t.Errorf("Average() = %v, want 0", ma.Average()) } }