txmgr: call EstimateGas correctly for blob tx

[zhiqiangxu]

SimpleTxManager is a generic tool for sending tx used by various components, and the candidate tx may be a blob tx.

This PR makes EstimateGas work correctly for blob tx.

[geoknee]

Thanks @zhiqiangxu. Are you able to add or modify unit tests for this?

[zhiqiangxu]

Is there an example for such test?

It needs to be like:

1. Prepare a contract that's requiring tx blob(e.g., revert if no blob)
2. Do estimation to call it.

Without this PR it should revert.

[geoknee]

Is there an example for such test?

It needs to be like:

1. Prepare a contract that's requiring tx blob(e.g., revert if no blob)
2. Do estimation to call it.

Without this PR it should revert.

I don't think we need to go to those lengths. I was thinking we could trigger the code path in question inside this test or variant of it where the candidate.GasLimit = 0:

optimism/op-service/txmgr/txmgr_test.go

Lines 590 to 612 in 2526dc0

	// TestTxMgr_CraftBlobTx ensures that the tx manager will create blob transactions as expected.
	func TestTxMgr_CraftBlobTx(t *testing.T) {
	t.Parallel()
	h := newTestHarness(t)
	candidate := h.createBlobTxCandidate()
	// Craft the transaction.
	gasTipCap, gasFeeCap, _ := h.gasPricer.feesForEpoch(h.gasPricer.epoch + 1)
	tx, err := h.mgr.craftTx(context.Background(), candidate)
	require.Nil(t, err)
	require.NotNil(t, tx)
	require.Equal(t, byte(types.BlobTxType), tx.Type())
	// Validate the gas tip cap and fee cap.
	require.Equal(t, gasTipCap, tx.GasTipCap())
	require.Equal(t, gasFeeCap, tx.GasFeeCap())
	require.Equal(t, defaultMinBlobTxFee, tx.BlobGasFeeCap())
	// Validate the nonce was set correctly using the backend.
	require.Equal(t, uint64(startingNonce), tx.Nonce())
	// Check that the gas was set using the gas limit.
	require.Equal(t, candidate.GasLimit, tx.Gas())

Then we would need to assert that the gas limit is estimated high enough. If you run the test with and without your changes, I think you would expect different gas in each case -- this helps to understand the nature of the bug here.

[zhiqiangxu]

Sorry I don't quite understand how's your plan to test it.

The original implementation doesn't carry the blob hash over when estimating, but it doesn't necessarily lead to higher or lower gas IMO.

[geoknee]

Ah yes I think you are right, the transaction will be charged for blobGas but this is independent of the regular gas estimated in backend.EstimateGas. But this then begs the question, what difference does it make to put the blob information inside the call to EstimateGas? Is it not working correctly already?

[zhiqiangxu]

Ah yes I think you are right, the transaction will be charged for blobGas but this is independent of the regular gas estimated in backend.EstimateGas. But this then begs the question, what difference does it make to put the blob information inside the call to EstimateGas? Is it not working correctly already?

Yeah it will not work for a contract that's requiring tx blob(e.g., revert if no blob) or any contract that's making use of blobs.

It's quite hard to simulate it in unit test, but I'm quite confident it's a correct change.

[geoknee]

I see, the gas estimation could completely in that corner case. On reflection it doesn't seem worth the effort to add any more tests for this.