2652 Membership::set_first_epoch #2654

pls148 · 2025-02-21T01:03:48Z

This PR:

Adds a fn to Membership called set_first_epoch
Adds impl of that fn in both hotshot and sequencer side
Adds testing to StaticCommittee to ensure set_first_epoch is called appropriately

This PR does not:

Key places to review:

ss-es · 2025-02-24T21:02:49Z

hotshot-task-impls/src/helpers.rs

        if let Some(decided_leaf_info) = res.leaf_views.last() {
            decide_epoch_root(&decided_leaf_info.leaf, epoch_height, membership).await;
        } else {
            tracing::info!("No decided leaf while a view has been decided.");
        }
    }

+    if let Some(ref decided_upgrade_cert) = res.decided_upgrade_cert {


can we combine this if let and the one 2 levels down (i.e. if let (Some(..), Some(..)) = (.., ..) or similar)

ss-es · 2025-02-24T21:03:53Z

hotshot-testing/src/test_runner.rs

+            temp_memberships
+                .set_first_epoch(<TYPES as NodeType>::Epoch::new(1), INITIAL_DRB_RESULT);
+        }
+


I don't think we should do this in hotshot-testing, is there a reason we can't just do this in SystemContext::init?

Ended up removing this entirely as claim_block_with_num_nodes updates the number of nodes already.

ss-es · 2025-02-24T21:11:35Z

hotshot/src/lib.rs

@@ -297,6 +298,16 @@ impl<TYPES: NodeType, I: NodeImplementation<TYPES>, V: Versions> SystemContext<T
            config.epoch_height,
        );

+        if let Some(epoch) = epoch {
+            // #2652 REVIEW NOTE: This epoch can't be right; what if a node starts up between when we upgrade to epochs
+            // and when we're at epoch+2? This would cause the current node to propagate the non-epochs stake table


I'm also not sure if this is the right way to do this

can we maybe move this to start_consensus and do this in the same place we broadcast the genesis QC (~ line 440)? that seems like it might be closer to the semantics we want

I think there the check should be on V::Base::VERSION not anything else. this won't handle (at all) the case where we're restarting with epochs, but that should be handled by @bfish713's catchup

I feel like we don't need to set this epoch when we start consensus. If we set it to our current epoch (based on the anchor leaf) I think that value will never be correct. I think we should only set this when the epoch upgrade succeeds and it should be set to a specific epoch (i.e the upgrade is agree to start epochs at a specific height)

if we start up with epochs enabled for genesis, the Membership trait needs to know that epochs 1 & 2 have no epoch root/drb

I agree we should not use the anchor leaf, but I think we have to specifically do it for genesis unless I'm missing something

I think this needs to be handled in a general way, if epochs are enabled when we start, we need to load with an epoch root or roots. I think this should be included in the HotshotInitializer, We should also make sure we can load up again with the stored epoch, like how we do with the anchor leaf.

I agree we can't just ignore at startup, I was mistaken. I was only considering the mainnet upgrade

lukaszrzasik

I don't really have any comments. I'll let someone else approve after the discussion with the setting the initial first epoch is resolved.

I do have a question but I'll take it offline as this more of a generic question and I don't think it will affect the PR in any way.

ss-es

left some comments. I think notably there aren't any methods storing the initializer info, will that be a separate PR?

otherwise I think it looks fine overall. I'll leave to @bfish713 or @lukaszrzasik to approve (since I accidentally merged the upgrade logic into this branch and I don't want to approve my own code)

ss-es · 2025-02-28T16:36:49Z

hotshot/src/lib.rs

@@ -243,6 +244,53 @@ impl<TYPES: NodeType, I: NodeImplementation<TYPES>, V: Versions> SystemContext<T
        .await
    }

+    async fn load_start_epoch_info(


is there a reason this is a method on SystemContext?

it seems like it should either be a method on Membership or a stand-alone function

ss-es · 2025-02-28T16:43:04Z

hotshot/src/lib.rs

+        // If the first element in start_epoch_info does not have a block header,
+        // use that to set_first_epoch instead
+        if let Some(epoch_info) = start_epoch_info.first() {
+            if epoch_info.block_header.is_none() {
+                membership
+                    .write()
+                    .await
+                    .set_first_epoch(epoch_info.epoch, INITIAL_DRB_RESULT);
+            }
+        }


I would actually suggest just calling set_first_epoch in the loop, as part of the else on if let Some(block_header)

I think doing it the way it's done here is more likely to result in unexpected behaviour, because we can silently discard parts of the initializer. and I don't think there's much to gain from second-guessing the initializer: if it's misconfigured and results in set_first_epoch being called twice, it shouldn't be load_start_epoch_info's job to second-guess it

I do think we should log by the way, maybe at tracing::debug!(), every call to set_first_epoch or add_drb_result

done and done

pls148 · 2025-02-28T18:12:34Z

left some comments. I think notably there aren't any methods storing the initializer info, will that be a separate PR?

otherwise I think it looks fine overall. I'll leave to @bfish713 or @lukaszrzasik to approve (since I accidentally merged the upgrade logic into this branch and I don't want to approve my own code)

Addressed. The methods for storing it are part of a separate task

bfish713 · 2025-02-28T21:03:23Z

hotshot-task-impls/src/helpers.rs

+            let decided_block_number = decided_leaf_info.leaf.block_header().block_number();
+            let first_epoch_number =
+                TYPES::Epoch::new(epoch_from_block_number(decided_block_number, epoch_height));


I thought the plan was to have the block height configured for the first epoch, but his code is getting the epoch from when we decide the upgrade. Is that change for a different PR or am I misunderstanding/misremembering?

bfish713 · 2025-02-28T21:07:43Z

hotshot-task-impls/src/helpers.rs

+    let epoch_height = consensus_reader.epoch_height;
+    drop(consensus_reader);

+    if with_epochs && res.new_decided_view_number.is_some() {


FYI this function stops being called when we upgrade, decide_from_proposal_2 gets activated, any logic specific to epochs would need to be in there too

my understanding here was that once we're in epochs we don't want to set_first_epoch any more anyway. Either way this logic moved due to the next comment

you are correct once we in epochs we don't have to worry about epoch upgrade stuff

bfish713 · 2025-02-28T21:12:51Z

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

@@ -390,21 +391,44 @@ pub(crate) async fn handle_quorum_proposal_validated<
        .await
    };

