-
Notifications
You must be signed in to change notification settings - Fork 464
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
Use gas price in place of effective gas price for initial balance check #359
Merged
Merged
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
3bcf9bf
use gas price in place of effective gas price for initial balance check
gd-0 8f623bf
use gas price when refunding account
gd-0 cbbcb5c
sub effective_price * gas from caller balance instead of max_fee
gd-0 2c4cb1f
checked arithmetic for balance_check + value
gd-0 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I said the incorrect thing, didn't take into account that, in finalization, we need to return the difference of
(gas_price-effective_price)*gas_limit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm still not quite sure what you mean. I tried applying this change and the tests are still failing.
Also when checking Geth it looks like gasPrice is used? https://github.com/ethereum/go-ethereum/blob/3a79a99f804c4bd2002fe52768c965c4be8106ea/core/state_transition.go#L400
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At the start:
we have done it as:
balance -= min(base_fee+tip,max_gas_price)*gas_limit
=>balance -= effective*gas_limit
and now we are doing:
balance -= max_gas_price * gas_limit
In finalize we need to return it back:
previously it was:
balance += effective*(remaining_gas+gas_refund)
but now it needs to take into account additional gas that we subbed, and it would become:
balance +=(gas_price-effective_price)*gas_limit+effective*(remaining-refunded)
it can be done as you did below, as do this only for the balance check. Not sure why it panics, will need to look at at detail
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
aha gotcha. That makes sense, thanks :D
Geth does it the way I did it below by only subtracting the effective price from the account: https://github.com/ethereum/go-ethereum/blob/0c9eb8c9a47285526078beef4ab5aed5a90ab938/core/state_transition.go#L217 so this seems like the best way to go imo? *note
st.gasPrice
here is effective_gas_price: https://github.com/ethereum/go-ethereum/blob/0c9eb8c9a47285526078beef4ab5aed5a90ab938/core/types/transaction.go#L633There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, I agree it is better to do as your latest, it is best to be aligned with geth with this kind of sensitive things.