Re-enable dedup and shuffling as defaults (for test networks) #9397

bchocho · 2023-07-31T21:41:42Z

Description

Introduce a new Missing version for OnChainExecutionConfig that will maintain backwards compatibility for before the config was registered. Use this when the config is missing from epoch manager.

Otherwise, use default_for_genesis for all new networks e.g., forge, devnet.

Test Plan

Observe that e2e tests use both shuffle and dedup.

sitalkedia

LGTM

sitalkedia · 2023-08-02T14:51:27Z

types/src/on_chain_config/execution_config.rs

@@ -106,7 +106,7 @@ pub struct ExecutionConfigV3 {
 impl Default for ExecutionConfigV3 {
    fn default() -> Self {
        Self {
-            transaction_shuffler_type: TransactionShufflerType::NoShuffling,
+            transaction_shuffler_type: TransactionShufflerType::SenderAwareV2(32),
            block_gas_limit: None,


@danielxiangzl - Should we enable block gas limit by default for Forge and other testings?

Yes, but only after the execution on-chain config registry is fixed. I still find it hacky to set the config values via default for test networks. Maybe do what Igor suggested and set the block gas limit to be 35000 for genesis?

This will be used only for Forge and existing integration tests, so I say we enable gas limit so that our tests exercise the block gas limit before rolling this out. For testnet/mainnet, this is no-op as they are still using the V1.

Actually I checked and we are on V3 in mainnet and testnet. But it still shouldn't matter, right?

If the execution on-chain config registry is not fixed, and main/testnet reads config via default (V3), then it will be a problem. So just to double check, is it fixed already?

How are we on V3 in mainnet? If so, I think this is a dangerous change to make..

The governance proposal for v1.5 was V3

Since the registry fix is not rolled out yet, I think we should not enable shuffling by default in V3 - if we are already in V3 in mainnet. The reason for that being if by any chance we roll out this change without the registry fix, then we break the network.

Let's add V4 below Missing, and return no shuffling in v3? Then we don't need new shuffling enum either

igor-aptos · 2023-08-02T15:38:10Z

types/src/on_chain_config/execution_config.rs

@@ -106,7 +106,7 @@ pub struct ExecutionConfigV3 {
 impl Default for ExecutionConfigV3 {


if we want to permanently have different things, I would have :
default_for_genesis
default_if_missing
basically default_for_genesis is used for any new network. default_if_missing is the backward compatible thing (needed for reply for example), to have old behavior if flag is missing

and I would remove Default macro completely, so people don't explicitly pick the one they want.

currently it is not critical, but if at any point in the future we add anything in the execution config that affects transaction output (not the order/which ones get on chain), we would need to have a separation, as replay will start with missing config

sitalkedia · 2023-08-02T18:41:09Z

types/src/on_chain_config/execution_config.rs

@@ -106,7 +106,7 @@ pub struct ExecutionConfigV3 {
 impl Default for ExecutionConfigV3 {
    fn default() -> Self {
        Self {
-            transaction_shuffler_type: TransactionShufflerType::NoShuffling,
+            transaction_shuffler_type: TransactionShufflerType::SenderAwareV2(32),
            block_gas_limit: None,


Since the registry fix is not rolled out yet, I think we should not enable shuffling by default in V3 - if we are already in V3 in mainnet. The reason for that being if by any chance we roll out this change without the registry fix, then we break the network.

This reverts commit 48a39a1.

sitalkedia · 2023-08-02T21:11:19Z

types/src/on_chain_config/execution_config.rs

@@ -11,13 +11,17 @@ pub enum OnChainExecutionConfig {
    V1(ExecutionConfigV1),
    V2(ExecutionConfigV2),
    V3(ExecutionConfigV3),
+    /// To maintain backwards compatibility on replay, we must ensure that any new features resolve
+    /// to previous behavior (before OnChainExecutionConfig was registered) in case of Missing.
+    Missing,


It is pretty easy to make a mistake and add the next version V4 after V3 and before Missing and thus breaking backward compatibility. Can you add a comment here to add V4 after Missing ?

sitalkedia · 2023-08-02T21:12:57Z

types/src/on_chain_config/execution_config.rs

@@ -106,8 +118,8 @@ pub struct ExecutionConfigV3 {
 impl Default for ExecutionConfigV3 {
    fn default() -> Self {


Is this being used anywhere? If so, better to change them to default_for_genesis or default_if_missing and get rid of this?

Nevermind, just realized, this is called from default_for_genesis function, but still may be it will be less confusing to remove Default trait for ExecutionConfigV3 and return the desired value from default_for_genesis function..

Agreed with this, maybe even just inline there

igor-aptos

Thanks!

sitalkedia

Thanks for fixing the config mess @bchocho 🙌

github-actions · 2023-08-02T22:18:18Z

✅ Forge suite `compat` success on `aptos-node-v1.5.1` ==> `c85d9849bd99882d69e15e437881867a51490dee`

Compatibility test results for aptos-node-v1.5.1 ==> c85d9849bd99882d69e15e437881867a51490dee (PR)
1. Check liveness of validators at old version: aptos-node-v1.5.1
compatibility::simple-validator-upgrade::liveness-check : committed: 4911 txn/s, latency: 6586 ms, (p50: 6300 ms, p90: 9700 ms, p99: 12400 ms), latency samples: 181720
2. Upgrading first Validator to new version: c85d9849bd99882d69e15e437881867a51490dee
compatibility::simple-validator-upgrade::single-validator-upgrade : committed: 1817 txn/s, latency: 15907 ms, (p50: 18700 ms, p90: 22300 ms, p99: 22600 ms), latency samples: 92680
3. Upgrading rest of first batch to new version: c85d9849bd99882d69e15e437881867a51490dee
compatibility::simple-validator-upgrade::half-validator-upgrade : committed: 1858 txn/s, latency: 15799 ms, (p50: 18700 ms, p90: 22200 ms, p99: 22400 ms), latency samples: 92900
4. upgrading second batch to new version: c85d9849bd99882d69e15e437881867a51490dee
compatibility::simple-validator-upgrade::rest-validator-upgrade : committed: 3160 txn/s, latency: 9540 ms, (p50: 10200 ms, p90: 12700 ms, p99: 13600 ms), latency samples: 135900
5. check swarm health
Compatibility test for aptos-node-v1.5.1 ==> c85d9849bd99882d69e15e437881867a51490dee passed
Test Ok

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

github-actions · 2023-08-02T22:19:23Z

✅ Forge suite `realistic_env_max_load` success on `c85d9849bd99882d69e15e437881867a51490dee`

two traffics test: inner traffic : committed: 6404 txn/s, latency: 6127 ms, (p50: 5900 ms, p90: 7600 ms, p99: 12500 ms), latency samples: 2766800
two traffics test : committed: 100 txn/s, latency: 3002 ms, (p50: 2900 ms, p90: 3700 ms, p99: 4300 ms), latency samples: 1840
Max round gap was 1 [limit 4] at version 1442381. Max no progress secs was 3.9159079 [limit 10] at version 1442381.
Test Ok

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

github-actions · 2023-08-02T22:27:46Z

✅ Forge suite `framework_upgrade` success on `aptos-node-v1.5.1` ==> `c85d9849bd99882d69e15e437881867a51490dee`

Compatibility test results for aptos-node-v1.5.1 ==> c85d9849bd99882d69e15e437881867a51490dee (PR)
Upgrade the nodes to version: c85d9849bd99882d69e15e437881867a51490dee
framework_upgrade::framework-upgrade::full-framework-upgrade : committed: 4461 txn/s, latency: 7312 ms, (p50: 7800 ms, p90: 10500 ms, p99: 11000 ms), latency samples: 165060
5. check swarm health
Compatibility test for aptos-node-v1.5.1 ==> c85d9849bd99882d69e15e437881867a51490dee passed
Test Ok

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

### Description Introduce a new `Missing` version for `OnChainExecutionConfig` that will maintain backwards compatibility for before the config was registered. Use this when the config is missing from epoch manager. Otherwise, use `default_for_genesis` for all new networks e.g., forge, devnet. ### Test Plan Observe that e2e tests use both shuffle and dedup.

Re-enable dedup in forge, and cleanup turning on shuffling for tests

5e29659

bchocho added the CICD:run-e2e-tests when this label is present github actions will run all land-blocking e2e tests from the PR label Jul 31, 2023

bchocho requested a review from sitalkedia July 31, 2023 21:41

sitalkedia approved these changes Jul 31, 2023

View reviewed changes

This comment has been minimized.

Sign in to view

bchocho marked this pull request as ready for review July 31, 2023 22:16

bchocho requested review from gregnazario, 0xmigo and banool as code owners July 31, 2023 22:16

bchocho requested review from igor-aptos and zekun000 July 31, 2023 22:17

This comment has been minimized.

Sign in to view

bchocho requested a review from JoshLind August 1, 2023 00:38

Change the default directly

48a39a1

bchocho changed the title ~~Re-enable dedup in forge, and cleanup turning on shuffling for tests~~ Re-enable dedup and shuffling as defaults (for test networks) Aug 2, 2023

bchocho requested a review from sitalkedia August 2, 2023 00:14

This comment has been minimized.

Sign in to view

sitalkedia reviewed Aug 2, 2023

View reviewed changes

igor-aptos requested changes Aug 2, 2023

View reviewed changes

sitalkedia requested changes Aug 2, 2023

View reviewed changes

Revert "Change the default directly"

c38f1dd

This reverts commit 48a39a1.

Split execution config default, and add a Missing config version

9df66f6

bchocho requested review from davidiw and movekevin as code owners August 2, 2023 20:24

cleanup V3 default

35024b5

This comment has been minimized.

Sign in to view

sitalkedia reviewed Aug 2, 2023

View reviewed changes

igor-aptos approved these changes Aug 2, 2023

View reviewed changes

More cleanup addressing comments

c85d984

bchocho requested a review from sitalkedia August 2, 2023 21:44

sitalkedia approved these changes Aug 2, 2023

View reviewed changes

bchocho enabled auto-merge (squash) August 2, 2023 21:47

This comment has been minimized.

Sign in to view

bchocho merged commit 285a61c into main Aug 2, 2023

bchocho deleted the brian/enable-dedup-in-forge branch August 2, 2023 22:27

		@@ -106,7 +106,7 @@ pub struct ExecutionConfigV3 {
		impl Default for ExecutionConfigV3 {

		@@ -106,8 +118,8 @@ pub struct ExecutionConfigV3 {
		impl Default for ExecutionConfigV3 {
		fn default() -> Self {

Re-enable dedup and shuffling as defaults (for test networks) #9397

Re-enable dedup and shuffling as defaults (for test networks) #9397

Conversation

bchocho commented Jul 31, 2023 • edited Loading

Description

Test Plan

sitalkedia 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.

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

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.

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

igor-aptos left a comment

Choose a reason for hiding this comment

sitalkedia 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.

github-actions bot commented Aug 2, 2023

✅ Forge suite compat success on aptos-node-v1.5.1 ==> c85d9849bd99882d69e15e437881867a51490dee

github-actions bot commented Aug 2, 2023

✅ Forge suite realistic_env_max_load success on c85d9849bd99882d69e15e437881867a51490dee

github-actions bot commented Aug 2, 2023

✅ Forge suite framework_upgrade success on aptos-node-v1.5.1 ==> c85d9849bd99882d69e15e437881867a51490dee

bchocho commented Jul 31, 2023 •

edited

Loading

✅ Forge suite `compat` success on `aptos-node-v1.5.1` ==> `c85d9849bd99882d69e15e437881867a51490dee`

✅ Forge suite `realistic_env_max_load` success on `c85d9849bd99882d69e15e437881867a51490dee`

✅ Forge suite `framework_upgrade` success on `aptos-node-v1.5.1` ==> `c85d9849bd99882d69e15e437881867a51490dee`