Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(protocol): remove gas limit check #14087

Closed
wants to merge 21 commits into from
Closed

Conversation

dantaik
Copy link
Contributor

@dantaik dantaik commented Jul 2, 2023

Completely remove gasLimit check and only check gasUsed is smaller than an config. The circuits will need to verify that for any block, gasLimit >= gasUsed (probably without checking gasLimit <= some threshold).

This will enable proposers to include transactions that have hugh gasLimit but very small gasUsed.

@dantaik dantaik self-assigned this Jul 2, 2023
@dantaik dantaik marked this pull request as ready for review July 2, 2023 04:22
cyberhorsey
cyberhorsey previously approved these changes Jul 2, 2023
@Brechtpd
Copy link
Contributor

Brechtpd commented Jul 2, 2023

This is a bit of a tricky one which is why I wanted to avoid this scenario, but I guess it has to be solved at some point.

I want to make sure we're on the same page of the expected behavior. I'm also not sure how Ethereum currently behaves in this case, I would think a block using more than 30M gas is an invalid block and can never end up in the chain?

For us to support we'll have to execute the transaction up til the block gas limit, when gasUsed > block gas limit we have to stop the execution of the transaction somehow (some special error case). We then have two options:

  1. Stop the execution of the current transaction and revert it completely (turning it in an invalid tx with no state changes, because the transaction still needs to be includable in another block afterwards as it was executed wrongly in this block). All other transactions after this one also needs to be turned into invalid transactions that should be skipped.
  2. Stop the execution of the current transaction and turn the block in an empty block so automatically all transactions can be included in another valid block.

We also have to watch out that we don't create a DoS possibility because nodes also have to execute these transactions to find out how much gas is actually used, so with 2) for example they would have to do a lot of work to just throw everything away. This should still count towards our target gas/s so the EIP 1559 base fee should still go up as if the block was full following this approach.

So 1) seems to be a reasonable approach, but also in this case we'll have to force the gas_used for the basefee calculation of the current block to be the block gas limit, because also in this case the nodes had to execute transactions up until that limit, even if all the work had to be reverted and the actual gas_used would be 0.

So regardless of how many transactions we turn invalid, forcing the gas_used to the block gas limit for the EIP-1559 calculation seems to be required. So then the decision is just how many transactions we make invalid. I think the only concern we have to have for this decision is implementation complexity, because block proposers won't make these invalid blocks so it doesn't really matter what happens exactly. I think complexity wise they will be very similar for the circuits (though certainly not easy!) so we may as well go with 1), but I"ll try to think about it some more. I assume for the node this doesn't matter that much?

@dantaik
Copy link
Contributor Author

dantaik commented Jul 2, 2023

Brecht, both options you've suggested could potentially work, but I believe Option 1 would be more effective. This is because it allows for the inclusion of certain transactions within the block, which enhances efficiency. However, the primary issue here doesn't stem from the client's implementation; it's actually about the circuit's complexity.

Even if the n-th transaction (the tx in question) pushes the total gas used beyond the block's maximum allowable gas used, it's necessary for this transaction to be executed so its trace logs can be produced for the Zero-Knowledge Proof (ZKP) in order to confirm that the transaction in question did indeed consume enough amount of gas to overflow the block gas used maximum. This makes this specific transaction unique compared to all previous transactions. However, I am uncertain whether zkEVM can handle such a situation efficiently and easily.

If we consider the total gas used by all preceding transactions to be G, and the gas used by the transaction in question is represented by g, the proposer fee should be calculated using the sum of G+g, rather than just G, and I believe g is known to circuits and can be provided to the protocol contracts.

@Brechtpd
Copy link
Contributor

Brechtpd commented Jul 2, 2023

I think we can reuse the out-of-gas error for the last transaction, because if we we had to cap the tx gas limit because of the block gas limit, than any out-of-gas error is caused because of the block gas limit. It's still a special case that needs to be added, but maybe doesn't need to have that special handling if we can do it like that, on both the node and the circuits side. Still a bit tricky for the other stuff, because the nonce/ETH transferred at the start of the transaction now also needs to be revertible (which currently is not the case).

G+g for the basefee calculation will be basically the block gas limit. And yeah the trace until the block gas limit is used will have to generated (which is the DoS attack vector), so the node could use the same trick as described to do so if that's convenient there.

Brecht, both options you've suggested could potentially work, but I believe Option 1 would be more effective. This is because it allows for the inclusion of certain transactions within the block, which enhances efficiency.

I don't think the efficiency is important as I wrote in my first post. Proposers won't propose blocks like this because they waste money by having to post the data of the transaction on-chain knowing that it won't be able to be executed (and so won't get them any fee). So only trying to reduce complexity should be important here.

@dantaik
Copy link
Contributor Author

dantaik commented Jul 3, 2023

I suggest us with the following actions:

  1. David and Jeff: try to implement the feature in client to exclude the first out-of-gas transaction and all other transactions after it, but try to produce a as large block as possible.
  2. If the client is efficient enough against malicious attacks, Brecht takes over with circuits changes.
  3. Dani and myself will follow up with this PR with potential more changes.

@dantaik dantaik changed the title feat(protocol): remove gas limit check feat(protocol): remove gas limit check [do not merge] Jul 3, 2023
@dantaik dantaik changed the base branch from alpha-4 to prevrandao July 3, 2023 05:00
Base automatically changed from prevrandao to alpha-4 July 3, 2023 05:02
@dantaik dantaik dismissed cyberhorsey’s stale review July 3, 2023 05:02

