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

FeeHistory estimator #13833

Merged
merged 49 commits into from
Sep 4, 2024
Merged

FeeHistory estimator #13833

merged 49 commits into from
Sep 4, 2024

Conversation

dimriou
Copy link
Collaborator

@dimriou dimriou commented Jul 11, 2024

Fixes: https://smartcontract-it.atlassian.net/browse/BCI-3747

Introduces a new gas estimator that consolidates the functionalities of Suggested Price and Block History estimators.
For legacy transactions it utilized eth_gasPrice and for dynamic transactions eth_feeHistory.

@dimriou dimriou marked this pull request as ready for review July 18, 2024 18:45
@dimriou dimriou requested review from a team as code owners July 18, 2024 18:45
@dimriou
Copy link
Collaborator Author

dimriou commented Sep 2, 2024

The new changes look good to me. I do notice there might be some potential bottleneck, every time we refresh the RefreshDynamicPrice the fee_history will be invoked over the past f.config.BlockHistorySize number of blocks. If the f.config.BlockHistorySize is large for some chains, we will have very slow performance and it's wasteful as we could save the results for most of the previous blocks.

So implement some sort of cache in the future might be helpful, so we only fetch the latest blocks that are not in the cache, and cleanup the cache entries that earlier than the current - f.config.BlockHistorySize

@huangzhen1997 Let me explain why this is not a very realistic scenario: eth_feeHistory RPC call is very fast regardless of how large the history size is, considering the size is within reasonable limits. BlockHistorySize is not expected to be considerably large anyway, because we need the most up-to-date values from the network. Since we're already going to make an RPC call we're going to pay the RTT time, so requesting all values adds negligible delay vs having a complex caching logic in the code.

huangzhen1997
huangzhen1997 previously approved these changes Sep 3, 2024
@dimriou dimriou added this pull request to the merge queue Sep 4, 2024
Merged via the queue into develop with commit 1ea9f79 Sep 4, 2024
151 checks passed
@dimriou dimriou deleted the universal_estimator branch September 4, 2024 02:36
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.

5 participants