Rework gas logic #12676

dariorussi · 2023-06-25T19:33:17Z

Description

This is a refactor of the gas logic that better isolates version checks and gas operations.
The change looks bigger than it is because of deletion and protocol changes.

Gas model v1 has been deleted entirely given it was a pre-mainnet solution. That removed code for the gas model v1, a bunch of code in TemporaryStore, some tests, and a bit of cleanup in several spots.
crates/sui-cost-tables has been removed and the 2 files needed have been moved in crates/sui-types; that does not make any change in dependencies so it's good.

We introduced a GasCharger which is the main instance that tracks everything about gas in transactions.
Gas objects are also "embedded" in there, so that is the only object that is passed around during execution.
We can and should clean up a bit more around system vs users transactions but this is already a decent step forward, and that clean up is non trivial, so better have that in a different PR.

I still have to clean up tests and introduce a better model for gas testing. They are brittle and not easy to manage still. In another PR though.

We also introduce a new protocol version that protects towards a new check on the budget to verify that it is greater than the minimum charge (the lowest bucket of execution). That does not change the cost of any successful transaction, it will simply not allow to pay under minimum cost, run out of gas anyway, but still run (with all implications). Now transactions are verified at signing time that the budget is over the minimum.

Test Plan

Existing tests, few adjusted for minimum budget that has changed.
I will sync a full node from genesis both for mainnet and testnet to verify that no fork has been introduced.
I will also run a node that performs conservation checks to make sure things are good.

If your changes are not user-facing and not a breaking change, you can skip the following section. Otherwise, please indicate what changed, and then add to the Release Notes section as highlighted during the release process.

Type of Change (Check all that apply)

protocol change
user-visible impact
breaking change for a client SDKs
breaking change for FNs (FN binary must upgrade)
breaking change for validators or node operators (must upgrade binaries)
breaking change for on-chain data layout
necessitate either a data wipe or data migration

Release notes

Minimum budget for gas has changed to be the same as the minimum charge.
Before it was possible to provide a budget less than the minimum transaction charge, in which case the transaction was guaranteed to fail for insufficient gas, however gas coins would still be combined together, when more than one was provided. Effectively it was possible to get a merge operation under the minimum charge.
With the current changes a transaction will only be accepted if the budget covers for the minimum charges.
It is important to point out that any and every transaction that was successful before it will still be so with no change in gas. In other words the breaking nature of this change is only over certain transactions that failed with "insufficient gas" before and now would not be able to enter the system.

vercel · 2023-06-25T19:33:24Z

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

6 Ignored Deployments

Name	Status	Updated (UTC)
explorer	⬜️ Ignored (Inspect)	Jul 4, 2023 4:26pm
explorer-storybook	⬜️ Ignored (Inspect)	Jul 4, 2023 4:26pm
multisig-toolkit	⬜️ Ignored (Inspect)	Jul 4, 2023 4:26pm
sui-kiosk	⬜️ Ignored (Inspect)	Jul 4, 2023 4:26pm
sui-wallet-kit	⬜️ Ignored (Inspect)	Jul 4, 2023 4:26pm
wallet-adapter	⬜️ Ignored (Inspect)	Jul 4, 2023 4:26pm

dariorussi · 2023-06-26T16:13:40Z

Cargo.toml

@@ -384,7 +383,7 @@ move-disassembler = { path = "external-crates/move/tools/move-disassembler" }
 move-package = { path = "external-crates/move/tools/move-package" }
 move-unit-test = { path = "external-crates/move/tools/move-unit-test" }
 move-vm-config = { path = "external-crates/move/move-vm/config" }
-move-vm-test-utils = { path = "external-crates/move/move-vm/test-utils" }
+move-vm-test-utils = { path = "external-crates/move/move-vm/test-utils", features = ["tiered-gas"] }


@tzakian anything we should change here now that we only have tiered gas, or is it?

This is fine to leave as-is IMO. Another option could be to make this the default and the non-tiered model on the Move side the non-default, although I don't think that really buys anything. Longer-term the gas meter for unit testing should be trait-ified just like the rest of the gas system and then we shouldn't need this...

dariorussi · 2023-06-26T19:50:44Z

crates/transaction-fuzzer/src/account_universe/transfer_gen.rs

-        Just(PROTOCOL_CONFIG.base_tx_cost_fixed() - 1),
-        Just(PROTOCOL_CONFIG.base_tx_cost_fixed()),
-        Just(PROTOCOL_CONFIG.base_tx_cost_fixed() + 1),
+        PROTOCOL_CONFIG.base_tx_cost_fixed() / 2..=PROTOCOL_CONFIG.base_tx_cost_fixed() * 2000,


