Lr/increment epoch #3828

lukaszrzasik · 2024-10-31T10:12:43Z

Part of #3730

This PR:

Adds logic to update epoch if relevant when views are updated
Adds view and epoch updating logic when Quorum Certificate is formed

This PR does not:

Key places to review:

… last block

lukaszrzasik · 2024-11-07T12:55:37Z

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

+        broadcast_event(
+            Arc::new(HotShotEvent::ViewChange(
+                self.view_number + 1,
+                next_leaf_epoch,
+            )),
+            &self.sender,
+        )
+        .await;


@bfish713 I've moved emitting ViewChange so now it's after the shared state has been updated. Is that fine?

I think it should not matter much, we could even maybe just put the view change in the QuorumVoteDependenciesValidated handler

Or even QuorumVoteSend handler. But either way this should be ok

Ok one problem is that some of the previous stuff is faillable, so we might not emit the event in some cases we did before, like we didn't see the last proposal and can't fetch it in time, does the epoch number calculation depend on the state update?

Yes, it does depend on it. It's because the epoch depends on the block number. We need to check whether the next proposed leaf will be for the next block or the same block again. We do that by checking the 3-chain.

Now that I think about it again, I don't think that we should switch epoch when we vote for the last leaf of the 3-chain. The fact that a node votes for it doesn't mean that the eQC will be formed and a node should switch to a new epoch only after seeing an eQC.

Is it important to switch view as soon as we vote for the proposal?

Yes we most switch views here, switching views is basically the same as starting the timeout for the next view. If we don't do it here and the next proposal doesn't come in are behavior will not be correct

I've changed the logic so that the epoch is only updated after validating the proposal or when a node formed a QC.

lukaszrzasik · 2024-11-07T14:23:20Z

Moving back to draft because there is some regression in restart test. Need to figure that out.

ss-es

some random comments, looks fine on the whole

have some longer questions I will ask on zulip (about the quorum_vote and consensus.rs files)

ss-es · 2024-10-31T14:37:00Z

crates/example-types/src/storage_types.rs

