Add blob length errors for engine api and transition tool.

[spencer-tb]

Adds errors for the blob length within transition tool and engine api. This lets us generate the expected execution-spec-tests fixtures, and return the appropriate INVALID payload status when running the fixtures in hive.

[roberto-bayardo]

Seems a little odd to do validation checks on # of blobs here when there are no other validation checks being performed. what makes blobs validation special enough to be an exception here?

[spencer-tb]

I don't disagree here, maybe there is somewhere else it can be added?

I initially wanted to add the check here only for 0 blobs: https://github.com/mdehoog/go-ethereum/blob/eip-4844/cmd/evm/internal/t8ntool/execution.go#L179, as the t8n-tool should return the rejected txs for erroneous # blobs.

[roberto-bayardo]

If there are too many blobs then state transition should already fail with "data gas limit reached".

I see some tx validity checks performed here:

go-ethereum/cmd/evm/internal/t8ntool/transaction.go

Line 67 in 7d7c642

func Transaction(ctx *cli.Context) error {

Would checking for 0 blobs there work?

[spencer-tb]

I'll omit the > max blobs check :D

If we have the check there the t8n-tool fails which means that we can't generate the erroneous fixtures. What about something like this:

if chainConfig.IsCancun(pre.Env.Timestamp) && tx.Type() == types.BlobTxType && len(tx.DataHashes()) == 0 {
	err := fmt.Errorf("blob transaction with zero blobs")
	log.Warn("rejected tx", "index", i, "hash", tx.Hash(), "error", err)
	rejectedTxs = append(rejectedTxs, &rejectedTx{i, err.Error()})
	continue
}

Here: cmd/evm/internal/t8ntool/execution.go

[spencer-tb]

After some more thought this seems more appropriate: check for zero data gas if in Cancun here: core/state_transition.go#L240

if dataGasUsed == 0 {
	return fmt.Errorf("%w: cancun is active but DataGasUsed is 0. .....Context.Time)
}

[roberto-bayardo]

If we want to check for 0 blobs & error out in state_transition, then I think the more explicit check looking at dataHashes len is more clear. Also datagasused == 0 is going to be expected for non-blob txs.

If this validity check should be performed in state transition then I guess the current check is as good as any. Is state_transition supposed to catch any invalid tx? I'd always assumed basic tx validity checking was done before we got to this point and therefore doing it at this point would be redundant.

[roberto-bayardo]

Maybe @Inphi has more insight into this.

[Inphi]

I'm not keen on redundant checks in state transition either. Though it looks like this issue has been addressed.

[Inphi]

LGTM. I do think we need to assert non-zero blobs/versionedHash in areas where a blob transaction may be imported directly without the blob itself (maybe by the CL). We're still figuring out how that should work (or if it's even desirable). So for now I'm OK with having zero-blob checks in ExecutableDataToBlock.