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

Fix units in timed feature flags #15391

Merged
merged 1 commit into from
Nov 26, 2024
Merged

Fix units in timed feature flags #15391

merged 1 commit into from
Nov 26, 2024

Conversation

vineethk
Copy link
Contributor

@vineethk vineethk commented Nov 25, 2024

Description

Previously, timed feature flags activation time used millis as the unit, last_reconfiguration_time used micros as the unit, and these two were compared to see if timed feature flag was enabled (which was always true for any date on which blockchain has been around).

  1. This PR fixes the above issue.
  2. We also always enable DisableInvariantViolationCheckInSwapLoc (keeping the same semantics as before, but applying a minor fix).
  3. We remove the replay override for EntryCompatibility (as there are transactions on mainnet before this feature was activated that fail the corresponding check).

How Has This Been Tested?

  • Existing tests
  • Will be running replay-verify
  • Added some new tests

Type of Change

  • Bug fix

Which Components or Systems Does This Change Impact?

  • Validator Node

Copy link

trunk-io bot commented Nov 25, 2024

⏱️ 2h 30m total CI duration on this PR
Slowest 15 Jobs Cumulative Duration Recent Runs
execution-performance / single-node-performance 50m 🟩🟩
rust-move-tests 12m 🟩
rust-move-tests 12m 🟩
rust-move-tests 10m
test-target-determinator 9m 🟩🟩
execution-performance / test-target-determinator 9m 🟩🟩
rust-cargo-deny 9m 🟩🟩🟩🟩
check 7m 🟩🟩
rust-move-tests 6m
rust-doc-tests 5m
rust-doc-tests 5m 🟩
check-dynamic-deps 4m 🟩🟩🟩🟩🟩
fetch-last-released-docker-image-tag 3m 🟩🟩
general-lints 3m 🟩🟩🟩🟩🟩
semgrep/ci 2m 🟩🟩🟩🟩🟩

settingsfeedbackdocs ⋅ learn more about trunk.io

Copy link
Contributor Author

vineethk commented Nov 25, 2024

@vineethk vineethk force-pushed the vk/timed_feature_flags_fix branch from 7b9d699 to 1d9b5b7 Compare November 25, 2024 20:08
@vineethk vineethk marked this pull request as ready for review November 25, 2024 20:09
Copy link
Contributor

@runtian-zhou runtian-zhou left a comment

Choose a reason for hiding this comment

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

Thanks!

types/src/on_chain_config/mod.rs Show resolved Hide resolved
types/src/on_chain_config/mod.rs Outdated Show resolved Hide resolved
types/src/on_chain_config/timed_features.rs Show resolved Hide resolved
types/src/on_chain_config/timed_features.rs Outdated Show resolved Hide resolved
types/src/on_chain_config/timed_features.rs Outdated Show resolved Hide resolved
@vineethk vineethk force-pushed the vk/timed_feature_flags_fix branch from 1d9b5b7 to 12e2f0b Compare November 26, 2024 00:53
@vineethk vineethk enabled auto-merge (squash) November 26, 2024 00:55
@vineethk vineethk added the v1.24 label Nov 26, 2024

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Copy link
Contributor

✅ Forge suite realistic_env_max_load success on 12e2f0b4fe20770e4a2d8f042311432f6ea6a11d

two traffics test: inner traffic : committed: 14183.05 txn/s, latency: 2802.44 ms, (p50: 2700 ms, p70: 2700, p90: 3000 ms, p99: 4300 ms), latency samples: 5392720
two traffics test : committed: 100.05 txn/s, latency: 2062.25 ms, (p50: 1400 ms, p70: 1600, p90: 2200 ms, p99: 18700 ms), latency samples: 1640
Latency breakdown for phase 0: ["MempoolToBlockCreation: max: 2.231, avg: 1.394", "ConsensusProposalToOrdered: max: 0.327, avg: 0.293", "ConsensusOrderedToCommit: max: 0.378, avg: 0.362", "ConsensusProposalToCommit: max: 0.670, avg: 0.656"]
Max non-epoch-change gap was: 0 rounds at version 0 (avg 0.00) [limit 4], 1.31s no progress at version 46654 (avg 0.20s) [limit 15].
Max epoch-change gap was: 0 rounds at version 0 (avg 0.00) [limit 4], 15.79s no progress at version 2822134 (avg 15.79s) [limit 16].
Test Ok

Copy link
Contributor

✅ Forge suite framework_upgrade success on f436adbe4384d6c5fd296addbb7f52d4be77231b ==> 12e2f0b4fe20770e4a2d8f042311432f6ea6a11d

Compatibility test results for f436adbe4384d6c5fd296addbb7f52d4be77231b ==> 12e2f0b4fe20770e4a2d8f042311432f6ea6a11d (PR)
Upgrade the nodes to version: 12e2f0b4fe20770e4a2d8f042311432f6ea6a11d
framework_upgrade::framework-upgrade::full-framework-upgrade : committed: 1385.31 txn/s, submitted: 1386.93 txn/s, failed submission: 1.62 txn/s, expired: 1.62 txn/s, latency: 2224.64 ms, (p50: 2100 ms, p70: 2400, p90: 3600 ms, p99: 4800 ms), latency samples: 119620
framework_upgrade::framework-upgrade::full-framework-upgrade : committed: 1417.25 txn/s, submitted: 1419.51 txn/s, failed submission: 2.26 txn/s, expired: 2.26 txn/s, latency: 2173.81 ms, (p50: 2100 ms, p70: 2100, p90: 3300 ms, p99: 5100 ms), latency samples: 125580
5. check swarm health
Compatibility test for f436adbe4384d6c5fd296addbb7f52d4be77231b ==> 12e2f0b4fe20770e4a2d8f042311432f6ea6a11d passed
Upgrade the remaining nodes to version: 12e2f0b4fe20770e4a2d8f042311432f6ea6a11d
framework_upgrade::framework-upgrade::full-framework-upgrade : committed: 1334.86 txn/s, submitted: 1337.72 txn/s, failed submission: 2.85 txn/s, expired: 2.85 txn/s, latency: 2262.00 ms, (p50: 2100 ms, p70: 2400, p90: 3900 ms, p99: 5700 ms), latency samples: 121600
Test Ok

Copy link
Contributor

✅ Forge suite compat success on f436adbe4384d6c5fd296addbb7f52d4be77231b ==> 12e2f0b4fe20770e4a2d8f042311432f6ea6a11d

Compatibility test results for f436adbe4384d6c5fd296addbb7f52d4be77231b ==> 12e2f0b4fe20770e4a2d8f042311432f6ea6a11d (PR)
1. Check liveness of validators at old version: f436adbe4384d6c5fd296addbb7f52d4be77231b
compatibility::simple-validator-upgrade::liveness-check : committed: 13524.59 txn/s, latency: 2510.34 ms, (p50: 1600 ms, p70: 1800, p90: 2500 ms, p99: 26700 ms), latency samples: 546320
2. Upgrading first Validator to new version: 12e2f0b4fe20770e4a2d8f042311432f6ea6a11d
compatibility::simple-validator-upgrade::single-validator-upgrading : committed: 2063.45 txn/s, submitted: 2063.60 txn/s, expired: 0.16 txn/s, latency: 3717.56 ms, (p50: 4100 ms, p70: 4200, p90: 4300 ms, p99: 4500 ms), latency samples: 144529
compatibility::simple-validator-upgrade::single-validator-upgrade : committed: 7751.99 txn/s, latency: 4150.54 ms, (p50: 4400 ms, p70: 4500, p90: 5600 ms, p99: 6100 ms), latency samples: 256860
3. Upgrading rest of first batch to new version: 12e2f0b4fe20770e4a2d8f042311432f6ea6a11d
compatibility::simple-validator-upgrade::half-validator-upgrading : committed: 6156.69 txn/s, latency: 4654.69 ms, (p50: 5200 ms, p70: 5500, p90: 5600 ms, p99: 5700 ms), latency samples: 115260
compatibility::simple-validator-upgrade::half-validator-upgrade : committed: 6187.66 txn/s, latency: 5259.21 ms, (p50: 5600 ms, p70: 5800, p90: 6700 ms, p99: 7300 ms), latency samples: 211060
4. upgrading second batch to new version: 12e2f0b4fe20770e4a2d8f042311432f6ea6a11d
compatibility::simple-validator-upgrade::rest-validator-upgrading : committed: 12527.54 txn/s, latency: 2245.68 ms, (p50: 2500 ms, p70: 2500, p90: 2700 ms, p99: 2800 ms), latency samples: 217820
compatibility::simple-validator-upgrade::rest-validator-upgrade : committed: 10471.89 txn/s, latency: 3023.41 ms, (p50: 2600 ms, p70: 2900, p90: 5900 ms, p99: 7500 ms), latency samples: 370560
5. check swarm health
Compatibility test for f436adbe4384d6c5fd296addbb7f52d4be77231b ==> 12e2f0b4fe20770e4a2d8f042311432f6ea6a11d passed
Test Ok

@vineethk vineethk merged commit 4d4c200 into main Nov 26, 2024
124 of 142 checks passed
@vineethk vineethk deleted the vk/timed_feature_flags_fix branch November 26, 2024 01:33
github-actions bot pushed a commit that referenced this pull request Nov 26, 2024
Copy link
Contributor

💚 All backports created successfully

Status Branch Result
aptos-release-v1.24

Questions ?

Please refer to the Backport tool documentation and see the Github Action logs for details

sherry-x pushed a commit that referenced this pull request Nov 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants