l2geth: gas related API changes by tynes · Pull Request #661 · ethereum-optimism/optimism

tynes · 2021-04-27T22:56:02Z

Description

update eth_estimateGas to have a min gas value that is returned. This is due to Metamask failing for transactions when the gas limit is less than 21000.
remove check for transaction gasLimit being greater than the block gas limit

@K-Ho

Closes #608
Closes #607

changeset-bot · 2021-04-27T22:56:04Z

🦋 Changeset detected

Latest commit: 35f5526

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages

Name	Type
@eth-optimism/integration-tests	Patch
@eth-optimism/l2geth	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

tynes · 2021-04-28T22:39:37Z

Note: we charge the entire tx fee up front and do not give refunds like how L1 normally does so this means that there is a floor for how expensive transactions are. I think that breaking up the sequencer entrypoint into multiple steps would fix some problems here

karlfloersch

Left a quick comment on removing the gas limit check. Other than that looks good!

karlfloersch · 2021-04-28T22:48:02Z

l2geth/eth/api_backend.go

-			// Prevent transactions from being submitted if the gas limit too high
-			if signedTx.Gas() >= b.gasLimit {
-				return fmt.Errorf("Transaction gasLimit (%d) is greater than max gasLimit (%d)", signedTx.Gas(), b.gasLimit)
-			}


Could be a good idea that we don't remove this check entirely, but instead just replace it with a call to DoEstimateGas + a ~50% safety margin. That way we don't end up with a tx that is way incorrectly priced

I'm open if you don't think this is worth the complexity

+1 on this idea

If we add this before the execution price goes into the state and the sequencer changes the values then this will brick a lot things and will require manual gasLimits to be set

There is currently a bug in the gas estimation logic that is unknown. The only way that this would be safe is ignore the case when there is an error, otherwise a set of transactions will not be able to be sent

I think we should follow up with this addition after we figure out the bug in gas estimation

I've opened up an issue here: #694

Ok I'm down to do this! Let's make sure to prioritize the follow up though

karlfloersch · 2021-04-28T22:52:53Z

l2geth/internal/ethapi/api.go

 	fee := core.CalculateRollupFee(*args.Data, uint64(gasUsed), dataPrice, executionPrice).Uint64() / defaultGasPrice
+	if fee < 21000 {
+		fee = 21000
+	}


Note that we probably should set the default gasPrice to 0.1 gwei considering our minimum gas limit has to be 21k. At 21k the minimum fee would end up being 0.000021 ETH which is just a little too high

Agree with this, just a matter of changing the value of defaultGasPrice

I've updated the defaultGasPrice to be 0.1 gwei

gakonst · 2021-04-29T11:51:50Z

l2geth/eth/api_backend.go

-			// Prevent transactions from being submitted if the gas limit too high
-			if signedTx.Gas() >= b.gasLimit {
-				return fmt.Errorf("Transaction gasLimit (%d) is greater than max gasLimit (%d)", signedTx.Gas(), b.gasLimit)
-			}


+1 on this idea

gakonst · 2021-04-29T11:53:08Z

l2geth/eth/api_backend_test.go

-	if err.Error() != fmt.Sprintf("Transaction gasLimit (%d) is greater than max gasLimit (%d)", gasLimit, backend.GasLimit()) {
-		t.Fatalf("Unexpected error type: %s", err)
-	}


Given the suggestion above, I think this test could/should be revived and test the condition that the transaction being submitted is within the gas estimation limit.

gakonst · 2021-04-29T11:53:22Z

l2geth/internal/ethapi/api.go

 	fee := core.CalculateRollupFee(*args.Data, uint64(gasUsed), dataPrice, executionPrice).Uint64() / defaultGasPrice
+	if fee < 21000 {
+		fee = 21000
+	}


Agree with this, just a matter of changing the value of defaultGasPrice

tynes · 2021-04-29T21:39:19Z

The integration tests are breaking due to the defaultGasPrice changing

karlfloersch

LGTM!

karlfloersch · 2021-04-29T21:39:36Z

l2geth/eth/api_backend.go

-			// Prevent transactions from being submitted if the gas limit too high
-			if signedTx.Gas() >= b.gasLimit {
-				return fmt.Errorf("Transaction gasLimit (%d) is greater than max gasLimit (%d)", signedTx.Gas(), b.gasLimit)
-			}


Ok I'm down to do this! Let's make sure to prioritize the follow up though

tynes · 2021-04-29T21:54:03Z

Closing in favor of #695

chore: cgt sync 2

l2geth: prevent fees lower than 21000

fe6b45f

tynes requested review from karlfloersch and smartcontracts as code owners April 27, 2021 22:56

l2geth: remove old check for too high tx gaslimit

44ea816

tynes force-pushed the geth/eth-gas branch from b89e3a1 to cd1aaaa Compare April 28, 2021 20:31

tynes added 2 commits April 28, 2021 13:31

tests: update to use new min gas estimated value

e9f8ac9

chore: add changeset

cd1aaaa

tynes requested review from K-Ho and gakonst April 28, 2021 20:50

tynes added A-geth C-feature Category: features labels Apr 28, 2021

karlfloersch reviewed Apr 28, 2021

View reviewed changes

gakonst suggested changes Apr 29, 2021

View reviewed changes

api: make default gas price 0.1 gwei

35f5526

tynes mentioned this pull request Apr 29, 2021

Prevent Sequencer from Accepting Transactions with Too Large of Fee #694

Closed

karlfloersch approved these changes Apr 29, 2021

View reviewed changes

tynes mentioned this pull request Apr 29, 2021

rpc: gas fixes #695

Merged

tynes closed this Apr 29, 2021

tynes deleted the geth/eth-gas branch April 29, 2021 21:57

OptimismBot pushed a commit that referenced this pull request Oct 30, 2025

Merge pull request #661 from defi-wonderland/chore/cgt-sync-2

4a56665

chore: cgt sync 2

theochap pushed a commit that referenced this pull request Dec 10, 2025

chore(derive): test channel reader flushing (#661)

0a1ca04

Conversation

tynes commented Apr 27, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

changeset-bot bot commented Apr 27, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🦋 Changeset detected

Uh oh!

tynes commented Apr 28, 2021

Uh oh!

karlfloersch left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tynes commented Apr 29, 2021

Uh oh!

karlfloersch left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tynes commented Apr 29, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

tynes commented Apr 27, 2021 •

edited

Loading

changeset-bot bot commented Apr 27, 2021 •

edited

Loading