add support for blob tx #2

blockchaindevsh · 2024-04-09T14:25:47Z

Summary of this PR:

allow l2 blob tx if enablel2blob is specified.
add blobs field to RollupCostData, and update newL1CostFuncEcotone accordingly, basically a blob tx will have two extra fees: da fee and da proof fee.
fix Transaction.RollupCostData so that the result is consistent when called from worker and state processor.
port changes from upstream geth to support estimate gas for blob tx.

qizhou · 2024-04-10T03:12:43Z

core/txpool/validation.go

@@ -69,7 +69,7 @@ func ValidateTransaction(tx *types.Transaction, head *types.Header, signer types
 	if tx.Type() == types.DepositTxType {
 		return core.ErrTxTypeNotSupported
 	}
-	if opts.Config.IsOptimism() && tx.Type() == types.BlobTxType {
+	if false && opts.Config.IsOptimism() && tx.Type() == types.BlobTxType {


Can be put a flag to enable L2 blob rather than false/true (blow)?

Done, the flag is named enablel2blob.

… otherwise the result will be different when called from state processor 2. l2 blob tx will have two extra fees for L1CostFunc: da fee and da proof fee 3. only check versionedHashes when it's not nil

qizhou

I feel the description of the PR should be updated.

qizhou · 2024-04-27T13:32:36Z

core/types/transaction.go

@@ -378,7 +378,7 @@ func (tx *Transaction) RollupCostData() RollupCostData {
 	if v := tx.rollupCostData.Load(); v != nil {
 		return v.(RollupCostData)
 	}
-	data, err := tx.MarshalBinary()
+	data, err := tx.WithoutBlobTxSidecar().MarshalBinary()


I think we should add a few comments about why use non BLOB (but with BLOB hash) version of tx.

Another question is in which case the tx will contain the BLOB (as the derivation stage, we cannot derive the BLOB from L1).

I've updated with some comments.

qizhou · 2024-04-27T13:33:16Z

eth/ethconfig/config.go

@@ -167,6 +167,8 @@ type Config struct {

 	OverrideOptimismInterop *uint64 `toml:",omitempty"`

+	EnableL2Blob bool


I would add a few comments of like "Enable accepting EIP-4844 BLOBs on L2"

qizhou · 2024-04-27T13:35:34Z

params/protocol_params.go

@@ -47,6 +47,9 @@ const (
 	LogDataGas            uint64 = 8     // Per byte in a LOG* operation's data.
 	CallStipend           uint64 = 2300  // Free gas given at beginning of call.

+	BlobDAFee      = 2000


Comments are needed here.

qizhou · 2024-04-27T13:36:12Z

ethclient/ethclient.go

+	if msg.AccessList != nil {
+		arg["accessList"] = msg.AccessList
+	}
+	if msg.BlobGasFeeCap != nil {


What is the purpose of this parameter?

It's used here, so that a blob tx will not be included if the specified blob gas fee cap is smaller than BlobBaseFee of current block.

Geth v1.13.13 supported it on Feb 21, and op-geth has not merged it...

It seems I mis-understood Qi's question.

Here's more context about this change:ethereum-optimism#299

blockchaindevsh · 2024-04-27T14:00:30Z

I feel the description of the PR should be updated.

Done.

qizhou · 2024-04-27T15:07:29Z

core/types/rollup_cost.go

@@ -213,6 +214,7 @@ func newL1CostFuncEcotone(l1BaseFee, l1BlobBaseFee, l1BaseFeeScalar, l1BlobBaseF
 		fee = new(big.Int).Add(calldataCostPerByte, blobCostPerByte)
 		fee = fee.Mul(fee, calldataGasUsed)
 		fee = fee.Div(fee, ecotoneDivisor)
+		fee = fee.Add(fee, big.NewInt(int64(costData.blobs*params.BlobDAFee)))


Need to deal with integer overflow case?

Good catch, fixed!

qizhou · 2024-04-27T15:10:45Z

params/protocol_params.go

@@ -47,6 +47,9 @@ const (
 	LogDataGas            uint64 = 8     // Per byte in a LOG* operation's data.
 	CallStipend           uint64 = 2300  // Free gas given at beginning of call.

+	BlobDAFee      = 2000 // Fee for storing blob to DA provider
+	BlobDAProofGas = 200  // Gas for storing DA proof to span batch


I would add a comment It should be average proof size * TxDataNonZeroGasEIP2028

qzhodl · 2024-04-28T05:49:57Z

cmd/geth/config.go

@@ -194,6 +194,8 @@ func makeFullNode(ctx *cli.Context) (*node.Node, ethapi.Backend) {
 		cfg.Eth.OverrideVerkle = &v
 	}

+	cfg.Eth.EnableL2Blob = ctx.Bool(utils.EnableL2Blob.Name)


Do we need to check ctx.IsSet()?

I think their effects are the same here.

qzhodl · 2024-04-28T06:27:11Z

ethclient/ethclient.go

+	if msg.AccessList != nil {
+		arg["accessList"] = msg.AccessList
+	}
+	if msg.BlobGasFeeCap != nil {


Geth v1.13.13 supported it on Feb 21, and op-geth has not merged it...

Geth v1.14.7 update code review

enable blob tx

615c2ae

qizhou reviewed Apr 10, 2024

View reviewed changes

zhiqiangxu added 2 commits April 10, 2024 21:02

configure 4844 with "enable4844" option

1950c95

always overwrite

b8ccf69

blockchaindevsh force-pushed the support_4844 branch from 0be1126 to b8ccf69 Compare April 10, 2024 13:09

rename flag to "enablel2blob"

2fb15ee

blockchaindevsh assigned dajuguan and JustXxx Apr 10, 2024

qizhou assigned qzhodl Apr 13, 2024

zhiqiangxu added 2 commits April 15, 2024 20:21

add enablel2blob to nodeFlags

ba3a634

support estimate gas for blob tx

687267a

blockchaindevsh force-pushed the support_4844 branch from 4459385 to 687267a Compare April 15, 2024 15:51

blockchaindevsh force-pushed the support_4844 branch from 6062a09 to 0b47f6e Compare April 27, 2024 05:25

1. fix RollupCostData when called from worker.commitBlobTransaction,…

aec7ec2

… otherwise the result will be different when called from state processor 2. l2 blob tx will have two extra fees for L1CostFunc: da fee and da proof fee 3. only check versionedHashes when it's not nil

blockchaindevsh force-pushed the support_4844 branch from 0b47f6e to aec7ec2 Compare April 27, 2024 05:29

qizhou reviewed Apr 27, 2024

View reviewed changes

blockchaindevsh force-pushed the support_4844 branch from 683d5d5 to bec7267 Compare April 27, 2024 13:47

add more comment

adf8a3f

blockchaindevsh force-pushed the support_4844 branch from bec7267 to adf8a3f Compare April 27, 2024 13:54

qizhou reviewed Apr 27, 2024

View reviewed changes

qzhodl reviewed Apr 28, 2024

View reviewed changes

zhiqiangxu added 2 commits April 28, 2024 19:49

fix for review

edc950e

skip check versionedHashes when it's empty

b999c7a

blockchaindevsh mentioned this pull request Apr 30, 2024

EthStorage Devs Meeting #78 Agenda ethstorage/pm#83

Closed

qizhou approved these changes Apr 30, 2024

View reviewed changes

qzhodl approved these changes May 1, 2024

View reviewed changes

blockchaindevsh force-pushed the support_4844 branch from dc717da to bfb02ed Compare June 20, 2024 15:07

fix "blob transaction without blobs in miner"

19971ab

blockchaindevsh force-pushed the support_4844 branch from bfb02ed to 19971ab Compare June 20, 2024 15:10

remove BlobDAProofGas

5ef2b0d

blockchaindevsh merged commit 93729e7 into ethstorage:op-es Jul 11, 2024

This was referenced Sep 7, 2024

QKC/OP Devs Meeting #17 Agenda ethstorage/pm#120

Closed

EthStorage Devs Meeting #97 Agenda ethstorage/pm#124

Closed

qizhou pushed a commit that referenced this pull request Sep 18, 2024

Merge pull request #2 from mdehoog/dp/v1.14.7-code-review

6f9fab0

Geth v1.14.7 update code review

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add support for blob tx #2

add support for blob tx #2

blockchaindevsh commented Apr 9, 2024 •

edited

Loading

qizhou Apr 10, 2024

blockchaindevsh Apr 10, 2024 •

edited

Loading

qizhou left a comment

qizhou Apr 27, 2024

blockchaindevsh Apr 27, 2024

qizhou Apr 27, 2024

blockchaindevsh Apr 27, 2024

qizhou Apr 27, 2024

blockchaindevsh Apr 27, 2024

qizhou Apr 27, 2024

blockchaindevsh Apr 27, 2024

qzhodl Apr 28, 2024

blockchaindevsh Apr 29, 2024 •

edited

Loading

blockchaindevsh commented Apr 27, 2024

qizhou Apr 27, 2024

blockchaindevsh Apr 28, 2024

qizhou Apr 27, 2024

blockchaindevsh Apr 28, 2024

qzhodl Apr 28, 2024

blockchaindevsh Apr 28, 2024 •

edited

Loading

qzhodl Apr 28, 2024

		@@ -167,6 +167,8 @@ type Config struct {

		OverrideOptimismInterop *uint64 `toml:",omitempty"`

		EnableL2Blob bool

add support for blob tx #2

add support for blob tx #2

Conversation

blockchaindevsh commented Apr 9, 2024 • edited Loading

Choose a reason for hiding this comment

blockchaindevsh Apr 10, 2024 • edited Loading

Choose a reason for hiding this comment

qizhou left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blockchaindevsh Apr 29, 2024 • edited Loading

Choose a reason for hiding this comment

blockchaindevsh commented Apr 27, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blockchaindevsh Apr 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blockchaindevsh commented Apr 9, 2024 •

edited

Loading

blockchaindevsh Apr 10, 2024 •

edited

Loading

blockchaindevsh Apr 29, 2024 •

edited

Loading

blockchaindevsh Apr 28, 2024 •

edited

Loading