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

Move usage of eth_maxPriorityFeePerGas to eth_feeHistory #2259

Closed
kclowes opened this issue Dec 15, 2021 · 8 comments
Closed

Move usage of eth_maxPriorityFeePerGas to eth_feeHistory #2259

kclowes opened this issue Dec 15, 2021 · 8 comments

Comments

@kclowes
Copy link
Collaborator

kclowes commented Dec 15, 2021

Since eth_maxPriorityFeePerGas is Geth-specific, we should stop using it to fill in gas values. We should use eth_feeHistory instead. I think it still makes sense to keep the RPC endpoint in web3/eth.py, but all places we call eth_maxPriorityFeePerGas should be changed.

Comment from #2237:

Hi, I'd like to add that eth_maxPriorityFeePerGas isn't part of ETH JSON-RPC spec. It is an endpoint Geth added on its own. Clearly some ETH providers don't have a plan to add it NomicFoundation/hardhat#1664. Maybe using eth_feeHistory is a better idea since it's part of the spec?

Also, it would be great if this can be a built-in middleware so that users could choose between eth_maxPriorityFeePerGas (or eth_feeHistory) and eth_gasPrice based on the chain they want to use.

Originally posted by @guoyiteng in #2237 (comment)

@broper2
Copy link
Contributor

broper2 commented Dec 17, 2021

Hi @kclowes, good excuse to dig into some more web3.py code. I will put together a PR for this.

@kclowes
Copy link
Collaborator Author

kclowes commented Dec 17, 2021

Awesome, thanks @broper2!

@broper2
Copy link
Contributor

broper2 commented Dec 17, 2021

Hi @kclowes, wanted to run something by you before submitting PR. I do think the underlying data used in eth_maxPriorityFeePerGas implementation is available in eth_feeHistory, and we could add a simple function to estimate a reasonable "tip" default.

However, as I read about this more, I found this discussion:
ethereum/pm#328 (comment)

Seems we may be able to leverage relation between gas_price and get_block('latest')['baseFeePerGas'] to get an exact value of what eth_maxPriorityFeePerGas would return. The relationship (web3.eth.gas_price - web3.eth.get_block('latest')['baseFeePerGas']) == web3.eth.max_priority_fee seems to hold in some quick functional testing I have done.

I don't claim to be ETH expert, so please tell me if I am missing something, or if this looks ok as a no-impact change. Thanks!

@kclowes
Copy link
Collaborator Author

kclowes commented Dec 18, 2021

That looks good to me, but @fselmo has done way more of the dynamic fee work so I'll defer to him. Do you have any thoughts @fselmo? Thanks for looking into it, @broper2!

@broper2 broper2 mentioned this issue Dec 19, 2021
@fselmo
Copy link
Collaborator

fselmo commented Dec 20, 2021

Seems we may be able to leverage relation between gas_price and get_block('latest')['baseFeePerGas'] to get an exact value of what eth_maxPriorityFeePerGas would return. The relationship (web3.eth.gas_price - web3.eth.get_block('latest')['baseFeePerGas']) == web3.eth.max_priority_fee seems to hold in some quick functional testing I have done.

I don't think this is good enough. Some testing on my end suggests that at every new block transition (most likely) there is inconsistency in the values for eth_maxPriorityFeePerGas and (eth_gasPrice - latest_base_fee), therefore this is simply not good enough to hold.

I did some quick-and-dirty testing calling the below method on an infura connection multiple times until I got False and the inequality printed. It seems to happen about once every few seconds or so and then re-stabilizes to True which makes me think it's at the new block transition but I haven't really inspected it too much. Only enough to know this is not ideal.

def max_prio_calc():
    gp = w3.eth.gas_price
    bf = w3.eth.get_block('latest')['baseFeePerGas']
    mpf = w3.eth.max_priority_fee

    print(f'gas_price: {gp}  |  base_fee: {bf}  |  max_priority_fee: {mpf}')
    
    theory_holds = mpf == gp - bf
    print(theory_holds)

    if not theory_holds:
        print(f'{mpf} != {gp - bf}')

e.g.

>>> max_prio_calc()
gas_price: 78216696028  |  base_fee: 77168141801  |  max_priority_fee: 1250000000
False
1250000000 != 1048554227

