Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[forge] Add a test for validation set changes #4279

Merged
merged 1 commit into from Sep 30, 2022
Merged

Conversation

zcchahaha
Copy link
Contributor

@zcchahaha zcchahaha commented Sep 16, 2022

Description

  • Add a test where 1/3 validators leave the validator set and then rejoin. More complex tests can be added in separate PRs.
  • Add a feature to make test framework add_move_file function compilation conditional due to some move dependencies issues when introducing CliTestFramework to forge. https://aptos-org.slack.com/archives/C036SHTFHGR/p1663364319592519

This change is Reviewable

@zcchahaha zcchahaha added the CICD:run-e2e-tests when this label is present github actions will run all land-blocking e2e tests from the PR label Sep 16, 2022
@zcchahaha zcchahaha force-pushed the validator_set_change branch 2 times, most recently from b1a072b to c6cd22f Compare September 16, 2022 21:57
@zcchahaha zcchahaha force-pushed the validator_set_change branch 3 times, most recently from a3b2932 to 92a97c5 Compare September 19, 2022 00:23
@sitalkedia
Copy link
Contributor

@zcchahaha - Is this ready for review?

@zcchahaha
Copy link
Contributor Author

@sitalkedia Not yet. Still try to figure out some dependencies issues. https://aptos-org.slack.com/archives/C036SHTFHGR/p1663364319592519

@zcchahaha zcchahaha force-pushed the validator_set_change branch 3 times, most recently from 226409d to 7a4cb71 Compare September 19, 2022 21:18
@zcchahaha zcchahaha marked this pull request as draft September 19, 2022 21:21
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@zcchahaha zcchahaha force-pushed the validator_set_change branch 3 times, most recently from 1e9f946 to 023620c Compare September 20, 2022 06:58
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@zcchahaha zcchahaha changed the title [forge] Add tests for validation set changes [forge] Add a test for validation set changes Sep 20, 2022
@zcchahaha zcchahaha marked this pull request as ready for review September 20, 2022 16:18
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

Copy link
Contributor

@igor-aptos igor-aptos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some comments inline, but overall looks good


let mut cli = runtime.block_on(async {
CliTestFramework::new(
swarm.validators().next().unwrap().rest_api_endpoint(),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use rest_client.clone() here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The parameter is a URL, not a rest client.

.await
.unwrap();

reconfig(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh, you should probably do reconfig once at the end, this will create bunch of epochs.

we also want to test that they can all ask to leave simultaneously

in the follow-up randomized test you are mentioning below, you can randomly select how many validators to leave simulatenously, before reconfig.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a reconfig after the loop.

for operator_index in validator_cli_indices.iter().rev().take(num_validators / 3) {
cli.join_validator_set(*operator_index, None).await.unwrap();

reconfig(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same, to reconfig once , after the loop

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions
Copy link
Contributor

✅ Forge suite compat success on 843b204dce971d98449b82624f4f684c7a18b991 ==> dbf36e3875ea6240d8f3722800242823eb022b40

Compatibility test results for 843b204dce971d98449b82624f4f684c7a18b991 ==> dbf36e3875ea6240d8f3722800242823eb022b40 (PR)
1. Check liveness of validators at old version: 843b204dce971d98449b82624f4f684c7a18b991
compatibility::simple-validator-upgrade::liveness-check : 7700 TPS, 4911 ms latency, 11100 ms p99 latency,no expired txns
2. Upgrading first Validator to new version: dbf36e3875ea6240d8f3722800242823eb022b40
compatibility::simple-validator-upgrade::single-validator-upgrade : 4967 TPS, 7633 ms latency, 10100 ms p99 latency,no expired txns
3. Upgrading rest of first batch to new version: dbf36e3875ea6240d8f3722800242823eb022b40
compatibility::simple-validator-upgrade::half-validator-upgrade : 5448 TPS, 6950 ms latency, 9000 ms p99 latency,no expired txns
4. upgrading second batch to new version: dbf36e3875ea6240d8f3722800242823eb022b40
compatibility::simple-validator-upgrade::rest-validator-upgrade : 7368 TPS, 4930 ms latency, 9300 ms p99 latency,no expired txns
5. check swarm health
Compatibility test for 843b204dce971d98449b82624f4f684c7a18b991 ==> dbf36e3875ea6240d8f3722800242823eb022b40 passed
Test Ok

@github-actions
Copy link
Contributor

✅ Forge suite land_blocking success on dbf36e3875ea6240d8f3722800242823eb022b40

performance benchmark with full nodes : 7188 TPS, 5532 ms latency, 7900 ms p99 latency,no expired txns
Test Ok

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CICD:run-e2e-tests when this label is present github actions will run all land-blocking e2e tests from the PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants