Skip leader slots until a vote lands #15607

sakridge · 2021-03-01T22:13:31Z

Problem

Nodes starting up after clearing their ledger can double sign for a slot when they start their leader slot again and are not aware they already produced a block for that slot.

Summary of Changes

Check to see if we have landed a vote since starting the node and wait to produce leader slots until then.

Fixes #

sakridge · 2021-03-01T23:00:59Z

@carllin does it look reasonable at all?

core/src/replay_stage.rs

carllin · 2021-03-01T23:47:39Z

hmmm so if somebody is blowing away their ledger and restarting, I'm not sure this would help, i.e.

0 (root)----1---2---3 (leader slot)

They then blow away the ledger, vote on slot 1, set has_voted to true on slot 2, and recreate another version of the leader slot on slot 3.

sakridge · 2021-03-01T23:51:14Z

hmmm so if somebody is blowing away their ledger and restarting, I'm not sure this would help, i.e.

0 (root)----1---2---3 (leader slot)

They then blow away the ledger, vote on slot 1, set has_voted to true on slot 2, and recreate another version of the leader slot on slot 3.

Slot 2 will be frozen already if they created slot 3 on top of it. So they won't be able to land their vote for slot 1 in slot 2. The has_voted tower state maybe is the problem. It actually does need to check the specific vote signatures to see if they landed when it came up the 2nd time.

carllin · 2021-03-01T23:59:05Z

hmmm so if somebody is blowing away their ledger and restarting, I'm not sure this would help, i.e.
0 (root)----1---2---3 (leader slot)
They then blow away the ledger, vote on slot 1, set has_voted to true on slot 2, and recreate another version of the leader slot on slot 3.

Slot 2 will be frozen already if they created slot 3 on top of it. So they won't be able to land their vote for slot 1 in slot 2. The has_voted tower state maybe is the problem. It actually does need to check the specific vote signatures to see if they landed when it came up the 2nd time.

yeah it seems at the very least you'll have to check that the vote signature landed in a rooted fork to ensure you're near the tip.

Also of note, I think there's still an edge case if you were a bit ahead of the major fork on a different fork, i.e.

             1-2----20 (your leader slot) (minor fork)
         /
(root)
         \
            3---4-- 5 (major fork) --RESTART-- 6---7

If you restart on the major fork and vote on slot 6, which lands in slot 7, you may recreate your vote slot again for slot 20 on the major fork.

mvines · 2021-03-02T00:53:15Z

I was thinking we'd suppress this behavior when --wait-for-supermajority is given for the restart case

sakridge · 2021-03-13T18:22:40Z

I need to handle the case where the node is the sole bootstrap leader or maybe 1 of many or maybe any case where the node might not be able to land a vote and needs to produce slots anyway. I was trying to think if there is an elegant way to figure that out.

codecov · 2021-03-14T01:00:52Z

Codecov Report

Merging #15607 (dac0512) into master (e817a6d) will increase coverage by 0.0%.
The diff coverage is 72.0%.

@@           Coverage Diff           @@
##           master   #15607   +/-   ##
=======================================
  Coverage    80.0%    80.0%           
=======================================
  Files         410      410           
  Lines      109070   109102   +32     
=======================================
+ Hits        87338    87389   +51     
+ Misses      21732    21713   -19

t-nelson

lgtm with nits!

t-nelson · 2021-03-19T19:15:51Z

core/src/replay_stage.rs

@@ -293,6 +296,8 @@ impl ReplayStage {
                let mut partition_exists = false;
                let mut skipped_slots_info = SkippedSlotsInfo::default();
                let mut replay_timing = ReplayTiming::default();
+                let mut voted_signatures = Vec::new();


Suggested change

let mut voted_signatures = Vec::new();

let mut voted_signatures = Vec::with_capacity(201);

I chose a 200 signature limit for how large this can be. I'm not sure we need to size it initially, most cases should not use the full 200 and the path isn't that performance sensitive.

validator/src/main.rs

core/src/replay_stage.rs

carllin · 2021-03-20T00:20:24Z

validator/src/main.rs

@@ -1360,6 +1360,13 @@ pub fn main() {
                .help("After processing the ledger and the next slot is SLOT, wait until a \
                       supermajority of stake is visible on gossip before starting PoH"),
        )
+        .arg(
+            Arg::with_name("no_wait_for_vote_to_start_leader")


might be good to add a warning here that if > 33% of the cluster goes down and restarts, everyone will need to set this flag, otherwise even on restart nobody will make progress.

If 33% goes down, we have to do a manual restart with --wait-for-supermajority flag, right? In that case we detect that we had to wait for supermajority and we skip this check for the vote. So, it shouldn't be necessary to use it in that case. The only case we need it is if you are starting a bootstrap leader(s) without --wait-for-supermajority

core/src/validator.rs

carllin · 2021-03-20T00:32:53Z

core/src/validator.rs

@@ -627,16 +629,19 @@ impl Validator {
            check_poh_speed(&genesis_config, None);
        }

-        if wait_for_supermajority(
+        let (failed, did_wait) = wait_for_supermajority(


nit: did_wait -> is_wait_unnecessary, since in some cases like on hard forks, a wait didn't occur but is necessary (or even better is_wait_necessary, but then we have to flip the booleans around below)

carllin · 2021-03-20T00:40:30Z

core/src/validator.rs

    if let Some(wait_for_supermajority) = config.wait_for_supermajority {
        match wait_for_supermajority.cmp(&bank.slot()) {
-            std::cmp::Ordering::Less => return false,
+            std::cmp::Ordering::Less => return (false, false),


is the second bool equal to false ever an acceptable case? I.e. if everyone restarts together and is waiting for their vote to land in a root, nobody will vote, and so everyone will stall right?

did_wait can be false, yes. Because nodes could still have --wait-for-supermajority even when the network has passed the wait slot given and they are trying to join the network. They wouldn't wait at all because the slot they loaded from is already past the wait-for slot and the shred version matches. In this case they should wait to vote because they have joined the network that is already started producing blocks.

core/src/replay_stage.rs

mvines

I bet we'll need a no_wait_for_vote_to_start_leader: true in this config block as well for TestValidator:

solana/core/src/test_validator.rs

Lines 402 to 426 in d76ad33

    
           let validator_config = ValidatorConfig { 
        
               rpc_addrs: Some(( 
        
                   SocketAddr::new(IpAddr::V4(Ipv4Addr::new(0, 0, 0, 0)), node.info.rpc.port()), 
        
                   SocketAddr::new( 
        
                       IpAddr::V4(Ipv4Addr::new(0, 0, 0, 0)), 
        
                       node.info.rpc_pubsub.port(), 
        
                   ), 
        
               )), 
        
               rpc_config, 
        
               accounts_hash_interval_slots: 100, 
        
               account_paths: vec![ledger_path.join("accounts")], 
        
               poh_verify: false, // Skip PoH verification of ledger on startup for speed 
        
               snapshot_config: Some(SnapshotConfig { 
        
                   snapshot_interval_slots: 100, 
        
                   snapshot_path: ledger_path.join("snapshot"), 
        
                   snapshot_package_output_path: ledger_path.to_path_buf(), 
        
                   archive_format: ArchiveFormat::Tar, 
        
                   snapshot_version: SnapshotVersion::default(), 
        
               }), 
        
               enforce_ulimit_nofile: false, 
        
               warp_slot: config.warp_slot, 
        
               bpf_jit: !config.no_bpf_jit, 
        
               validator_exit: config.validator_exit.clone(), 
        
               ..ValidatorConfig::default() 
        
           };

sakridge · 2021-03-23T01:11:58Z

I bet we'll need a no_wait_for_vote_to_start_leader: true in this config block as well for TestValidator:

solana/core/src/test_validator.rs

Lines 402 to 426 in d76ad33

let validator_config = ValidatorConfig {

rpc_addrs: Some((

SocketAddr::new(IpAddr::V4(Ipv4Addr::new(0, 0, 0, 0)), node.info.rpc.port()),

SocketAddr::new(

IpAddr::V4(Ipv4Addr::new(0, 0, 0, 0)),

node.info.rpc_pubsub.port(),

),

)),

rpc_config,

accounts_hash_interval_slots: 100,

account_paths: vec![ledger_path.join("accounts")],

poh_verify: false, // Skip PoH verification of ledger on startup for speed

snapshot_config: Some(SnapshotConfig {

snapshot_interval_slots: 100,

snapshot_path: ledger_path.join("snapshot"),

snapshot_package_output_path: ledger_path.to_path_buf(),

archive_format: ArchiveFormat::Tar,

snapshot_version: SnapshotVersion::default(),

}),

enforce_ulimit_nofile: false,

warp_slot: config.warp_slot,

bpf_jit: !config.no_bpf_jit,

validator_exit: config.validator_exit.clone(),

..ValidatorConfig::default()

};

added, thanks

(cherry picked from commit b99ae8f) # Conflicts: # core/src/consensus.rs # core/src/replay_stage.rs

(cherry picked from commit b99ae8f)

(cherry picked from commit b99ae8f) Co-authored-by: sakridge <[email protected]>

behzadnouri · 2021-03-26T20:09:37Z

Should have multinode-demo/validator.sh also been updated?
Starting a gce cluster shows that nodes get stuck in

INFO  solana_core::replay_stage] Haven't landed a vote, so skipping my leader slot

sakridge · 2021-03-26T20:34:07Z

Should have multinode-demo/validator.sh also been updated?
Starting a gce cluster shows that nodes get stuck in
INFO  solana_core::replay_stage] Haven't landed a vote, so skipping my leader slot

let me look

sakridge · 2021-03-26T21:43:48Z

Should have multinode-demo/validator.sh also been updated?
Starting a gce cluster shows that nodes get stuck in
INFO  solana_core::replay_stage] Haven't landed a vote, so skipping my leader slot

I don't think that is the cause of it. Those validators are not staked at the beginning, so they can wait to land a rooted vote. I had that print in a bad spot but: #16156 should fix it.

This reverts commit b99ae8f.

behzadnouri · 2021-03-26T23:07:20Z

I see buildkite/solana/stable failing but it passes when I revert this commit.
Also on gce it seems to have impacted replay metrics.
Left is master, right is this commit reverted.

replay-slot-stats::total_shreds:

replay_stage-replay-transactions:

sakridge · 2021-03-26T23:11:23Z

I see buildkite/solana/stable failing but it passes when I revert this commit.
Also on gce it seems to have impacted replay metrics.
Left is master, right is this commit reverted.

can you try with #16156

behzadnouri · 2021-03-26T23:15:58Z

#16156 is also failing buildkite/solana/stable:
https://buildkite.com/solana-labs/solana/builds/42989

This reverts commit b99ae8f.

sakridge · 2021-03-26T23:36:56Z

Stabled passed here with it:
https://buildkite.com/solana-labs/solana/builds/42978
https://buildkite.com/solana-labs/solana/builds/42993
https://buildkite.com/solana-labs/solana/builds/42970
https://buildkite.com/solana-labs/solana/builds/42962
https://buildkite.com/solana-labs/solana/builds/42962

It looks like a flaky condition in the test to me, it shouldn't really affect that test. The test doesn't actually trigger any of the modified paths of this PR.

carllin reviewed Mar 1, 2021

View reviewed changes

core/src/replay_stage.rs Outdated Show resolved Hide resolved

sakridge force-pushed the leader-start-until-vote branch from 8aa468f to f5cfa8f Compare March 1, 2021 23:47

sakridge force-pushed the leader-start-until-vote branch 3 times, most recently from ffa2626 to e6c7867 Compare March 2, 2021 04:29

sakridge added the CI Pull Request is ready to enter CI label Mar 2, 2021

sakridge force-pushed the leader-start-until-vote branch from e6c7867 to a78cb26 Compare March 2, 2021 04:38

solana-grimes removed the CI Pull Request is ready to enter CI label Mar 2, 2021

sakridge added the CI Pull Request is ready to enter CI label Mar 2, 2021

sakridge force-pushed the leader-start-until-vote branch from a78cb26 to e654f4b Compare March 13, 2021 17:56

solana-grimes removed the CI Pull Request is ready to enter CI label Mar 13, 2021

sakridge force-pushed the leader-start-until-vote branch 2 times, most recently from eab57d7 to 73dced3 Compare March 13, 2021 23:00

sakridge marked this pull request as ready for review March 14, 2021 00:31

sakridge requested review from carllin and t-nelson March 16, 2021 23:34

t-nelson previously approved these changes Mar 19, 2021

View reviewed changes

carllin reviewed Mar 20, 2021

View reviewed changes

core/src/replay_stage.rs Outdated Show resolved Hide resolved

carllin reviewed Mar 20, 2021

View reviewed changes

core/src/validator.rs Outdated Show resolved Hide resolved

carllin reviewed Mar 20, 2021

View reviewed changes

sakridge force-pushed the leader-start-until-vote branch from 73dced3 to d77e912 Compare March 20, 2021 20:45

sakridge force-pushed the leader-start-until-vote branch from d77e912 to 3652411 Compare March 23, 2021 00:49

mvines reviewed Mar 23, 2021

View reviewed changes

core/src/replay_stage.rs Outdated Show resolved Hide resolved

mvines reviewed Mar 23, 2021

View reviewed changes

sakridge force-pushed the leader-start-until-vote branch from 3652411 to e37884f Compare March 23, 2021 01:05

sakridge force-pushed the leader-start-until-vote branch from e37884f to 220b5bd Compare March 23, 2021 19:05

sakridge added the v1.6 label Mar 23, 2021

sakridge closed this Mar 25, 2021

sakridge force-pushed the leader-start-until-vote branch from 220b5bd to 2aea352 Compare March 25, 2021 17:21

sakridge reopened this Mar 25, 2021

Skip leader slots until a vote lands

dac0512

sakridge force-pushed the leader-start-until-vote branch from 29ac6eb to dac0512 Compare March 25, 2021 19:48

sakridge merged commit b99ae8f into solana-labs:master Mar 26, 2021

sakridge deleted the leader-start-until-vote branch March 26, 2021 01:54

mergify bot pushed a commit that referenced this pull request Mar 26, 2021

Skip leader slots until a vote lands (#15607)

2119e5e

(cherry picked from commit b99ae8f) # Conflicts: # core/src/consensus.rs # core/src/replay_stage.rs

mergify bot mentioned this pull request Mar 26, 2021

Skip leader slots until a vote lands (bp #15607) #16147

Merged

sakridge added a commit that referenced this pull request Mar 26, 2021

Skip leader slots until a vote lands (#15607)

4e26266

(cherry picked from commit b99ae8f)

mergify bot added a commit that referenced this pull request Mar 26, 2021

Skip leader slots until a vote lands (#15607) (#16147)

d940c5b

(cherry picked from commit b99ae8f) Co-authored-by: sakridge <[email protected]>

behzadnouri added a commit to behzadnouri/solana that referenced this pull request Mar 26, 2021

Revert "Skip leader slots until a vote lands (solana-labs#15607)"

e04587a

This reverts commit b99ae8f.

behzadnouri added a commit to behzadnouri/solana that referenced this pull request Mar 26, 2021

Revert "Skip leader slots until a vote lands (solana-labs#15607)"

088a2a9

This reverts commit b99ae8f.

sakridge added a commit to sakridge/solana that referenced this pull request Mar 26, 2021

Revert "Skip leader slots until a vote lands (solana-labs#15607)"

52000a2

This reverts commit b99ae8f.

t-nelson mentioned this pull request May 5, 2021

Catchup & RPC getHealth Results Do Not Match #16957

Closed

brooksprumo mentioned this pull request Aug 23, 2021

backport 19361 v16 #19379

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Skip leader slots until a vote lands #15607

Skip leader slots until a vote lands #15607

sakridge commented Mar 1, 2021

sakridge commented Mar 1, 2021

carllin commented Mar 1, 2021 •

edited

Loading

sakridge commented Mar 1, 2021

carllin commented Mar 1, 2021 •

edited

Loading

mvines commented Mar 2, 2021

sakridge commented Mar 13, 2021

codecov bot commented Mar 14, 2021 •

edited

Loading

t-nelson left a comment

t-nelson Mar 19, 2021

mvines Mar 23, 2021

sakridge Mar 23, 2021

carllin Mar 20, 2021

sakridge Mar 20, 2021 •

edited

Loading

carllin Mar 20, 2021

carllin Mar 20, 2021

sakridge Mar 20, 2021

mvines left a comment

sakridge commented Mar 23, 2021

behzadnouri commented Mar 26, 2021

sakridge commented Mar 26, 2021

sakridge commented Mar 26, 2021

behzadnouri commented Mar 26, 2021

sakridge commented Mar 26, 2021 •

edited

Loading

behzadnouri commented Mar 26, 2021

sakridge commented Mar 26, 2021

	let mut voted_signatures = Vec::new();
	let mut voted_signatures = Vec::with_capacity(201);

	let validator_config = ValidatorConfig {
	rpc_addrs: Some((
	SocketAddr::new(IpAddr::V4(Ipv4Addr::new(0, 0, 0, 0)), node.info.rpc.port()),
	SocketAddr::new(
	IpAddr::V4(Ipv4Addr::new(0, 0, 0, 0)),
	node.info.rpc_pubsub.port(),
	),
	)),
	rpc_config,
	accounts_hash_interval_slots: 100,
	account_paths: vec![ledger_path.join("accounts")],
	poh_verify: false, // Skip PoH verification of ledger on startup for speed
	snapshot_config: Some(SnapshotConfig {
	snapshot_interval_slots: 100,
	snapshot_path: ledger_path.join("snapshot"),
	snapshot_package_output_path: ledger_path.to_path_buf(),
	archive_format: ArchiveFormat::Tar,
	snapshot_version: SnapshotVersion::default(),
	}),
	enforce_ulimit_nofile: false,
	warp_slot: config.warp_slot,
	bpf_jit: !config.no_bpf_jit,
	validator_exit: config.validator_exit.clone(),
	..ValidatorConfig::default()
	};

Skip leader slots until a vote lands #15607

Skip leader slots until a vote lands #15607

Conversation

sakridge commented Mar 1, 2021

Problem

Summary of Changes

sakridge commented Mar 1, 2021

carllin commented Mar 1, 2021 • edited Loading

sakridge commented Mar 1, 2021

carllin commented Mar 1, 2021 • edited Loading

mvines commented Mar 2, 2021

sakridge commented Mar 13, 2021

codecov bot commented Mar 14, 2021 • edited Loading

Codecov Report

t-nelson left a comment

Choose a reason for hiding this comment

t-nelson Mar 19, 2021

Choose a reason for hiding this comment

mvines Mar 23, 2021

Choose a reason for hiding this comment

sakridge Mar 23, 2021

Choose a reason for hiding this comment

carllin Mar 20, 2021

Choose a reason for hiding this comment

sakridge Mar 20, 2021 • edited Loading

Choose a reason for hiding this comment

carllin Mar 20, 2021

Choose a reason for hiding this comment

carllin Mar 20, 2021

Choose a reason for hiding this comment

sakridge Mar 20, 2021

Choose a reason for hiding this comment

mvines left a comment

Choose a reason for hiding this comment

sakridge commented Mar 23, 2021

behzadnouri commented Mar 26, 2021

sakridge commented Mar 26, 2021

sakridge commented Mar 26, 2021

behzadnouri commented Mar 26, 2021

sakridge commented Mar 26, 2021 • edited Loading

behzadnouri commented Mar 26, 2021

sakridge commented Mar 26, 2021

carllin commented Mar 1, 2021 •

edited

Loading

carllin commented Mar 1, 2021 •

edited

Loading

codecov bot commented Mar 14, 2021 •

edited

Loading

sakridge Mar 20, 2021 •

edited

Loading

sakridge commented Mar 26, 2021 •

edited

Loading