The base branch was changed.

@dantaik
Copy link
Contributor Author

dantaik commented Jul 8, 2023

I discussed this issue with @davidtaikocha today, and we believe we can ignore it for now -- it seems this is only an issue for testnets where some proposers do not consider the cost in test Ether. On mainnet with real Ether, most proposers won't be as irrational.

@dantaik dantaik closed this Jul 8, 2023
@dantaik dantaik reopened this Jul 27, 2023
config: getConfig(),
gasAmount: gasLimit
});
function getBlockFee() public view returns (uint64) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shall we also change the calculation of L2 EIP-1559? since its "purchasing" the block.gaslimit.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If block.gaslimit is set to blockMaxGasUsed I think it's fine.

@@ -27,7 +27,8 @@ library TaikoConfig {
blockMaxVerificationsPerTx: 10,
// Set it to 6M, since its the upper limit of the Alpha-2
// testnet's circuits.
blockMaxGasLimit: 6_000_000,
blockMaxGasUsed: 6_000_000,
blockMaxGasLimit: 12_000_000,
Copy link
Member

@davidtaikocha davidtaikocha Jul 28, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm shall we change this to txMaxGasLimit? looks like if there is a block gas limit, its still easy for malicious users to attack by simply increasing their tx.gasLimit?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, but I think the txMaxGasLimit should simply be blockMaxGasUsed, and then simply make all transactions with a gas limit higher than that invalid.

Doing this we could avoid needing custom support for stopping the execution of a tx with a partial trace if we can prove blocks up till blockMaxGasUsed * 2, which of course is not super efficient but may be worth it to reduce complexity, but I'm not really sure how big of a simplification it would actually be, I have to think about it some more.

And temporarily to be compatible with the current system, we can set blockMaxGasLimit to blockMaxTransactions * blockMaxGasUsed.

@dantaik
Copy link
Contributor Author

dantaik commented Jul 29, 2023

Ready for another round of review, guys. @Brechtpd @davidtaikocha @adaki2004 @cyberhorsey

@@ -17,10 +17,11 @@ library TaikoData {
// This number is calculated from blockMaxProposals to make
// the 'the maximum value of the multiplier' close to 20.0
uint256 blockMaxVerificationsPerTx;
uint32 blockMaxGasLimit;
// The max gas used per block, including anchor.
uint32 blockAndTxMaxGasUsed;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just want to double check if we will use this value to reject transactions with tx.limit > blockAndTxMaxGasUsed because then the name is a bit confusing (gasLimit vs gasUsed). I'm not sure if Ethereum itself already does something like this e.g. if a tx has a gas limit of 100M gas but the block gas limit is 10M gas, is it rejected or can it still be included in a block if the tx ends up only using < 10M gas?

Copy link
Member

@davidtaikocha davidtaikocha Jul 30, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if Ethereum itself already does something like this e.g. if a tx has a gas limit of 100M gas but the block gas limit is 10M gas, is it rejected or can it still be included in a block if the tx ends up only using < 10M gas?

image

No all transactions accumulated gasLimit can not exceed block.gasLimit, so in go-ethereum, if its building a block, that transaction will just be rejected; if it received a block like this from the network, the whole block will be treat as invalid. So i think, if we just use blockAndTxMaxGasUsed as block.gasLimit, it will break the ethereum protocol, maybe use uint256.max or just set a txMaxGasLimit then use the all txs accumulated gasLimit as the block.gasLimit will be more ethereum competitive?

Copy link
Contributor

@Brechtpd Brechtpd Jul 30, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So Ethereum has a pretty similar problem we currently have? As in only a single tx with a tx.gasLimit close to the block gas limit can be included? The only difference with our previous approach is that transactions with a smaller tx.gasLimit cannot be included anymore while in Ethereum blocks that would still be possible.

I think blockAndTxMaxGasUsed should still be okay no? So there is a block gas limit, and each tx.gasLimit needs to be <= block.gasLimit. But to be 100% compatible with Ethereum we would also have to add an additional check that l(Br)_u + tx.gasLimit <= block.gasLimit as well. That seems like a pretty easy check to add.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

set a txMaxGasLimit then use the all txs accumulated gasLimit as the block.gasLimit will be more ethereum competitive?

I think this would be a pretty big change (similar to the previous approach) where the the tx.gasLimit is a pretty important value. In Ethereum it's a mostly unimportant value where you can set it to a high value to make sure it will be successful with no real impact (except I guess for the rule above, but for any real use case should still be much smaller than block.gasLimit).

@dantaik dantaik requested a review from Brechtpd July 30, 2023 13:37
@davidtaikocha davidtaikocha linked an issue Jul 31, 2023 that may be closed by this pull request
@dantaik
Copy link
Contributor Author

dantaik commented Aug 1, 2023

@Brechtpd @davidtaikocha @cyberhorsey @dantaik
So after a offline discussion, we decided not to move forward with this PR, but rather just increase the gas limit for the next testnet.

@dantaik dantaik closed this Aug 1, 2023
@dantaik dantaik deleted the remove_gas_limit_check branch September 25, 2023 11:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

speed transition
5 participants