@tzakian take a look here, minimum cost is now a variable of the gas price so we cannot make it a hard number.
I think the logic here is still fine as it will pick a random number in a range that is good or bas and it's the same logic as before. Before it was very well defined and precise, now we cannot

This looks fine to me! The main thing here was to test the boundary conditions around the lower bound which this seems to still be doing, so I'm happy with it :)

lxfind

It's quite hard to see all the places where we depend on the gas_model_version.
The cleanups are on the right direction, but I think we should move all the functions that checks on gas_model_version into the same place, either in a dedicated file or in protocol config file. That way you can have an easy systematic view on what gets enabled/disabled in what gas_model_version.

dariorussi · 2023-06-26T22:59:40Z

It's quite hard to see all the places where we depend on the gas_model_version. The cleanups are on the right direction, but I think we should move all the functions that checks on gas_model_version into the same place, either in a dedicated file or in protocol config file. That way you can have an easy systematic view on what gets enabled/disabled in what gas_model_version.

I am going to try that and we'll see how it looks.
I considered it but then I felt you will have checks that are very specific to the code you are running and having them locally felt more natural, but we'll see how it looks

emmazzz · 2023-06-27T05:40:24Z

crates/sui-json-rpc/src/unit_tests/rpc_server_tests.rs

@@ -110,7 +110,7 @@ async fn test_public_transfer_object() -> Result<(), anyhow::Error> {
    let gas = objects.clone().last().unwrap().object().unwrap().object_id;

    let transaction_bytes: TransactionBlockBytes = http_client
-        .transfer_object(address, obj, Some(gas), 10_000.into(), address)


I find it hard to believe that these tests worked with gas budget set to 10000. I would imagine the transaction would fail with out of gas. Did the transactions actually succeed? Or were we using dummy RGP of 1 MIST or something?

that is correct, a bunch of tests simply failed, and happily moved along.
I was pretty shocked too if that is what you mean, and maybe the indication that we need a bit more attention around our testing infra...

dariorussi · 2023-06-27T15:39:06Z

added a gas_predicate.rs file to unify protocol version related "predicates" as suggested by @lxfind

dariorussi · 2023-06-27T15:42:43Z

for @tzakian and for @oxade to confirm, the deleted limits test for v1 are there also for later versions, so we should be able to delete them just fine

tzakian

Overall looks great to me! A couple smaller things, but the main thing is those panics when smashing gas that we discussed...they're still making my palms sweaty, but I'm honestly not sure what the better option is there.

tzakian · 2023-06-28T15:15:46Z

Cargo.toml

@@ -384,7 +383,7 @@ move-disassembler = { path = "external-crates/move/tools/move-disassembler" }
 move-package = { path = "external-crates/move/tools/move-package" }
 move-unit-test = { path = "external-crates/move/tools/move-unit-test" }
 move-vm-config = { path = "external-crates/move/move-vm/config" }
-move-vm-test-utils = { path = "external-crates/move/move-vm/test-utils" }
+move-vm-test-utils = { path = "external-crates/move/move-vm/test-utils", features = ["tiered-gas"] }


This is fine to leave as-is IMO. Another option could be to make this the default and the non-tiered model on the Move side the non-default, although I don't think that really buys anything. Longer-term the gas meter for unit testing should be trait-ified just like the rest of the gas system and then we shouldn't need this...

tzakian · 2023-06-28T15:36:31Z

crates/sui-types/src/gas.rs

+            // transaction and certificate input checks must have insured that all gas coins
+            // are valid
+            .unwrap_or_else(|_| {
+                panic!(


So conflicted on this...

From a purely-subjective-this-would-make-my-palms-sweat-less standpoint I'd prefer invariant violations here, but we've discussed and these being invariant violations/panics here doesn't really change anything (currently). I'd probably lean towards invariant violations since it seems weird to be creating them in the map, but then just immediately tossing them away and panicking. Either that or change the invariant violations in the map to panic!s as well.

yeah we talked about it and let me ask you, if we went with the invariant violation now (as we discussed we want to got there eventually) what would we do? still panicking but up the stack?
also (and we talked about this already) just wanted to point out that some of the panics there, were also there in the previous version. They simply used to unwrap before and now the unwrap_or and panicking with more proper information.

tzakian · 2023-06-28T15:56:32Z

crates/transaction-fuzzer/src/account_universe/transfer_gen.rs

-        Just(PROTOCOL_CONFIG.base_tx_cost_fixed() - 1),
-        Just(PROTOCOL_CONFIG.base_tx_cost_fixed()),
-        Just(PROTOCOL_CONFIG.base_tx_cost_fixed() + 1),
+        PROTOCOL_CONFIG.base_tx_cost_fixed() / 2..=PROTOCOL_CONFIG.base_tx_cost_fixed() * 2000,


This looks fine to me! The main thing here was to test the boundary conditions around the lower bound which this seems to still be doing, so I'm happy with it :)

tzakian · 2023-06-28T16:04:34Z

crates/sui-types/src/temporary_store.rs

@@ -1385,6 +955,7 @@ impl<'backing> TemporaryStore<'backing> {
    /// amount of SUI. We need these information for conservation check.
    pub fn check_sui_conserved(
        &self,
+        gas_summary: &GasCostSummary,


it is used, line 1070 and below, no?

this change (from #12676) broke compatibility since it was not gated - if the change was necessary we will have to put in a feature flag

## Description This is a refactor of the gas logic that better isolates version checks and gas operations. The change looks bigger than it is because of deletion and protocol changes. Gas model v1 has been deleted entirely given it was a pre-mainnet solution. That removed code for the gas model v1, a bunch of code in `TemporaryStore`, some tests, and a bit of cleanup in several spots. `crates/sui-cost-tables` has been removed and the 2 files needed have been moved in `crates/sui-types`; that does not make any change in dependencies so it's good. We introduced a `GasCharger` which is the main instance that tracks everything about gas in transactions. Gas objects are also "embedded" in there, so that is the only object that is passed around during execution. We can and should clean up a bit more around system vs users transactions but this is already a decent step forward, and that clean up is non trivial, so better have that in a different PR. I still have to clean up tests and introduce a better model for gas testing. They are brittle and not easy to manage still. In another PR though. We also introduce a new protocol version that protects towards a new check on the budget to verify that it is greater than the minimum charge (the lowest bucket of execution). That does not change the cost of any successful transaction, it will simply not allow to pay under minimum cost, run out of gas anyway, but still run (with all implications). Now transactions are verified at signing time that the budget is over the minimum. ## Test Plan Existing tests, few adjusted for minimum budget that has changed. I will sync a full node from genesis both for mainnet and testnet to verify that no fork has been introduced. I will also run a node that performs conservation checks to make sure things are good. --- If your changes are not user-facing and not a breaking change, you can skip the following section. Otherwise, please indicate what changed, and then add to the Release Notes section as highlighted during the release process. ### Type of Change (Check all that apply) - [ ] protocol change - [ ] user-visible impact - [ ] breaking change for a client SDKs - [ ] breaking change for FNs (FN binary must upgrade) - [ ] breaking change for validators or node operators (must upgrade binaries) - [ ] breaking change for on-chain data layout - [ ] necessitate either a data wipe or data migration ### Release notes

this change (from #12676) broke compatibility since it was not gated - if the change was necessary we will have to put in a feature flag

## Description This is a refactor of the gas logic that better isolates version checks and gas operations. The change looks bigger than it is because of deletion and protocol changes. Gas model v1 has been deleted entirely given it was a pre-mainnet solution. That removed code for the gas model v1, a bunch of code in `TemporaryStore`, some tests, and a bit of cleanup in several spots. `crates/sui-cost-tables` has been removed and the 2 files needed have been moved in `crates/sui-types`; that does not make any change in dependencies so it's good. We introduced a `GasCharger` which is the main instance that tracks everything about gas in transactions. Gas objects are also "embedded" in there, so that is the only object that is passed around during execution. We can and should clean up a bit more around system vs users transactions but this is already a decent step forward, and that clean up is non trivial, so better have that in a different PR. I still have to clean up tests and introduce a better model for gas testing. They are brittle and not easy to manage still. In another PR though. We also introduce a new protocol version that protects towards a new check on the budget to verify that it is greater than the minimum charge (the lowest bucket of execution). That does not change the cost of any successful transaction, it will simply not allow to pay under minimum cost, run out of gas anyway, but still run (with all implications). Now transactions are verified at signing time that the budget is over the minimum. ## Test Plan Existing tests, few adjusted for minimum budget that has changed. I will sync a full node from genesis both for mainnet and testnet to verify that no fork has been introduced. I will also run a node that performs conservation checks to make sure things are good. --- If your changes are not user-facing and not a breaking change, you can skip the following section. Otherwise, please indicate what changed, and then add to the Release Notes section as highlighted during the release process. ### Type of Change (Check all that apply) - [ ] protocol change - [ ] user-visible impact - [ ] breaking change for a client SDKs - [ ] breaking change for FNs (FN binary must upgrade) - [ ] breaking change for validators or node operators (must upgrade binaries) - [ ] breaking change for on-chain data layout - [ ] necessitate either a data wipe or data migration ### Release notes

this change (from #12676) broke compatibility since it was not gated - if the change was necessary we will have to put in a feature flag

dariorussi force-pushed the regas branch 3 times, most recently from abae0b2 to aaf7c91 Compare June 25, 2023 22:30

dariorussi mentioned this pull request Jun 25, 2023

Line up min gas charges with min computation charges #12428

Closed

7 tasks

dariorussi force-pushed the regas branch from aaf7c91 to 3ea651d Compare June 26, 2023 16:11

dariorussi commented Jun 26, 2023

View reviewed changes

dariorussi force-pushed the regas branch from 3ea651d to 1fac934 Compare June 26, 2023 16:29

dariorussi marked this pull request as ready for review June 26, 2023 16:33

dariorussi requested review from lxfind, tzakian, sblackshear, emmazzz and oxade June 26, 2023 16:33

dariorussi commented Jun 26, 2023

View reviewed changes

dariorussi force-pushed the regas branch from 1fac934 to 543fc8b Compare June 26, 2023 20:25

lxfind reviewed Jun 26, 2023

View reviewed changes

dariorussi requested review from tnowacki and amnn June 26, 2023 23:00

emmazzz reviewed Jun 27, 2023

View reviewed changes

dariorussi force-pushed the regas branch 3 times, most recently from 54e5769 to 4d61029 Compare June 27, 2023 15:39

tzakian approved these changes Jun 28, 2023

View reviewed changes

dariorussi force-pushed the regas branch 3 times, most recently from fc04dc4 to 66e9ab1 Compare July 4, 2023 15:32

Rework gas logic

1e13f07

dariorussi force-pushed the regas branch from 66e9ab1 to 1e13f07 Compare July 4, 2023 16:26

dariorussi merged commit 8917ce4 into main Jul 4, 2023
35 checks passed

dariorussi deleted the regas branch July 4, 2023 17:16

mystenmark mentioned this pull request Jul 5, 2023

Revert change to system transaction gas data #12848

Merged

mystenmark added a commit that referenced this pull request Jul 5, 2023

Revert change to system transaction gas data (#12848)

d0c78f3

this change (from #12676) broke compatibility since it was not gated - if the change was necessary we will have to put in a feature flag

longbowlu pushed a commit that referenced this pull request Jul 6, 2023

Revert change to system transaction gas data (#12848)

4538d3f

this change (from #12676) broke compatibility since it was not gated - if the change was necessary we will have to put in a feature flag

ebmifa pushed a commit that referenced this pull request Jul 12, 2023

Revert change to system transaction gas data (#12848)

be9a63f

this change (from #12676) broke compatibility since it was not gated - if the change was necessary we will have to put in a feature flag

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rework gas logic #12676

Rework gas logic #12676

dariorussi commented Jun 25, 2023 •

edited

vercel bot commented Jun 25, 2023 •

edited

dariorussi Jun 26, 2023

tzakian Jun 28, 2023

dariorussi Jun 26, 2023

tzakian Jun 28, 2023

lxfind left a comment

dariorussi commented Jun 26, 2023

emmazzz Jun 27, 2023

dariorussi Jun 27, 2023

dariorussi commented Jun 27, 2023

dariorussi commented Jun 27, 2023

tzakian left a comment

tzakian Jun 28, 2023

tzakian Jun 28, 2023

dariorussi Jun 28, 2023

tzakian Jun 28, 2023

tzakian Jun 28, 2023

dariorussi Jun 28, 2023 •

edited

Rework gas logic #12676

Rework gas logic #12676

Conversation

dariorussi commented Jun 25, 2023 • edited

Description

Test Plan

Type of Change (Check all that apply)

Release notes

vercel bot commented Jun 25, 2023 • 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

lxfind left a comment

Choose a reason for hiding this comment

dariorussi commented Jun 26, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dariorussi commented Jun 27, 2023

dariorussi commented Jun 27, 2023

tzakian 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

dariorussi Jun 28, 2023 • edited

Choose a reason for hiding this comment

dariorussi commented Jun 25, 2023 •

edited

vercel bot commented Jun 25, 2023 •

edited

dariorussi Jun 28, 2023 •

edited