[storage_gas] implement customizable storage gas config #3810

lightmark · 2022-09-02T22:57:49Z

Description

We implement a customizable curve for storage-based gas base unit and map x <-[0, 10000] to y<-[0, 10000].
storage gas = min_gas + (max_gas - min_gas) * (current_usage / target_utilization)

The curve is a vector of point(x, y), both are basis points.

curve's points' y value must be monotonically non-decreasing, and x values must be monotonically increasing.
curve's points must start with a point of x = 0 and end with a point of x = 10000.

Test Plan

ut

This change is

aptos-move/framework/aptos-framework/sources/state_storage.move

davidiw

Cool stuff.

However, this is a breaking change.

We can't just delete GasParameter but we can stop using it.

We'll also need to figure out how to upgrade long lived testnet.

aptos-move/framework/aptos-framework/sources/storage_gas.move

aptos-move/framework/aptos-framework/sources/configs/gas_schedule.move

aptos-move/framework/aptos-framework/sources/state_storage.move

aptos-move/framework/aptos-framework/sources/storage_gas.move

wrwg

This is not a compatible change. I have not seen a clean indicating on slack that we are ok with breaking changes now.

Frankly, I also think this is a lot of new code in the last minute. There are little tests with this code, or do I miss them?

aptos-move/framework/aptos-framework/sources/state_storage.move

wrwg · 2022-09-09T18:28:44Z

aptos-move/framework/aptos-framework/sources/state_storage.move

-    /// epoch for gas calculation of the entire next epoch. -- The data is one
-    /// epoch older than ideal, but the Vm doesn't need to worry about reloading
-    /// gas parameters after the first txn of an epoch.
-    struct GasParameter has key, store {


You can't remove this without reset of the chain.

That's something we'll discuss with @davidiw ... Yeah, I guess I have to keep and put it to the end of the file for now tho.

vgao1996

Overall LGTM! Thanks for the hard work!

vgao1996 · 2022-09-12T17:22:13Z

aptos-move/framework/aptos-framework/sources/storage_gas.move

+            };
+        };
+        // Check the corner case that current_usage_bps drops before the first point.
+        let (left, right) = if (current_usage_bps < vector::borrow(points, i).x) {


This could only happen when i == 0, right?

yes. it is a corner case.

Then I wonder if it's clearer to replace the is with 0s?

vgao1996 · 2022-09-12T17:29:08Z

aptos-move/framework/aptos-framework/sources/storage_gas.move

+        while (i <= len) {
+            let cur = if (i == 0) { &Point {x: 0, y: 0} } else { vector::borrow(points, i - 1) };
+            let next = if (i == len) { &Point {x: BASIS_POINT_DENOMINATION, y: BASIS_POINT_DENOMINATION} } else { vector::borrow(points, i) };
+            assert!(cur.x < next.x && cur.y <= next.y, error::invalid_argument(EINVALID_MONOTONICALLY_NON_DECREASING_CURVE));
+            i = i + 1;
+        }


Since you already checked the points at the two ends, you can just loop from i = 0 to (len - 2) inclusive and get rid of the special checks.

I checked the points at the ends. but if doesn't tells me points[len - 1].x < end_point.x and points[len - 1].y <= end_point.y. So tho I have len points, actually we have len + 2 in total and have to compare len + 1 pairs that's why we loop i in [0, len], which has len + 1 iterations.

vgao1996 · 2022-09-12T17:36:21Z

aptos-move/framework/aptos-framework/sources/storage_gas.move

+                Point {x: 2000, y: 300},
+                Point {x: 3000, y: 600},
+                Point {x: 4000, y: 1000},
+                Point {x: 5000, y: 1500},


@msmouse when we discussed previously, you said something like "we want the the cost to be 1.15x of the base cost at 50% utilization". However here it's base + 0.15x the max price hike. Just to double check: which one is correct?

0.15x the max price hike
^ is what I meant, sorry

aptos-move/framework/aptos-framework/sources/storage_gas.move

aptos-move/framework/aptos-framework/sources/configs/gas_schedule.move

aptos-move/framework/aptos-framework/sources/storage_gas.move

msmouse · 2022-09-15T19:09:03Z

aptos-move/framework/aptos-framework/sources/storage_gas.move

+                new_point(9500, 6372),
+                new_point(9900, 9138),


movekevin · 2022-09-15T20:02:12Z

aptos-move/framework/aptos-framework/sources/configs/gas_schedule.move

@@ -44,4 +46,11 @@ module aptos_framework::gas_schedule {
        // Need to trigger reconfiguration so validator nodes can sync on the updated gas schedule.
        reconfiguration::reconfigure();
    }
+
+    public fun set_storage_gas_config(aptos_framework: &signer, config: StorageGasConfig) {


Why do we need this extra function? Can't we just call storage_gas::set_config directly?

we can. But I think @vgao1996 insists we control gas-related params from a single module.

Can't we just call storage_gas::set_config directly?

I'm totally fine with calling storage_gas::set_config directly.

@vgao1996 insists we control gas-related params from a single module.

Well I did prefer us having a single entity to manage all gas-related stuff, but my definition for this single entity is pretty broad -- the combined group of "gas_schedule" & "storage" gas is conceptually still a single entity to me ;)

With that being said I remember there were some technical issues preventing us from having everything in the same module in the first place, that is, modules cannot have cyclic dependencies and I have a feeling that the same issue also applies here.

Correct me if I'm wrong:

When we update the gas schedule, we want to call reconfigure(), so the module this entry point is in needs to depend on reconfiguration. However, storage_gas has an on_reconfig event so the reconfiguration module needs to depend on it. Therefore we cannot really call reconfigure() in storage_gas, but have to do it in a different module...

@vgao1996 is correct about the reconfig. Yes, we want reconfig after set_storage_gas_config but we cannot do it inside storage_gas. we can only set_config inside.

movekevin · 2022-09-15T20:03:48Z

aptos-move/framework/aptos-framework/sources/storage_gas.move

+        *borrow_global_mut<StorageGasConfig>(@aptos_framework) = config;
+    }
+
+    public(friend) fun initialize(aptos_framework: &signer) {


This is not compatible with LLT as we'd not redo genesis. @areshand @wrwg Do you know if a module's initialize function would be called on a package republish if the module is brand new?

uhhhh, we definitely need a way to initialize the module. But what do you mean by republish?

alright. It seems I have to rename it as "init_module"

What you should be able to do is add an init_module which calls this function here. This should cover the case when the module is loaded the first time during upgrade, whereas this function covers genesis.

movekevin · 2022-09-15T23:32:52Z

aptos-move/framework/aptos-framework/sources/state_storage.move

    }

    public(friend) fun on_new_block(epoch: u64) acquires StateStorageUsage {
+        assert!(


Why add these checks now? If this fails, it crashes the entire network

if the check doesn't fail, the next line will also fail, right?

movekevin · 2022-09-15T23:33:32Z

aptos-move/framework/aptos-framework/sources/state_storage.move

+        usage: Usage,
+    }
+
+    public(friend) fun on_reconfig() {


You can delete friend-only functions now I think. cc @wrwg

let me try.

movekevin · 2022-09-15T23:33:41Z

aptos-move/framework/aptos-framework/sources/state_storage.move

+    }
+
+    // ======================== deprecated ============================
+    friend aptos_framework::reconfiguration;


Can we just remove this then?

it breaks the module_upgrade_test

Weird cc @wrwg. I'm not aware of friend statements being checked in compatibility validation

The friend_a_private feature is merged, but the feature isn't on by default. And it can't be turned on before rolled out, strictly spoken (because of replay).

For now any changes to friend function or module declarations will cause the upgrade to fail. But we can clean this once we upgraded testnet. So keep everything for now, remove later.

I think it changes visibility. It took me a while to figure it out.

The friend M decls need to be a superset of the old module. With the new feature this check is not longer happening, but you can't use that yet.

movekevin · 2022-09-15T23:35:43Z

aptos-move/framework/aptos-framework/sources/storage_gas.move

+    friend aptos_framework::genesis;
+    friend aptos_framework::reconfiguration;
+
+    const ESTORAGE_GAS_CONFIG: u64 = 0;


These are not visible to end users, but can we still add comments here explaining what these error codes are for?

to me it is quite self-explanatory. But I can add some.

aptos-move/framework/aptos-framework/sources/storage_gas.move

movekevin · 2022-09-15T23:37:02Z

aptos-move/framework/aptos-framework/sources/storage_gas.move

+        *borrow_global_mut<StorageGasConfig>(@aptos_framework) = config;
+    }
+
+    fun init_module(aptos_framework: &signer) {


Did you verify that this works in the context of a package upgrade?

Is there an easy way to do that?
is that something fn init_module() tries to do?

aptos-move/framework/aptos-framework/sources/storage_gas.move

movekevin · 2022-09-15T23:39:50Z

aptos-move/framework/aptos-framework/sources/storage_gas.move

+        calculate_gas(config.target_usage, usage, &config.write_curve)
+    }
+
+    public(friend) fun on_reconfig() acquires StorageGas, StorageGasConfig {


Can this fail? If this fails, it crashes the network

all the code triggered by reconfig will crash the network if fails. I think we avoid all the possible overflow issues. But if you can anything risky, I am glad to discuss.

github-actions · 2022-09-16T00:00:37Z

Forge is running suite `land_blocking` on `82e3a4493d70d868862a9b7c4434f7e01e12398b`

Forge is running suite `compat` on `testnet` ==> `82e3a4493d70d868862a9b7c4434f7e01e12398b`

✅ Forge suite `land_blocking` success on `82e3a4493d70d868862a9b7c4434f7e01e12398b`

performance benchmark with full nodes : 7933 TPS, 4998 ms latency, 8700 ms p99 latency,no expired txns
Test Ok

✅ Forge suite `compat` success on `testnet` ==> `82e3a4493d70d868862a9b7c4434f7e01e12398b`

Compatibility test results for testnet ==> 82e3a4493d70d868862a9b7c4434f7e01e12398b (PR)
1. Check liveness of validators at old version: testnet
compatibility::simple-validator-upgrade::liveness-check : 8158 TPS, 4646 ms latency, 7300 ms p99 latency,no expired txns
2. Upgrading first Validator to new version: 82e3a4493d70d868862a9b7c4434f7e01e12398b
compatibility::simple-validator-upgrade::single-validator-upgrade : 6044 TPS, 5980 ms latency, 8000 ms p99 latency,no expired txns
3. Upgrading rest of first batch to new version: 82e3a4493d70d868862a9b7c4434f7e01e12398b
compatibility::simple-validator-upgrade::half-validator-upgrade : 5920 TPS, 6279 ms latency, 8500 ms p99 latency,no expired txns
4. upgrading second batch to new version: 82e3a4493d70d868862a9b7c4434f7e01e12398b
compatibility::simple-validator-upgrade::rest-validator-upgrade : 7603 TPS, 4657 ms latency, 7000 ms p99 latency,no expired txns
5. check swarm health
Compatibility test for testnet ==> 82e3a4493d70d868862a9b7c4434f7e01e12398b passed
Test Ok

Forge is running suite `compat` on `testnet` ==> `dca00b535428b63341fbc4733f2b2c30a1973b00`

Forge is running suite `land_blocking` on `dca00b535428b63341fbc4733f2b2c30a1973b00`

✅ Forge suite `land_blocking` success on `dca00b535428b63341fbc4733f2b2c30a1973b00`

performance benchmark with full nodes : 7599 TPS, 5221 ms latency, 7500 ms p99 latency,no expired txns
Test Ok

✅ Forge suite `compat` success on `testnet` ==> `dca00b535428b63341fbc4733f2b2c30a1973b00`

Compatibility test results for testnet ==> dca00b535428b63341fbc4733f2b2c30a1973b00 (PR)
1. Check liveness of validators at old version: testnet
compatibility::simple-validator-upgrade::liveness-check : 8270 TPS, 4524 ms latency, 8300 ms p99 latency,no expired txns
2. Upgrading first Validator to new version: dca00b535428b63341fbc4733f2b2c30a1973b00
compatibility::simple-validator-upgrade::single-validator-upgrade : 5807 TPS, 6176 ms latency, 8300 ms p99 latency,no expired txns
3. Upgrading rest of first batch to new version: dca00b535428b63341fbc4733f2b2c30a1973b00
compatibility::simple-validator-upgrade::half-validator-upgrade : 6128 TPS, 6016 ms latency, 7900 ms p99 latency,no expired txns
4. upgrading second batch to new version: dca00b535428b63341fbc4733f2b2c30a1973b00
compatibility::simple-validator-upgrade::rest-validator-upgrade : 7757 TPS, 4660 ms latency, 8800 ms p99 latency,no expired txns
5. check swarm health
Compatibility test for testnet ==> dca00b535428b63341fbc4733f2b2c30a1973b00 passed
Test Ok

movekevin

Approved pending the potential risk of storage_gas::on_reconfig() aborting. I double checked and didn't see any potential aborts beyond overflow/underflow. Would be great to have @vgao1996 and @davidiw double check this as well

lightmark · 2022-09-16T00:07:29Z

aptos-move/framework/aptos-framework/sources/storage_gas.move

+    fun calculate_gas(max_usage: u64, current_usage: u64, curve: &GasCurve): u64 {
+        let capped_current_usage = if (current_usage > max_usage) max_usage else current_usage;
+        let points = &curve.points;
+        let num_points = vector::length(points);
+        let current_usage_bps = capped_current_usage * BASIS_POINT_DENOMINATION / max_usage;
+
+        // Check the corner case that current_usage_bps drops before the first point.
+        let (left, right) = if (num_points == 0) {
+            (&Point {x: 0, y: 0}, &Point {x: BASIS_POINT_DENOMINATION, y: BASIS_POINT_DENOMINATION})
+        } else {
+            let (i, j) = (0, num_points - 1);
+            while (i < j) {
+                let mid = j - (j - i) / 2;
+                if (current_usage_bps < vector::borrow(points, mid).x) {
+                    j = mid - 1;
+                } else {
+                    i = mid;
+                };
+            };
+            if (current_usage_bps < vector::borrow(points, 0).x) {
+                (&Point {x: 0, y: 0}, vector::borrow(points, 0))
+            } else {
+                // Check the corner case that current_usage_bps drops after the last point.
+                (
+                    vector::borrow(points, i),
+                    if (i == num_points - 1) {
+                        &Point { x: BASIS_POINT_DENOMINATION, y: BASIS_POINT_DENOMINATION }
+                    } else {
+                        vector::borrow(points, i + 1)
+                    }
+                )
+            }
+        };
+
+        curve.min_gas + (curve.max_gas - curve.min_gas) * (left.y + (current_usage_bps - left.x) * (right.y - left.y) / (right.x - left.x)) / BASIS_POINT_DENOMINATION
+    }


@davidiw @vgao1996 Can our SMART smart contract engineers help review the code to reassure us that it will not abort in any cases?

line 209 would not overflow because, line 186 checks max_usage * BASIS_POINT_DENOMINATION <= u64::MAX

Please review the binary search logic is valid.

Please check 239 will not overflow or underflow( such as current_usage_bps - left.x).

Some background:
in validate_curve(), we check 0 <= point.x, point.y <= BASIS_POINT_DENOMINATION, x is strictly monotonically increasing while y is monotonically non-decreasing;
max_gas * BASIS_POINT_DENOMINATION <= u64::MAX.

Yeah when I read this part of the code previously I also checked that the code would not overflow/underflow

Unblocking... Haven't reviewed

msmouse · 2022-09-16T03:59:38Z

aptos-move/framework/aptos-framework/sources/storage_gas.move

+        let item_curve = base_8192_exponential_curve(10, 10000);
+        let byte_curve = base_8192_exponential_curve(1, 1000);


@lightmark This is wrong.. the base 8192 curve is for max / min = 100x, the base 1M curve is for 1000x..

But none the less, we need to update these together with @vgao1996 's initial gas parameters. @vgao1996 if I'm reading your pending diff correctly, we should set min_price for read to be 1000, which is the value of load_data.per_byte in the current schedule, right? And according to the discussion yesterday, the max will be 100x that, if we are going with the base 8192 curve.

We should set both in the same governance proposal.

And / or we should send another PR to make init_module set things to the "correct" numbers already?

lightmark requested review from msmouse and vgao1996 September 2, 2022 22:57

lightmark requested review from davidiw, movekevin and wrwg as code owners September 2, 2022 22:57

lightmark commented Sep 2, 2022

View reviewed changes

aptos-move/framework/aptos-framework/sources/state_storage.move Outdated Show resolved Hide resolved

lightmark force-pushed the storage_gas_curve branch from ef26764 to a5eaa0a Compare September 2, 2022 23:50

davidiw previously requested changes Sep 6, 2022

View reviewed changes

lightmark force-pushed the storage_gas_curve branch from a5eaa0a to b63c8f1 Compare September 9, 2022 01:23

lightmark changed the title ~~[storage_gas] implement a 1/4 arc curve for storage gas params~~ [storage_gas] implement customizable storage gas config Sep 9, 2022

lightmark force-pushed the storage_gas_curve branch from b63c8f1 to fef2ad1 Compare September 9, 2022 01:36

lightmark commented Sep 9, 2022

View reviewed changes

aptos-move/framework/aptos-framework/sources/storage_gas.move Outdated Show resolved Hide resolved

lightmark force-pushed the storage_gas_curve branch from fef2ad1 to ca4a16a Compare September 9, 2022 01:38

msmouse approved these changes Sep 9, 2022

View reviewed changes

msmouse self-requested a review September 9, 2022 15:49

wrwg reviewed Sep 9, 2022

View reviewed changes

lightmark force-pushed the storage_gas_curve branch from ca4a16a to 36120c6 Compare September 10, 2022 00:14

lightmark requested a review from areshand September 10, 2022 00:15

vgao1996 approved these changes Sep 12, 2022

View reviewed changes

msmouse requested a review from davidiw September 12, 2022 20:59

msmouse reviewed Sep 12, 2022

View reviewed changes

msmouse self-requested a review September 12, 2022 22:21

lightmark force-pushed the storage_gas_curve branch 4 times, most recently from 14690e6 to 82e3a44 Compare September 14, 2022 21:39

msmouse approved these changes Sep 15, 2022

View reviewed changes

lightmark enabled auto-merge (squash) September 15, 2022 19:24

movekevin requested changes Sep 15, 2022

View reviewed changes

lightmark force-pushed the storage_gas_curve branch from 82e3a44 to c5e483f Compare September 15, 2022 22:59

[storage_gas] implement customizable storage gas config

dca00b5

lightmark force-pushed the storage_gas_curve branch from c5e483f to dca00b5 Compare September 15, 2022 23:12

movekevin reviewed Sep 15, 2022

View reviewed changes

aptos-move/framework/aptos-framework/sources/storage_gas.move Show resolved Hide resolved

movekevin reviewed Sep 15, 2022

View reviewed changes

movekevin approved these changes Sep 16, 2022

View reviewed changes

lightmark commented Sep 16, 2022

View reviewed changes

lightmark merged commit 29f746b into main Sep 16, 2022

lightmark deleted the storage_gas_curve branch September 16, 2022 01:08

msmouse reviewed Sep 16, 2022

View reviewed changes

		let item_curve = base_8192_exponential_curve(10, 10000);
		let byte_curve = base_8192_exponential_curve(1, 1000);

[storage_gas] implement customizable storage gas config #3810

[storage_gas] implement customizable storage gas config #3810

Conversation

lightmark commented Sep 2, 2022 • edited

Description

Test Plan

davidiw left a comment • edited

Choose a reason for hiding this comment

wrwg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vgao1996 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vgao1996 Sep 12, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Sep 16, 2022

Forge is running suite land_blocking on 82e3a4493d70d868862a9b7c4434f7e01e12398b

Forge is running suite compat on testnet ==> 82e3a4493d70d868862a9b7c4434f7e01e12398b

✅ Forge suite land_blocking success on 82e3a4493d70d868862a9b7c4434f7e01e12398b

✅ Forge suite compat success on testnet ==> 82e3a4493d70d868862a9b7c4434f7e01e12398b

Forge is running suite compat on testnet ==> dca00b535428b63341fbc4733f2b2c30a1973b00

Forge is running suite land_blocking on dca00b535428b63341fbc4733f2b2c30a1973b00

✅ Forge suite land_blocking success on dca00b535428b63341fbc4733f2b2c30a1973b00

✅ Forge suite compat success on testnet ==> dca00b535428b63341fbc4733f2b2c30a1973b00

movekevin left a comment

Choose a reason for hiding this comment

lightmark Sep 16, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lightmark commented Sep 2, 2022 •

edited

davidiw left a comment •

edited

vgao1996 Sep 12, 2022 •

edited

Forge is running suite `land_blocking` on `82e3a4493d70d868862a9b7c4434f7e01e12398b`

Forge is running suite `compat` on `testnet` ==> `82e3a4493d70d868862a9b7c4434f7e01e12398b`

✅ Forge suite `land_blocking` success on `82e3a4493d70d868862a9b7c4434f7e01e12398b`

✅ Forge suite `compat` success on `testnet` ==> `82e3a4493d70d868862a9b7c4434f7e01e12398b`

Forge is running suite `compat` on `testnet` ==> `dca00b535428b63341fbc4733f2b2c30a1973b00`

Forge is running suite `land_blocking` on `dca00b535428b63341fbc4733f2b2c30a1973b00`

✅ Forge suite `land_blocking` success on `dca00b535428b63341fbc4733f2b2c30a1973b00`

✅ Forge suite `compat` success on `testnet` ==> `dca00b535428b63341fbc4733f2b2c30a1973b00`

lightmark Sep 16, 2022 •

edited