From d42fc0b9dc580616d89f0fc420fccde604a73233 Mon Sep 17 00:00:00 2001 From: zhiqiangxu <652732310@qq.com> Date: Thu, 26 Sep 2024 19:50:27 +0800 Subject: [PATCH] op-batcher,op-e2e: replace magic numbers like 6 with consts, eg MaxBlobsPerBlobTx (#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 --- op-batcher/batcher/channel_test.go | 2 +- op-batcher/batcher/config.go | 10 ++++++---- op-batcher/batcher/config_test.go | 8 +++++--- op-e2e/actions/batcher/eip4844_test.go | 5 +++-- op-e2e/actions/helpers/l2_batcher.go | 4 ++-- op-e2e/system/da/eip4844_test.go | 13 +++++++------ op-service/eth/blob.go | 12 +++++++----- 7 files changed, 31 insertions(+), 23 deletions(-) diff --git a/op-batcher/batcher/channel_test.go b/op-batcher/batcher/channel_test.go index 8dec9d9e108b..3585ea8b99f6 100644 --- a/op-batcher/batcher/channel_test.go +++ b/op-batcher/batcher/channel_test.go @@ -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, diff --git a/op-batcher/batcher/config.go b/op-batcher/batcher/config.go index 250d893e2a71..ac8bad7791a7 100644 --- a/op-batcher/batcher/config.go +++ b/op-batcher/batcher/config.go @@ -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" @@ -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 } diff --git a/op-batcher/batcher/config_test.go b/op-batcher/batcher/config_test.go index f8fb08a703da..4b90ebaccb68 100644 --- a/op-batcher/batcher/config_test.go +++ b/op-batcher/batcher/config_test.go @@ -1,6 +1,7 @@ package batcher_test import ( + "fmt" "testing" "time" @@ -8,6 +9,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" "github.com/ethereum-optimism/optimism/op-service/log" "github.com/ethereum-optimism/optimism/op-service/metrics" "github.com/ethereum-optimism/optimism/op-service/oppprof" @@ -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", diff --git a/op-e2e/actions/batcher/eip4844_test.go b/op-e2e/actions/batcher/eip4844_test.go index 6d77a3961788..1447a07a2076 100644 --- a/op-e2e/actions/batcher/eip4844_test.go +++ b/op-e2e/actions/batcher/eip4844_test.go @@ -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" ) @@ -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) diff --git a/op-e2e/actions/helpers/l2_batcher.go b/op-e2e/actions/helpers/l2_batcher.go index d9e6fe3dbec5..352774a9968f 100644 --- a/op-e2e/actions/helpers/l2_batcher.go +++ b/op-e2e/actions/helpers/l2_batcher.go @@ -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 diff --git a/op-e2e/system/da/eip4844_test.go b/op-e2e/system/da/eip4844_test.go index 332da11f9d6f..27cc1db0d21d 100644 --- a/op-e2e/system/da/eip4844_test.go +++ b/op-e2e/system/da/eip4844_test.go @@ -2,6 +2,7 @@ package da import ( "context" + "fmt" "math/big" "math/rand" "testing" @@ -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) @@ -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) @@ -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()...) @@ -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") diff --git a/op-service/eth/blob.go b/op-service/eth/blob.go index 9e51c568634f..b7cf4524a48a 100644 --- a/op-service/eth/blob.go +++ b/op-service/eth/blob.go @@ -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 (