From 28496b0180df20a4460c792e25ff3f7484a5af19 Mon Sep 17 00:00:00 2001 From: Felix Lange Date: Tue, 4 Feb 2025 12:56:17 +0100 Subject: [PATCH] params: change blob count types to int {Latest,}MaxBlobsPerBlock returns a length, which is traditionally an int in Go. When comparing with blob gas (uint64) a conversion is required, but I think it's actually helpful. Kind of makes it clearer that you're working with different units. When comparing against a slice length, no conversion is needed. I'm also adding MaxBlobGasPerBlock to simplify the most common expression involving MaxBlobsPerBlock. --- cmd/evm/internal/t8ntool/execution.go | 3 +- consensus/misc/eip4844/eip4844.go | 11 +++--- consensus/misc/eip4844/eip4844_test.go | 6 ++-- core/txpool/blobpool/blobpool_test.go | 2 +- core/txpool/blobpool/limbo.go | 2 +- core/txpool/blobpool/slotter.go | 2 +- core/txpool/validation.go | 2 +- internal/ethapi/transaction_args.go | 6 ++-- miner/worker.go | 8 +++-- params/config.go | 49 +++++++++++++++++--------- tests/state_test_util.go | 15 ++++---- 11 files changed, 64 insertions(+), 42 deletions(-) diff --git a/cmd/evm/internal/t8ntool/execution.go b/cmd/evm/internal/t8ntool/execution.go index 38e6d11f98..05218bb460 100644 --- a/cmd/evm/internal/t8ntool/execution.go +++ b/cmd/evm/internal/t8ntool/execution.go @@ -242,7 +242,8 @@ func (pre *Prestate) Apply(vmConfig vm.Config, chainConfig *params.ChainConfig, txBlobGas := uint64(0) if tx.Type() == types.BlobTxType { txBlobGas = uint64(params.BlobTxBlobGasPerBlob * len(tx.BlobHashes())) - if used, max := blobGasUsed+txBlobGas, chainConfig.MaxBlobsPerBlock(pre.Env.Timestamp)*params.BlobTxBlobGasPerBlob; used > max { + max := chainConfig.MaxBlobGasPerBlock(pre.Env.Timestamp) + if used := blobGasUsed + txBlobGas; used > max { err := fmt.Errorf("blob gas (%d) would exceed maximum allowance %d", used, max) log.Warn("rejected tx", "index", i, "err", err) rejectedTxs = append(rejectedTxs, &rejectedTx{i, err.Error()}) diff --git a/consensus/misc/eip4844/eip4844.go b/consensus/misc/eip4844/eip4844.go index c6547349ae..b9b8230a80 100644 --- a/consensus/misc/eip4844/eip4844.go +++ b/consensus/misc/eip4844/eip4844.go @@ -42,8 +42,9 @@ func VerifyEIP4844Header(config *params.ChainConfig, parent, header *types.Heade return errors.New("header is missing blobGasUsed") } // Verify that the blob gas used remains within reasonable limits. - if max := config.MaxBlobsPerBlock(header.Time) * params.BlobTxBlobGasPerBlob; *header.BlobGasUsed > max { - return fmt.Errorf("blob gas used %d exceeds maximum allowance %d", *header.BlobGasUsed, max) + maxBlobGas := config.MaxBlobGasPerBlock(header.Time) + if *header.BlobGasUsed > maxBlobGas { + return fmt.Errorf("blob gas used %d exceeds maximum allowance %d", *header.BlobGasUsed, maxBlobGas) } if *header.BlobGasUsed%params.BlobTxBlobGasPerBlob != 0 { return fmt.Errorf("blob gas used %d not a multiple of blob gas per blob %d", header.BlobGasUsed, params.BlobTxBlobGasPerBlob) @@ -60,7 +61,7 @@ func VerifyEIP4844Header(config *params.ChainConfig, parent, header *types.Heade // blobs on top of the excess blob gas. func CalcExcessBlobGas(config *params.ChainConfig, parent *types.Header) uint64 { var ( - target = config.TargetBlobsPerBlock(parent.Time) * params.BlobTxBlobGasPerBlob + targetGas = uint64(config.TargetBlobsPerBlock(parent.Time)) * params.BlobTxBlobGasPerBlob parentExcessBlobGas uint64 parentBlobGasUsed uint64 ) @@ -69,10 +70,10 @@ func CalcExcessBlobGas(config *params.ChainConfig, parent *types.Header) uint64 parentBlobGasUsed = *parent.BlobGasUsed } excessBlobGas := parentExcessBlobGas + parentBlobGasUsed - if excessBlobGas < target { + if excessBlobGas < targetGas { return 0 } - return excessBlobGas - target + return excessBlobGas - targetGas } // CalcBlobFee calculates the blobfee from the header's excess blob gas field. diff --git a/consensus/misc/eip4844/eip4844_test.go b/consensus/misc/eip4844/eip4844_test.go index b903492e18..1ad3f98937 100644 --- a/consensus/misc/eip4844/eip4844_test.go +++ b/consensus/misc/eip4844/eip4844_test.go @@ -29,11 +29,11 @@ func TestCalcExcessBlobGas(t *testing.T) { var ( config = params.MainnetChainConfig targetBlobs = config.TargetBlobsPerBlock(*config.CancunTime) - targetBlobGas = targetBlobs * params.BlobTxBlobGasPerBlob + targetBlobGas = uint64(targetBlobs) * params.BlobTxBlobGasPerBlob ) var tests = []struct { excess uint64 - blobs uint64 + blobs int want uint64 }{ // The excess blob gas should not increase from zero if the used blob @@ -56,7 +56,7 @@ func TestCalcExcessBlobGas(t *testing.T) { {params.BlobTxBlobGasPerBlob - 1, targetBlobs - 1, 0}, } for i, tt := range tests { - blobGasUsed := tt.blobs * params.BlobTxBlobGasPerBlob + blobGasUsed := uint64(tt.blobs) * params.BlobTxBlobGasPerBlob parent := &types.Header{ Time: *config.CancunTime, ExcessBlobGas: &tt.excess, diff --git a/core/txpool/blobpool/blobpool_test.go b/core/txpool/blobpool/blobpool_test.go index 6f88766cbf..0187e8d9a0 100644 --- a/core/txpool/blobpool/blobpool_test.go +++ b/core/txpool/blobpool/blobpool_test.go @@ -1073,7 +1073,7 @@ func TestChangingSlotterSize(t *testing.T) { store.Close() // Mimic a blobpool with max blob count of 6 upgrading to a max blob count of 24. - for _, maxBlobs := range []uint64{6, 24} { + for _, maxBlobs := range []int{6, 24} { statedb, _ := state.New(types.EmptyRootHash, state.NewDatabaseForTesting()) statedb.AddBalance(addr1, uint256.NewInt(1_000_000_000), tracing.BalanceChangeUnspecified) statedb.AddBalance(addr2, uint256.NewInt(1_000_000_000), tracing.BalanceChangeUnspecified) diff --git a/core/txpool/blobpool/limbo.go b/core/txpool/blobpool/limbo.go index f2af1fca1a..d5992f2906 100644 --- a/core/txpool/blobpool/limbo.go +++ b/core/txpool/blobpool/limbo.go @@ -48,7 +48,7 @@ type limbo struct { } // newLimbo opens and indexes a set of limboed blob transactions. -func newLimbo(datadir string, maxBlobsPerTransaction uint64) (*limbo, error) { +func newLimbo(datadir string, maxBlobsPerTransaction int) (*limbo, error) { l := &limbo{ index: make(map[common.Hash]uint64), groups: make(map[uint64]map[uint64]common.Hash), diff --git a/core/txpool/blobpool/slotter.go b/core/txpool/blobpool/slotter.go index cc8aa387c7..84ccc0f27b 100644 --- a/core/txpool/blobpool/slotter.go +++ b/core/txpool/blobpool/slotter.go @@ -25,7 +25,7 @@ package blobpool // The slotter also creates a shelf for 0-blob transactions. Whilst those are not // allowed in the current protocol, having an empty shelf is not a relevant use // of resources, but it makes stress testing with junk transactions simpler. -func newSlotter(maxBlobsPerTransaction uint64) func() (uint32, bool) { +func newSlotter(maxBlobsPerTransaction int) func() (uint32, bool) { slotsize := uint32(txAvgSize) slotsize -= uint32(blobSize) // underflows, it's ok, will overflow back in the first return diff --git a/core/txpool/validation.go b/core/txpool/validation.go index f450689d07..d155801c97 100644 --- a/core/txpool/validation.go +++ b/core/txpool/validation.go @@ -134,7 +134,7 @@ func ValidateTransaction(tx *types.Transaction, head *types.Header, signer types if len(hashes) == 0 { return errors.New("blobless blob transaction") } - if max := opts.Config.MaxBlobsPerBlock(head.Time); uint64(len(hashes)) > max { + if max := opts.Config.MaxBlobsPerBlock(head.Time); len(hashes) > max { return fmt.Errorf("too many blobs in transaction: have %d, permitted %d", len(hashes), max) } // Ensure commitments, proofs and hashes are valid diff --git a/internal/ethapi/transaction_args.go b/internal/ethapi/transaction_args.go index a33bd9b1c0..7c156da01d 100644 --- a/internal/ethapi/transaction_args.go +++ b/internal/ethapi/transaction_args.go @@ -121,9 +121,9 @@ func (args *TransactionArgs) setDefaults(ctx context.Context, b Backend, skipGas if args.BlobHashes != nil && len(args.BlobHashes) == 0 { return errors.New(`need at least 1 blob for a blob transaction`) } - maxBlobsPerTransaction := b.ChainConfig().MaxBlobsPerBlock(b.CurrentHeader().Time) - if args.BlobHashes != nil && uint64(len(args.BlobHashes)) > maxBlobsPerTransaction { - return fmt.Errorf(`too many blobs in transaction (have=%d, max=%d)`, len(args.BlobHashes), maxBlobsPerTransaction) + maxBlobs := b.ChainConfig().MaxBlobsPerBlock(b.CurrentHeader().Time) + if args.BlobHashes != nil && len(args.BlobHashes) > maxBlobs { + return fmt.Errorf(`too many blobs in transaction (have=%d, max=%d)`, len(args.BlobHashes), maxBlobs) } // create check diff --git a/miner/worker.go b/miner/worker.go index 4f20b315d2..9f6efa220a 100644 --- a/miner/worker.go +++ b/miner/worker.go @@ -281,7 +281,8 @@ func (miner *Miner) commitBlobTransaction(env *environment, tx *types.Transactio // isn't really a better place right now. The blob gas limit is checked at block validation time // and not during execution. This means core.ApplyTransaction will not return an error if the // tx has too many blobs. So we have to explicitly check it here. - if env.blobs+len(sc.Blobs) > int(miner.chainConfig.MaxBlobsPerBlock(env.header.Time)) { + maxBlobs := miner.chainConfig.MaxBlobsPerBlock(env.header.Time) + if env.blobs+len(sc.Blobs) > maxBlobs { return errors.New("max data blobs reached") } receipt, err := miner.applyTransaction(env, tx) @@ -330,7 +331,7 @@ func (miner *Miner) commitTransactions(env *environment, plainTxs, blobTxs *tran } // If we don't have enough blob space for any further blob transactions, // skip that list altogether - if !blobTxs.Empty() && env.blobs >= int(miner.chainConfig.MaxBlobsPerBlock(env.header.Time)) { + if !blobTxs.Empty() && env.blobs >= miner.chainConfig.MaxBlobsPerBlock(env.header.Time) { log.Trace("Not enough blob space for further blob transactions") blobTxs.Clear() // Fall though to pick up any plain txs @@ -369,7 +370,8 @@ func (miner *Miner) commitTransactions(env *environment, plainTxs, blobTxs *tran // blobs or not, however the max check panics when called on a chain without // a defined schedule, so we need to verify it's safe to call. if miner.chainConfig.IsCancun(env.header.Number, env.header.Time) { - if left := miner.chainConfig.MaxBlobsPerBlock(env.header.Time) - uint64(env.blobs); left < (ltx.BlobGas / params.BlobTxBlobGasPerBlob) { + left := miner.chainConfig.MaxBlobsPerBlock(env.header.Time) - env.blobs + if left < int(ltx.BlobGas/params.BlobTxBlobGasPerBlob) { log.Trace("Not enough blob space left for transaction", "hash", ltx.Hash, "left", left, "needed", ltx.BlobGas/params.BlobTxBlobGasPerBlob) txs.Pop() continue diff --git a/params/config.go b/params/config.go index 6130417e95..21b366f9be 100644 --- a/params/config.go +++ b/params/config.go @@ -17,6 +17,7 @@ package params import ( + "errors" "fmt" "math" "math/big" @@ -476,11 +477,10 @@ func (c *ChainConfig) Description() string { return banner } -// BlobConfig specifies the target and max blobs per block for the associated -// fork. +// BlobConfig specifies the target and max blobs per block for the associated fork. type BlobConfig struct { - Target uint64 `json:"target"` - Max uint64 `json:"max"` + Target int `json:"target"` + Max int `json:"max"` UpdateFraction uint64 `json:"baseFeeUpdateFraction"` } @@ -493,7 +493,7 @@ type BlobScheduleConfig struct { // TargetBlobsPerBlock returns the target blobs per block associated with // requested time. -func (c *ChainConfig) TargetBlobsPerBlock(time uint64) uint64 { +func (c *ChainConfig) TargetBlobsPerBlock(time uint64) int { if c.BlobScheduleConfig == nil { return 0 } @@ -511,9 +511,8 @@ func (c *ChainConfig) TargetBlobsPerBlock(time uint64) uint64 { } } -// MaxBlobsPerBlock returns the max blobs per block associated with -// requested time. -func (c *ChainConfig) MaxBlobsPerBlock(time uint64) uint64 { +// MaxBlobsPerBlock returns the max blobs per block for a block at the given timestamp. +func (c *ChainConfig) MaxBlobsPerBlock(time uint64) int { if c.BlobScheduleConfig == nil { return 0 } @@ -531,9 +530,14 @@ func (c *ChainConfig) MaxBlobsPerBlock(time uint64) uint64 { } } +// MaxBlobsPerBlock returns the maximum blob gas that can be spent in a block at the given timestamp. +func (c *ChainConfig) MaxBlobGasPerBlock(time uint64) uint64 { + return uint64(c.MaxBlobsPerBlock(time)) * BlobTxBlobGasPerBlob +} + // LatestMaxBlobsPerBlock returns the latest max blobs per block defined by the // configuration, regardless of the currently active fork. -func (c *ChainConfig) LatestMaxBlobsPerBlock() uint64 { +func (c *ChainConfig) LatestMaxBlobsPerBlock() int { s := c.BlobScheduleConfig if s == nil { return 0 @@ -758,11 +762,10 @@ func (c *ChainConfig) CheckConfigForkOrder() error { } } - // Check that all forks with blobs explicitly define the blob schedule - // configuration. + // Check that all forks with blobs explicitly define the blob schedule configuration. bsc := c.BlobScheduleConfig if bsc == nil { - bsc = &BlobScheduleConfig{} + bsc = new(BlobScheduleConfig) } for _, cur := range []struct { name string @@ -773,17 +776,31 @@ func (c *ChainConfig) CheckConfigForkOrder() error { {name: "prague", timestamp: c.PragueTime, config: bsc.Prague}, {name: "verkle", timestamp: c.VerkleTime, config: bsc.Verkle}, } { + if cur.config != nil { + if err := cur.config.validate(); err != nil { + return fmt.Errorf("invalid blob configuration for fork %s: %v", cur.name, err) + } + } if cur.timestamp != nil { - // If the fork is configured, verify the blob schedule is valid. + // If the fork is configured, a blob schedule must be defined for it. if cur.config == nil { return fmt.Errorf("unsupported fork configuration: missing blob configuration entry for %v in schedule", cur.name) } - if cur.config.UpdateFraction == 0 { - return fmt.Errorf("unsupported fork configuration: %v update fraction must be defined and non-zero", cur.name) - } } } + return nil +} +func (bc *BlobConfig) validate() error { + if bc.Max < 0 { + return errors.New("max < 0") + } + if bc.Target < 0 { + return errors.New("target < 0") + } + if bc.UpdateFraction == 0 { + return errors.New("update fraction must be defined and non-zero") + } return nil } diff --git a/tests/state_test_util.go b/tests/state_test_util.go index 7119fe189d..b0ba95bec2 100644 --- a/tests/state_test_util.go +++ b/tests/state_test_util.go @@ -276,13 +276,14 @@ func (t *StateTest) RunNoVerify(subtest StateSubtest, vmconfig vm.Config, snapsh return st, common.Hash{}, 0, err } - { // Blob transactions may be present after the Cancun fork. - // In production, - // - the header is verified against the max in eip4844.go:VerifyEIP4844Header - // - the block body is verified against the header in block_validator.go:ValidateBody - // Here, we just do this shortcut smaller fix, since state tests do not - // utilize those codepaths - if config.IsCancun(new(big.Int), block.Time()) && uint64(len(msg.BlobHashes)) > config.MaxBlobsPerBlock(block.Time()) { + // Blob transactions may be present after the Cancun fork. + // In production, + // - the header is verified against the max in eip4844.go:VerifyEIP4844Header + // - the block body is verified against the header in block_validator.go:ValidateBody + // Here, we just do this shortcut smaller fix, since state tests do not + // utilize those codepaths. + if config.IsCancun(new(big.Int), block.Time()) { + if len(msg.BlobHashes) > config.MaxBlobsPerBlock(block.Time()) { return st, common.Hash{}, 0, errors.New("blob gas exceeds maximum") } }