Lr/eqc voting #3851

lukaszrzasik · 2024-11-05T13:38:42Z

This PR:

Introduces the concept of "epoch safety check". The epoch safety check makes sure that:
- the proposed block and its justify QC's block are from the same epoch or
- the justify QC is an Extended Quorum Certificate for the previous epoch.
Adds logic to broadcast the vote for the last proposal in eQC 3-chain.
Adds logic to not check whether the node is the leader when receiving a vote for the last proposal in eQC 3-chain. This allows each node to form the eQC on their own.

This PR does not:

Key places to review:

… last block

* tests and CI * remove `async-std` * `fmt` * fix doc build * remove counter tests * remove counter tests * build w/o lockfile * `lock` -> `std` * Revert "`lock` -> `std`" This reverts commit 21ebf05. * lock * `async_broadcast` * overflow * Revert "`async_broadcast`" This reverts commit f03bb57. * `try_send`

lukaszrzasik · 2024-11-08T17:26:56Z

crates/types/src/consensus.rs

+        if parent_leaf.height() == 0 {
+            return true;
+        }


I'm not sure but this is probably not enough. Is there a way to check if the leaf is the genesis leaf?

maybe just check if proposed_leaf.justify_qc is the gensis qc?

I think maybe the only way is if the QC view number is the genesis view TYPES::View::genesis

I've changed the condition to use TYPES::View::genesis as you suggested. Getting rid of the magic number is an additional benefit.

bfish713

I think it makes sense, I need to digest a bit more on how all the logic fits together

bfish713 · 2024-11-08T17:37:35Z

crates/task-impls/src/consensus/handlers.rs

+    if is_vote_leaf_extended {
+        // We do not check if we are the leader when receiving a vote for the last eQC proposal.
+        // Every node should receive the votes and form an eQC.
+        tracing::debug!("We have received a vote for the last eQC proposal");
+    } else {
+        // Are we the leader for this view?
+        ensure!(
+            task_state
+                .quorum_membership
+                .leader(vote.view_number() + 1, task_state.cur_epoch)?
+                == task_state.public_key,
+            info!(
+                "We are not the leader for view {:?}",
+                vote.view_number() + 1
+            )
+        );
+    }


nit: ensure!(is_vote_leaf_extended || we_are_leader)

Done, thank you!

bfish713 · 2024-11-08T17:43:12Z

crates/types/src/consensus.rs

+        let Some(View {
+            view_inner: ViewInner::Leaf {
+                leaf: leaf_commit, ..
+            },
+        }) = self.validated_state_map.get(&parent_leaf.view_number())
+        else {
+            tracing::error!("Couldn't find the provided old leaf in the consensus");
+            return false;
+        };


If we have the parent leaf, and need the commit why not just use parent_leaf.commit() instead of looking up in the map (where the leaf presumably already came from)

Done. I guess I got confused by two commit methods existing for the Leaf. I didn't want to use the upgrade lock and make the method async.

bfish713 · 2024-11-08T17:46:06Z

crates/types/src/consensus.rs

@@ -963,6 +966,25 @@ impl<TYPES: NodeType> Consensus<TYPES> {
        let epoch_number = epoch_from_block_number(next_block_number, self.epoch_height);
        Ok(TYPES::Epoch::new(epoch_number))
    }
+
+    /// Returns true if the `parent_leaf` formed an eQC for the previous epoch to the `proposed_leaf`
+    pub fn check_eqc(&self, proposed_leaf: &Leaf<TYPES>, parent_leaf: &Leaf<TYPES>) -> bool {


This function is checking does proposed_leaf contain an eQC as it' justify QC, is that right?

Yes, correct. I could remove the parent_leaf parameter, use the proposed_leaf's justify qc to get the parent leaf from the shared state instead, if it makes more sense.

I was just making sure I was correct, if we already have the parent then it's fine to pass it in, but probably this function makes sense as just starting from the proposed_leaf

bfish713 · 2024-11-08T17:51:29Z

crates/types/src/consensus.rs

+        if parent_leaf.height() == 0 {
+            return true;
+        }


maybe just check if proposed_leaf.justify_qc is the gensis qc?

bfish713 · 2024-11-08T17:53:41Z

crates/task-impls/src/network.rs

+                let is_vote_leaf_extended = self
+                    .consensus
+                    .read()
+                    .await
+                    .is_leaf_extended(vote.data.leaf_commit);
+                let transmit_type = if is_vote_leaf_extended {
+                    tracing::debug!("We're voting for the last eQC proposal. Broadcast the vote.");
+                    TransmitType::Broadcast
+                } else {
+                    let leader = match self.quorum_membership.leader(view_number, self.epoch) {
+                        Ok(l) => l,
+                        Err(e) => {
+                            tracing::warn!(
+                                "Failed to calculate leader for view number {:?}. Error: {:?}",
+                                view_number,
+                                e
+                            );
+                            return None;
+                        }
+                    };
+                    TransmitType::Direct(leader)


I feel like this logic is more appropriate in the quorum_vote task, maybe just add the ExtendedVoteSend and handle that event here instead

This is a good idea. Done, thank you.

bfish713 · 2024-11-08T17:57:13Z

crates/task-impls/src/helpers.rs

+        // The proposal is safe if
+        // 1. the proposed block and the justify QC block belong to the same epoch or
+        // 2. the justify QC is the eQC for the previous block
+        let proposal_epoch =


Do we also need all this logic in the validate_proposal_liveness path, I suppose we can't actually do it there because we are missing the previous proposal but don't we need something to check the epochs?

I think we cannot check the epochs there. We can only calculate the epoch based on the block number. I don't think we have an access to the parent's block number if we don't have the parent leaf. Did I miss something?

I don't think so, I was just trying to be sure we didn't. I also don't think we can do the check in the liveness path, and we won't vote in that case either

ss-es

some minor things that stuck out to me

ss-es · 2024-11-12T16:07:59Z

crates/task-impls/src/network.rs

@@ -399,6 +399,16 @@ impl<
                    TransmitType::Direct(leader),
                ))
            }
+            HotShotEvent::ExtendedQuorumVoteSend(vote) => {


is there a reason we don't want to just use QuorumVoteSend?

Yes, I've followed @bfish713 's comment here. I agree that the logic belongs more to the voting part than the networking part.

ss-es · 2024-11-12T16:16:39Z

crates/task-impls/src/quorum_vote/handlers.rs

@@ -298,6 +298,7 @@ pub(crate) async fn submit_vote<TYPES: NodeType, I: NodeImplementation<TYPES>, V
    storage: Arc<RwLock<I::Storage>>,
    leaf: Leaf<TYPES>,
    vid_share: Proposal<TYPES, VidDisperseShare<TYPES>>,
+    extended_vote: bool,


I'm not sure I understand the reason for this either -- are we planning for the submit_vote to actually be different for an eqc vs a regular qc? it seems like the only difference is we create a different internal hotshot event (which gets broadcast in exactly the same way)

The only difference is that the last vote in 3-chain is broadcast instead of just sent to the next leader. To differentiate between the last vote in the 3-chain against any other vote, we need to check the two leaves up the chain. That logic seems to belong to the voting part instead of networking part. We now use the new ExtendedQuorumVoteSend event to differentiate on the networking part.

lukaszrzasik added 30 commits October 23, 2024 15:50

Initial commit

d941ba9

Propose the same block at the end of the epoch

0ea759a

No VID and DAC required for the additional last block proposals

e6c6492

Merge branch 'main' into lr/epoch-end

0db0add

Guard against division by 0

d074292

Traverse the leaves to check an eQC

8505870

Merge branch 'main' into lr/epoch-end

cff7d76

Remove unneeded async in function definition

5304ca5

Remove trace

c6603af

Merge branch 'main' into lr/epoch-end

de41a5a

Merge branch 'main' into lr/epoch-end

680bab3

Make sure proposal is for the same block if justify QC references the…

bca2f2b

… last block

Merge branch 'main' into lr/epoch-end

4ac1db5

Initial commit

4073351

Gate epoch proposal logic

7f7986d

Gate epochs voting logic

2f3b23b

Create helper method to check if QC is part of 3-chain

eecb9f8

Merge branch 'main' into lr/epoch-end

e733582

Merge branch 'lr/epoch-end' into lr/increment-epoch

8a03eeb

Merge branch 'main' into lr/epoch-end

4a51e13

Merge branch 'main' into lr/increment-epoch

1c8daf0

Update epoch on ViewChange event

1489fa0

Merge branch 'main' into lr/epoch-end

fab49f2

Merge branch 'lr/epoch-end' into lr/increment-epoch

073cca6

Adjust tests

8afbf58

Update view and epoch when QC is formed

615d14e

Fixes after review

224748b

Get rid of nasty nested if-elses

bd0d841

Fix fmt

6cec489

Merge branch 'lr/epoch-end' into lr/increment-epoch

687c5c7

rob-maron and others added 9 commits November 8, 2024 12:00

Fix fmt

e09dd6f

Merge branch 'main' into lr/increment-epoch

a653525

Simplify code

de5a488

Move a helper function to types crate

ab4293a

Merge branch 'lr/increment-epoch' into lr/eqc-voting

4075e5d

Add epoch safety check

7c46afc

Merge branch 'main' into lr/increment-epoch

0330ca4

Merge branch 'lr/increment-epoch' into lr/eqc-voting

4ad900f

lukaszrzasik commented Nov 8, 2024

View reviewed changes

lukaszrzasik marked this pull request as ready for review November 8, 2024 17:27

lukaszrzasik requested a review from bfish713 as a code owner November 8, 2024 17:27

github-actions bot assigned bfish713 and lukeiannucci Nov 8, 2024

lukaszrzasik assigned ss-es and shenkeyao Nov 8, 2024

bfish713 reviewed Nov 8, 2024

View reviewed changes

lukaszrzasik added 5 commits November 12, 2024 12:01

Merge branch 'main' into lr/increment-epoch

8928fb0

Rename storage method

0c70cc6

Clean up the code

3abcbc4

Merge branch 'lr/increment-epoch' into lr/eqc-voting

bda9546

Compare to genesis view

5f181c3

ss-es reviewed Nov 12, 2024

View reviewed changes

lukaszrzasik added 2 commits November 13, 2024 11:39

Merge branch 'main' into lr/increment-epoch

f022612

Merge branch 'lr/increment-epoch' into lr/eqc-voting

355f5a6

bfish713 approved these changes Nov 13, 2024

View reviewed changes

Base automatically changed from lr/increment-epoch to main November 13, 2024 18:13

Merge branch 'main' into lr/eqc-voting

c7fb70e

lukaszrzasik merged commit 32e6970 into main Nov 13, 2024
16 checks passed

lukaszrzasik deleted the lr/eqc-voting branch November 13, 2024 19:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Lr/eqc voting #3851

Lr/eqc voting #3851

lukaszrzasik commented Nov 5, 2024 •

edited

Loading

lukaszrzasik Nov 8, 2024

bfish713 Nov 8, 2024

bfish713 Nov 12, 2024

lukaszrzasik Nov 12, 2024

bfish713 left a comment

bfish713 Nov 8, 2024

lukaszrzasik Nov 12, 2024

bfish713 Nov 8, 2024

lukaszrzasik Nov 12, 2024

bfish713 Nov 8, 2024

lukaszrzasik Nov 12, 2024

bfish713 Nov 12, 2024

bfish713 Nov 8, 2024

bfish713 Nov 8, 2024

lukaszrzasik Nov 12, 2024

bfish713 Nov 8, 2024

lukaszrzasik Nov 12, 2024

bfish713 Nov 12, 2024

ss-es left a comment

ss-es Nov 12, 2024

lukaszrzasik Nov 12, 2024

ss-es Nov 12, 2024

lukaszrzasik Nov 12, 2024

Lr/eqc voting #3851

Lr/eqc voting #3851

Conversation

lukaszrzasik commented Nov 5, 2024 • edited Loading

This PR:

This PR does not:

Key places to review:

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

bfish713 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

ss-es 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

lukaszrzasik commented Nov 5, 2024 •

edited

Loading