Add CLI config validation of op-batcher to prevent submitting Blobs into Alt DA

[Kourin1996]

Fixes https://github.com/celo-org/celo-blockchain-planning/issues/784

This PR adds a new validation of CLI configuration of op-batcher. op-batcher throws error in runtime when submitting blobs type txdata into Alt DA.

This PR adds new validation for the 'op-batcher' CLI configuration. 'op-batcher' throws an error at runtime when submitting blobs of type txdata into Alt DA

optimism/op-batcher/batcher/driver.go

Lines 594 to 601 in 81fde1a

	func (l *BatchSubmitter) publishToAltDAAndL1(txdata txData, queue *txmgr.Queue[txRef], receiptsCh chan txmgr.TxReceipt[txRef], daGroup *errgroup.Group) {
	// sanity checks
	if nf := len(txdata.frames); nf != 1 {
	l.Log.Crit("Unexpected number of frames in calldata tx", "num_frames", nf)
	}
	if txdata.asBlob {
	l.Log.Crit("Unexpected blob txdata with AltDA enabled")
	}

The validation added in this PR prevents both Alt DA mode and Blob types from being enabled at the same time.

FYI: If the TxData type is 'auto', the txdata type will be set to 'calldata' for now.

[karlb]

The devnet-celo job only fails due to flakiness from what I can see (#235).

[karlb]

FYI: If the TxData type is 'auto', the txdata type will be set to 'calldata' for now.

It looks different to me in

optimism/op-batcher/batcher/service.go

Lines 210 to 215 in 9c20efe

	case flags.BlobsType, flags.AutoType:
	if !cfg.TestUseMaxTxSizeForBlobs {
	// account for version byte prefix
	cc.MaxFrameSize = eth.MaxBlobDataSize - 1
	}
	cc.UseBlobs = true

Am I looking at the wrong thing?

[Kourin1996]

@karlb I need double check. I have to follow some code. Will do now. Thank you!

[Kourin1996]

@karlb

As you mentioned, UseBlobs is set for for auto batch data type.
However, this will be reset below this in the same function.

optimism/op-batcher/batcher/service.go

Lines 257 to 268 in 81fde1a

	if cfg.DataAvailabilityType == flags.AutoType {
	// copy blobs config and use hardcoded calldata fallback config for now
	calldataCC := cc
	calldataCC.TargetNumFrames = 1
	calldataCC.MaxFrameSize = 120_000
	calldataCC.UseBlobs = false
	calldataCC.ReinitCompressorConfig()
	bs.ChannelConfig = NewDynamicEthChannelConfig(bs.Log, 10*time.Second, bs.TxManager, cc, calldataCC)
	} else {
	bs.ChannelConfig = cc
	}

NewDynamicEthChannelConfig is a module that fetches configuration dynamically. As far as I search the code, UseBlobs won't update regardless of the batch data type right now.

[karlb]

As you mentioned, UseBlobs is set for for auto batch data type.
However, this will be reset below this in the same function.

I see, this should be ok then. I still wonder if it wouldn't be safer and more explicit to disallow the combination of AltDA and AutoType. There is no advantage of that compared to directly setting AltDA + CallData and the resulting behaviour is unclear from just looking at the config options.

Eventually, I'll trust whatever upstream says on this aspect (and we should upstream it), since they will be the ones making sure the constraints are met for all the changes going into upstream. So I won't insist on further restricting the check.

[Kourin1996]

@karlb

I still wonder if it wouldn't be safer and more explicit to disallow the combination of AltDA and AutoType.

Eventually, I'll trust whatever upstream says on this aspect (and we should upstream it), since they will be the ones making sure the constraints are met for all the changes going into upstream.

Thank you so much for sharing your opinion.

I thought we would not have issues right now. But we can disable such case for now as far as we don't use it now. Which is more safer. At the end, I just follow the original suggestion by auditor. It's more straight.

Sorry for troubling you but please approve it again. Thank you!

[karlb]

I still think this is a good candidate for upstreaming. Do you want to open an upstream PR @Kourin1996? Otherwise I'll give it a go.