Skip to content

fix(GPO): min suggested tip cap if there's congestion to avoid filtering through DefaultIgnorePrice #883

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

Merged
merged 6 commits into from
Jul 10, 2024

Conversation

jonastheis
Copy link

@jonastheis jonastheis commented Jul 5, 2024

1. Purpose or design rationale of this PR

The gas price oracle uses a oracle.ignorePrice to filter out transactions below a certain price/tip. By default it is set to DefaultIgnorePrice=2 wei.

This is problematic as we return a gas price of 1 wei if there's no congestion. This means that other nodes that did not configure with --gpo.ignoreprice=1 nor gpo.congestionthreshold will always ignore these low prices and never adjust their price prediction down.

By using 2 wei as the default price if there's no congestion, other nodes that did not explicitly configure the GPO accordingly will also adjust their prices down without needing to upgrade to this version.

Additionally, we set DefaultIgnorePrice=1 wei so that in the future we can reduce to 1 wei as the default price with no congestion.

2. PR title

Your PR title must follow conventional commits (as we are doing squash merge for each PR), so it must start with one of the following types:

  • build: Changes that affect the build system or external dependencies (example scopes: yarn, eslint, typescript)
  • ci: Changes to our CI configuration files and scripts (example scopes: vercel, github, cypress)
  • docs: Documentation-only changes
  • feat: A new feature
  • fix: A bug fix
  • perf: A code change that improves performance
  • refactor: A code change that doesn't fix a bug, or add a feature, or improves performance
  • style: Changes that do not affect the meaning of the code (white-space, formatting, missing semi-colons, etc)
  • test: Adding missing tests or correcting existing tests

3. Deployment tag versioning

Has the version in params/version.go been updated?

  • [] This PR doesn't involve a new deployment, git tag, docker image tag, and it doesn't affect traces
  • Yes

4. Breaking change label

Does this PR have the breaking-change label?

  • This PR is not a breaking change
  • Yes

@jonastheis jonastheis force-pushed the jt/fix-gpo-min-tip branch from f303d0c to bdbf820 Compare July 8, 2024 03:19
@jonastheis jonastheis requested a review from colinlyguo July 8, 2024 06:08
colinlyguo
colinlyguo previously approved these changes Jul 8, 2024
0xmountaintop
0xmountaintop previously approved these changes Jul 8, 2024
@jonastheis jonastheis dismissed stale reviews from 0xmountaintop and colinlyguo via 5d187a3 July 10, 2024 02:56
@jonastheis jonastheis merged commit ae45e12 into develop Jul 10, 2024
@jonastheis jonastheis deleted the jt/fix-gpo-min-tip branch July 10, 2024 04:31
0xmountaintop pushed a commit that referenced this pull request Aug 1, 2024
0xmountaintop added a commit that referenced this pull request Aug 1, 2024
* feat: congestion-aware gas price oracle (#790)

* Implement functionality to reset gas price / suggested tip to minimal value when there's no congestion

* Add flags to configure congestion value and initialize gas price oracle accordingly

* Fix and add tests to make sure GPO works as expected depending on pre- or post-Curie (EIP 1559) upgrade

* Apply review suggestions

* chore: auto version bump [bot]

---------

Co-authored-by: omerfirmak <omerfirmak@users.noreply.github.com>

* fix eth/gasprice/gasprice_test.go

* minor

* fix(GPO): min suggested tip cap if there's congestion to avoid filtering through `DefaultIgnorePrice` (#883)

* fix tests

* fix tests

---------

Co-authored-by: Jonas Theis <4181434+jonastheis@users.noreply.github.com>
Co-authored-by: omerfirmak <omerfirmak@users.noreply.github.com>
lwedge99 pushed a commit to sentioxyz/scroll-geth that referenced this pull request Aug 27, 2024
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.

3 participants