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.

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

Implement Express Lane Timeboost #2561

Open
wants to merge 110 commits into
base: master
Choose a base branch
from
Open

Conversation

rauljordan
Copy link
Contributor

@rauljordan rauljordan commented Aug 8, 2024

Background

At the time of writing, the Arbitrum sequencer is centralized and offers a first-come, first-serve transaction ordering policy. Txs have a current delay of approximately 250ms, which is the time the sequencer takes to produce an ordered list of txs to emit in the form of an L2 block. The current policy does not handle MEV that occurs naturally on L2, and leads to latency races offline to get faster access to the sequencer ingress server.

A new policy has been proposed, known as Express Lane Timeboost, which allows participants to bid for the rights of priority sequencing using their funds instead of hardware. In “rounds” that start at each minute mark, participants can submits bids to participate in a sealed, second-price auction for control of the next round’s “express lane”. During a round, all non-express lane txs get their first arrival timestamp delayed by some amount of time (250ms), while the express lane controller does not. The express lane controller can also choose to transfer their rights in a round.

The sequencer itself does not need to manage auctions, but simply needs to know the current round number and the address of the express lane controller for that round. From there, it can delay non-express lane txs by a nominal amount required by the protocol and validate that a tx should go through the express lane.

This PR contains the complete implementation of the system with all its components. The smart contract changes are contained within OffchainLabs/nitro-contracts/tree/express-lane-auction-all-merged.

Basic Readings

To read more about timeboost, see the AIP, the research specification, and design doc although the design doc is not fully updated yet.

Reviewing

Recommend to look at the basic readings, then look at system_tests/timeboost_test.go to understand how it all fits together. Then, look at bid validator and auctioneer. Finally, the sequencer changes.

Features

  • Bidder client that allows participants to join the auction and submit bids to a bid validator
  • Bid validator that receives bids over the internet, validates them, and inserts validated items into Redis stream
  • Auctioneer server that consumes validated bids from Redis stream.
  • Auctioneer at the 45 second mark, submits the top two bids to a privileged sequencer endpoint
  • Ability to persist validated bids to a local DB (sqlite) in the auctioneer server
  • System tests are added that assert express lane txs have an advantage in the emitted sequencer feed

Sequencer Changes

The changes to the sequencer hot path are quite simple. In a nutshell, if a transaction is received, it checks the following:
If timeboost is enabled AND there is an express lane controller set AND it is not coming from the express lane, it delays the tx's first arrival timestamp by some amount (250ms).

To determine if a transaction is a valid express lane tx, the sequencer runs a background thread called the expressLaneService, which is scraping events from the ExpressLaneAuction.sol smart contract. Express lane transactions arrive via a different sequencer endpoint than the normal one, called timeboost_sendExpressLaneTransaction. The message looks as follows:

{
  "type": "object",
  "properties": {
    "chainId": {
      "type": "bigInt",
      "description": "chain id of the target chain"
    },
    "round": {
      "type": "uint64",
      "description": "round number (0-indexed) for the round the bidder wants to become the controller of"
    },
    "auctionContractAddress": {
      "type": "address",
      "description": "hex string of the auction contract address that the bid corresponds to"
    },
    "sequenceNumber": {
      "type": "uint64",
      "description": "the per-round nonce of express lane submissions. Each submission to the express lane during a round increases this sequence number by one, and if submissions are received out of order, the sequencer will queue them for processing in order. This is reset to 0 at each round"
    },
    "transaction": {
      "type": "bytes",
      "description": "hex string of the RLP encoded transaction payload that submitter wishes to be sequenced through the express lane"
    },
    "options": {
      "type": "ArbitrumConditionalOptions",
      "description": "conditional options for Arbitrum transactions, supported by normal sequencer endpoint https://github.com/OffchainLabs/go-ethereum/blob/48de2030c7a6fa8689bc0a0212ebca2a0c73e3ad/arbitrum_types/txoptions.go#L71"
    },
    "signature": {
      "type": "bytes",
      "description": "Ethereum signature over the bytes encoding of (keccak256(TIMEBOOST_BID), padTo32Bytes(chainId), auctionContractAddress, uint64ToBytes(round), uint64ToBytes(sequenceNumber), transaction)"
    }
  },
}

