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

deprecate FairnessShuffler #13705

Merged
merged 1 commit into from
Jul 9, 2024
Merged

deprecate FairnessShuffler #13705

merged 1 commit into from
Jul 9, 2024

Conversation

msmouse
Copy link
Contributor

@msmouse msmouse commented Jun 14, 2024

Description

Not gonna be used on testnet or mainnet any more.

Type of Change

  • Refactoring

Which Components or Systems Does This Change Impact?

  • Other (specify) -- tests and devnet genesis

How Has This Been Tested?

build
compatibility tests

Key Areas to Review

Checklist

  • I have read and followed the CONTRIBUTING doc
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I identified and added all stakeholders and component owners affected by this change as reviewers
  • I tested both happy and unhappy path of the functionality
  • I have made corresponding changes to the documentation

Copy link

trunk-io bot commented Jun 14, 2024

⏱️ 5h 39m total CI duration on this PR
Job Cumulative Duration Recent Runs
rust-targeted-unit-tests 1h 35m 🟥🟩🟥🟩
test-fuzzers 1h 9m 🟩🟩
rust-move-tests 1h 4m 🟩🟩🟩🟩
rust-lints 28m 🟩🟩🟩🟩
rust-smoke-tests 26m 🟩
run-tests-main-branch 23m 🟩🟩🟩🟩🟩
general-lints 12m 🟩🟩🟩🟩🟩 (+1 more)
rust-move-tests 9m 🟩
check-dynamic-deps 8m 🟩🟩🟩🟩🟩 (+1 more)
semgrep/ci 2m 🟩🟩🟩🟩🟩 (+1 more)
file_change_determinator 1m 🟩🟩🟩🟩🟩 (+1 more)
file_change_determinator 59s 🟩🟩🟩🟩🟩 (+1 more)
permission-check 17s 🟩🟩🟩🟩🟩 (+1 more)
permission-check 17s 🟩🟩🟩🟩🟩 (+1 more)
permission-check 16s 🟩🟩🟩🟩🟩 (+1 more)
permission-check 14s 🟩🟩🟩🟩🟩 (+1 more)

🚨 3 jobs on the last run were significantly faster/slower than expected

Job Duration vs 7d avg Delta
rust-move-tests 15m 9m +74%
rust-targeted-unit-tests 22m 13m +69%
run-tests-main-branch 6m 5m +28%

settingsfeedbackdocs ⋅ learn more about trunk.io

@msmouse msmouse force-pushed the 0614-deprecate-fairness-shuffler branch from cb35ecb to 0085b7b Compare June 14, 2024 21:42
@msmouse msmouse marked this pull request as ready for review June 14, 2024 21:43
@msmouse msmouse requested review from igor-aptos and sitalkedia June 14, 2024 21:43
@@ -152,7 +148,7 @@ pub enum TransactionShufflerType {
NoShuffling,
DeprecatedSenderAwareV1(u32),
SenderAwareV2(u32),
Fairness {
DeprecatedFairness {
Copy link
Contributor

Choose a reason for hiding this comment

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

why not just remove it if it's never enabled?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thought we enabled it briefly on testnet? @igor-aptos do you remember?

Copy link
Contributor

Choose a reason for hiding this comment

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

yup enabled, found the perf issue, and reverted.

@msmouse msmouse force-pushed the 0614-deprecate-fairness-shuffler branch 4 times, most recently from af126c0 to 3669519 Compare June 21, 2024 01:33
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.

we need to keep the enum, but replay/state sync don't shuffle - so we don't need to keep the code - we can just remove it?

@msmouse msmouse force-pushed the 0614-deprecate-fairness-shuffler branch from 3669519 to 4678eff Compare July 9, 2024 19:42
@msmouse msmouse enabled auto-merge (squash) July 9, 2024 19:43

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.

@msmouse
Copy link
Contributor Author

msmouse commented Jul 9, 2024

@igor-aptos Compat won't pass because we've been using the FairnessShuffler as the genesis default.
Will land as is and remove code in a later release.

@msmouse msmouse force-pushed the 0614-deprecate-fairness-shuffler branch from 4678eff to 811ece7 Compare July 9, 2024 20:52

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Copy link
Contributor

github-actions bot commented Jul 9, 2024

✅ Forge suite compat success on 1c2ee7082d6eff8c811ee25d6f5a7d00860a75d5 ==> 811ece74e7140d69f17d638345859c54cf373749

Compatibility test results for 1c2ee7082d6eff8c811ee25d6f5a7d00860a75d5 ==> 811ece74e7140d69f17d638345859c54cf373749 (PR)
1. Check liveness of validators at old version: 1c2ee7082d6eff8c811ee25d6f5a7d00860a75d5
compatibility::simple-validator-upgrade::liveness-check : committed: 8168.8049221283645 txn/s, latency: 3366.0050436005627 ms, (p50: 2700 ms, p90: 4500 ms, p99: 22500 ms), latency samples: 355500
2. Upgrading first Validator to new version: 811ece74e7140d69f17d638345859c54cf373749
compatibility::simple-validator-upgrade::single-validator-upgrading : committed: 3601.7724400441416 txn/s, latency: 7205.108454051607 ms, (p50: 8700 ms, p90: 9700 ms, p99: 10100 ms), latency samples: 88360
compatibility::simple-validator-upgrade::single-validator-upgrade : committed: 3270.7764310767343 txn/s, latency: 9497.434702861336 ms, (p50: 9400 ms, p90: 14800 ms, p99: 15100 ms), latency samples: 136300
3. Upgrading rest of first batch to new version: 811ece74e7140d69f17d638345859c54cf373749
compatibility::simple-validator-upgrade::half-validator-upgrading : committed: 3233.252309603511 txn/s, latency: 8075.893427579365 ms, (p50: 8500 ms, p90: 9700 ms, p99: 10100 ms), latency samples: 80640
compatibility::simple-validator-upgrade::half-validator-upgrade : committed: 3208.4682146118744 txn/s, latency: 9576.002078471412 ms, (p50: 9200 ms, p90: 15000 ms, p99: 15300 ms), latency samples: 137120
4. upgrading second batch to new version: 811ece74e7140d69f17d638345859c54cf373749
compatibility::simple-validator-upgrade::rest-validator-upgrading : committed: 235.29888980620592 txn/s, submitted: 560.7003776057782 txn/s, failed submission: 224.0011956305671 txn/s, expired: 325.4014877995723 txn/s, latency: 51483.970954356846 ms, (p50: 57500 ms, p90: 58100 ms, p99: 58100 ms), latency samples: 16870
compatibility::simple-validator-upgrade::rest-validator-upgrade : committed: 6620.278732339367 txn/s, latency: 4977.363819635489 ms, (p50: 5100 ms, p90: 7500 ms, p99: 8400 ms), latency samples: 232640
5. check swarm health
Compatibility test for 1c2ee7082d6eff8c811ee25d6f5a7d00860a75d5 ==> 811ece74e7140d69f17d638345859c54cf373749 passed
Test Ok

Copy link
Contributor

github-actions bot commented Jul 9, 2024

✅ Forge suite realistic_env_max_load success on 811ece74e7140d69f17d638345859c54cf373749

two traffics test: inner traffic : committed: 8307.32222103085 txn/s, latency: 4721.886135676767 ms, (p50: 4500 ms, p90: 6100 ms, p99: 11700 ms), latency samples: 3586760
two traffics test : committed: 100.05213671850274 txn/s, latency: 2137.342857142857 ms, (p50: 2000 ms, p90: 2400 ms, p99: 4900 ms), latency samples: 1820
Latency breakdown for phase 0: ["QsBatchToPos: max: 0.221, avg: 0.214", "QsPosToProposal: max: 0.190, avg: 0.181", "ConsensusProposalToOrdered: max: 0.310, avg: 0.291", "ConsensusOrderedToCommit: max: 0.381, avg: 0.371", "ConsensusProposalToCommit: max: 0.671, avg: 0.662"]
Max round gap was 1 [limit 4] at version 1820506. Max no progress secs was 5.382115 [limit 15] at version 1820506.
Test Ok

Copy link
Contributor

github-actions bot commented Jul 9, 2024

✅ Forge suite framework_upgrade success on 1c2ee7082d6eff8c811ee25d6f5a7d00860a75d5 ==> 811ece74e7140d69f17d638345859c54cf373749

Compatibility test results for 1c2ee7082d6eff8c811ee25d6f5a7d00860a75d5 ==> 811ece74e7140d69f17d638345859c54cf373749 (PR)
Upgrade the nodes to version: 811ece74e7140d69f17d638345859c54cf373749
framework_upgrade::framework-upgrade::full-framework-upgrade : committed: 1071.0486597132804 txn/s, submitted: 1073.8002298088827 txn/s, failed submission: 2.7515700956025184 txn/s, expired: 2.7515700956025184 txn/s, latency: 2914.4361271676303 ms, (p50: 2100 ms, p90: 5700 ms, p99: 10900 ms), latency samples: 93420
framework_upgrade::framework-upgrade::full-framework-upgrade : committed: 1193.2209024543617 txn/s, submitted: 1195.8650261302278 txn/s, failed submission: 2.64412367586583 txn/s, expired: 2.64412367586583 txn/s, latency: 2722.2251107977436 ms, (p50: 1800 ms, p90: 5100 ms, p99: 9900 ms), latency samples: 99280
5. check swarm health
Compatibility test for 1c2ee7082d6eff8c811ee25d6f5a7d00860a75d5 ==> 811ece74e7140d69f17d638345859c54cf373749 passed
Upgrade the remaining nodes to version: 811ece74e7140d69f17d638345859c54cf373749
framework_upgrade::framework-upgrade::full-framework-upgrade : committed: 1200.2977485099952 txn/s, submitted: 1202.3023281605035 txn/s, failed submission: 2.0045796505084352 txn/s, expired: 2.0045796505084352 txn/s, latency: 2738.098534050844 ms, (p50: 2100 ms, p90: 4900 ms, p99: 10500 ms), latency samples: 107780
Test Ok

@msmouse msmouse merged commit 1ff08be into main Jul 9, 2024
47 checks passed
@msmouse msmouse deleted the 0614-deprecate-fairness-shuffler branch July 9, 2024 21:24
zi0Black pushed a commit that referenced this pull request Jul 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants