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

Fixes #2259, remove dependency on eth_maxPriorityFeePerGas #2276

Merged
merged 4 commits into from
Feb 1, 2022

Conversation

broper2
Copy link
Contributor

@broper2 broper2 commented Dec 24, 2021

What was wrong?

Related to Issue #2259:
Removal of eth_maxPriorityFeePerGas in determining transaction param defaults, as its Geth-specific.

How was it fixed?

Create native tip estimator based on Geth eth_maxPriorityFeePerGas implementation details (see https://docs.alchemy.com/alchemy/guides/eip-1559/gas-estimator).

Testing: Logic was tested using unit tests, I added a static return for feeHistory endpoint that evaluated max_priority_fee to double what was current testing default for easy verification. Also ran some functional testing....I looped for about an hour comparing this native implementation to what eth_maxPriorityFeePerGas was returning. On average, the different was about 359394279 Wei. I think this is reasonable....but also know its not my opinion that counts :) Feel free to test further.


@fselmo:
I abstracted out the logic from above into a utility method, changed some of the params, added min and max limits to the estimated fee, added tests. I also kept eth_maxPriorityFeePerGas as the default if it is available and only resort to the fee history calculation if it is not (while warning the user that the fallback method was triggered).

TODO:

  • Add newsfragment
  • Add cute animal picture

Cute animal picture:

20220115_122713

@fselmo
Copy link
Collaborator

fselmo commented Feb 1, 2022

Hey @broper2 thanks for spending some time on this! I took your suggestion but made some tweaks and abstracted it out into a utility method. Tweaks were mostly to make sure we can provide decent values all the time and don't contribute to fee bloating when fee prices are high:

  • Keep the tip within a gwei range of: 1 < fee < 1.5
  • Use the 5th percentile and only the last 10 blocks for the calculation (yielded more accurate results)

I also kept the eth_maxPriorityFeePerGas call and only resort to the fee history calculation if that's not available.

One last thing I'm a bit unsure of is turning on the eth-tester eth_feeHistory method with a static return like that. That would mean even when requesting the last 10 or 20 blocks we still only return lists with static lengths of 2 for those values. Thoughts? I lean towards turning this back off for now and rethinking it for another PR perhaps if this feature is requested.

Thanks again! Thoughts on these changes?

edit: I went ahead and removed the eth-tester eth_feeHistory static return since it feels a bit out of scope for this PR (and could benefit from some more brainstorming) and since I was able to test the fee utility indirectly by using the results-generating middleware.

Refactor idea from PR ethereum#2259 into sync and async fee utility methods. Change params passed into eth_feeHistory to values that allowed for better results when we tested locally. Add a min and max to the estimated fee history so that we don't allow unsuspecting users to contribute to fee bloating. Max and min values keep the priority fee within a range that healthy blocks should accept, so these transactions would be accepted when fee prices settle from high-fee periods.
@fselmo fselmo force-pushed the pr/maxPriorityFee branch 2 times, most recently from 5190a70 to 11973e3 Compare February 1, 2022 19:01
Copy link
Contributor

@pacrob pacrob left a comment

Choose a reason for hiding this comment

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

lgtm!

Copy link
Member

@wolovim wolovim left a comment

Choose a reason for hiding this comment

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

thanks for the walkthrough! 🚢

@fselmo fselmo merged commit df45de4 into ethereum:master Feb 1, 2022
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.

4 participants