[gas] remove test_only and optional, and support versioned gas schedule #5794

vgao1996 · 2022-12-07T01:59:52Z

This makes a few improvements to the macros we use to generate the bindings between the Rust representation of the gas schedule and its on-chain form. Specifically, this

Removes test_only. test_only doubles the complexity of the macros and all it does is to allow us to assign non-zero gas costs to two test-only natives in move-stdlib, which isn't really necessary.
Replaces the optional marker with a new syntax that allows one to define a versioned gas schedule.

Here's how the versioned gas schedule work:

/* before */
"foo" // or
optional "foo"

/* after */
"foo" // means the entry is expected to be found in all versions, under key "foo"
{ 1.. => "foo" } // means the entry is expected to be found in version 1 and above, under key "foo"
{ 3..=4 => "foo" } // means the entry is expected to be found in version 3 and 4 under key "foo", but not above 3 or below 4
{ 1 => "foo", 2.. => "bar" } // means the entry is expected to be found under key "foo" in version, and under key "bar" in version 2 and above

This new system effectively allows one to rename or delete gas parameters.

movekevin · 2022-12-09T19:18:36Z

api/src/context.rs

@@ -912,14 +912,15 @@ impl Context {

            let gas_schedule_params =
                match GasScheduleV2::fetch_config(&storage_adapter).and_then(|gas_schedule| {
+                    let featrure_version = gas_schedule.feature_version;


nit: typo featrure_version => feature_version

Also nit: Why not just inline this if this var is only used once?

nit: typo featrure_version => feature_version

Thanks for catching this! I can't believe I made so many typos yesterday...

Also nit: Why not just inline this if this var is only used once?

Tried that. Borrow checker's not happy.

movekevin · 2022-12-09T19:19:43Z

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


    // Reusing SHA2-512's cost from Ristretto
-    [.hash.sha2_512.base, optional "hash.sha2_512.base", 3_240],
-    [.hash.sha2_512.per_byte, optional "hash.sha2_512.per_byte", 60],
+    [.hash.sha2_512.base, { 4.. => "hash.sha2_512.base" }, 3_240],


Does this mean 4 or above or exactly 4?

4 or above. It's basically a rust pattern

davidiw · 2022-12-11T03:39:27Z

api/src/context.rs

@@ -912,14 +912,15 @@ impl Context {

            let gas_schedule_params =
                match GasScheduleV2::fetch_config(&storage_adapter).and_then(|gas_schedule| {
+                    let featrure_version = gas_schedule.feature_version;


Suggested change

let featrure_version = gas_schedule.feature_version;

let feature_version = gas_schedule.feature_version;

davidiw · 2022-12-11T03:39:41Z

api/src/context.rs

                    let gas_schedule = gas_schedule.to_btree_map();
-                    AptosGasParameters::from_on_chain_gas_schedule(&gas_schedule)
+                    AptosGasParameters::from_on_chain_gas_schedule(&gas_schedule, featrure_version)


Suggested change

AptosGasParameters::from_on_chain_gas_schedule(&gas_schedule, featrure_version)

AptosGasParameters::from_on_chain_gas_schedule(&gas_schedule, feature_version)

github-actions · 2022-12-18T02:57:23Z

✅ Forge suite `land_blocking` success on `33c71d8d1aff23137735e39ff8d2fa802bb4422f`

performance benchmark with full nodes : 6359 TPS, 6236 ms latency, 12900 ms p99 latency,(!) expired 600 out of 2715900 txns
Test Ok

Grafana dashboard
Humio Logs
Test runner output
Test run is land-blocking

github-actions · 2022-12-18T02:59:53Z

✅ Forge suite `compat` success on `testnet_2d8b1b57553d869190f61df1aaf7f31a8fc19a7b` ==> `33c71d8d1aff23137735e39ff8d2fa802bb4422f`

Compatibility test results for testnet_2d8b1b57553d869190f61df1aaf7f31a8fc19a7b ==> 33c71d8d1aff23137735e39ff8d2fa802bb4422f (PR)
1. Check liveness of validators at old version: testnet_2d8b1b57553d869190f61df1aaf7f31a8fc19a7b
compatibility::simple-validator-upgrade::liveness-check : 7312 TPS, 5301 ms latency, 7300 ms p99 latency,no expired txns
2. Upgrading first Validator to new version: 33c71d8d1aff23137735e39ff8d2fa802bb4422f
compatibility::simple-validator-upgrade::single-validator-upgrade : 4638 TPS, 8779 ms latency, 11200 ms p99 latency,no expired txns
3. Upgrading rest of first batch to new version: 33c71d8d1aff23137735e39ff8d2fa802bb4422f
compatibility::simple-validator-upgrade::half-validator-upgrade : 4534 TPS, 8890 ms latency, 11700 ms p99 latency,no expired txns
4. upgrading second batch to new version: 33c71d8d1aff23137735e39ff8d2fa802bb4422f
compatibility::simple-validator-upgrade::rest-validator-upgrade : 6372 TPS, 6068 ms latency, 10300 ms p99 latency,no expired txns
5. check swarm health
Compatibility test for testnet_2d8b1b57553d869190f61df1aaf7f31a8fc19a7b ==> 33c71d8d1aff23137735e39ff8d2fa802bb4422f passed
Test Ok

Grafana dashboard
Humio Logs
Test runner output
Test run is land-blocking

vgao1996 requested review from davidiw, lightmark, alinush, msmouse and wrwg December 7, 2022 01:59

vgao1996 force-pushed the vgs branch from 05eb532 to 4fa6df7 Compare December 7, 2022 02:12

vgao1996 requested review from movekevin, zekun000, banool and gregnazario as code owners December 7, 2022 02:12

vgao1996 force-pushed the vgs branch 2 times, most recently from 693d085 to 517d911 Compare December 7, 2022 19:44

movekevin approved these changes Dec 9, 2022

View reviewed changes

davidiw approved these changes Dec 11, 2022

View reviewed changes

movekevin mentioned this pull request Dec 18, 2022

[crypto+move] add support for Blake2b-256 in Move #5436

Merged

vgao1996 added 2 commits December 18, 2022 01:59

[gas] remove test_only

f8801ec

[gas] versioned gas schedule -- params

fe436d6

vgao1996 force-pushed the vgs branch from 517d911 to 72045e2 Compare December 18, 2022 02:09

[gas] versioned gas schedule -- natives

33c71d8

vgao1996 force-pushed the vgs branch from 72045e2 to 33c71d8 Compare December 18, 2022 02:09

vgao1996 enabled auto-merge (squash) December 18, 2022 02:13

This comment has been minimized.

Sign in to view

vgao1996 merged commit 7f74006 into aptos-labs:main Dec 18, 2022

Markuze mentioned this pull request Dec 26, 2022

markuze/aptos perf #5998

Closed

Markuze mentioned this pull request Jan 3, 2023

[Network] Adding AptosPerf Client #6054

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[gas] remove test_only and optional, and support versioned gas schedule #5794

[gas] remove test_only and optional, and support versioned gas schedule #5794

vgao1996 commented Dec 7, 2022 •

edited

Loading

movekevin Dec 9, 2022

movekevin Dec 9, 2022

vgao1996 Dec 9, 2022

movekevin Dec 9, 2022

vgao1996 Dec 9, 2022

davidiw Dec 11, 2022

davidiw Dec 11, 2022

This comment has been minimized.

This comment has been minimized.

github-actions bot commented Dec 18, 2022

github-actions bot commented Dec 18, 2022

	let featrure_version = gas_schedule.feature_version;
	let feature_version = gas_schedule.feature_version;

	AptosGasParameters::from_on_chain_gas_schedule(&gas_schedule, featrure_version)
	AptosGasParameters::from_on_chain_gas_schedule(&gas_schedule, feature_version)

[gas] remove test_only and optional, and support versioned gas schedule #5794

[gas] remove test_only and optional, and support versioned gas schedule #5794

Conversation

vgao1996 commented Dec 7, 2022 • edited Loading

movekevin Dec 9, 2022

Choose a reason for hiding this comment

movekevin Dec 9, 2022

Choose a reason for hiding this comment

vgao1996 Dec 9, 2022

Choose a reason for hiding this comment

movekevin Dec 9, 2022

Choose a reason for hiding this comment

vgao1996 Dec 9, 2022

Choose a reason for hiding this comment

davidiw Dec 11, 2022

Choose a reason for hiding this comment

davidiw Dec 11, 2022

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

github-actions bot commented Dec 18, 2022

✅ Forge suite land_blocking success on 33c71d8d1aff23137735e39ff8d2fa802bb4422f

github-actions bot commented Dec 18, 2022

✅ Forge suite compat success on testnet_2d8b1b57553d869190f61df1aaf7f31a8fc19a7b ==> 33c71d8d1aff23137735e39ff8d2fa802bb4422f

vgao1996 commented Dec 7, 2022 •

edited

Loading

✅ Forge suite `land_blocking` success on `33c71d8d1aff23137735e39ff8d2fa802bb4422f`

✅ Forge suite `compat` success on `testnet_2d8b1b57553d869190f61df1aaf7f31a8fc19a7b` ==> `33c71d8d1aff23137735e39ff8d2fa802bb4422f`