Ed/timeout #1794

elliedavidson · 2023-09-21T19:26:56Z

This PR:

Starts hotshot with a QCFormed event instead of ViewChange event
Updates view logic such that replicas enter new views when they see a QC, TC, or ViewSyncCert for the previous view. They no longer increment their view after successfully voting on a proposal (see this thread here for more detail)
Updates view sync logic to handle the new view change logic - view sync is now triggered after 3 timeouts: 1st timeout indicates we didn't see a QC for the next view and should send a timeout certificate, second timeout is waiting for said timeout certificate, third time is when a second leader in a row is unresponsive, meaning we should view sync
Removes lots of dead code
Replaces "2" types with their native types
Updates VoteType trait with more getter functions
Renames from_signatures_and_commitment to create_certificate
Adds a TimeoutExchange to facilitate TimeoutVotes but does not expose it outside of NodeImplementation. We can do the same for the other exchanges in this issue: Remove Exchanges #1799
Adds timeout tests for both the web server and libp2p networks

This PR does not:

Accumulate accumulators: Merge DA / Timeout Accumulators into a single default accumulator #1797. We should move toward a single default accumulator implementation to get rid of redundant code
Unit tests for timeout: Unit tests for Timeout #1798
Fix previous known inefficiencies in the consensus code (even though it touches most of that code) since that would be too large a PR

Use just async_std test_timeout to run the timeout test

…w of qc

…eout message

bfish713 · 2023-10-11T19:24:38Z

crates/task-impls/src/consensus.rs

+                let Some(parent) = parent else {
+                    error!(
+                        "Proposal's parent missing from storage with commitment: {:?}",
+                        justify_qc.leaf_commitment()
+                    );
+                    return;


This will reintroduce the error we fix last week, the replica will not be able to vote and timeout the next view because the proposal isn't stored.

Latest push should fix this. Instead of return right away here, it first updates the state map and then returns. Let me know if it still incorrect.

crates/task-impls/src/da.rs

bfish713 · 2023-10-11T19:37:15Z

crates/testing/tests/timeout.rs

+    // TODO ED Reduce down to 5 nodes once memory network issues is resolved
+    // https://github.com/EspressoSystems/HotShot/issues/1790
+    let mut metadata = TestMetadata {
+        total_nodes: 10,
+        start_nodes: 10,
+        ..Default::default()
+    };


I think these issues are fixed? Can we run this with 5 nodes or are you still hitting an issue?

I still get the same error. I actually ran the timeout test on the develop branch. Looks like the same thing happens. The test works with the web server network but fails with the memory network. Looking at the logs it looks like some messages just don't make it to the nodes like they are supposed to.

Hmm ok I'll look into this one

I did update the timeout_test to have both libp2p and the webserver, though. Just to ensure something weird wasn't also happening with one of them.

crates/testing/tests/timeout.rs

bfish713 · 2023-10-11T20:02:11Z

crates/types/src/vote.rs

-/// respectively.
-///
-/// TODO GG used only in election.rs; move this to there and make it private?
-pub struct VoteAccumulator<TOKEN, COMMITMENT: CommitmentBounds, TYPES: NodeType> {


Love getting rid of this!

bfish713 · 2023-10-11T20:02:56Z

crates/types/src/vote.rs

-    TOKEN: Clone + VoteToken,
-{
-    #![allow(clippy::too_many_lines)]
-    fn append(


I see append implemented for TimeCertificate, did we already have it for the other certificate types, I can't find it in this PR and am confused since it obviously works

Yes, they were in a previous PR!

dailinsubjam

Except for some dead codes, all LGTM!

dailinsubjam · 2023-10-12T15:09:24Z

crates/hotshot-signature-key/src/bn254/bn254_priv.rs

@@ -54,6 +54,7 @@ impl BLSPrivKey {
    }
 }

+// #[allow(clippy::incorrect_partial_ord_impl_on_ord_type)]


Do we want to get rid of this dead code?

I kept this in because my local clippy needed it. I expect once CI's clippy is upgraded we'll need to uncomment these. :)

dailinsubjam · 2023-10-12T15:09:53Z

crates/hotshot-signature-key/src/bn254/bn254_pub.rs

@@ -27,6 +27,7 @@ pub struct BLSPubKey {
    pub_key: VerKey,
 }

+// #[allow(clippy::incorrect_partial_ord_impl_on_ord_type)]


Also dead code here?

dailinsubjam · 2023-10-12T15:12:07Z

crates/libp2p-networking/src/network/node/handle.rs

@@ -136,6 +136,9 @@ impl<S: Default + Debug> NetworkNodeHandle<S> {
    ///
    /// Will panic if a handler is already spawned
    #[allow(clippy::unused_async)]
+    // // Tokio and async_std disagree how this function should be linted
+    // #[allow(clippy::ignored_unit_patterns)]


Also dead code here?

bfish713 · 2023-10-12T19:37:26Z

crates/task-impls/src/consensus.rs

-                                error!(?proposal.signature, "Could not verify proposal.");
-                                return;
-                            }
+                    return;


In the fix branch we were updating the view here to avoid timing out in the view that might have actually been successful. I'm not 100% sure either way so I'll leave up to you. If we don't update the view like in this PR, then the view will time out and the network might move on to future views without us and we'll be stuck trying to catch back up.

If we do update the view then we might time out on a future view when everyone else times out because this is a bogus QC that nobody voted on.

If after this is deployed and we see one node constantly behind, this is probably the culprit. On the other hand if we see nodes entering view sync at different times in the current deployment this change would fix it.

elliedavidson added 30 commits September 18, 2023 14:17

TimeoutCert trait and TimeoutVote2 trait done

56b6663

TimeoutExchange wireframe

9aa591b

Add TimeoutExchangeType trait

95f1aac

Compiles with timeout_exchange added to consensus task

1acb8a2

Rough addition of create_timeout_message on timeout_exchange

2d55199

Add Timeout events to networking task

809a27a

Consensus task properly recieves TimeoutVotes

14e5336

Add timeout exchange and accumulator to consensus task, compiles

d428557

Comments

81dd019

Vote collection is receiving timeout votes correctly

112a404

Bug - proposals after timeout don't have timeout cert and have too lo…

f6968ce

…w of qc

Fix previous bug

a519b8e

view change location updated; need to handle update view sent for tim…

5d3c145

…eout message

Some bugs fixed, still triggering view sync when it shouldn't

409e8ef

Memory network is not receiving DA proposals correctly

a3017eb

Update timeout test to use libp2p

98c397d

comments

76e64cf

Fix test_consensus_task test after timeout logic update

5cdcff5

test_consensus_vote working

ef68488

test_basic passes

037de03

All tests passing?

89a14a3

merge with main

702dbd9

Tests pass?

70da94b

Cleaning up TODOs

86183cb

More todos

a9eb94e

Fix lints

e9d1424

rename functions

3fe3d22

Lint imports

7767f5e

Additional cleanup

24c6b22

Additional cleanup

a8027c7

elliedavidson removed request for dailinsubjam, shenkeyao and rob-maron October 11, 2023 18:34

remove get_commit func

fba4909

elliedavidson requested review from shenkeyao, bfish713, von-steinkirch, dailinsubjam and rob-maron October 11, 2023 19:07

bfish713 requested changes Oct 11, 2023

View reviewed changes

elliedavidson added 15 commits October 11, 2023 17:37

Fix subtraction overflow

5e01be8

Merge with develop

f684c3e

Update state map even if no parent

5eb3afa

Test timeout on both libp2p and web

cf095c4

lint

721abe5

comments

b3fb79b

update metrics

1e7eb39

metric updates

bdd8012

Create own timeout cert validation function

8acca28

lints:

944c98a

Comments

dff85fd

Comments

914e890

lints

0fb23e3

Add return if proposal is invliad

689f959

lints

6fd0f40

dailinsubjam approved these changes Oct 12, 2023

View reviewed changes

bfish713 approved these changes Oct 12, 2023

View reviewed changes

elliedavidson merged commit 9c5c83c into develop Oct 12, 2023

elliedavidson deleted the ed/timeout branch October 12, 2023 19:36

bfish713 reviewed Oct 12, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ed/timeout #1794

Ed/timeout #1794

elliedavidson commented Sep 21, 2023 •

edited

Loading

bfish713 Oct 11, 2023

elliedavidson Oct 12, 2023

bfish713 Oct 11, 2023

elliedavidson Oct 12, 2023

bfish713 Oct 12, 2023

elliedavidson Oct 12, 2023

bfish713 Oct 11, 2023

bfish713 Oct 11, 2023

elliedavidson Oct 12, 2023

dailinsubjam left a comment

dailinsubjam Oct 12, 2023

elliedavidson Oct 12, 2023

dailinsubjam Oct 12, 2023

dailinsubjam Oct 12, 2023

bfish713 Oct 12, 2023

bfish713 Oct 12, 2023

Ed/timeout #1794

Ed/timeout #1794

Conversation

elliedavidson commented Sep 21, 2023 • edited Loading

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

dailinsubjam 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

elliedavidson commented Sep 21, 2023 •

edited

Loading