+    if let Some(cert) = &task_state.staged_epoch_upgrade_certificate {
+        if leaf_views.last().unwrap().leaf.height() >= task_state.epoch_upgrade_block_height {


So here we are waiting to apply the upgrade until the configured height, but we already set the first epoch, I think that needs to happen here

bfish713 · 2025-02-28T21:42:48Z

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

+            membership
+                .write()
+                .await
+                .set_first_epoch(first_epoch_number, INITIAL_DRB_RESULT);


Set this in the block below

if let Some(cert) = decided_upgrade_cert.clone() { if cert.data.new_version == V::Epochs::VERSION { task_state.staged_epoch_upgrade_certificate = Some(cert);

bfish713 · 2025-02-28T21:55:36Z

hotshot-task-impls/src/helpers.rs

+    let epoch_height = consensus_reader.epoch_height;
+    drop(consensus_reader);

+    if with_epochs && res.new_decided_view_number.is_some() {


you are correct once we in epochs we don't have to worry about epoch upgrade stuff

bfish713 · 2025-02-28T21:56:10Z

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

@@ -390,21 +391,56 @@ pub(crate) async fn handle_quorum_proposal_validated<
        .await
    };

+    if let Some(cert) = &task_state.staged_epoch_upgrade_certificate {


clear this option after we apply the upgrade so we don't hit this on every proposal

pls148 requested review from philippecamacho, ImJeremyHe, sveitser, jbearer, tbro and imabdulbasit as code owners February 21, 2025 01:03

pls148 marked this pull request as draft February 21, 2025 01:03

pls148 force-pushed the ps/2652 branch 7 times, most recently from 516ba06 to 2ff3c8b Compare February 24, 2025 18:52

pls148 marked this pull request as ready for review February 24, 2025 19:35

pls148 force-pushed the ps/2652 branch 3 times, most recently from a97d405 to 4bf4660 Compare February 24, 2025 20:55

ss-es reviewed Feb 24, 2025

View reviewed changes

lukaszrzasik reviewed Feb 25, 2025

View reviewed changes

pls148 force-pushed the ps/2652 branch 10 times, most recently from 681a0b5 to 8e85bb6 Compare February 27, 2025 23:17

pls148 force-pushed the ps/2652 branch 6 times, most recently from b987c63 to 8710c56 Compare February 28, 2025 05:16

ss-es reviewed Feb 28, 2025

View reviewed changes

pls148 force-pushed the ps/2652 branch from 8710c56 to ffc7362 Compare February 28, 2025 18:11

bfish713 requested changes Feb 28, 2025

View reviewed changes

pls148 force-pushed the ps/2652 branch from ffc7362 to 9c3b3b6 Compare February 28, 2025 21:40

bfish713 reviewed Feb 28, 2025

View reviewed changes

pls148 force-pushed the ps/2652 branch from 9c3b3b6 to 74f3fa8 Compare February 28, 2025 21:51

bfish713 reviewed Feb 28, 2025

View reviewed changes

pls148 force-pushed the ps/2652 branch from 74f3fa8 to 1b5e941 Compare February 28, 2025 21:58

bfish713 approved these changes Feb 28, 2025

View reviewed changes

pls148 force-pushed the ps/2652 branch from 1b5e941 to 244d761 Compare March 1, 2025 17:18

pls148 and others added 6 commits March 3, 2025 12:44

2652 Membership::set_first_epoch

b8d7d2a

fix for comments in PR review

4f3cd21

add logic to block upgrade

bdd40c3

fix

64641ac

rustfmt

c21d8df

testing stuff

a583123

pls148 force-pushed the ps/2652 branch from 244d761 to a583123 Compare March 3, 2025 20:47

pls148 enabled auto-merge (squash) March 3, 2025 20:49

pls148 merged commit 4771ff5 into main Mar 3, 2025
50 of 51 checks passed

pls148 deleted the ps/2652 branch March 3, 2025 21:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

2652 Membership::set_first_epoch #2654

2652 Membership::set_first_epoch #2654

pls148 commented Feb 21, 2025

ss-es Feb 24, 2025

ss-es Feb 24, 2025

pls148 Feb 28, 2025

ss-es Feb 24, 2025

bfish713 Feb 24, 2025

ss-es Feb 25, 2025

bfish713 Feb 25, 2025

bfish713 Feb 25, 2025

lukaszrzasik left a comment

ss-es left a comment

ss-es Feb 28, 2025

ss-es Feb 28, 2025

ss-es Feb 28, 2025

pls148 Feb 28, 2025

pls148 commented Feb 28, 2025

bfish713 Feb 28, 2025

bfish713 Feb 28, 2025

pls148 Feb 28, 2025

bfish713 Feb 28, 2025

bfish713 Feb 28, 2025

bfish713 Feb 28, 2025

bfish713 Feb 28, 2025

bfish713 Feb 28, 2025 •

edited

Loading

2652 Membership::set_first_epoch #2654

2652 Membership::set_first_epoch #2654

Conversation

pls148 commented Feb 21, 2025

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

Choose a reason for hiding this comment

lukaszrzasik left a 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

pls148 commented Feb 28, 2025

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

bfish713 Feb 28, 2025 • edited Loading

Choose a reason for hiding this comment

bfish713 Feb 28, 2025 •

edited

Loading