[leader election] Improve leader election for first 20 rounds #5683

igor-aptos · 2022-11-22T22:20:03Z

exclude_rounds is 20 by default, which means we don't consider last 20 rounds in leader reputation history - but we do so from the same epoch - we consider everyone is caught up with the previous epoch.

genesis is epoch=0, round=0
first block is epoch=1 round=1, and that is a reconfig, that triggers epoch 2. because votes in block metadata are for previous round, they are empty in that round. So in that round - only "active" node is proposer.

For first 20 rounds of epoch 2, that is the only history we have - and so proposer of epoch=1 round=1 will always be elected for the first 20 rounds.

That can cause issues in tests, as they might be executed over very few rounds. So removing epoch=1,round=1 from being considered for history in epoch 2.

We can make this change without backward compatible gating, as this only affects first window rounds (10 * num validators), and chain will continue successfully after that (and all active chains are pass the epoch 2)

Description

Test Plan

used test_basic_fault_tolerance test, and confirmed before the change that leader doesn't change for first 20 rounds, and after this change it does.

exclude_rounds is 20 by default, which means we don't consider last 20 rounds in leader reputation history - but we do so from the same epoch - we consider everyone is caught up with the previous epoch. genesis is epoch=0, round=0 first block is epoch=1 round=1, and that is a reconfig, that triggers epoch 2. because votes in block metadata are for previous round, they are empty in that round. So in that round - only "active" node is proposer. For first 20 rounds of epoch 2, that is the only history we have - and so proposer of epoch=1 round=1 will always be elected for the first 20 rounds. That can cause issues in tests, as they might be executed over very few rounds. So removing epoch=1,round=1 from being considered for history in epoch 2. We can make this change without backward compatible gating, as this only affects first window rounds (10 * num validators), and chain will continue successfully after that (and all active chains are pass the epoch 2)

bchocho · 2022-11-22T22:29:55Z

consensus/src/epoch_manager.rs

                let first_epoch_to_consider = std::cmp::max(
-                    1,
+                    if epoch_state.epoch == 1 { 1 } else { 2 },


I don’t quite understand why we want to consider epoch=1 round=1 when we are still in epoch=1?
i.e., why can’t we just change the min epoch to consider as 2 rather than 1

we do have multiple rounds in epoch 1, they are just short-circuted and removed after we move to epoch 2.
so we should have leader election work normally in epoch 1. we just don't want to take history from epoch 1 into epoch 2 leader election

https://gist.githubusercontent.com/igor-aptos/8127b53add8fd9e9a83d9dc299a4eb92/raw/6f6506f2f6ca8a583ab830e26e3a93a7f9940947/gistfile1.txt

this expression is getting ugly, maybe this is simpler?

if epoch <= 2 { use_history_from_previous_epoch_max_count = 0; }

That's not the same. use_history_from_previous_epoch_max_count is 5 by default (in case we have multiple consecutive reconfigs/governance proposals. So we want first_epoch_to_consider to be 2 for epoch = 6 too.

hmm as long as after epoch 1 we have more than 20 rounds it should be good enough?

I see what you are saying, but even though expression is shorter, I find it much more confusing at what it does. I think with the comment, the code is understandable.

so I am going to land as is.

zekun000

good findings!

zekun000 · 2022-11-22T23:35:58Z

consensus/src/epoch_manager.rs

                let first_epoch_to_consider = std::cmp::max(
-                    1,
+                    if epoch_state.epoch == 1 { 1 } else { 2 },


this expression is getting ugly, maybe this is simpler?

if epoch <= 2 { use_history_from_previous_epoch_max_count = 0; }

bchocho

LGTM

github-actions · 2022-11-23T00:49:55Z

✅ Forge suite `land_blocking` success on `0a0e99eee17535b391ab7371c2144220068ee62a`

performance benchmark with full nodes : 6278 TPS, 6152 ms latency, 16400 ms p99 latency,(!) expired 8560 out of 2689520 txns
Test Ok

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

github-actions · 2022-11-23T00:50:36Z

✅ Forge suite `compat` success on `testnet_2d8b1b57553d869190f61df1aaf7f31a8fc19a7b` ==> `0a0e99eee17535b391ab7371c2144220068ee62a`

Compatibility test results for testnet_2d8b1b57553d869190f61df1aaf7f31a8fc19a7b ==> 0a0e99eee17535b391ab7371c2144220068ee62a (PR)
1. Check liveness of validators at old version: testnet_2d8b1b57553d869190f61df1aaf7f31a8fc19a7b
compatibility::simple-validator-upgrade::liveness-check : 7521 TPS, 5123 ms latency, 6800 ms p99 latency,no expired txns
2. Upgrading first Validator to new version: 0a0e99eee17535b391ab7371c2144220068ee62a
compatibility::simple-validator-upgrade::single-validator-upgrade : 4543 TPS, 9148 ms latency, 12300 ms p99 latency,no expired txns
3. Upgrading rest of first batch to new version: 0a0e99eee17535b391ab7371c2144220068ee62a
compatibility::simple-validator-upgrade::half-validator-upgrade : 4738 TPS, 8534 ms latency, 10800 ms p99 latency,no expired txns
4. upgrading second batch to new version: 0a0e99eee17535b391ab7371c2144220068ee62a
compatibility::simple-validator-upgrade::rest-validator-upgrade : 6904 TPS, 5562 ms latency, 9600 ms p99 latency,no expired txns
5. check swarm health
Compatibility test for testnet_2d8b1b57553d869190f61df1aaf7f31a8fc19a7b ==> 0a0e99eee17535b391ab7371c2144220068ee62a passed
Test Ok

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

…labs#5683) exclude_rounds is 20 by default, which means we don't consider last 20 rounds in leader reputation history - but we do so from the same epoch - we consider everyone is caught up with the previous epoch. genesis is epoch=0, round=0 first block is epoch=1 round=1, and that is a reconfig, that triggers epoch 2. because votes in block metadata are for previous round, they are empty in that round. So in that round - only "active" node is proposer. For first 20 rounds of epoch 2, that is the only history we have - and so proposer of epoch=1 round=1 will always be elected for the first 20 rounds. That can cause issues in tests, as they might be executed over very few rounds. So removing epoch=1,round=1 from being considered for history in epoch 2. We can make this change without backward compatible gating, as this only affects first window rounds (10 * num validators), and chain will continue successfully after that (and all active chains are pass the epoch 2)

igor-aptos requested review from bchocho, JoshLind and sitalkedia November 22, 2022 22:20

igor-aptos requested a review from zekun000 as a code owner November 22, 2022 22:20

bchocho reviewed Nov 22, 2022

View reviewed changes

zekun000 approved these changes Nov 22, 2022

View reviewed changes

igor-aptos added the CICD:run-e2e-tests when this label is present github actions will run all land-blocking e2e tests from the PR label Nov 23, 2022

bchocho approved these changes Nov 23, 2022

View reviewed changes

This comment has been minimized.

Sign in to view

igor-aptos merged commit 49d77e6 into main Nov 23, 2022

igor-aptos deleted the igor/first_20_rounds_leader_election branch November 23, 2022 22:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[leader election] Improve leader election for first 20 rounds #5683

[leader election] Improve leader election for first 20 rounds #5683

igor-aptos commented Nov 22, 2022

bchocho Nov 22, 2022

igor-aptos Nov 22, 2022

zekun000 Nov 22, 2022

igor-aptos Nov 23, 2022

zekun000 Nov 23, 2022

igor-aptos Nov 23, 2022 •

edited

Loading

zekun000 left a comment

zekun000 Nov 22, 2022

bchocho left a comment

This comment has been minimized.

This comment has been minimized.

github-actions bot commented Nov 23, 2022

github-actions bot commented Nov 23, 2022

[leader election] Improve leader election for first 20 rounds #5683

[leader election] Improve leader election for first 20 rounds #5683

Conversation

igor-aptos commented Nov 22, 2022

Description

Test Plan

bchocho Nov 22, 2022

Choose a reason for hiding this comment

igor-aptos Nov 22, 2022

Choose a reason for hiding this comment

zekun000 Nov 22, 2022

Choose a reason for hiding this comment

igor-aptos Nov 23, 2022

Choose a reason for hiding this comment

zekun000 Nov 23, 2022

Choose a reason for hiding this comment

igor-aptos Nov 23, 2022 • edited Loading

Choose a reason for hiding this comment

zekun000 left a comment

Choose a reason for hiding this comment

zekun000 Nov 22, 2022

Choose a reason for hiding this comment

bchocho left a comment

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

github-actions bot commented Nov 23, 2022

✅ Forge suite land_blocking success on 0a0e99eee17535b391ab7371c2144220068ee62a

github-actions bot commented Nov 23, 2022

✅ Forge suite compat success on testnet_2d8b1b57553d869190f61df1aaf7f31a8fc19a7b ==> 0a0e99eee17535b391ab7371c2144220068ee62a

igor-aptos Nov 23, 2022 •

edited

Loading

✅ Forge suite `land_blocking` success on `0a0e99eee17535b391ab7371c2144220068ee62a`

✅ Forge suite `compat` success on `testnet_2d8b1b57553d869190f61df1aaf7f31a8fc19a7b` ==> `0a0e99eee17535b391ab7371c2144220068ee62a`