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

epic: estimate_gas undershoots necessary gas #1051

Open
1 of 2 tasks
greged93 opened this issue May 3, 2024 · 3 comments
Open
1 of 2 tasks

epic: estimate_gas undershoots necessary gas #1051

greged93 opened this issue May 3, 2024 · 3 comments
Labels
bug Inconsistencies or issues which will cause a problem for users or implementors. epic A macro-issue that tracks multiple sub-issues and tasks

Comments

@greged93
Copy link
Collaborator

greged93 commented May 3, 2024

Bug Report

Current behavior:
The current flow for the gas estimation in Kakarot greatly divergences from the flow in Reth/Geth: both Reth and Geth use a binary search given a lower and upper bound for gas limit (link for Reth) whereas Kakarot sets the gas limit in the call to 5 millions and returns the gas_used + gas_refund as a gas estimation.

This can cause issue for example in the following scenario:

  • Current available gas is 5M, used 10k: internal call in contract requires a gas limit of 95k but uses only 10k gas.
  • Call proceeds and tx ends with 30k gas used.
  • Kakarot returns 30k estimate gas, while transaction actually requires at least 95k gas in order to pass.

Tasks:

Tasks

  1. good first issue
@greged93 greged93 added the bug Inconsistencies or issues which will cause a problem for users or implementors. label May 3, 2024
@enitrat
Copy link
Contributor

enitrat commented May 3, 2024

see kkrt-labs/kakarot#1133

anukkrit149 pushed a commit to karnotxyz/kakarot-rpc that referenced this issue Aug 9, 2024
<!--- Please provide a general summary of your changes in the title
above -->

<!-- Give an estimate of the time you spent on this PR in terms of work
days.
Did you spend 0.5 days on this PR or rather 2 days?  -->

Time spent on this PR: 0.5 day

## Pull request type

<!-- Please try to limit your pull request to one type,
submit multiple pull requests if needed. -->

Please check the type of change your PR introduces:

- [ ] Bugfix
- [x] Feature
- [ ] Code style update (formatting, renaming)
- [ ] Refactoring (no functional changes, no api changes)
- [ ] Build related changes
- [ ] Documentation content changes
- [ ] Other (please describe):

## What is the current behavior?
Missing sepolia to the list of testnets
<!-- Please describe the current behavior that you are modifying,
or link to a relevant issue. -->

Resolves #NA

## What is the new behavior?
Add sepolia to the list of testnets.

<!-- Reviewable:start -->
- - -
This change is [<img src="https://reviewable.io/review_button.svg"
height="34" align="absmiddle"
alt="Reviewable"/>](https://reviewable.io/reviews/kkrt-labs/kakarot/1051)
<!-- Reviewable:end -->
@Eikix
Copy link
Member

Eikix commented Oct 2, 2024

Is this still stuck?

@greged93
Copy link
Collaborator Author

greged93 commented Oct 2, 2024

this isn't stuck, we just decided not to implement the binary search method as long as the other method works.

@greged93 greged93 added the epic A macro-issue that tracks multiple sub-issues and tasks label Oct 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Inconsistencies or issues which will cause a problem for users or implementors. epic A macro-issue that tracks multiple sub-issues and tasks
Projects
Status: 📅 Next sprint
Development

No branches or pull requests

3 participants