Lr/double quorum #3922

lukaszrzasik · 2024-11-26T14:25:04Z

Closes #<ISSUE_NUMBER>

This PR:

Modifies the QuorumProposal and Leaf2 types to include the optional next epoch justify QC.
Adds a new type NextEpochQuorumData2. It's a thin wrapper around QuorumData2. The additional type makes it possible to distinguish between vote and certificate type aliases.
Adds a new hotshot event NextEpochQc2Formed to signal that a QC for has been formed using votes from the nodes in the next epoch.
Adds next epoch vote collectors in the consensus task. The collectors collect votes from the nodes that belong to the next epoch. Given enough votes an additional (next epoch) QC is formed.
Adds logic to save the payload early. The payload is stored as soon as the block is formed and the node is able send the DA proposal. Up until now the payload was stored only after a node received and validated the DA proposal. The payload might be needed earlier to be able to calculate VID shares for the nodes in the next epoch.
Adds logic to attach the next epoch QC to the proposal if the proposal's normal justify QC is for the last block in the epoch.
Modifies the proposal validating logic. Adds additional checks:
- if it's an epoch transition proposal, make sure the next epoch justify QC exists
- make sure the justify QC and the next epoch justify QC are for the same leaf
- validate the signatures on the next epoch justify QC
Modifies the voting logic so that the nodes from the next epoch vote for the last block in the previous epoch.
Adds logic to send the VID shares for the next epoch nodes when sending a proposal for the last block in the epoch.
Adds a field in the shared consensus state that stores the highest known next epoch QC. Adds a similar field to the storage.
Adds an epoch parameter to the threshold methods in the Membership trait. This is required because the thresholds depend on the number of nodes in the given epoch.
Modifies OverallSafetyTask. It now uses the total nodes number and threshold directly from the Membership object. This is required to properly test the types with the variable stake table.

This PR does not:

Modify the DA logic. It means that the next epoch nodes do not participate in the DAC formation.
Modify the timeout and view sync logic. These will be adjusted in the next PRs.

Key places to review:

shenkeyao · 2024-12-05T22:05:13Z

crates/types/src/utils.rs

+    if block_number == 0 || epoch_height == 0 {
+        false
+    } else {
+        block_number % epoch_height == 0


As mentioned on Zulip, I think this should be block_number + 1.

Nvm now I understand that block 0 is a special case.

ss-es

I think it makes sense overall, just some comments

ss-es · 2024-12-15T18:52:01Z

crates/hotshot/src/lib.rs

+    /// Next epoch highest QC that was seen
+    next_epoch_high_qc: Option<NextEpochQuorumCertificate2<TYPES>>,


This is a bit unclear to me (also it's not clear we need it in the hotshot initializer; does it cause problems to default it to None?)

can the comment be updated to be more descriptive about where this should come from?

We might need it if a node restarted during epoch transition and is about to propose. I've adjusted the comment.

ss-es · 2024-12-15T18:58:12Z

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

+        // If we haven't upgraded to Epochs just return None right away
+        if self
+            .upgrade_lock
+            .version(self.view_number)


you can use version_infallible here I think

good point, thank you!

ss-es · 2024-12-15T19:02:06Z

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

+        if let Some(next_epoch_qc) = self.consensus.read().await.next_epoch_high_qc() {
+            if next_epoch_qc.data.leaf_commit == high_qc.data.leaf_commit {
+                // We have it already, no reason to wait
+                return Some(next_epoch_qc.clone());
+            }
+        };


I'm not sure about this, can't we pass this directly to this function call as an Option<> (like we pass high_qc)? Do we have to lock consensus state here?

I'm not sure what you mean here. I've created an issue to follow up on the unresolved comments from this PR: #3978

ss-es · 2024-12-15T19:06:36Z

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

+        if justify_qc.view_number() != next_epoch_justify_qc.view_number()
+            || justify_qc.data.epoch != next_epoch_justify_qc.data.epoch
+            || justify_qc.data.leaf_commit != next_epoch_justify_qc.data.leaf_commit
+        {
+            bail!("Next epoch justify qc exists but it's not equal with justify qc.");
+        }


can we replace this with ensure!()? I feel it's easier to read the check that way

maybe the one right after this as well

In general, I agree. I used bail! here to be consistent with the surrounding code. We might consider creating a task for rewriting all the bail / ensure statements in this function. For now I've added your comment to the follow up issue: #3978

ss-es · 2024-12-15T19:43:33Z

crates/task-impls/src/vid.rs

@@ -157,6 +166,65 @@ impl<TYPES: NodeType, I: NodeImplementation<TYPES>> VidTaskState<TYPES, I> {
                return None;
            }

+            HotShotEvent::QuorumProposalSend(proposal, _) => {


is there a reason we can't do this on BlockRecv? (is it because we aren't currently passing the block number there?)

Yes, there is a reason. This is related to the discussion we had about the epoch acquired at the moment when BlockRecv is issued being unreliable. In the next PR I've slightly changed the view change logic. It seems that thanks to that the BlockRecv's epoch has become reliable. I need to test it some more and, if everything works as expected, I will adjust the code here as well.

ss-es · 2024-12-15T19:46:55Z

crates/types/src/consensus.rs

+        &mut self,
+        high_qc: NextEpochQuorumCertificate2<TYPES>,
+    ) -> Result<()> {
+        ensure!(


can we rewrite this to avoid the unwrap call even if we know it's fine?

e.g. let Some(qc) = self.next_epoch... else { return Ok(()); }

Of course, I've changed it.

pls148 · 2024-12-17T20:01:35Z

crates/hotshot/src/lib.rs

@@ -493,6 +494,7 @@ impl<TYPES: NodeType, I: NodeImplementation<TYPES>, V: Versions> SystemContext<T

        let api = self.clone();
        let view_number = api.consensus.read().await.cur_view();
+        let epoch = api.consensus.read().await.cur_epoch();


let consensus_reader = api.consensus.read().await;
let view_number = consensus_reader.cur_view();
let epoch = consensus_reader.cur_epoch();
drop(consensus_reader);

pls148

Generally looks good. I suggested one change regarding locking, but I wouldn't consider it too critical as I'll be making a cleanup pass later on for locking behavior.

I'd also address @ss-es comments before merging!

lukaszrzasik · 2024-12-17T20:28:51Z

Generally looks good. I suggested one change regarding locking, but I wouldn't consider it too critical as I'll be making a cleanup pass later on for locking behavior.

I'd also address @ss-es comments before merging!

I actually answered Salman's comments earlier today but forgot to publish my answer. I'll adjust the lock / drop part.

lukaszrzasik and others added 30 commits November 15, 2024 10:41

Initial commit

c3aa374

WIP: adding epoch to proposal and vote data, not compiling yet

09ca3cb

Make it compile

51cb073

Merge branch 'main' into lr/variable-stake-table

69d9538

Adjust tests

7539714

Add a test type for two stake tables for even and odd epochs

1bd8716

Debugging

ce2fd93

Fix extended voting

5c8604e

Merge branch 'lr/fix-extended-voting' into lr/variable-stake-table

358ac8a

Try "in epoch transition" approach

7ac36b1

Continue debugging

77efd42

Use correct epoch with Membership

a5b9729

Adjust tests and lints

50c6b79

Merge branch 'main' into lr/variable-stake-table

e0d00ee

Adapt to variable stake table after merge

606d509

Fix accidentally pulled bug in eQC rule

4d5ae09

Merge branch 'main' into lr/variable-stake-table

504c3be

Commit includes epoch for vote and proposal data types

dbf46db

Prune dependencies (#3787)

4688aca

Merge branch 'main' into lr/variable-stake-table

e7eee96

Merge branch 'main' into lr/variable-stake-table

4c3568b

add new message types and gate outgoing messages

131168a

Use the proper message for the proposal response

91d6033

Modify commit for Leaf2 and QuorumData2

0446338

Adjust tests

0a598ac

Merge branch 'main' into lr/variable-stake-table

dd51a2a

Clean up debug traces

c0b94ba

Merge branch 'main' into lr/variable-stake-table

f658a36

Initial commit for double quorum

812f601

Merge branch 'main' into lr/variable-stake-table

ebe4602

lukaszrzasik added 9 commits December 2, 2024 17:11

Use epoch from vote and add more tests

d77771d

Merge branch 'main' into lr/variable-stake-table

6819a41

Adjust marketplace ver number

285858d

Merge branch 'lr/variable-stake-table' into lr/double-quorum

8b90f01

Merge branch 'main' into lr/variable-stake-table

128a7ae

Merge branch 'lr/variable-stake-table' into lr/double-quorum

806c878

Epochs without Marketplace and adjust tests

23d000d

Merge branch 'main' into lr/variable-stake-table

356a0e2

Merge branch 'lr/variable-stake-table' into lr/double-quorum

89bce25

shenkeyao reviewed Dec 5, 2024

View reviewed changes

lukaszrzasik added 2 commits December 6, 2024 12:43

Merge branch 'main' into lr/variable-stake-table

627636b

Merge branch 'lr/variable-stake-table' into lr/double-quorum

999c3fa

shenkeyao mentioned this pull request Dec 9, 2024

[DRB] - Verify the DRB result from the computation one epoch in advance #3948

Merged

Base automatically changed from lr/variable-stake-table to main December 12, 2024 16:29

ss-es added 2 commits December 12, 2024 12:51

Merge branch 'main' into lr/double-quorum

ec8aab2

fix merge

719ebfd

ss-es reviewed Dec 15, 2024

View reviewed changes

lukaszrzasik added 5 commits December 16, 2024 16:54

Fixes after merge

fe90479

Merge branch 'main' into lr/double-quorum

e762de4

Fix vid share handling

729db94

Submit transactions to the correct epoch

0d05d8f

Address review comments

a5b3797

pls148 reviewed Dec 17, 2024

View reviewed changes

pls148 approved these changes Dec 17, 2024

View reviewed changes

pls148 self-assigned this Dec 17, 2024

Use one lock to get two values

ae1fec3

lukaszrzasik merged commit a927746 into main Dec 18, 2024
17 checks passed

lukaszrzasik deleted the lr/double-quorum branch December 18, 2024 16:16

sveitser mentioned this pull request Dec 19, 2024

bump version: 0.5.83 #3981

Draft

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Lr/double quorum #3922

Lr/double quorum #3922

lukaszrzasik commented Nov 26, 2024 •

edited

Loading

shenkeyao Dec 5, 2024

shenkeyao Dec 5, 2024

ss-es left a comment

ss-es Dec 15, 2024

lukaszrzasik Dec 17, 2024

ss-es Dec 15, 2024

lukaszrzasik Dec 17, 2024

ss-es Dec 15, 2024

lukaszrzasik Dec 17, 2024

ss-es Dec 15, 2024

ss-es Dec 15, 2024

lukaszrzasik Dec 17, 2024

ss-es Dec 15, 2024

lukaszrzasik Dec 17, 2024

ss-es Dec 15, 2024

lukaszrzasik Dec 17, 2024

pls148 Dec 17, 2024

pls148 left a comment

lukaszrzasik commented Dec 17, 2024

		/// Next epoch highest QC that was seen
		next_epoch_high_qc: Option<NextEpochQuorumCertificate2<TYPES>>,

Lr/double quorum #3922

Lr/double quorum #3922

Conversation

lukaszrzasik commented Nov 26, 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

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

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

pls148 left a comment

Choose a reason for hiding this comment

lukaszrzasik commented Dec 17, 2024

lukaszrzasik commented Nov 26, 2024 •

edited

Loading