Skip to content

api: estimate gas logging#438

Closed
tynes wants to merge 3 commits intomasterfrom
debug/estimate-gas
Closed

api: estimate gas logging#438
tynes wants to merge 3 commits intomasterfrom
debug/estimate-gas

Conversation

@tynes
Copy link
Copy Markdown
Contributor

@tynes tynes commented Apr 13, 2021

Description
This PR will add the gas used into the logline for eth_estimateGas so that we can see how much gas transactions are using

@tynes tynes force-pushed the debug/estimate-gas branch from 8dce4d5 to adad213 Compare April 13, 2021 18:32
@changeset-bot
Copy link
Copy Markdown

changeset-bot Bot commented Apr 13, 2021

⚠️ No Changeset found

Latest commit: 2f6b0ec

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

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

@gakonst
Copy link
Copy Markdown
Contributor

gakonst commented Apr 15, 2021

@tynes what's the status of this? Is this a feature we want on master? Can imagine it'd generate a lot of redundant logs.

@tynes
Copy link
Copy Markdown
Contributor Author

tynes commented Apr 15, 2021

Lets tag this as "Do not merge", this branch is useful for debugging the problems we have been seeing recently when gasPrice is not set to 0.

We have determined that 2 instances of the eth_estimateGas bug were due to user failure, in particular not setting the from. We haven't confirmed that it is a user problem on the side of the Synthetix oracle, and we still see the logline gas required exceeds allowance (%d) or always failing transaction in the production logs.

We forked geth before the revert messages would be passed back through the RPC, see ethereum/go-ethereum#21083

Can imagine it'd generate a lot of redundant logs.

This adds no additional loglines, just adds a single additional value to a logline that already exists

Happy to close when we confirm with Synthetix that the bug is a user error

@tynes tynes force-pushed the debug/estimate-gas branch from adad213 to 9c42428 Compare April 20, 2021 17:34
@tynes
Copy link
Copy Markdown
Contributor Author

tynes commented Apr 20, 2021

@K-Ho I have rebased and added an additional commit on top of this PR. It does 2 things:

  • logs the estimated gas amount which will differentiate always failing txs from txs that are actually out of gas 27ccb56
  • logs the internal variables of the estimate gas equation and add error wrappers to know exactly where the error is happening 9c42428

@tynes
Copy link
Copy Markdown
Contributor Author

tynes commented Apr 20, 2021

The transaction fees on L2 are currently overpriced on Kovan and underpriced on Mainnet. @K-Ho can you provide an update? #515

Maybe we do want the commits that add the updated logs? 2f6b0ec is now part of #519, so that will be removed from this branch when it gets rebased again

This branch is continued to be used for debugging

@tynes
Copy link
Copy Markdown
Contributor Author

tynes commented Apr 28, 2021

Going to close this because it hasn't been used in awhile

@tynes tynes closed this Apr 28, 2021
@smartcontracts smartcontracts deleted the debug/estimate-gas branch June 23, 2021 21:46
theochap pushed a commit that referenced this pull request Dec 10, 2025
Removes the TODO comments in the host server; The catch-all error
handling here fits our usecase.
theochap pushed a commit that referenced this pull request Jan 15, 2026
this returns an option because deposits are not signed.
emhane added a commit that referenced this pull request Feb 3, 2026
Closes op-rs/op-reth#427

---------

Co-authored-by: itschaindev <jagrutk@protonmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants