Bullshark Fallback #30

neilgiri · 2022-03-20T22:13:49Z

Updated code for the fallback path of Bullshark.

Fixed two tests and try steady/fallback commit Fixed tests Fixed rust build Fixed formatting

asonnino · 2022-03-21T12:26:37Z

Just a quick note: I would recommend against merging this PR (containing the code of the fallback) into the Bullshark branch, it will be better to create a new branch for the fallback.

asonnino

don't forget to run cargo fmt and to remove dead code :)
Do the test names match the test content? It seems you made some modifications there. We should keep the original tests (designed for the common case), and add tests for the fallback.

asonnino · 2022-03-21T12:28:39Z

consensus/src/lib.rs

+            .fb_validator_sets
+            .entry(fb_wave)
+            .or_insert(BTreeSet::new());
+        //println!("commit round {} fbwave {} size {}", certificate.round(), fb_wave, fb_sets.len());


asonnino · 2022-03-21T12:28:44Z

consensus/src/lib.rs

+            .map(|(_, x)| self.committee.stake(&x.origin()))
+            .sum();
+
+        //println!("fb stake {}", stake);


asonnino · 2022-03-21T12:28:49Z

consensus/src/lib.rs

+            .filter(|(_, x)| ss_sets.contains(&x.origin()))
+            .map(|(_, x)| self.committee.stake(&x.origin()))
+            .sum();
+        //println!("ss stake {} round {} sswave {} size {}", stake, certificate.round(), ss_wave, ss_sets.len());


asonnino · 2022-03-21T12:28:53Z

consensus/src/lib.rs

+            .entry(fb_wave)
+            .or_insert(BTreeSet::new())
+            .insert(certificate.origin());
+        //println!("fb_size round {} wave {} size {}", certificate.round(), fb_wave, state.fb_validator_sets.entry(fb_wave).or_insert(BTreeSet::new()).len());


asonnino · 2022-03-21T12:29:37Z

consensus/src/lib.rs

+        let ss_wave = ((certificate.round() as f64) / 2.0).ceil() as u64;
+        let fb_wave = ((certificate.round() as f64) / 4.0).ceil() as u64;


Let's avoid float arithmetic, we should be able to work with u64

asonnino · 2022-03-21T12:33:46Z

consensus/src/tests/consensus_tests.rs

@@ -118,12 +118,13 @@ async fn commit_one() {

    // Ensure the first 4 ordered certificates are from round 1 (they are the parents of the committed
    // leader); then the leader's certificate should be committed.
-    for _ in 1..=4 {
+    /*for _ in 1..=1 {


asonnino · 2022-03-21T12:33:59Z

consensus/src/tests/consensus_tests.rs

@@ -133,7 +134,7 @@ async fn dead_node() {
    // Make the certificates.
    let mut keys: Vec<_> = keys().into_iter().map(|(x, _)| x).collect();
    keys.sort(); // Ensure we don't remove one of the leaders.
-    let _ = keys.pop().unwrap();
+                 //let _ = keys.pop().unwrap();


asonnino · 2022-03-21T12:34:10Z

consensus/src/tests/consensus_tests.rs

+    //let (_, certificate) = mock_certificate(keys[1], 5, parents);
+    //certificates.push_back(certificate);


asonnino · 2022-03-21T12:34:14Z

consensus/src/tests/consensus_tests.rs

@@ -252,7 +258,7 @@ async fn not_enough_support() {
    }

    // We should commit 2 leaders (rounds 2 and 4).
-    for _ in 1..=3 {
+    /*for _ in 1..=3 {


asonnino · 2022-03-21T12:34:21Z

consensus/src/tests/consensus_tests.rs

 }

-// Run for 7 dag rounds. Node 0 (the leader of round 2) is missing for rounds 1 and 2,
+// Run for 5 dag rounds. Node 0 (the leader of round 2) is missing for rounds 1 and 2,


Was testing the fallback commit code path. Since the fallback wave 1 consists of rounds 1-4, we run for 5 DAG rounds so that the certificate in round 5 triggers a fallback commit.

asonnino · 2022-03-28T15:55:28Z

consensus/src/lib.rs

+        let ss_wave = certificate.round() / 2;
+        let fb_wave = certificate.round() / 4;


Isn't there a missing offset of 1? (if not, why were you using 'ceil'?)

I changed all of the wave indices to be 0-indexed, which means that ceil is no longer needed. For example, round 1 before corresponded to steady state wave 1 (ceil(round / 2)) whereas now it corresponds to steady state wave 0.

asonnino · 2022-03-28T15:56:29Z

consensus/src/lib.rs

-        // in the fallback
-        let fb_leader = self.try_fallback_commit(&certificate, max(fb_wave - 1, 1), state);
+        // in the current steady state wave
+        let fb_leader = self.try_fallback_commit(&certificate, fb_wave - 1, state);


Can't this underflow?

No, since if fb_wave = 0 then either ss_wave = 0 or ss_wave = 1. If ss_wave is 0 then the function returns None from if statement from before. If ss_wave = 1 then the mode is already determined since all validators begin from steady state wave 1, and so the function will return None. I will add an explicit underflow check to be safe.

asonnino · 2022-03-28T15:56:36Z

consensus/src/lib.rs


        if state
            .fb_validator_sets
-            .entry(max(fb_wave - 1, 1))
+            .entry(fb_wave - 1)


same comment

asonnino · 2022-03-28T15:58:31Z

consensus/src/tests/consensus_tests.rs

 }

-// Run for 5 dag rounds. The leaders of round 2 does not have enough support, but the leader of
-// round 4 does. The leader of rounds 2 and 4 should thus be committed.
+// Run for 7 dag rounds. The leaders of round 2 does not have enough support (f+1 votes), but the leader of


Isn't the threshold back to 2f+1 now that we have a fallback?

Yes the threshold is 2f+1 but once the fallback leader in fallback wave 1 (leader of round 4) is committed we must check whether earlier leaders may have been committed. We thus commit the steady state leader of round 2 (steady state wave 1) since there are f+1 votes for it and less than f+1 votes for the fallback leader of fallback wave 0.

Added first draft implementation of bullshark async

f40d2c2

Fixed two tests and try steady/fallback commit Fixed tests Fixed rust build Fixed formatting

facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Mar 20, 2022

asonnino suggested changes Mar 21, 2022

View reviewed changes

neilgiri added 2 commits March 21, 2022 15:57

Fixed formatting/test, zero-indexed indices

0d58c8a

Fixed fallback bug, added additional tests

ff47eee

asonnino reviewed Mar 28, 2022

View reviewed changes

neilgiri added 4 commits March 30, 2022 21:16

Updated to match pseudocode

1808bc9

Updated to match new pseudocode

abc62a3

Revert back to previous commit because of new pseudocode typo

1a7c2b7

Updated to match pseudocode's new indexing

cbb1d21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bullshark Fallback #30

Bullshark Fallback #30

neilgiri commented Mar 20, 2022

asonnino commented Mar 21, 2022

asonnino left a comment

asonnino Mar 21, 2022

asonnino Mar 21, 2022

asonnino Mar 21, 2022

asonnino Mar 21, 2022

asonnino Mar 21, 2022

asonnino Mar 21, 2022

asonnino Mar 21, 2022

asonnino Mar 21, 2022

asonnino Mar 21, 2022

asonnino Mar 21, 2022

neilgiri Mar 21, 2022

asonnino Mar 28, 2022

neilgiri Mar 29, 2022

asonnino Mar 28, 2022

neilgiri Mar 29, 2022

asonnino Mar 28, 2022

asonnino Mar 28, 2022

neilgiri Mar 29, 2022

		let ss_wave = ((certificate.round() as f64) / 2.0).ceil() as u64;
		let fb_wave = ((certificate.round() as f64) / 4.0).ceil() as u64;

		//let (_, certificate) = mock_certificate(keys[1], 5, parents);
		//certificates.push_back(certificate);

		let ss_wave = certificate.round() / 2;
		let fb_wave = certificate.round() / 4;

Bullshark Fallback #30

Are you sure you want to change the base?

Bullshark Fallback #30

Conversation

neilgiri commented Mar 20, 2022

asonnino commented Mar 21, 2022

asonnino left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment