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

Consistent gas metering #1219

Merged
merged 2 commits into from
Apr 5, 2022
Merged

Consistent gas metering #1219

merged 2 commits into from
Apr 5, 2022

Conversation

lxfind
Copy link
Contributor

@lxfind lxfind commented Apr 1, 2022

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:

  1. In gas.rs (this is where to start reviewing this PR too), we introduce a few primitive structs to capture our gas metering process: A SuiGasStatus meter is defined to track gas usage across the entire transaction. Within it, what we really are using is the GasStatus meter implemented in Move VM. Each time we are charging gas fee, we deduct it from the GasStatus. 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.
  2. We divide gas cost into two categories: computation cost and storage cost This is useful since very likely we will be distributing them separately. In the 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)
  3. We define a gas constant table, with a list of constants on how we charge various operations during the transaction. Detailed comments are explained inline. Notably, we charge a flat minimum fee for every transaction.
  4. For object mutations, it's important to note that there are two parts of the cost: a computation cost due to mutating the storage, and a storage cost for any extras bytes added to the storage (or in the case if the storage bytes decreased, we provide storage_rebate, which is tracked separately)

Transaction execution changes:

  1. We now check gas budget first thing during a transaction execution, before fetching any other objects. This helps with DDoS attacks that doesn't provide a valid gas budget. If the gas budget satisfies the minimum requirement, we proceed creating the gas meter. This meter is used across the transaction.
  2. The following things charge gas today: (1) We charge a flat minimum transaction fee upfront. (2) We charge for all object read from the db per byte (3) Move VM execution (4) Move module publish computation cost for verification and linking (5) For all objects that are mutated in the end, we charge altogether. Depending on how an object is mutated, it will charge either computation cost only, or combined with storage cost/rebate.
  3. The TransactionEffects now includes the gas cost summary within the execution status (a summary includes computation_cost, storage_cost and storage_rebate)

TODO: While this PR is being reviewed, I will start adding dedicated tests for gas metering.

@codecov
Copy link

codecov bot commented Apr 1, 2022

Codecov Report

Merging #1219 (25e90aa) into main (c2096c0) will increase coverage by 0.02%.
The diff coverage is 97.38%.

❗ Current head 25e90aa differs from pull request most recent head 9e0cd9d. Consider uploading reports for the commit 9e0cd9d to get more accurate results

@@            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     
Impacted Files Coverage Δ
sui_programmability/adapter/src/adapter.rs 88.70% <92.00%> (+2.82%) ⬆️
sui_core/src/authority.rs 94.95% <94.17%> (+0.69%) ⬆️
sui_core/src/gateway_state.rs 91.18% <94.44%> (+0.07%) ⬆️
sui_core/src/unit_tests/move_integration_tests.rs 93.33% <95.83%> (+0.01%) ⬆️
sui_core/src/unit_tests/gas_tests.rs 97.38% <97.38%> (ø)
sui_core/src/authority/temporary_store.rs 81.49% <100.00%> (+1.59%) ⬆️
sui_core/src/execution_engine.rs 100.00% <100.00%> (+34.69%) ⬆️
sui_core/src/transaction_input_checker.rs 87.68% <100.00%> (-3.48%) ⬇️
sui_core/src/unit_tests/authority_tests.rs 87.45% <100.00%> (-0.92%) ⬇️
...ammability/adapter/src/unit_tests/adapter_tests.rs 98.55% <100.00%> (+0.88%) ⬆️
... and 20 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c2096c0...9e0cd9d. Read the comment docs.

@lxfind lxfind requested a review from awelc April 1, 2022 22:58
Copy link
Collaborator

@sblackshear sblackshear left a 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 and storage_gas_price constants that are constant within an epoch, but can change between them. We could store them on-chain in a shared Epoch object?
  • Use these prices as multipliers while computing refunds or debiting the gas object

sui_types/src/gas.rs Outdated Show resolved Hide resolved
sui_types/src/gas.rs Outdated Show resolved Hide resolved
sui_types/src/gas.rs Outdated Show resolved Hide resolved
// 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 {
Copy link
Collaborator

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?

Copy link
Contributor Author

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.

Copy link
Collaborator

@sblackshear sblackshear Apr 4, 2022

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.

sui_types/src/object.rs Show resolved Hide resolved
@lxfind lxfind force-pushed the top_level_gas_status branch 2 times, most recently from 1993e7f to ad436d3 Compare April 5, 2022 06:17
@lxfind
Copy link
Contributor Author

lxfind commented Apr 5, 2022

A few changes:

  1. Added comprehensive tests in gas_tests.rs
  2. After discussions with Alonso, we now use one single constant for storage per byte cost.
  3. Added various TODOs: need to be more accurate on calculating object size; need to keep track object storage fun in the object, which will be used for rebate. (also refer to the issue description)

@lxfind lxfind force-pushed the top_level_gas_status branch 2 times, most recently from 031acd4 to 25e90aa Compare April 5, 2022 16:23
@lxfind lxfind enabled auto-merge (rebase) April 5, 2022 17:31
@lxfind lxfind merged commit f9ef178 into main Apr 5, 2022
@lxfind lxfind deleted the top_level_gas_status branch April 5, 2022 17:48
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.

None yet

2 participants