Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fix minor comments from Timeboost PR #2787

Open
wants to merge 5 commits into
base: express-lane-timeboost-input-validation
Choose a base branch
from
Open

Fix minor comments from Timeboost PR #2787

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
b7d1b06
Rename delay param to isExpressLaneController
Tristan-Wilson Nov 12, 2024
436f831
Use ptr to sequencer instead of member fn for ES
Tristan-Wilson Nov 12, 2024
b991d76
Change log level for future EL sequence number
Tristan-Wilson Nov 12, 2024
f5ca4bf
Use interface instead of ptr to seq for tests
Tristan-Wilson Nov 13, 2024
23869a2
Fix reversed boolean condition
Tristan-Wilson Nov 15, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 10 additions & 9 deletions execution/gethexec/express_lane_service.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,9 +31,14 @@ type expressLaneControl struct {
controller common.Address
}

type transactionPublisher interface {
publishTransactionImpl(context.Context, *types.Transaction, *arbitrum_types.ConditionalOptions, bool) error
}

type expressLaneService struct {
stopwaiter.StopWaiter
sync.RWMutex
transactionPublisher transactionPublisher
auctionContractAddr common.Address
initialTimestamp time.Time
roundDuration time.Duration
Expand All @@ -47,6 +52,7 @@ type expressLaneService struct {
}

func newExpressLaneService(
transactionPublisher transactionPublisher,
auctionContractAddr common.Address,
sequencerClient *ethclient.Client,
bc *core.BlockChain,
Expand Down Expand Up @@ -79,6 +85,7 @@ pending:
roundDuration := arbmath.SaturatingCast[time.Duration](roundTimingInfo.RoundDurationSeconds) * time.Second
auctionClosingDuration := arbmath.SaturatingCast[time.Duration](roundTimingInfo.AuctionClosingSeconds) * time.Second
return &expressLaneService{
transactionPublisher: transactionPublisher,
auctionContract: auctionContract,
chainConfig: chainConfig,
initialTimestamp: initialTimestamp,
Expand Down Expand Up @@ -231,12 +238,6 @@ func (es *expressLaneService) isWithinAuctionCloseWindow(arrivalTime time.Time)
func (es *expressLaneService) sequenceExpressLaneSubmission(
ctx context.Context,
msg *timeboost.ExpressLaneSubmission,
publishTxFn func(
parentCtx context.Context,
tx *types.Transaction,
options *arbitrum_types.ConditionalOptions,
delay bool,
) error,
) error {
es.Lock()
defer es.Unlock()
Expand All @@ -254,7 +255,7 @@ func (es *expressLaneService) sequenceExpressLaneSubmission(
}
// Log an informational warning if the message's sequence number is in the future.
if msg.SequenceNumber > control.sequence {
log.Warn("Received express lane submission with future sequence number", "SequenceNumber", msg.SequenceNumber)
log.Info("Received express lane submission with future sequence number", "SequenceNumber", msg.SequenceNumber)
}
// Put into the sequence number map.
es.messagesBySequenceNumber[msg.SequenceNumber] = msg
Expand All @@ -265,11 +266,11 @@ func (es *expressLaneService) sequenceExpressLaneSubmission(
if !exists {
break
}
if err := publishTxFn(
if err := es.transactionPublisher.publishTransactionImpl(
ctx,
nextMsg.Transaction,
msg.Options,
false, /* no delay, as it should go through express lane */
true, /* no delay, as it should go through express lane */
); err != nil {
// If the tx failed, clear it from the sequence map.
delete(es.messagesBySequenceNumber, msg.SequenceNumber)
Expand Down
60 changes: 36 additions & 24 deletions execution/gethexec/express_lane_service_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -231,10 +231,21 @@ func Test_expressLaneService_validateExpressLaneTx(t *testing.T) {
}
}

type stubPublisher struct {
publishFn func(parentCtx context.Context, tx *types.Transaction, options *arbitrum_types.ConditionalOptions, delay bool) error
}

func (s *stubPublisher) publishTransactionImpl(parentCtx context.Context, tx *types.Transaction, options *arbitrum_types.ConditionalOptions, isExpressLaneController bool) error {
return s.publishFn(parentCtx, tx, options, isExpressLaneController)
}

func Test_expressLaneService_sequenceExpressLaneSubmission_nonceTooLow(t *testing.T) {
ctx, cancel := context.WithCancel(context.Background())
defer cancel()
els := &expressLaneService{
transactionPublisher: &stubPublisher{func(parentCtx context.Context, tx *types.Transaction, options *arbitrum_types.ConditionalOptions, delay bool) error {
return nil
}},
messagesBySequenceNumber: make(map[uint64]*timeboost.ExpressLaneSubmission),
roundControl: lru.NewCache[uint64, *expressLaneControl](8),
}
Expand All @@ -244,17 +255,20 @@ func Test_expressLaneService_sequenceExpressLaneSubmission_nonceTooLow(t *testin
msg := &timeboost.ExpressLaneSubmission{
SequenceNumber: 0,
}
publishFn := func(parentCtx context.Context, tx *types.Transaction, options *arbitrum_types.ConditionalOptions, delay bool) error {
return nil
}
err := els.sequenceExpressLaneSubmission(ctx, msg, publishFn)

err := els.sequenceExpressLaneSubmission(ctx, msg)
require.ErrorIs(t, err, timeboost.ErrSequenceNumberTooLow)
}

func Test_expressLaneService_sequenceExpressLaneSubmission_duplicateNonce(t *testing.T) {
ctx, cancel := context.WithCancel(context.Background())
defer cancel()
numPublished := 0
els := &expressLaneService{
transactionPublisher: &stubPublisher{func(parentCtx context.Context, tx *types.Transaction, options *arbitrum_types.ConditionalOptions, delay bool) error {
numPublished += 1
return nil
}},
roundControl: lru.NewCache[uint64, *expressLaneControl](8),
messagesBySequenceNumber: make(map[uint64]*timeboost.ExpressLaneSubmission),
}
Expand All @@ -264,39 +278,36 @@ func Test_expressLaneService_sequenceExpressLaneSubmission_duplicateNonce(t *tes
msg := &timeboost.ExpressLaneSubmission{
SequenceNumber: 2,
}
numPublished := 0
publishFn := func(parentCtx context.Context, tx *types.Transaction, options *arbitrum_types.ConditionalOptions, delay bool) error {
numPublished += 1
return nil
}
err := els.sequenceExpressLaneSubmission(ctx, msg, publishFn)
err := els.sequenceExpressLaneSubmission(ctx, msg)
require.NoError(t, err)
// Because the message is for a future sequence number, it
// should get queued, but not yet published.
require.Equal(t, 0, numPublished)
// Sending it again should give us an error.
err = els.sequenceExpressLaneSubmission(ctx, msg, publishFn)
err = els.sequenceExpressLaneSubmission(ctx, msg)
require.ErrorIs(t, err, timeboost.ErrDuplicateSequenceNumber)
}

func Test_expressLaneService_sequenceExpressLaneSubmission_outOfOrder(t *testing.T) {
ctx, cancel := context.WithCancel(context.Background())
defer cancel()
numPublished := 0
publishedTxOrder := make([]uint64, 0)
els := &expressLaneService{
roundControl: lru.NewCache[uint64, *expressLaneControl](8),
messagesBySequenceNumber: make(map[uint64]*timeboost.ExpressLaneSubmission),
}
els.roundControl.Add(0, &expressLaneControl{
sequence: 1,
})
numPublished := 0
publishedTxOrder := make([]uint64, 0)
control, _ := els.roundControl.Get(0)
publishFn := func(parentCtx context.Context, tx *types.Transaction, options *arbitrum_types.ConditionalOptions, delay bool) error {
els.transactionPublisher = &stubPublisher{func(parentCtx context.Context, tx *types.Transaction, options *arbitrum_types.ConditionalOptions, delay bool) error {
numPublished += 1
publishedTxOrder = append(publishedTxOrder, control.sequence)
return nil
}
}}

els.roundControl.Add(0, &expressLaneControl{
sequence: 1,
})

messages := []*timeboost.ExpressLaneSubmission{
{
SequenceNumber: 10,
Expand All @@ -315,14 +326,14 @@ func Test_expressLaneService_sequenceExpressLaneSubmission_outOfOrder(t *testing
},
}
for _, msg := range messages {
err := els.sequenceExpressLaneSubmission(ctx, msg, publishFn)
err := els.sequenceExpressLaneSubmission(ctx, msg)
require.NoError(t, err)
}
// We should have only published 2, as we are missing sequence number 3.
require.Equal(t, 2, numPublished)
require.Equal(t, len(messages), len(els.messagesBySequenceNumber))

err := els.sequenceExpressLaneSubmission(ctx, &timeboost.ExpressLaneSubmission{SequenceNumber: 3}, publishFn)
err := els.sequenceExpressLaneSubmission(ctx, &timeboost.ExpressLaneSubmission{SequenceNumber: 3})
require.NoError(t, err)
require.Equal(t, 5, numPublished)
}
Expand All @@ -340,14 +351,15 @@ func Test_expressLaneService_sequenceExpressLaneSubmission_erroredTx(t *testing.
numPublished := 0
publishedTxOrder := make([]uint64, 0)
control, _ := els.roundControl.Get(0)
publishFn := func(parentCtx context.Context, tx *types.Transaction, options *arbitrum_types.ConditionalOptions, delay bool) error {
els.transactionPublisher = &stubPublisher{func(parentCtx context.Context, tx *types.Transaction, options *arbitrum_types.ConditionalOptions, delay bool) error {
if tx == nil {
return errors.New("oops, bad tx")
}
numPublished += 1
publishedTxOrder = append(publishedTxOrder, control.sequence)
return nil
}
}}

messages := []*timeboost.ExpressLaneSubmission{
{
SequenceNumber: 1,
Expand All @@ -368,10 +380,10 @@ func Test_expressLaneService_sequenceExpressLaneSubmission_erroredTx(t *testing.
}
for _, msg := range messages {
if msg.Transaction == nil {
err := els.sequenceExpressLaneSubmission(ctx, msg, publishFn)
err := els.sequenceExpressLaneSubmission(ctx, msg)
require.ErrorContains(t, err, "oops, bad tx")
} else {
err := els.sequenceExpressLaneSubmission(ctx, msg, publishFn)
err := els.sequenceExpressLaneSubmission(ctx, msg)
require.NoError(t, err)
}
}
Expand Down
9 changes: 5 additions & 4 deletions execution/gethexec/sequencer.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -433,10 +433,10 @@ func ctxWithTimeout(ctx context.Context, timeout time.Duration) (context.Context
}

func (s *Sequencer) PublishTransaction(parentCtx context.Context, tx *types.Transaction, options *arbitrum_types.ConditionalOptions) error {
return s.publishTransactionImpl(parentCtx, tx, options, true /* delay tx if express lane is active */)
return s.publishTransactionImpl(parentCtx, tx, options, false /* delay tx if express lane is active */)
}

func (s *Sequencer) publishTransactionImpl(parentCtx context.Context, tx *types.Transaction, options *arbitrum_types.ConditionalOptions, delay bool) error {
func (s *Sequencer) publishTransactionImpl(parentCtx context.Context, tx *types.Transaction, options *arbitrum_types.ConditionalOptions, isExpressLaneController bool) error {
config := s.config()
// Only try to acquire Rlock and check for hard threshold if l1reader is not nil
// And hard threshold was enabled, this prevents spamming of read locks when not needed
Expand Down Expand Up @@ -482,7 +482,7 @@ func (s *Sequencer) publishTransactionImpl(parentCtx context.Context, tx *types.
}

if s.config().Timeboost.Enable && s.expressLaneService != nil {
if delay && s.expressLaneService.currentRoundHasController() {
if isExpressLaneController && s.expressLaneService.currentRoundHasController() {
Tristan-Wilson marked this conversation as resolved.
Show resolved Hide resolved
time.Sleep(s.config().Timeboost.ExpressLaneAdvantage)
}
}
Expand Down Expand Up @@ -536,7 +536,7 @@ func (s *Sequencer) PublishExpressLaneTransaction(ctx context.Context, msg *time
if err := s.expressLaneService.validateExpressLaneTx(msg); err != nil {
return err
}
return s.expressLaneService.sequenceExpressLaneSubmission(ctx, msg, s.publishTransactionImpl)
return s.expressLaneService.sequenceExpressLaneSubmission(ctx, msg)
}

func (s *Sequencer) PublishAuctionResolutionTransaction(ctx context.Context, tx *types.Transaction) error {
Expand Down Expand Up @@ -1253,6 +1253,7 @@ func (s *Sequencer) StartExpressLane(ctx context.Context, auctionContractAddr co
seqClient := ethclient.NewClient(rpcClient)

els, err := newExpressLaneService(
s,
auctionContractAddr,
seqClient,
s.execEngine.bc,
Expand Down
Loading