Again, I haven't looked into this too much yet (other than testing the suggestion) but I would be good with building a calculation from scratch that is equal to what geth is doing or some other method that is more robust using eth_feeHistory. I also wouldn't mind putting the responsibility of specifying the maxPriorityFeePerGas on the user if the eth_maxPriorityFeePerGas RPC method isn't available (though this is less ideal).

Thoughts?

@broper2
Copy link
Contributor

broper2 commented Dec 20, 2021

Yeah its a good point. From reading about this, I am not convinced we will be able to replicate behavior of RPC endpoint eth_maxPriorityFeePerGas perfectly either way, but if that is accepted I can put together a calculation and see if it gets closer over number of tries (I am actually guessing that it will not be as good, but don't mind trying).

If the goal is really to never deviate from what eth_maxPriorityFeePerGas returns, I think it may make better sense to put responsibility on user.

Let me know what you think @fselmo

@fselmo
Copy link
Collaborator

fselmo commented Dec 21, 2021

@broper2 yeah I hear you... I don't necessarily care that we replicate what geth is doing... something else that is reasonable would work well here too. The reason I don't like the eth_gasPrice approach is that sometimes it doesn't make sense at all and thus isn't reliable. I just ran it for less than a minute and I got a negative value because the base_fee was higher than the gas_price at that moment.

e.g.

>>> max_prio_calc()
gas_price: 67953109912  |  base_fee: 74750863316  |  max_priority_fee: 1500000000
False
1500000000 != -6797753404

Also, the more I think about it, the more I don't think I'm ok with putting the responsibility on the user. The barrier to entry is still pretty high for web3 and one of our main goals is to make the user experience better and make development more inviting.

In the end I think this ended up being a bit more complicated of a task than we expected but thank you for starting the conversation on it. Let me know if you are still interested in pursuing this. No sweat if not. I wouldn't mind scooping this up after the holidays. I would probably start looking at what geth is doing and any other examples (if they exist) and look to implement a similar internal approach for when the eth_maxPriorityFeePerGas method is not available.

@broper2
Copy link
Contributor

broper2 commented Dec 21, 2021

Good catch with the negative value, should have considered that. And no problem, I'll give it a go creating an estimate to this endpoint in the next few days. I am just trying to dig into some web3 code so don't mind the added complexity at all.

When I was researching this before, I came across this Alchemy blog which gives a lot of info on best way to do exactly this calculation. Will circle back when I have that PR updated. Thanks @fselmo

https://docs.alchemy.com/alchemy/guides/eip-1559/gas-estimator

broper2 added a commit to broper2/web3.py that referenced this issue Dec 24, 2021
fselmo pushed a commit to broper2/web3.py that referenced this issue Feb 1, 2022
fselmo added a commit to broper2/web3.py that referenced this issue Feb 1, 2022
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 added a commit to broper2/web3.py that referenced this issue Feb 1, 2022
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 closed this as completed in 84fd27a Feb 1, 2022
fselmo added a commit that referenced this issue Feb 1, 2022
Refactor idea from PR #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.
dsahdr added a commit to Rock-n-Block/web3.py that referenced this issue Feb 15, 2022
* move default_account and default_block properties and setters to BaseEth so Eth and AsyncEth can access

* Feature/ens request (ethereum#2319)

* fixed ens contract function called twice

* newsfragment

* small typo in documentation

* add newsfragment

* Only apply ``to_hexbytes`` formatter if value is not null

* asyncify eth.get_logs (ethereum#2310)

* asyncify eth.get_logs

* factor out `assert_contains_log` function

Co-authored-by: Paul Robinson <paul@pacrob.com>

* Add Github link to the main doc landing

Because Github link is extremely useful

* Newsfragment for github link to docs

* Update typing extensions to allow v4 (ethereum#2217)

* Update typing extensions to allow v4

* Loosen typing-extensions version

* Add newsfragment for typing-extensions bump

* Try out new py-evm requirements in eth-tester

* Remove xfails for newly passing eth-tester tests

* Upgrade eth-account requirement

* Add newsfragment for eth-tester bump

* correct misspellings and update referenced geth version

* Compile release notes

* Bump version: 5.26.0 → 5.27.0

* Add Async Geth Personal module (ethereum#2299)

* fix: Missing commas (ethereum#2327)

* fix: Missing commas

* Add newsfragment for exception retry middleware whitelist

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

* Fixes ethereum#2259, remove dependency on eth_maxPriorityFeePerGas

* fix lint and integration tests

* refactor: utility for estimating maxPriorityFeePerGas via eth_feeHistory

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.

* add tests for max_priority_fee when eth_maxPriorityFeePerGas is not available

* asyncify eth.syncing

* formatting and validation middleware async support

* Properly test unused code in test

* Align NamedTuples (ethereum#2312)

* Align NamedTuples

* Add NamedTuple alignment test.

* Add newsfragment for NamedTuple change

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

* rm ens.utils.dict_copy

Signed-off-by: Harmouch101 <mahmoudddharmouchhh@gmail.com>

* fixed lint error

Signed-off-by: Harmouch101 <mahmoudddharmouchhh@gmail.com>

* Update main.py

* add newsfragment

* Feature/async geth admin (ethereum#2329)

* Added BaseGethPersonal to geth.py

* Added AsyncGethPersonal and test

* Added Docs

* lint fixes

* news fragment update

* removed import_raw_key test for now

* mypy typing issues

* typo

* Added AsyncGethAdmin and BaseGethAdmin. Also, fixed test due to typing

* made suggested changes

* made suggested changes

* fixed spelling errors

* added test and docs

* newsfragment

* merge conflict

* remove setSolc

* copy in suggested test

* forgot to check liniting before commit

* linting

* linting

* Properly initialize external modules

Properly initialize modules that do not inherit from the `web3.module.Module` class. We weren't properly testing self-referential, non-static methods with this functionality and so a test was also added for this.

* correct docs for external modules

* Refactor attach_module logic

* Allow for accepting the ``Web3`` instance as the first argument in any module's ``__init()`` method, rather than requiring a module to inherit from ``web3.module.Module`` if it needs to make use of the ``Web3`` instance.

* Update tests to test the above change.

* Add a more friendly error message if the module has more than one __init__() argument. Add test for this error message / case.

* recorrect docs for external modules

* Compile release notes

* Bump version: 5.27.0 → 5.28.0

* Add 'Breaking Changes' and 'Deprecation' to our valid newsfragment types (ethereum#2340)

* Add 'Breaking Change' and 'Deprecation' to our valid newsfragment types

* Add newsfragment for new newsfragment categories

* Remove removal section of release notes

* Drop python 3.6 (ethereum#2343)

* Drop python 3.6

* Remove parity tests

* Add newsfragment for py36 drop

* Fix gas types (ethereum#2330)

* fix: correct type for effectiveGasPrice (Wei, not int)

* fix: correct type for gas and gas_limit (int, not Wei)

* lint: removed unused type imports of Wei

* Add newsfragment for Wei/int typing fixes

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

* Upgrade websockets dependency to v10+ (ethereum#2324)

* Require websockets v10+

- Remove event loop parameter

* Add newsfragment for websockets upgrade

* ➕ Add Python 3.10 support (ethereum#2175)

* ➕ Add Python 3.10 support to CI

* Dropped support for all parities

* Change docker image to use 3.10

* Update pytest-asyncio plugin

* Mark async fixture as such, clean up pytest DeprecationWarnings

Signed-off-by: Harmouch101 <mahmoudddharmouchhh@gmail.com>

Co-authored-by: Felipe Selmo <fselmo2@gmail.com>
Co-authored-by: kclowes <kclowes@users.noreply.github.com>

* add fork description

* [NBA-39] add multiple nodes for web3 HTTPProvider (#1)

* Update README.md

* Update README.md

* Update README.md

* typo fix

Co-authored-by: pacrob <paul@pacrob.com>
Co-authored-by: AlwaysData <dbfreem2@yahoo.com>
Co-authored-by: alex <67626131+alexpvpmindustry@users.noreply.github.com>
Co-authored-by: Felipe Selmo <fselmo2@gmail.com>
Co-authored-by: DefiDebauchery <75273961+DefiDebauchery@users.noreply.github.com>
Co-authored-by: Mikko Ohtamaa <mikko@opensourcehacker.com>
Co-authored-by: kclowes <kclowes@users.noreply.github.com>
Co-authored-by: Marek Šuppa <mrshu@users.noreply.github.com>
Co-authored-by: broper2 <brianroper7@gmail.com>
Co-authored-by: Călina Cenan <calina@cenan.net>
Co-authored-by: Harmouch101 <mahmoudddharmouchhh@gmail.com>
Co-authored-by: Marc Garreau <marcdgarreau@gmail.com>
Co-authored-by: coccoinomane <guido.pettinari@gmail.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

No branches or pull requests

3 participants