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

Merged
merged 11 commits into from
Nov 21, 2024
Merged

Fix minor comments from Timeboost PR #2787

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
11 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
066ff17
Merge remote-tracking branch 'origin/express-lane-timeboost' into exp…
Tristan-Wilson Nov 20, 2024
dc65eef
Merge remote-tracking branch 'origin/expres-lane-timeboost-fix-pr-com…
Tristan-Wilson Nov 20, 2024
969dc1d
Refactor stubPublisher
Tristan-Wilson Nov 20, 2024
5a02894
Change transactionPublisher interface
Tristan-Wilson Nov 20, 2024
cc75bf2
Remove uncessary locking
Tristan-Wilson Nov 21, 2024
3b67813
Merge branch 'express-lane-timeboost' into expres-lane-timeboost-fix-…
Tristan-Wilson Nov 21, 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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • an interface with private function can get weird if we want to move code around.
  • an interface should make sense as itself and func-names should self-document
  • I think you only ever pass "true" in that bool, is that right?
    Mybe have something like "PublishFastLaneTransaction" or similar that doesn't accept the bool and is trivially implemented in the sequencer or a wrapper?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Refactored it along those lines.

}

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 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not critical:
seems easier to have the stubPublisher hold array of txs and have a publishTransaction function that appends to it. Some tests could use the array, some could use just it's length and some could ignore it completley.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Refactored it along those lines.

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