The submission itself contains a tx payload, which MAY not be from the express lane controller. As long as the submission is signed by the controller, that is sufficient. Submissions have a specific nonce, called a sequence, to ensure that submissions are processed in order. This is different from the inner nonce of the payload tx. The sequencer keeps a queue of submissions and ensures it processes them in order. That is, if a submission N is received before N-1, it will get queued for submission once N arrives.

Bid Validator Architecture

Bids are limited to 5 bids per sender, but there are no limits to the number of bidders in a single round. To alleviate potential scaling concerns, we adopt a simple architecture of separating the bid validators from the auctioneer. The bid validators filter out invalid items and publish validated results to a Redis stream. In a simplified diagram, here's what it will look like:

Screenshot 2024-08-08 at 11 45 55

Dependencies Added

  • github.com/golang-jwt/jwt/v4 for the authenticated endpoint from the auctioneer to the sequencer
  • github.com/stretchr/testify for testing utilities (will probably have to remove)
  • github.com/mattn/go-sqlite3 for the bids DB
  • github.com/jmoiron/sqlx for the bids DB
  • github.com/DATA-DOG/go-sqlmock for testing the bids DB

Notes

There are several parts of this implementation that are likely not ideal:

Chicken and the egg problem in sequencer
Cannot start sequencer without express lane, but cannot deploy auction for express lane without starting sequencer. To solve this in tests, we have a separate func called StartExpressLaneService in the sequencer. In prod, we don’t have this issue because we can deploy the contracts before we upgrade the sequencer to timeboost, but what to do about tests?

Janky prioritizing of auction resolution txs
The sequencer exposes an authenticated endpoint auctioneer_submitAuctionResolutionTransaction over the JWT Auth RPC for the auctioneer to use. When the auctioneer is ready to resolve an auction, it submits a tx to this endpoint, which the sequencer verifies for integrity. Then, the sequencer does the following:

log.Info("Prioritizing auction resolution transaction from auctioneer", "txHash", tx.Hash().Hex())
s.timeboostAuctionResolutionTx = tx
s.createBlock(ctx)

it immediately tries to put the item in the queue and create block. It also sets the tx as a property of the sequencer struct, and in the createBlock func, if this field is not nil, it gets put at the top of the queue. This is a bit janky in how it works and perhaps inefficient. Is there another way to prioritize a tx in the sequencer?

Sequencer opens an http connection to itself
The sequencer has a thread called expressLaneService which reads events from the auction smart contracts on L2 to determine express lane controllers. Because the sequencer does not have filtersystem API access, we instead open an RPC client against itself so we can create an ethclient to read logs and data from onchain. This doesn't seem ideal

References

@rauljordan
Copy link
Contributor Author

Update: implemented the functionality of using a logs subscription from the blockchain struct in the sequencer instead of an ethclient to read information about the express lane auction contract here https://github.com/OffchainLabs/nitro/compare/express-lane-timeboost...sub-logs-express-lane-timeboost?expand=1.
However, the logs are not received over the channel for some reason when running the system test under timeboost_test.go

Comment on lines +133 to +135
func (c *AutonomousAuctioneerConfig) Validate() error {
return nil
}
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the intention for this? Is it a todo or should it be removed?

Round uint64
AuctionContractAddress common.Address
Transaction *types.Transaction
Options *arbitrum_types.ConditionalOptions `json:"options"`
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the use case for having options?

