[testsuite] CLI & smoke test for multi-step governance proposal #5975

0xchloe · 2022-12-22T02:29:25Z

Description

Background

This PR is part of multi-step governance proposal work. In this PR, we

added e2e and unit tests to test out the multi-step governance proposal flow e2e;
added CLI support for (1) submitting multi-step proposal and (2) generating execution hash for a script.

Detailed Changes in this PR

Tests

Added a smoke test test_upgrade_flow_multi_step() in testsuite/smoke-test/src/upgrade.rs
Added two unit tests to test on-chain update of next_execution_hash in voting.move and aptos_governance.move.

CLI

Added a is_multi_step flag to CLI's SubmitProposal command in crates/aptos/src/governance/mod.rs.
Added GenerateExecutionHash to support generating execution hash of a script.
Redirected all previous multi-step release generation code to call GenerateExecutionHash for hash generation. (i.e., updated the get_execution_hash() function in aptos-release-builder to directly call GenerateExecutionHash for the sake of consistency).

Miscellaneous

Added a few support functions for testing propose in crates/aptos/src/test/mod.rs and testsuite/forge/src/interface/aptos.rs.

References

Design

AIP3: Multi-step Governance Proposal

Merged PRs

Test Plan

Tests included in this PR : )

movekevin · 2022-12-22T03:03:28Z

aptos-move/aptos-release-builder/src/components/framework.rs

-                _execution_hash =
-                    HashValue::sha3_256_of(result.last().unwrap().1.as_bytes()).to_string();
-                args.push(&_execution_hash);
+                _next_execution_hash_string = hex::encode(get_execution_hash(&result));


Why do we need to do hex encode here?

need to convert from bytes to string for the CLI command

Let's add a comment here as well why you have to convert it.

movekevin · 2022-12-22T03:04:21Z

aptos-move/aptos-release-builder/src/components/gas.rs

-                "gas_schedule::set_gas_schedule(framework_signer, gas_schedule_blob);"
-            );
+            if next_execution_hash.is_empty() {
+                emitln!(


What’s the diff between the if else cases here?

else has & before the framework_signer.

the testnet single-step generation had something like let framework_signer = &core_signer; so that their framework_signer is of type &signer, but for mainnet single-step and multi-step the framework_signer is of type signer

Let's add a comment describing this choice so the next person doesn't have to ask the question again.

gregnazario

Some comments around style / documentation that will help the future user.

Also, could we get an update to any governance docs since this introduces the new multi-step process?

gregnazario · 2023-01-10T22:50:47Z

aptos-move/aptos-release-builder/src/components/gas.rs

-                "gas_schedule::set_gas_schedule(framework_signer, gas_schedule_blob);"
-            );
+            if next_execution_hash.is_empty() {
+                emitln!(


Let's add a comment describing this choice so the next person doesn't have to ask the question again.

gregnazario · 2023-01-11T17:36:17Z

testsuite/smoke-test/src/upgrade.rs

+            cli.create_proposal(
+                validator_cli_index,
+                // placeholder url
+                "https://gist.githubusercontent.com/movekevin/057fb145b40866eff8c22c91fb9da919/raw/bab85f0f7434f008a8781eec7fcadd1ac5a55481/gistfile1.txt",


We should commit something directly to the codebase rather than use a gist from Kevin's account. Then reference that via the Github link.

Kevin's account could be suspended, removed, etc. And then this would magically stop working and we would be confused.

gregnazario · 2023-01-11T17:36:53Z

testsuite/smoke-test/src/upgrade.rs

+        cli.fund_account(i, Some(1000000000000000)).await.unwrap();
+
+        let mut keygen = KeyGen::from_os_rng();
+        let (validator_cli_index, _) =
+            init_validator_account(&mut cli, &mut keygen, Some(1000000000000000)).await;
+
+        cli.initialize_stake_owner(
+            i,
+            1000000000000000,


When using magic numbers, use a constant that describes what it is. I can't tell if these are all supposed to be the same number of 0s.

gregnazario · 2023-01-11T17:39:19Z

testsuite/smoke-test/src/upgrade.rs

+    let mut i = 0;
+    while i < 2 {


Looks like someone's been writing a lot of Move.

for i in 1..2 { }

Either that or take that whole first part and put it in a function and then just do the one off work in the middle:

function() create_proposal function()

gregnazario · 2023-01-11T17:42:41Z

aptos-move/aptos-release-builder/src/components/framework.rs

-                _execution_hash =
-                    HashValue::sha3_256_of(result.last().unwrap().1.as_bytes()).to_string();
-                args.push(&_execution_hash);
+                _next_execution_hash_string = hex::encode(get_execution_hash(&result));


Let's add a comment here as well why you have to convert it.

0xchloe · 2023-01-19T00:47:17Z

wanted to close this PR by saying that I'll optimize this later (was trying to optimize, and then ran into a stack overflow problem and realize there's a circular dependency somewhere in my optimized code, and I need to rewrite a series of functions. I'm half-way through it, but it's taking too long and not that important so I would like to land this PR first).

github-actions · 2023-01-19T01:29:50Z

✅ Forge suite `land_blocking` success on `f6d2df63c91ad371b57b864ccebaf9e35fe49ed8`

performance benchmark with full nodes : 6527 TPS, 6076 ms latency, 8100 ms p99 latency,(!) expired 140 out of 2787540 txns
Test Ok

github-actions · 2023-01-19T01:32:05Z

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

Compatibility test results for testnet_2d8b1b57553d869190f61df1aaf7f31a8fc19a7b ==> f6d2df63c91ad371b57b864ccebaf9e35fe49ed8 (PR)
1. Check liveness of validators at old version: testnet_2d8b1b57553d869190f61df1aaf7f31a8fc19a7b
compatibility::simple-validator-upgrade::liveness-check : 7393 TPS, 5278 ms latency, 6900 ms p99 latency,no expired txns
2. Upgrading first Validator to new version: f6d2df63c91ad371b57b864ccebaf9e35fe49ed8
compatibility::simple-validator-upgrade::single-validator-upgrade : 4547 TPS, 8812 ms latency, 11800 ms p99 latency,no expired txns
3. Upgrading rest of first batch to new version: f6d2df63c91ad371b57b864ccebaf9e35fe49ed8
compatibility::simple-validator-upgrade::half-validator-upgrade : 4693 TPS, 9086 ms latency, 13600 ms p99 latency,no expired txns
4. upgrading second batch to new version: f6d2df63c91ad371b57b864ccebaf9e35fe49ed8
compatibility::simple-validator-upgrade::rest-validator-upgrade : 6514 TPS, 5943 ms latency, 11200 ms p99 latency,no expired txns
5. check swarm health
Compatibility test for testnet_2d8b1b57553d869190f61df1aaf7f31a8fc19a7b ==> f6d2df63c91ad371b57b864ccebaf9e35fe49ed8 passed
Test Ok

0xchloe requested review from gregnazario, davidiw, movekevin and wrwg as code owners December 22, 2022 02:29

0xchloe requested review from runtian-zhou, lightmark and areshand December 22, 2022 02:29

0xchloe force-pushed the smoketest branch from 54015d0 to 9b7d84c Compare December 22, 2022 02:46

movekevin reviewed Dec 22, 2022

View reviewed changes

0xchloe requested a review from movekevin December 22, 2022 03:34

0xchloe force-pushed the smoketest branch 5 times, most recently from c201f62 to 8bfb727 Compare December 22, 2022 04:25

movekevin approved these changes Dec 23, 2022

View reviewed changes

gregnazario approved these changes Jan 11, 2023

View reviewed changes

0xchloe force-pushed the smoketest branch 4 times, most recently from b4fb9ac to af86ca9 Compare January 19, 2023 00:05

started writing smoke test

f6d2df6

0xchloe force-pushed the smoketest branch from af86ca9 to f6d2df6 Compare January 19, 2023 00:27

0xchloe enabled auto-merge (squash) January 19, 2023 00:45

This comment has been minimized.

Sign in to view

0xchloe merged commit accfde9 into main Jan 19, 2023

0xchloe deleted the smoketest branch January 19, 2023 01:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[testsuite] CLI & smoke test for multi-step governance proposal #5975

[testsuite] CLI & smoke test for multi-step governance proposal #5975

0xchloe commented Dec 22, 2022 •

edited

Loading

movekevin Dec 22, 2022

0xchloe Dec 22, 2022

gregnazario Jan 11, 2023

movekevin Dec 22, 2022

0xchloe Dec 22, 2022 •

edited

Loading

gregnazario Jan 10, 2023

gregnazario left a comment

gregnazario Jan 10, 2023

gregnazario Jan 11, 2023

gregnazario Jan 11, 2023

gregnazario Jan 11, 2023

gregnazario Jan 11, 2023

0xchloe commented Jan 19, 2023

This comment has been minimized.

This comment has been minimized.

github-actions bot commented Jan 19, 2023

github-actions bot commented Jan 19, 2023

[testsuite] CLI & smoke test for multi-step governance proposal #5975

[testsuite] CLI & smoke test for multi-step governance proposal #5975

Conversation

0xchloe commented Dec 22, 2022 • edited Loading

Description

Background

Detailed Changes in this PR

References

Test Plan

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

0xchloe Dec 22, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gregnazario 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

0xchloe commented Jan 19, 2023

This comment has been minimized.

This comment has been minimized.

github-actions bot commented Jan 19, 2023

✅ Forge suite land_blocking success on f6d2df63c91ad371b57b864ccebaf9e35fe49ed8

github-actions bot commented Jan 19, 2023

✅ Forge suite compat success on testnet_2d8b1b57553d869190f61df1aaf7f31a8fc19a7b ==> f6d2df63c91ad371b57b864ccebaf9e35fe49ed8

0xchloe commented Dec 22, 2022 •

edited

Loading

0xchloe Dec 22, 2022 •

edited

Loading

✅ Forge suite `land_blocking` success on `f6d2df63c91ad371b57b864ccebaf9e35fe49ed8`

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