Skip to content

Commit

Permalink
op-batcher,op-e2e: replace magic numbers like 6 with consts, eg MaxBl…
Browse files Browse the repository at this point in the history
…obsPerBlobTx (#11842)

* remove some magic numbers

* also check TargetNumFrames <= 6 for auto DA

* define eth.MaxBlobsPerBlobTx and replace magic 6

* also change the original params.MaxBlobGasPerBlock/params.BlobTxBlobGasPerBlob
  • Loading branch information
zhiqiangxu authored Sep 26, 2024
1 parent 4a608f6 commit d42fc0b
Show file tree
Hide file tree
Showing 7 changed files with 31 additions and 23 deletions.
2 changes: 1 addition & 1 deletion op-batcher/batcher/channel_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -160,7 +160,7 @@ func TestChannel_NextTxData_singleFrameTx(t *testing.T) {

func TestChannel_NextTxData_multiFrameTx(t *testing.T) {
require := require.New(t)
const n = 6
const n = eth.MaxBlobsPerBlobTx
lgr := testlog.Logger(t, log.LevelWarn)
ch, err := newChannel(lgr, metrics.NoopMetrics, ChannelConfig{
UseBlobs: true,
Expand Down
10 changes: 6 additions & 4 deletions op-batcher/batcher/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import (
"github.com/ethereum-optimism/optimism/op-batcher/compressor"
"github.com/ethereum-optimism/optimism/op-batcher/flags"
"github.com/ethereum-optimism/optimism/op-node/rollup/derive"
"github.com/ethereum-optimism/optimism/op-service/eth"
oplog "github.com/ethereum-optimism/optimism/op-service/log"
opmetrics "github.com/ethereum-optimism/optimism/op-service/metrics"
"github.com/ethereum-optimism/optimism/op-service/oppprof"
Expand Down Expand Up @@ -135,18 +136,19 @@ func (c *CLIConfig) Check() error {
if !derive.ValidCompressionAlgo(c.CompressionAlgo) {
return fmt.Errorf("invalid compression algo %v", c.CompressionAlgo)
}
if c.BatchType > 1 {
if c.BatchType > derive.SpanBatchType {
return fmt.Errorf("unknown batch type: %v", c.BatchType)
}
if c.CheckRecentTxsDepth > 128 {
return fmt.Errorf("CheckRecentTxsDepth cannot be set higher than 128: %v", c.CheckRecentTxsDepth)
}
if c.DataAvailabilityType == flags.BlobsType && c.TargetNumFrames > 6 {
return errors.New("too many frames for blob transactions, max 6")
}
if !flags.ValidDataAvailabilityType(c.DataAvailabilityType) {
return fmt.Errorf("unknown data availability type: %q", c.DataAvailabilityType)
}
// we want to enforce it for both blobs and auto
if c.DataAvailabilityType != flags.CalldataType && c.TargetNumFrames > eth.MaxBlobsPerBlobTx {
return fmt.Errorf("too many frames for blob transactions, max %d", eth.MaxBlobsPerBlobTx)
}
if err := c.MetricsConfig.Check(); err != nil {
return err
}
Expand Down
8 changes: 5 additions & 3 deletions op-batcher/batcher/config_test.go
Original file line number Diff line number Diff line change
@@ -1,13 +1,15 @@
package batcher_test

import (
"fmt"
"testing"
"time"

"github.com/ethereum-optimism/optimism/op-batcher/batcher"
"github.com/ethereum-optimism/optimism/op-batcher/compressor"
"github.com/ethereum-optimism/optimism/op-batcher/flags"
"github.com/ethereum-optimism/optimism/op-node/rollup/derive"
"github.com/ethereum-optimism/optimism/op-service/eth"
"github.com/ethereum-optimism/optimism/op-service/log"
"github.com/ethereum-optimism/optimism/op-service/metrics"
"github.com/ethereum-optimism/optimism/op-service/oppprof"
Expand Down Expand Up @@ -98,12 +100,12 @@ func TestBatcherConfig(t *testing.T) {
errString: "TargetNumFrames must be at least 1",
},
{
name: "larger 6 TargetNumFrames for blobs",
name: fmt.Sprintf("larger %d TargetNumFrames for blobs", eth.MaxBlobsPerBlobTx),
override: func(c *batcher.CLIConfig) {
c.TargetNumFrames = 7
c.TargetNumFrames = eth.MaxBlobsPerBlobTx + 1
c.DataAvailabilityType = flags.BlobsType
},
errString: "too many frames for blob transactions, max 6",
errString: fmt.Sprintf("too many frames for blob transactions, max %d", eth.MaxBlobsPerBlobTx),
},
{
name: "invalid compr ratio for ratio compressor",
Expand Down
5 changes: 3 additions & 2 deletions op-e2e/actions/batcher/eip4844_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import (
batcherFlags "github.com/ethereum-optimism/optimism/op-batcher/flags"
"github.com/ethereum-optimism/optimism/op-e2e/e2eutils"
"github.com/ethereum-optimism/optimism/op-node/rollup/sync"
"github.com/ethereum-optimism/optimism/op-service/eth"
"github.com/ethereum-optimism/optimism/op-service/testlog"
)

Expand Down Expand Up @@ -104,10 +105,10 @@ func TestEIP4844MultiBlobs(gt *testing.T) {
sequencer.ActBuildToL1Head(t)

// submit all new L2 blocks
batcher.ActSubmitAllMultiBlobs(t, 6)
batcher.ActSubmitAllMultiBlobs(t, eth.MaxBlobsPerBlobTx)
batchTx := batcher.LastSubmitted
require.Equal(t, uint8(types.BlobTxType), batchTx.Type(), "batch tx must be blob-tx")
require.Len(t, batchTx.BlobTxSidecar().Blobs, 6)
require.Len(t, batchTx.BlobTxSidecar().Blobs, eth.MaxBlobsPerBlobTx)

// new L1 block with L2 batch
miner.ActL1StartBlock(12)(t)
Expand Down
4 changes: 2 additions & 2 deletions op-e2e/actions/helpers/l2_batcher.go
Original file line number Diff line number Diff line change
Expand Up @@ -346,8 +346,8 @@ func (s *L2Batcher) ActL2BatchSubmitMultiBlob(t Testing, numBlobs int) {
if s.l2BatcherCfg.DataAvailabilityType != batcherFlags.BlobsType {
t.InvalidAction("ActL2BatchSubmitMultiBlob only available for Blobs DA type")
return
} else if numBlobs > 6 || numBlobs < 1 {
t.InvalidAction("invalid number of blobs %d, must be within [1,6]", numBlobs)
} else if numBlobs > eth.MaxBlobsPerBlobTx || numBlobs < 1 {
t.InvalidAction("invalid number of blobs %d, must be within [1,%d]", numBlobs, eth.MaxBlobsPerBlobTx)
}

// Don't run this action if there's no data to submit
Expand Down
13 changes: 7 additions & 6 deletions op-e2e/system/da/eip4844_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package da

import (
"context"
"fmt"
"math/big"
"math/rand"
"testing"
Expand Down Expand Up @@ -50,12 +51,12 @@ func testSystem4844E2E(t *testing.T, multiBlob bool, daType batcherFlags.DataAva
cfg.BatcherBatchType = derive.SpanBatchType
cfg.DeployConfig.L1GenesisBlockBaseFeePerGas = (*hexutil.Big)(big.NewInt(7000))

const maxBlobs = 6
const maxBlobs = eth.MaxBlobsPerBlobTx
var maxL1TxSize int
if multiBlob {
cfg.BatcherTargetNumFrames = 6
cfg.BatcherTargetNumFrames = eth.MaxBlobsPerBlobTx
cfg.BatcherUseMaxTxSizeForBlobs = true
// leads to 6 blobs for an L2 block with a user tx with 400 random bytes
// leads to eth.MaxBlobsPerBlobTx blobs for an L2 block with a user tx with 400 random bytes
// while all other L2 blocks take 1 blob (deposit tx)
maxL1TxSize = derive.FrameV0OverHeadSize + 100
cfg.BatcherMaxL1TxSizeBytes = uint64(maxL1TxSize)
Expand Down Expand Up @@ -129,7 +130,7 @@ func testSystem4844E2E(t *testing.T, multiBlob bool, daType batcherFlags.DataAva
opts.Value = big.NewInt(1_000_000_000)
opts.Nonce = 1 // Already have deposit
opts.ToAddr = &common.Address{0xff, 0xff}
// put some random data in the tx to make it fill up 6 blobs (multi-blob case)
// put some random data in the tx to make it fill up eth.MaxBlobsPerBlobTx blobs (multi-blob case)
opts.Data = testutils.RandomData(rand.New(rand.NewSource(420)), 400)
opts.Gas, err = core.IntrinsicGas(opts.Data, nil, false, true, true, false)
require.NoError(t, err)
Expand Down Expand Up @@ -207,7 +208,7 @@ func testSystem4844E2E(t *testing.T, multiBlob bool, daType batcherFlags.DataAva
if !multiBlob {
require.NotZero(t, numBlobs, "single-blob: expected to find L1 blob tx")
} else {
require.Equal(t, maxBlobs, numBlobs, "multi-blob: expected to find L1 blob tx with 6 blobs")
require.Equal(t, maxBlobs, numBlobs, fmt.Sprintf("multi-blob: expected to find L1 blob tx with %d blobs", eth.MaxBlobsPerBlobTx))
// blob tx should have filled up all but last blob
bcl := sys.L1BeaconHTTPClient()
hashes := toIndexedBlobHashes(blobTx.BlobHashes()...)
Expand Down Expand Up @@ -255,7 +256,7 @@ func TestBatcherAutoDA(t *testing.T) {
cfg.DeployConfig.L1GenesisBlockGasLimit = 2_500_000 // low block gas limit to drive up gas price more quickly
t.Logf("L1BlockTime: %d, L2BlockTime: %d", cfg.DeployConfig.L1BlockTime, cfg.DeployConfig.L2BlockTime)

cfg.BatcherTargetNumFrames = 6
cfg.BatcherTargetNumFrames = eth.MaxBlobsPerBlobTx

sys, err := cfg.Start(t)
require.NoError(t, err, "Error starting up system")
Expand Down
12 changes: 7 additions & 5 deletions op-service/eth/blob.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,14 +9,16 @@ import (
"github.com/ethereum/go-ethereum/common"
"github.com/ethereum/go-ethereum/common/hexutil"
"github.com/ethereum/go-ethereum/crypto/kzg4844"
"github.com/ethereum/go-ethereum/params"
)

const (
BlobSize = 4096 * 32
MaxBlobDataSize = (4*31+3)*1024 - 4
EncodingVersion = 0
VersionOffset = 1 // offset of the version byte in the blob encoding
Rounds = 1024 // number of encode/decode rounds
BlobSize = 4096 * 32
MaxBlobDataSize = (4*31+3)*1024 - 4
EncodingVersion = 0
VersionOffset = 1 // offset of the version byte in the blob encoding
Rounds = 1024 // number of encode/decode rounds
MaxBlobsPerBlobTx = params.MaxBlobGasPerBlock / params.BlobTxBlobGasPerBlob
)

var (
Expand Down

0 comments on commit d42fc0b

Please sign in to comment.