Conserve storage rebates in system transactions #10143

lxfind · 2023-03-30T15:26:43Z

This PR accumulates storage rebates during system transaction, and stuff it all into 0x5 object in the result state.
This ensures that we conserve SUI during system transaction.
In order to do this, I made the following changes:

Always go through gas charging process even for unmetered transaction
In unmetered mode, we track storage rebate in a separate field.

Enabled system transaction conservation check.

vercel · 2023-03-30T15:26:51Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

4 Ignored Deployments

Name	Status	Updated
explorer	⬜️ Ignored (Inspect)	Mar 30, 2023 at 11:28PM (UTC)
explorer-storybook	⬜️ Ignored (Inspect)	Mar 30, 2023 at 11:28PM (UTC)
sui-wallet-kit	⬜️ Ignored (Inspect)	Mar 30, 2023 at 11:28PM (UTC)
wallet-adapter	⬜️ Ignored (Inspect)	Mar 30, 2023 at 11:28PM (UTC)

crates/sui-adapter/src/execution_engine.rs

crates/sui-types/src/temporary_store.rs

sblackshear · 2023-03-30T16:13:31Z

crates/sui-adapter/src/execution_engine.rs

+            .clone();
+        // In unmetered execution, storage_rebate field of mutated object must be 0.
+        // If not, we would be dropping SUI on the floor by overriding it.
+        assert_eq!(system_state_wrapper.storage_rebate, 0);


What is the intuition for why this will be always be 0, given that we set it to a nonzero value below? E.g., if the system's storage_rebate is 0 at the end of epoch 1 and we set it to 100, won't this fail at the end of epoch 2?

This is the mutated state, not the original state. And when we mutate an object, we reset its storage_rebate (indicating that a user is paying for the new storage). But since in this case nobody is paying, its new storage_rebate must be 0

This makes sense to me as a high-level explanation.

But in the current gas charging implementation, I'm not sure there's a place where we reset the object's storage_rebate to 0 before setting it to its new value--I think we directly set it to the new value. I'm also not sure how unmetered code interacts with all of this. I think @dariorussi will know better.

dariorussi

so we are only tracking non refundable fee?
I may be confused a bit here but why not track all the other as well (storage cost and rebate)?
will that make some conservation checks not possible.
I feel I am missing something. Can someone explain?

dariorussi · 2023-03-30T16:04:17Z

crates/sui-types/src/gas.rs

@@ -350,6 +350,9 @@ pub struct SuiGasStatus<'a> {
    /// was the storage cost paid when the object was last mutated. It is not affected
    /// by the current storage gas unit price.
    storage_rebate: SuiGas,
+    /// Amount of storage rebate accumulated when we are running in unmetered mode (i.e. system transaction).
+    /// This allows us to track how much storage rebate we need to retain in system transactions.
+    unmetered_storage_rebate: SuiGas,


I assume this is just to avoid the headache of tracking the existing storage_rebate?
in my PR charging and tracking are distinct and wondering if we could use the regular rebate field.
Mostly curiosity as we do not have to change this

Tracking in a separate field is easier to deal with. But it should certainly be possible to track it in storage_rebate.
For example, we want to make sure that the resulting gas summary still have storage_rebate=0, which is important because nobody is getting these rebates (they all go to the next object).

sblackshear · 2023-03-30T16:15:44Z

crates/sui-types/src/gas.rs

@@ -350,6 +350,9 @@ pub struct SuiGasStatus<'a> {
    /// was the storage cost paid when the object was last mutated. It is not affected
    /// by the current storage gas unit price.
    storage_rebate: SuiGas,
+    /// Amount of storage rebate accumulated when we are running in unmetered mode (i.e. system transaction).
+    /// This allows us to track how much storage rebate we need to retain in system transactions.
+    unmetered_storage_rebate: SuiGas,


What would go wrong if we put the this amount into the ordinary storage_rebate field?

and to add to that, why not track the storage cost? mostly for my eduction

crates/sui-adapter/src/execution_engine.rs

mystenmark · 2023-03-30T23:32:31Z

crates/sui/tests/protocol_version_tests.rs

@@ -10,9 +10,13 @@ use test_utils::authority::start_node;
 #[should_panic]
 async fn test_validator_panics_on_unsupported_protocol_version() {
    let dir = tempfile::TempDir::new().unwrap();
+    let latest_version = ProtocolVersion::MAX;


this is fine, but this is actually causing a panic for a different reason, so this test is broken anyway. i need to fix it. but go ahead with this for now.

lxfind requested review from sblackshear, dariorussi and amnn March 30, 2023 15:26

lxfind requested review from oxade, tnowacki and tzakian March 30, 2023 15:26

lxfind force-pushed the fix-storage-rebate-for-system-tx branch from 24e5f3c to 60af05b Compare March 30, 2023 15:33