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

[Gas Metering] Add storage_rebate to Object and introduce gas price #1254

Merged
merged 2 commits into from
Apr 10, 2022

Conversation

lxfind
Copy link
Contributor

@lxfind lxfind commented Apr 6, 2022

This PR does two things together, since they are tightly coupled.
What we are trying to do here is to properly implement Alonso's storage rebate design:
Whenever an object is being mutated, what we do is to first think that this original object is being deleted, and hence we are giving back a storage rebate, and then the new version of the object is being inserted, which will charge a new storage cost.
Because we want to make sure that the storage fund never gets depleted, the storage rebate on the same object can never be bigger (in SUI) than the storage cost when the object was added. To achieve this, we need to track the amount of SUI the previous mutation paid for the storage, and this amount will be used as the rebate next time this same object is being mutated.
This PR achieves this by adding these two things:

  1. We are initializing the gas status with a computation gas unit price and a storage gas unit price. This will eventually come from epoch metadata but I haven't added that part.
  2. We are adding a storage_rebate field to Object. This value represents the amount of rebate (in SUI) someone will get if they ever delete this object from storage. It's set by the previous transaction that mutated this object. In this implementation, what we do is that every time when an object is being mutated (not just deleted), we first "delete" the old object and provide the storage rebate, and then charge the storage cost based on the current storage gas unit price.

@lxfind lxfind changed the title [RFC] Add storage_rebate to Object and introduce gas price Add storage_rebate to Object and introduce gas price Apr 6, 2022
@lxfind lxfind changed the title Add storage_rebate to Object and introduce gas price [Gas Metering] Add storage_rebate to Object and introduce gas price Apr 7, 2022
gas_status: &mut SuiGasStatus,
gas_object_size: usize,
gas_object: &mut Object,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could take Object to avoid the clone at 133 i the caller happens to have ownership?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We cannot unfortunately. We still need to actually deduct gas from the gas_object after this function returns.

object.object_data_size(),
storage_rebate,
)?;
if !object.is_read_only() {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it safe to instead do

if object.is_read_only() { continue }

at the beginning of the loop, or even at the beginning of the function? At least based on the name, it sounds like charge_gas_for_storage_changes should never be called for immutable objects.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not. Newly published package will show up in self.written as well, and we need to charge for storage too.

@codecov
Copy link

codecov bot commented Apr 9, 2022

Codecov Report

Merging #1254 (8bdc983) into main (640dcbe) will increase coverage by 0.11%.
The diff coverage is 98.97%.

❗ Current head 8bdc983 differs from pull request most recent head 2f6d9eb. Consider uploading reports for the commit 2f6d9eb to get more accurate results

@@            Coverage Diff             @@
##             main    #1254      +/-   ##
==========================================
+ Coverage   81.72%   81.84%   +0.11%     
==========================================
  Files          99       99              
  Lines       20613    20683      +70     
==========================================
+ Hits        16847    16929      +82     
+ Misses       3766     3754      -12     
Impacted Files Coverage Δ
sui_core/src/execution_engine.rs 100.00% <ø> (ø)
sui_types/src/gas.rs 95.91% <97.50%> (-4.09%) ⬇️
sui_core/src/authority.rs 94.95% <100.00%> (ø)
sui_core/src/authority/temporary_store.rs 83.52% <100.00%> (+2.03%) ⬆️
sui_core/src/gateway_state.rs 91.33% <100.00%> (ø)
sui_core/src/unit_tests/authority_tests.rs 87.38% <100.00%> (-0.07%) ⬇️
sui_core/src/unit_tests/consensus_tests.rs 96.90% <100.00%> (-0.11%) ⬇️
sui_core/src/unit_tests/gas_tests.rs 97.60% <100.00%> (+0.11%) ⬆️
...ammability/adapter/src/unit_tests/adapter_tests.rs 98.55% <100.00%> (ø)
sui_types/src/object.rs 81.49% <100.00%> (-1.07%) ⬇️
... and 8 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 640dcbe...2f6d9eb. Read the comment docs.

@lxfind lxfind enabled auto-merge (squash) April 10, 2022 04:19
@lxfind lxfind merged commit bd9f8ca into main Apr 10, 2022
@lxfind lxfind deleted the add-gas-price branch April 10, 2022 04:33
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