[gas] introduce new mechanism to charge storage fees based on fixed APT prices & per-category gas/fee limits #6683

vgao1996 · 2023-02-17T21:56:03Z

This makes a few additions to the gas metering system:

This introduces a new mechanism to charge storage-related fees based on flat APT prices that doesn't fluctuate according to the gas fee market, which would eventually allow us to decouple the execution costs and the storage costs and lower the execution costs greatly.
This adds a few per-category gas/fee limits, which would allow us to increase the total amount of gas allowed without necessarily resulting in long execution time.

Note that I'm still waiting for @msmouse to come up with the correct prices. Should also mention that it might look like we're double charging (intrinsic, write set gas for io v.s. storage fees) but that is not the case. Going forward, the former group will cover transient costs only, leaving the more permanent portion to the latter. We will adjust the gas parameters to make sure we make transactions significantly cheaper, NOT more expensive.

Some of the storage fees will be replaced by deposits in the future: #6514

vgao1996 · 2023-02-17T22:01:26Z

...s-move/e2e-tests/goldens/language_e2e_testsuite__tests__verify_txn__test_open_publishing.exp

                        },
                    },
                ),
            ),
            events: [],
-            gas_used: 650,
+            gas_used: 50841,


This is expected because we set the gas unit price to 1 in this test. Shall we change it to 100 so that it looks more similar to the range of numbers we had?

aptos-move/aptos-gas/src/transaction/mod.rs

msmouse · 2023-02-17T23:43:48Z

aptos-move/aptos-gas/src/gas_meter.rs

+        );
+        let gas_consumed_internal = InternalGas::new(
+            if gas_consumed_internal > u64::MAX as u128 {
+                u64::MAX


hmm.... shall we put in a error!()?

Unless we put in some really crazy numbers in the gas schedule, this should not happen at all...

I can try to put in an error!, but I bet we'll run into some nasty cyclic dependency issues so I won't push it hard this time.

msmouse · 2023-02-17T23:52:57Z

aptos-move/aptos-gas/src/transaction/mod.rs

+        let mut fee = Fee::zero();
+        let mut new_slots = NumSlots::zero();
+
+        for (key, op) in ops {


This loop is fairly similar to calculate_write_set_gas(), can we combine them? -- let calculate_write_set_gas() return (io_gas, storage_fee) -- I think it's better that way because it'll be very obvious for what we charge both the io and the storage and for what we charge io only, etc.

Tried. That didn't work well.

charge_write_set_gas and charge_storage_fee use different contexts (StorageGas vs TransactionGasParameters), so combining them would require you to create a function that takes in both, which seems unnecessarily complicated.

It would also make the versioning logic more messy.

types/src/contract_event.rs

types/src/event.rs

msmouse · 2023-02-18T00:03:30Z

The storage fees will be replaced by deposits in the future: #6514

Not exactly :-P
The deletion refund will be on top of this: The storage_fee_per_slot_create fee will be collected to the global storage deposit and subject to refund, but other fees will still go with other parts of the gas and be burned or redistributed as we see proper.

vgao1996 · 2023-02-18T00:51:44Z

@msmouse man you are dead serious about this. I've updated the description to say that only some of the fees will be replaced by deposits.

movekevin · 2023-02-22T03:56:20Z

aptos-move/aptos-vm/src/aptos_vm.rs

@@ -265,7 +265,13 @@ impl AptosVM {
            .finish(&mut (), gas_meter.change_set_configs())
            .map_err(|e| e.into_vm_status())?;
        // Charge gas for write set
-        gas_meter.charge_write_set_gas(user_txn_change_set_ext.write_set().iter())?;
+        gas_meter.charge_write_set_gas_for_io(user_txn_change_set_ext.write_set().iter())?;
+        gas_meter.charge_storage_fee(


This needs to be gated

It's gated inside charge_write_set_gas_for_io and charge_storage_fee.

movekevin · 2023-02-22T03:58:33Z

aptos-move/aptos-gas/src/gas_meter.rs

+        self.charge(amount)?;
+
+        self.io_gas_used += amount;
+        if self.feature_version >= 7 && self.io_gas_used > self.gas_params.txn.max_io_gas {


Can this break upgradability of aptos-framework package? We already have to add a mechanism to bypass tx size limit. Now that there's an io gas limit, how much headroom do we have before we need to bypass that limit as well?

This is a great question but conceptually, I don't think this is going to break framework upgrades. Our goal here is to reduce transaction costs, not increasing them. Having separate limits for each individual limits will likely result in more transactions being allowed -- old transactions should be able to pass as long as we configure the new limits no stricter than the old one.

movekevin · 2023-02-22T04:02:08Z

aptos-move/aptos-gas/src/transaction/mod.rs

+        [
+            storage_fee_per_state_slot_create: FeePerSlot,
+            { 7.. => "storage_fee_per_state_slot_create" },
+            50000,


How did you come up with these numbers?

@msmouse has a doc that explains the logic.

This appears to be a significant change. How should we update our docs to reflect it? Please let me help. Thanks!

@clay-aptos thank you so much for helping! We're still in the process of figuring out the actual value changes, but I anticipate the doc work to cover the following:

Explaining the new system to our users, especially how the flat-APT prices are converted back to gas units to maintain compatibility

Making it clear that this is part of the effort to reduce cost (certainly not increasing it)

What would come in the future (deposits)

Adding the release-notes tag so we consider highlighting this.

movekevin

Can we do benchmarking here? I'd also suggest a replay/analysis of past testnet/mainnet txs to understand how many txs would hit these limit.

zekun000 · 2023-02-24T05:43:22Z

aptos-move/aptos-gas/src/aptos_framework.rs

@@ -104,7 +104,7 @@ crate::natives::define_gas_parameters_for_natives!(GasParameters, "aptos_framewo
    [.code.request_publish.per_byte, "code.request_publish.per_byte", 2 * MUL],

    // Note(Gas): These are storage operations so the values should not be multiplied.
-    [.event.write_to_event_store.base, "event.write_to_event_store.base", 500_000],
+    [.event.write_to_event_store.base, "event.write_to_event_store.base", 300_000],


is this intended?

Yes, this is set according to Alden's doc. Although it should be clear that we still haven't finalized those values and they are subject to change

zekun000 · 2023-02-24T05:51:40Z

crates/transaction-emitter-lib/src/emitter/mod.rs

@@ -41,7 +41,7 @@ use std::{
 use tokio::{runtime::Handle, task::JoinHandle, time};

 // Max is 100k TPS for a full day.
-const MAX_TXNS: u64 = 10_000_000_000;
+const MAX_TXNS: u64 = 100_000_000;


this seems unnecessary change after updating the MAX_GAS_AMOUNT?

davidiw

can we do a quick cleanup of the numbers? so that we're roughly back at the same state we were before?

davidiw · 2023-02-24T07:09:54Z

aptos-move/aptos-gas/src/transaction/mod.rs

@@ -45,7 +53,7 @@ crate::params::define_gas_parameters!(
        [
            maximum_number_of_gas_units: Gas,
            "maximum_number_of_gas_units",
-            2_000_000
+            200_000_000


why did we increase this? where is the maximum execution units? I thought we were focusing on max execution units and then max gas units would be the additional available for storage?

My bad, it looks like I forgot to replace this with MAX_GAS_AMOUNT, which is now gated by a cargo feature flag.

zekun000

awesome work!

github-actions · 2023-03-01T19:05:41Z

✅ Forge suite `land_blocking` success on `efd350b507d4a690c6ee1c341dfc534ff5550a4e`

performance benchmark with full nodes : 5932 TPS, 6681 ms latency, 13500 ms p99 latency,(!) expired 20 out of 2533140 txns
Test Ok

github-actions · 2023-03-01T19:06:52Z

✅ Forge suite `compat` success on `testnet_2d8b1b57553d869190f61df1aaf7f31a8fc19a7b` ==> `efd350b507d4a690c6ee1c341dfc534ff5550a4e`

Compatibility test results for testnet_2d8b1b57553d869190f61df1aaf7f31a8fc19a7b ==> efd350b507d4a690c6ee1c341dfc534ff5550a4e (PR)
1. Check liveness of validators at old version: testnet_2d8b1b57553d869190f61df1aaf7f31a8fc19a7b
compatibility::simple-validator-upgrade::liveness-check : 7670 TPS, 5011 ms latency, 6700 ms p99 latency,no expired txns
2. Upgrading first Validator to new version: efd350b507d4a690c6ee1c341dfc534ff5550a4e
compatibility::simple-validator-upgrade::single-validator-upgrade : 4919 TPS, 8390 ms latency, 11800 ms p99 latency,no expired txns
3. Upgrading rest of first batch to new version: efd350b507d4a690c6ee1c341dfc534ff5550a4e
compatibility::simple-validator-upgrade::half-validator-upgrade : 4633 TPS, 8654 ms latency, 11800 ms p99 latency,no expired txns
4. upgrading second batch to new version: efd350b507d4a690c6ee1c341dfc534ff5550a4e
compatibility::simple-validator-upgrade::rest-validator-upgrade : 6886 TPS, 5638 ms latency, 9400 ms p99 latency,no expired txns
5. check swarm health
Compatibility test for testnet_2d8b1b57553d869190f61df1aaf7f31a8fc19a7b ==> efd350b507d4a690c6ee1c341dfc534ff5550a4e passed
Test Ok

github-actions · 2023-03-01T19:09:04Z

❌ Forge suite `framework_upgrade` failure on `cb4ba0a57c998c60cbab65af31a64875d2588ca5` ==> `efd350b507d4a690c6ee1c341dfc534ff5550a4e`

Forge test runner terminated:
Trailing Log Lines:
Compiling, may take a little while to download git dependencies...
FETCHING GIT DEPENDENCY https://github.com/aptos-labs/aptos-core.git
INCLUDING DEPENDENCY AptosFramework
INCLUDING DEPENDENCY AptosStdlib
INCLUDING DEPENDENCY MoveStdlib
BUILDING RunScript
thread 'main' panicked at 'called `Result::unwrap()` on an `Err` value: Loading or casting u16, u32, u256 integers not supported in bytecode version 5', /usr/local/cargo/git/checkouts/move-0639dd674f581c30/5fcfb4e/language/move-compiler/src/compiled_unit.rs:202:18
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
{"level":"INFO","source":{"package":"aptos_forge","file":"testsuite/forge/src/backend/k8s/cluster_helper.rs:281"},"thread_name":"main","hostname":"forge-framework-upgrade-pr-6683-1677696501-cb4ba0a57c998c60cbab","timestamp":"2023-03-01T19:08:56.845591Z","message":"Deleting namespace forge-framework-upgrade-pr-6683: Some(NamespaceStatus { phase: Some(\"Terminating\") })"}
{"level":"INFO","source":{"package":"aptos_forge","file":"testsuite/forge/src/backend/k8s/cluster_helper.rs:389"},"thread_name":"main","hostname":"forge-framework-upgrade-pr-6683-1677696501-cb4ba0a57c998c60cbab","timestamp":"2023-03-01T19:08:56.845615Z","message":"aptos-node resources for Forge removed in namespace: forge-framework-upgrade-pr-6683"}
Debugging output:
NAME                                    READY   STATUS      RESTARTS   AGE
aptos-node-0-validator-0                1/1     Running     0          10m
aptos-node-1-validator-0                1/1     Running     0          8m51s
aptos-node-2-validator-0                1/1     Running     0          7m1s
aptos-node-3-validator-0                1/1     Running     0          5m11s
aptos-node-4-validator-0                1/1     Running     0          3m20s
genesis-aptos-genesis-eforge108-nptp6   0/1     Completed   0          18m

vgao1996 requested review from msmouse and grao1991 February 17, 2023 21:56

vgao1996 requested review from davidiw, wrwg, zekun000, georgemitenkov and gelash as code owners February 17, 2023 21:56

vgao1996 commented Feb 17, 2023

View reviewed changes

vgao1996 changed the title ~~[gas] introduce new machanism to charge for storage fees based on fixed APT costs~~ [gas] introduce new machanism to charge storage fees based on fixed APT costs Feb 17, 2023

vgao1996 changed the title ~~[gas] introduce new machanism to charge storage fees based on fixed APT costs~~ [gas] introduce new machanism to charge storage fees based on fixed APT prices Feb 17, 2023

vgao1996 force-pushed the storage-gas-fees branch from 0c4c309 to bf56dd6 Compare February 17, 2023 22:07

grao1991 reviewed Feb 17, 2023

View reviewed changes

aptos-move/aptos-gas/src/transaction/mod.rs Outdated Show resolved Hide resolved

vgao1996 force-pushed the storage-gas-fees branch from bf56dd6 to 2107352 Compare February 17, 2023 22:24

msmouse reviewed Feb 18, 2023

View reviewed changes

vgao1996 changed the title ~~[gas] introduce new machanism to charge storage fees based on fixed APT prices~~ [gas] introduce new machanism to charge storage fees based on fixed APT prices & per-category gas/fee limits Feb 21, 2023

grao1991 approved these changes Feb 21, 2023

View reviewed changes

vgao1996 force-pushed the storage-gas-fees branch from d8607fe to 8b33806 Compare February 22, 2023 00:27

vgao1996 requested review from clay-aptos, saharct, gregnazario and movekevin as code owners February 22, 2023 00:27

vgao1996 force-pushed the storage-gas-fees branch from 8b33806 to e600470 Compare February 22, 2023 00:36

movekevin reviewed Feb 22, 2023

View reviewed changes

vgao1996 force-pushed the storage-gas-fees branch 2 times, most recently from 5e459af to 6eb71e7 Compare February 22, 2023 20:02

zekun000 reviewed Feb 24, 2023

View reviewed changes

davidiw reviewed Feb 24, 2023

View reviewed changes

vgao1996 force-pushed the storage-gas-fees branch from 803adb2 to eaa21f0 Compare February 24, 2023 18:04

This comment has been minimized.

Sign in to view

zekun000 approved these changes Feb 24, 2023

View reviewed changes

This comment has been minimized.

Sign in to view

vgao1996 added 2 commits February 24, 2023 19:19

[gas] introduce new machanism to charge for storage fees (separately)

327d2fa

[gas] introduce per-category gas/fee limits

efd350b

vgao1996 force-pushed the storage-gas-fees branch from eaa21f0 to efd350b Compare February 24, 2023 19:19

vgao1996 enabled auto-merge (squash) February 24, 2023 19:28

This comment has been minimized.

Sign in to view

vgao1996 merged commit 0a2aba9 into aptos-labs:main Feb 24, 2023

msmouse mentioned this pull request Mar 1, 2023

[AIP-17][Discussion] Reducing Execution Costs by Decoupling Transaction Storage and Execution Charges aptos-foundation/AIPs#79

Closed

clay-aptos added the release-notes This tag indicates the associated change should be documented in the Aptos Release Notes. label Mar 1, 2023

This comment has been minimized.

Sign in to view

vgao1996 mentioned this pull request Mar 2, 2023

[gas] infrastructure for gas profiling #6535

Merged

[gas] introduce new mechanism to charge storage fees based on fixed APT prices & per-category gas/fee limits #6683

[gas] introduce new mechanism to charge storage fees based on fixed APT prices & per-category gas/fee limits #6683

Conversation

vgao1996 commented Feb 17, 2023 • edited Loading

vgao1996 Feb 17, 2023 • edited Loading

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

msmouse commented Feb 18, 2023 • edited Loading

vgao1996 commented Feb 18, 2023 • edited Loading

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

movekevin 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

davidiw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

zekun000 left a comment

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

github-actions bot commented Mar 1, 2023

✅ Forge suite land_blocking success on efd350b507d4a690c6ee1c341dfc534ff5550a4e

github-actions bot commented Mar 1, 2023

✅ Forge suite compat success on testnet_2d8b1b57553d869190f61df1aaf7f31a8fc19a7b ==> efd350b507d4a690c6ee1c341dfc534ff5550a4e

github-actions bot commented Mar 1, 2023

❌ Forge suite framework_upgrade failure on cb4ba0a57c998c60cbab65af31a64875d2588ca5 ==> efd350b507d4a690c6ee1c341dfc534ff5550a4e

vgao1996 commented Feb 17, 2023 •

edited

Loading

vgao1996 Feb 17, 2023 •

edited

Loading

msmouse commented Feb 18, 2023 •

edited

Loading

vgao1996 commented Feb 18, 2023 •

edited

Loading

✅ Forge suite `land_blocking` success on `efd350b507d4a690c6ee1c341dfc534ff5550a4e`

✅ Forge suite `compat` success on `testnet_2d8b1b57553d869190f61df1aaf7f31a8fc19a7b` ==> `efd350b507d4a690c6ee1c341dfc534ff5550a4e`

❌ Forge suite `framework_upgrade` failure on `cb4ba0a57c998c60cbab65af31a64875d2588ca5` ==> `efd350b507d4a690c6ee1c341dfc534ff5550a4e`