@@ -99,6 +101,9 @@ impl<TYPES: NodeType> TestStorage<TYPES> {
    pub async fn last_actioned_view(&self) -> TYPES::View {
        self.inner.read().await.action
    }
+    pub async fn last_epoch(&self) -> TYPES::Epoch {


can this be last_actioned_epoch for consistency?

Done. Thank you!

ss-es · 2024-11-07T18:48:43Z

crates/types/src/consensus.rs

+        let epoch_number = if next_block_number % self.epoch_height == 0 {
+            next_block_number / self.epoch_height
+        } else {
+            next_block_number / self.epoch_height + 1
+        };


can this use the epoch_from_block_number function

Yeah, I would like to but I would have to make the types crate dependent on the task-impls crate. Do we want to do that?

I've moved the epoch_from_block_number function to the types crate. This way we can use it in all the places without adding new dependencies.

ss-es · 2024-11-07T18:52:48Z

crates/types/src/consensus.rs

+        if self.epoch_height == 0 {
+            return Ok(TYPES::Epoch::new(0));
+        }
+        let Some(validated_view) = self.validated_state_map.get(&view) else {


nit: I think this (and following let .. = .. else .. could be written more simply as let validated_view = self.validated_state_map.get(&view).context("...")?

I've updated the code. Thanks!

* 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`

bfish713 · 2024-11-13T15:30:57Z

crates/types/src/consensus.rs

+    /// 1. The leaf from the given view is not for the last block.
+    ///    The epoch should be calculated based on the next block number.


I don't think I understand this case, if the leaf is not the last block (say it's height 3 and epoch length is 100), then isn't this the trivial case and can just be handled without going into the validated state map? We already got the height before calling this function right?

Basically I expect the code on forming eqc to just do the decide rule, and if the block decided is the last of the epoch we can advance, all other cases we stay in the current epoch. I can see how the logic kind of does that, but it's hard to follow when the case for "I'm not even trying to forma an eqc" and "I just formed an eqc" end up being handled by the same line of code and It's a little confusing that is_leaf_forming_eqc is false when we form the eqc?

bfish713 · 2024-11-13T15:41:03Z

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

+                broadcast_event(
+                    Arc::new(HotShotEvent::ViewChange(new_view, next_view_epoch)),
+                    &event_sender,
+                )
+                .await;
+            }


I thought we discussed only doing the epoch change when we get the proposal containing this eQC. Idk how much it matters, and I think we'll discuss today the timeout path where this becomes relevant. It's mostly a matter of what epoch do I want to timeout to and when. I don't have a problem with this way, just noting.

lukaszrzasik added 26 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

lukaszrzasik marked this pull request as ready for review October 31, 2024 14:49

lukaszrzasik requested a review from bfish713 as a code owner October 31, 2024 14:49

lukaszrzasik assigned lukaszrzasik and bfish713 Oct 31, 2024

lukaszrzasik added 6 commits November 6, 2024 21:57

Correctly update epoch when voting and forming QC

50d9306

Merge branch 'main' into lr/increment-epoch

33e78e5

Send ViewChange event when voting after shared state's been updated

307e2c5

Adjust tests

4a764ee

Remove an obsolete DA ViewChange code

6db7096

Fetch proposal if missing when changing view after forming QC

1bd66cb

lukaszrzasik commented Nov 7, 2024

View reviewed changes

lukaszrzasik marked this pull request as draft November 7, 2024 14:22

lukaszrzasik added 2 commits November 7, 2024 18:53

Fix deadlock

e1c5a1d

Merge branch 'main' into lr/increment-epoch

5294a83

lukaszrzasik marked this pull request as ready for review November 7, 2024 18:05

ss-es reviewed Nov 7, 2024

View reviewed changes

lukaszrzasik and others added 11 commits November 8, 2024 12:00

Update epoch only when receiving or forming an eQC

e150806

update logging (#3844)

34b271a

Fix fmt

e09dd6f

Merge branch 'main' into lr/increment-epoch

a653525

Simplify code

de5a488

Move a helper function to types crate

ab4293a

Merge branch 'main' into lr/increment-epoch

0330ca4

Merge branch 'main' into lr/increment-epoch

8928fb0

Rename storage method

0c70cc6

Merge branch 'main' into lr/increment-epoch

f022612

bfish713 reviewed Nov 13, 2024

View reviewed changes

Remove unused code

759167d

bfish713 approved these changes Nov 13, 2024

View reviewed changes

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

lukaszrzasik deleted the lr/increment-epoch branch November 13, 2024 18:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Lr/increment epoch #3828

Lr/increment epoch #3828

lukaszrzasik commented Oct 31, 2024 •

edited

Loading

lukaszrzasik Nov 7, 2024

bfish713 Nov 7, 2024

bfish713 Nov 7, 2024

bfish713 Nov 7, 2024

lukaszrzasik Nov 7, 2024

bfish713 Nov 8, 2024

lukaszrzasik Nov 12, 2024

lukaszrzasik commented Nov 7, 2024

ss-es left a comment

ss-es Oct 31, 2024

lukaszrzasik Nov 12, 2024

ss-es Nov 7, 2024

lukaszrzasik Nov 8, 2024

lukaszrzasik Nov 8, 2024

ss-es Nov 7, 2024

lukaszrzasik Nov 12, 2024

bfish713 Nov 13, 2024

bfish713 Nov 13, 2024

		/// 1. The leaf from the given view is not for the last block.
		/// The epoch should be calculated based on the next block number.

Lr/increment epoch #3828

Lr/increment epoch #3828

Conversation

lukaszrzasik commented Oct 31, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lukaszrzasik commented Nov 7, 2024

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

lukaszrzasik commented Oct 31, 2024 •

edited

Loading