-
Notifications
You must be signed in to change notification settings - Fork 11.3k
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
Consistent gas metering #1219
Consistent gas metering #1219
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1219 +/- ##
==========================================
+ Coverage 81.05% 81.07% +0.02%
==========================================
Files 98 99 +1
Lines 20191 20464 +273
==========================================
+ Hits 16365 16592 +227
- Misses 3826 3872 +46
Continue to review full report at Codecov.
|
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.
This looks great!
Not a topic for this PR, but I think one big change we still need to make with gas is:
- Introduce these
computation_gas_price
andstorage_gas_price
constants that are constant within an epoch, but can change between them. We could store them on-chain in a sharedEpoch
object? - Use these prices as multipliers while computing refunds or debiting the gas object
// TODO: For now we just look at the size of the data. | ||
// Do we need to be accurate and look at the serialized size? | ||
pub fn object_data_size(&self) -> usize { | ||
match &self.data { |
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.
Should we include the metadata size as well?
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.
That's where it becomes tricky. To chase down the size of the metadata, we need to recursively go into each metadata struct and manually calculate their sizes. For instance, for Move object, we have a type_tag, which then contains Identifier of the module and function, and each Identifier is a Box of str and etc.
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.
Ah, got it + good point...
I think we can implement a StructTag::length()
function (or similar) that tells you how many characters are in the module name and function name. And it's probably worthwhile to do so because I can imagine many common cases where the length of the struct name and metadata dwarfs the size of the actual data (e.g., Coin<T>
). Not a blocker for this PR, of course.
1993e7f
to
ad436d3
Compare
A few changes:
|
031acd4
to
25e90aa
Compare
25e90aa
to
9e0cd9d
Compare
This PR introduces a consistent and top-down gas metering mechanism.
Previously, we are only introducing a gas meter during Move VM execution, while the gas charge on other things are more or less adhoc. This was causing some confusions, as well as making it difficult to evolve our gas mechanism.
This PR attempts to make the gas metering consistent and centralized:
gas.rs
:gas.rs
(this is where to start reviewing this PR too), we introduce a few primitive structs to capture our gas metering process: ASuiGasStatus
meter is defined to track gas usage across the entire transaction. Within it, what we really are using is theGasStatus
meter implemented in Move VM. Each time we are charging gas fee, we deduct it from theGasStatus
.GasStatus
is initiated with the gas_budget. Since now all transactions have a unified gas_budget, this allows us to create such status upfront, and charge it along the way.SuiGasStatus
, we keep track of storage cost used so far. At the end we use gas used in the meter to derive computation cost used. Furthermore, we track the amount of rebates that we will give back to senders if they ended up reducing on-chain storage. (the actual distribution of gas and rebate is not done yet)Transaction execution changes:
TODO: While this PR is being reviewed, I will start adding dedicated tests for gas metering.