select {
case <-ctx.Done():
return
case <-time.After(time.Millisecond * 250):
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if time.After(time.Millisecond * 250) is sufficient and whether it’s better to stream new blocks directly from the tx feed. Otherwise, there could be a race condition where, at 249.9ms, we check and find that the latest block number hasn’t changed, forcing us to wait another 250ms.

Comment on lines +210 to +228
for {
// Get the next message in the sequence.
nextMsg, exists := es.messagesBySequenceNumber[control.sequence]
if !exists {
break
}
if err := publishTxFn(
ctx,
nextMsg.Transaction,
msg.Options,
false, /* 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.Sequence)
return err
}
// Increase the global round sequence number.
control.sequence += 1
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor because we break, but we might want to check for context timeout here, and the caller of sequenceExpressLaneSubmission should set a deadline for when the round ends.

Copy link
Member

@Tristan-Wilson Tristan-Wilson left a comment

Choose a reason for hiding this comment

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

Submitting comments so far

es.LaunchThread(func(ctx context.Context) {
log.Info("Watching for new express lane rounds")
now := time.Now()
waitTime := es.roundDuration - time.Duration(now.Second())*time.Second - time.Duration(now.Nanosecond())
Copy link
Member

Choose a reason for hiding this comment

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

Round duration is configurable, but here and other places we're building in two assumptions:

  • round duration is a minute
  • that rounds start exactly on the minute mark.

Maybe it's fine for initial launch with the minute length rounds, but we should acknowledge all the places where we're making these assumptions and would need to change if the round length was to change.

)
es.Lock()
// Reset the sequence numbers map for the new round.
es.messagesBySequenceNumber = make(map[uint64]*timeboost.ExpressLaneSubmission)
Copy link
Member

Choose a reason for hiding this comment

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

Are there any meaningful races between the CurrentRound and messagesBySequenceNumber being reset when the timer ticks, which will necessarily lag CurrentRound? The worst I'm seeing is potentially lost messages when they are submitted at the changeover time.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ya there is a race here. I think it is safer to use two queues and rotate

execution/gethexec/express_lane_service.go Show resolved Hide resolved
Tristan's feedback
Copy link
Member

@Tristan-Wilson Tristan-Wilson left a comment

Choose a reason for hiding this comment

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

I've finished reviewing most of the code except the tests and will go through the tests tomorrow.

options: nil,
resultChan: make(chan error, 1),
returnedResult: &atomic.Bool{},
ctx: context.TODO(),
Copy link
Member

Choose a reason for hiding this comment

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

TODO context

timeboost/bid_validator.go Outdated Show resolved Hide resolved
timeboost/bid_validator.go Outdated Show resolved Hide resolved
execution/gethexec/sequencer.go Outdated Show resolved Hide resolved
if err != nil {
return err
}
err = PreCheckTx(c.bc, c.bc.Config(), block, statedb, arbos, msg.Transaction, msg.Options, c.config())
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it makes more sense to validateExpressLaneTx and then PreCheckTx. It's a cheaper dos protection but I can see how it would be hard to refactor

execution/gethexec/sequencer.go Outdated Show resolved Hide resolved
execution/gethexec/express_lane_service.go Show resolved Hide resolved
)
es.Lock()
// Reset the sequence numbers map for the new round.
es.messagesBySequenceNumber = make(map[uint64]*timeboost.ExpressLaneSubmission)
Copy link
Contributor

Choose a reason for hiding this comment

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

Ya there is a race here. I think it is safer to use two queues and rotate

execution/gethexec/express_lane_service.go Outdated Show resolved Hide resolved
execution/gethexec/express_lane_service_test.go Outdated Show resolved Hide resolved
execution/gethexec/express_lane_service_test.go Outdated Show resolved Hide resolved
execution/gethexec/express_lane_service.go Outdated Show resolved Hide resolved
Comment on lines +62 to +64
initialTimestamp := time.Unix(int64(roundTimingInfo.OffsetTimestamp), 0)
roundDuration := time.Duration(roundTimingInfo.RoundDurationSeconds) * time.Second
auctionClosingDuration := time.Duration(roundTimingInfo.AuctionClosingSeconds) * time.Second
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we can add an assertion here that the roundTimingInfo complies with the assumptions we've got baked into the code currently (eg duration == 1 minute, offset is a time at a minute boundary, closing duration is at least 2s but probably just assert it's 15s)

Copy link
Contributor

Choose a reason for hiding this comment

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

roundTimingInfo is retrieved from the smart contract, which is the source of truth we should adhere to. Unless we want to add another configuration to check against this single source of truth, I generally prefer less configuration, as it's less error-prone, but I'm open to changing if that's what we prefer!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed that we should rely on the contract here as the source of truth

Copy link
Member

Choose a reason for hiding this comment

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

My point is that the code is broken if it's anything other than 1 minute. The contract is the source of truth so we should check that the settings on the contract matches the assumptions we're currently making in the code, and assert if they are violated because otherwise the code will behave in unexpected ways.

ctx, cancel := context.WithCancel(context.Background())
defer cancel()
redisURL := redisutil.CreateTestRedis(ctx, t)
_ = redisURL
Copy link
Member

Choose a reason for hiding this comment

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

This test needs to be filled out.

system_tests/timeboost_test.go Outdated Show resolved Hide resolved
timeboost/bidder_client.go Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
s Automatically added by the CLA bot if the creator of a PR is registered as having signed the CLA.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants