Implement ErrGasUintOverflow error state

[KimiWu123]

Describe the feature you would like

spec: privacy-scaling-explorations/zkevm-specs#436
should cover privacy-scaling-explorations/zkevm-specs#421 as well

it depends on #1368

Additional context

No response

[KimiWu123]

Waiting for #1503 completed.

After discussed with CC, we decided to pull Scroll team's PR (which doesn't cover calldata gas uint overflow) and this issue only handles gas uint overflow for calldata. However, if gas uint overflow for calldata happens, it's invalid tx scenario. #1503 is handling invalid tx senario so I'll wait for #1503 completed.

[KimiWu123]

blocked by #1611

[ed255]

My understanding is that ErrGasUintOverflow is affected by the support of invalid txs, but invalid txs are in the specs but not in the circuit; so the current plan is to get invalid txs implemented in the circuit (which will get us a synced spec-circuits), and then implement ErrGasUintOverflow on top of that.

We've been discussing about features that are optional depending on the zkEVM setting (#1636). Maybe we can extend this features to the specs as well, and apply them to the invalid txs. This way we could have an implementation of ErrGasUintOverflow that is compatible with the specs with the invalid tx disabled; thus unlocking this issue.

What do you think?

[KimiWu123]

I'm not sure I understand it completely.

Maybe we can extend this features to the specs as well, and apply them to the invalid txs.

"this feature", do you mean #1636? If so, it makes sense to me.

thus unlocking this issue.

I don't understand why we can unlock this issue. As you said in the first paragraph, now we need to support ErrGasUintOverflow in invalid txs. No matter #1636 implemeted or not, we still need to implemente the logic of ErrGasUintOverflow in invalid txs.
I forgot the implementation details. This issue is blocked since we need a "framework" (not an accure term, what I want to say is like handle_tx in CircuitInputBuilder, having begin_tx, execution steps and then end_tx) to take care invalid txs, not only the logic of ErrGasUintOverflow. That's why this issue is blocked by #1611. Does it make sense to you? Also, I'm afraid that if we only implement the logic of ErrGasUintOverflow, we might need to go back to modify it after invalid txs works.

Before we dive into implementing #1636 in our spec, it looks like we might need another sync. Right now, there's a difference in how we deal with ErrGasUintOverflow between our spec and the circuit. In the spec, we've got a single gadget that takes care of all possible cases, but in the circuit, these cases are split into their own existing gadgets.

[ed255]

I'm not sure I understand it completely.

Maybe we can extend this features to the specs as well, and apply them to the invalid txs.

"this feature", do you mean #1636? If so, it makes sense to me.

Yes that's what I was thinking. My idea would be to achieve this in the short term:

	Spec ErrGasUintOverflow	Circuit ErrGasUintOverflow
feature=()	implemented	implemented
feature=(invalid_tx)	implemented	not implemented

thus unlocking this issue.

I don't understand why we can unlock this issue. As you said in the first paragraph, now we need to support ErrGasUintOverflow in invalid txs. No matter #1636 implemeted or not, we still need to implemente the logic of ErrGasUintOverflow in invalid txs. I forgot the implementation details. This issue is blocked since we need a "framework" (not an accure term, what I want to say is like handle_tx in CircuitInputBuilder, having begin_tx, execution steps and then end_tx) to take care invalid txs, not only the logic of ErrGasUintOverflow. That's why this issue is blocked by #1611. Does it make sense to you? Also, I'm afraid that if we only implement the logic of ErrGasUintOverflow, we might need to go back to modify it after invalid txs works.

I see. My suggestion aligns with having to go back to modify the ErrGasUintOverflow after invalid txs is implemented. But perhaps my suggestion would add more complexity than needed, and it's easier to wait for the invalid txs to be implemented in the circuit first. In any case, I think it would be nice that we consider the invalid tx as a feature that can be disabled, both in the spec and the circuit; but we can work on this once invalid tx is fully implemented.

Before we dive into implementing #1636 in our spec, it looks like we might need another sync. Right now, there's a difference in how we deal with ErrGasUintOverflow between our spec and the circuit. In the spec, we've got a single gadget that takes care of all possible cases, but in the circuit, these cases are split into their own existing gadgets.

Thanks for reporting this! Is there an issue for this? If not, could you create one to track this? Thanks!

[KimiWu123]

Thanks for reporting this! Is there an issue for this? If not, could you create one to track this? Thanks!

Just created, privacy-scaling-explorations/zkevm-specs#493

[KimiWu123]

Some conclusion after discussion with @ChihChengLiang and both of us think we don't need to implement ErrGasUintOverflow error in invalid tx case.

Some context about ErrGasUintOverflow

1. if a root call, we check whether the gas cost of calldata is over uint64_max.
2. if not a root call, we check whether the gas cost of memory operation is over uint64_max.

Here are some reasons for not implementing ErrGasUintOverflow in invalid tx case,

1. invalid tx is a special case (for Taiko, at least for now). An invalid tx won't be inclued in a block so we're be able to have an execution trace for ErrGasUintOverflow. It's more like our self-defined behavior.
2. The error, ErrGasUintOverflow, is covered by insufficient gas check in InvalidTxGadget.

• invalid tx PR for the reference: https://github.com/privacy-scaling-explorations/zkevm-circuits/pull/1677/files

@Brechtpd, would like hear your opinions.

[Brechtpd]

In the root case, can the transaction be included in a block? I don't know if that is the case or not per the Ethereum spec, but I assume it can't?

If it can't be included, then letting it use the invalid tx path makes more sense to me. Otherwise there would be multiple paths that are able to just skip a transaction without any state changes. So something like this:

• Invalid tx for transactions that need to be skipped as if they never happened
• BeginTx/EndTx for transactions that are actually part of a block

The invalid tx code can then use the feature config to enable/disable certain invalid cases.

Would this also make sense? The proposed solution also seems fine to me, but if I understand it correctly, I'm just concerned about certain behavioral things being done in multiple places when they could be grouped together.

[KimiWu123]

In the root case, can the transaction be included in a block? I don't know if that is the case or not per the Ethereum spec, but I assume it can't?

Your assumption is right, an invalid tx won't be included in a block in Ethereum.

If it can't be included, then letting it use the invalid tx path makes more sense to me. Otherwise there would be multiple paths that are able to just skip a transaction without any state changes. So something like this:

• Invalid tx for transactions that need to be skipped as if they never happened
• BeginTx/EndTx for transactions that are actually part of a block

Agree, this is what I was thinking we should collect simliar behaviors together. My plan was implementing this error in InvalidTxGadget.

---

What I was thinking is, do we still need to implement this error in InvalidTxGadget?

In bus-mapping, you're using !geth_trace.invalid to determine whether it's an invalid tx. No matter the tx is an insufficient gas error or ErrGasUintOverflow, it goes to InvalidTxGadget. I'm wondering if we still need to handle ErrGasUintOverflow since it was covered in insufficient gas case. Another point of view is there is no execution trace for "ErrGasUintOverflow + root tx" (since an invalid tx won't be included in Ethereum) so is it really necessary to implement this error?

Currently, only Taiko has invalid tx feature so I would like to know if there is any impacts and it makes sense to you if not implementing this error.

[Brechtpd]

In bus-mapping, you're using !geth_trace.invalid to determine whether it's an invalid tx. No matter the tx is an insufficient gas error or ErrGasUintOverflow, it goes to InvalidTxGadget. I'm wondering if we still need to handle ErrGasUintOverflow since it was covered in insufficient gas case. Another point of view is there is no execution trace for "ErrGasUintOverflow + root tx" (since an invalid tx won't be included in Ethereum) so is it really necessary to implement this error?

I think indeed no need to implement this error when the invalid tx error state is used because like you said it's already handled by the insufficient gas check there.

Currently, only Taiko has invalid tx feature so I would like to know if there is any impacts and it makes sense to you if not implementing this error.

What do you think of making invalid tx a default feature so it's not only used by Taiko, and instead just make it easy in InvalidTxGadget to disable all the Taiko specific error cases? Could be as easy as optionally removing some cases from the OR logic here (though of course some extra constraints are still added when just doing this so, but seems reasonable enough to keep things simple):

zkevm-circuits/zkevm-circuits/src/evm_circuit/execution/invalid_tx.rs

Lines 67 to 72 in ef14352

	let invalid_tx = or::expr([
	not::expr(is_nonce_match.expr()),
	insufficient_gas_limit.expr(),
	insufficient_balance.expr(),
	]);
	cb.require_equal("Tx needs to be invalid", invalid_tx.expr(), 1.expr());

[KimiWu123]

What do you think of making invalid tx a default feature so it's not only used by Taiko, and instead just make it easy in InvalidTxGadget to disable all the Taiko specific error cases? Could be as easy as optionally removing some cases from the OR logic here (though of course some extra constraints are still added when just doing this so, but seems reasonable enough to keep things simple):

We have an issue to add runtime feature set #1636. After this feature completed, it'll be easier to manage features like invalid tx. For now, making invalid tx a default feature make sense to me. From my personal point of view, any codes in our codebase are default features for me. We could decide what features are still in default or belonging to different feature sets after #1636 is done.

[KimiWu123]

Closed it as we don't need it.