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
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion sui_core/src/authority.rs
Original file line number Diff line number Diff line change
Expand Up @@ -127,7 +127,8 @@ impl AuthorityState {
object_id: gas_payment_id,
})?;
gas::check_gas_balance(&gas_object, gas_budget)?;
let gas_status = gas::start_gas_metering(gas_budget)?;
// TODO: Pass in real computation gas unit price and storage gas unit price.
let gas_status = gas::start_gas_metering(gas_budget, 1, 1)?;
Ok((gas_object, gas_status))
}

Expand Down
63 changes: 47 additions & 16 deletions sui_core/src/authority/temporary_store.rs
Original file line number Diff line number Diff line change
Expand Up @@ -118,32 +118,63 @@ impl<S> AuthorityTemporaryStore<S> {
/// the gas object hasn't been mutated yet. Passing in `gas_object_size` so that we can also charge
/// for the gas object mutation in advance.
pub fn charge_gas_for_storage_changes(
&self,
&mut self,
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.

) -> SuiResult {
for (object_id, (_object_ref, object)) in &self.written {
// Objects in written can be either mutation or creation.
// We figure it out by looking them up in `self.objects`.
let object_size = object.object_size_for_gas_metering();
let old_object_size = if let Some(old_obj) = self.objects.get(object_id) {
old_obj.object_size_for_gas_metering()
} else {
0
};
gas_status.charge_storage_mutation(old_object_size, object_size)?;
let mut objects_to_update = vec![];
// Also charge gas for mutating the gas object in advance.
let gas_object_size = gas_object.object_size_for_gas_metering();
gas_object.storage_rebate = gas_status.charge_storage_mutation(
gas_object_size,
gas_object_size,
gas_object.storage_rebate,
)?;
objects_to_update.push(gas_object.clone());

for (object_id, (_object_ref, object)) in &mut self.written {
let (old_object_size, storage_rebate) =
if let Some(old_object) = self.objects.get(object_id) {
(
old_object.object_size_for_gas_metering(),
old_object.storage_rebate,
)
} else {
(0, 0)
};
let new_storage_rebate = gas_status.charge_storage_mutation(
old_object_size,
object.object_size_for_gas_metering(),
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.

// We don't need to set storage rebate for immutable objects, as they will
// never be deleted.
object.storage_rebate = new_storage_rebate;
objects_to_update.push(object.clone());
}
}

for object_id in self.deleted.keys() {
// If an object is in `self.deleted`, and also in `self.objects`, we give storage rebate.
// Otherwise if an object is in `self.deleted` but not in `self.objects`, it means this
// object was unwrapped and then deleted. The rebate would have been provided already when
// mutating the object that wrapped this object.
if let Some(old_obj) = self.objects.get(object_id) {
gas_status.charge_storage_mutation(old_obj.object_size_for_gas_metering(), 0)?;
if let Some(old_object) = self.objects.get(object_id) {
gas_status.charge_storage_mutation(
old_object.object_size_for_gas_metering(),
0,
old_object.storage_rebate,
)?;
}
}
// Also charge gas for mutating the gas object in advance.
gas_status.charge_storage_mutation(gas_object_size, gas_object_size)?;

// Write all objects at the end only if all previous gas charges succeeded.
// This avoids polluting the temporary store state if this function failed.
for object in objects_to_update {
self.write_object(object);
}

Ok(())
}

Expand Down
9 changes: 6 additions & 3 deletions sui_core/src/execution_engine.rs
Original file line number Diff line number Diff line change
Expand Up @@ -132,15 +132,18 @@ fn execute_transaction<S: BackingPackageStore>(
temporary_store.reset();
}
temporary_store.ensure_active_inputs_mutated();
if let Err(err) = temporary_store
.charge_gas_for_storage_changes(&mut gas_status, gas_object.object_size_for_gas_metering())
if let Err(err) =
temporary_store.charge_gas_for_storage_changes(&mut gas_status, &mut gas_object)
{
result = Err(err);
// No need to roll back the temporary store here since `charge_gas_for_storage_changes`
// will not modify `temporary_store` if it failed.
}

let cost_summary = gas_status.summary(result.is_ok());
let gas_used = cost_summary.gas_used();
gas::deduct_gas(&mut gas_object, gas_used);
let gas_rebate = cost_summary.storage_rebate;
gas::deduct_gas(&mut gas_object, gas_used, gas_rebate);
temporary_store.write_object(gas_object);

// TODO: Return cost_summary so that the detailed summary exists in TransactionEffects for
Expand Down
3 changes: 2 additions & 1 deletion sui_core/src/gateway_state.rs
Original file line number Diff line number Diff line change
Expand Up @@ -222,7 +222,8 @@ where
) -> SuiResult<(Object, SuiGasStatus<'_>)> {
let gas_object = self.get_object(&gas_payment_id).await?;
gas::check_gas_balance(&gas_object, gas_budget)?;
let gas_status = gas::start_gas_metering(gas_budget)?;
// TODO: Pass in real computation gas unit price and storage gas unit price.
let gas_status = gas::start_gas_metering(gas_budget, 1, 1)?;
Ok((gas_object, gas_status))
}

Expand Down
11 changes: 2 additions & 9 deletions sui_core/src/unit_tests/authority_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1416,15 +1416,8 @@ async fn shared_object() {
use sui_types::object::MoveObject;

let content = GasCoin::new(shared_object_id, SequenceNumber::new(), 10);
let data = Data::Move(MoveObject::new(
/* type */ GasCoin::type_(),
content.to_bcs_bytes(),
));
Object {
data,
owner: Owner::SharedMutable,
previous_transaction: TransactionDigest::genesis(),
}
let obj = MoveObject::new(/* type */ GasCoin::type_(), content.to_bcs_bytes());
Object::new_move(obj, Owner::SharedMutable, TransactionDigest::genesis())
};

let authority = init_state_with_objects(vec![gas_object, shared_object]).await;
Expand Down
13 changes: 3 additions & 10 deletions sui_core/src/unit_tests/consensus_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ use sui_types::gas_coin::GasCoin;
use sui_types::messages::{
CertifiedTransaction, SignatureAggregator, Transaction, TransactionData,
};
use sui_types::object::{Data, MoveObject, Object, Owner};
use sui_types::object::{MoveObject, Object, Owner};
use sui_types::serialize::serialize_cert;
use test_utils::sequencer::Sequencer;

Expand Down Expand Up @@ -47,15 +47,8 @@ fn test_shared_object() -> Object {
let seed = "0x6666666666666660";
let shared_object_id = ObjectID::from_hex_literal(seed).unwrap();
let content = GasCoin::new(shared_object_id, SequenceNumber::new(), 10);
let data = Data::Move(MoveObject::new(
/* type */ GasCoin::type_(),
content.to_bcs_bytes(),
));
Object {
data,
owner: Owner::SharedMutable,
previous_transaction: TransactionDigest::genesis(),
}
let obj = MoveObject::new(/* type */ GasCoin::type_(), content.to_bcs_bytes());
Object::new_move(obj, Owner::SharedMutable, TransactionDigest::genesis())
}

/// Fixture: a few test certificates containing a shared object.
Expand Down
Loading