Skip to content

add full upgrade tests#4262

Draft
bfish713 wants to merge 3 commits intomainfrom
bf/upgrade
Draft

add full upgrade tests#4262
bfish713 wants to merge 3 commits intomainfrom
bf/upgrade

Conversation

@bfish713
Copy link
Copy Markdown
Contributor

@bfish713 bfish713 commented May 6, 2026

Closes #<ISSUE_NUMBER>

This PR:

This PR does not:

Key places to review:

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request implements the bridging logic for transitioning from the legacy protocol (0.4) to the new protocol (0.8). It introduces mechanisms to harvest legacy consensus state—including the decided anchor, undecided leaves, and validated states—and seed them into the new protocol's coordinator. Additionally, it adds support for forwarding legacy timeout votes and provides configuration overrides for upgrade offsets to enable zero-blackout upgrades. Review feedback identified that set_pre_cutover_anchor is incomplete and needs to synthesize a proposal and advance the consensus view to prevent stalls. It was also noted that current_epoch updates should be conditional on whether epochs are enabled.

Comment on lines +367 to +374
pub fn set_pre_cutover_anchor(&mut self, leaf: Leaf2<T>) {
let view = leaf.view_number();
if view <= self.last_decided_view {
return;
}
self.last_decided_view = view;
self.last_decided_leaf = leaf;
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

The set_pre_cutover_anchor implementation is incomplete. It only updates the anchor view/leaf but doesn't advance current_view or populate self.proposals. If the undecided chain is empty, the coordinator's start() method (or the leader of the first post-cutover view) will fail to find the parent proposal at cur_view, leading to a panic or a stall. It should also synthesize a proposal for the anchor leaf and advance the consensus state.

    pub fn set_pre_cutover_anchor(&mut self, leaf: Leaf2<T>) {
        let view = leaf.view_number();
        if view <= self.last_decided_view {
            return;
        }
        self.last_decided_view = view;
        self.last_decided_leaf = leaf.clone();

        // Ensure the anchor is available for parent lookups and serving.
        self.leaves.insert(view, leaf.clone());

        // Synthesize the new-protocol Proposal from the anchor leaf so that
        // the leader of the first post-cutover view can use it as a parent.
        let block_number = leaf.block_header().block_number();
        let epoch = EpochNumber::new(epoch_from_block_number(block_number, *self.epoch_height));
        let view_change_evidence = leaf.view_change_evidence.clone().and_then(|e| match e {
            ViewChangeEvidence2::Timeout(tc) => Some(tc),
            ViewChangeEvidence2::ViewSync(_) => None,
        });
        let proposal = Proposal {
            block_header: leaf.block_header().clone(),
            view_number: view,
            epoch,
            justify_qc: leaf.justify_qc().clone(),
            next_epoch_justify_qc: None,
            upgrade_certificate: leaf.upgrade_certificate().clone(),
            view_change_evidence,
            next_drb_result: leaf.next_drb_result,
            state_cert: None,
        };
        self.proposals.insert(view, proposal);
        self.proposed_views.insert(view);
        self.voted_1_views.insert(view);
        self.voted_2_views.insert(view);

        if view > self.current_view {
            self.current_view = view;
            self.current_epoch = if *self.epoch_height > 0 {
                Some(epoch)
            } else {
                None
            };
        }
    }

// than re-running the seeded ones.
if max_seeded_view > self.current_view {
self.current_view = max_seeded_view;
self.current_epoch = Some(max_seeded_epoch);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The current_epoch is updated to Some(max_seeded_epoch) without checking if epochs are actually enabled. If epoch_height is 0, current_epoch should typically remain None to avoid triggering epoch-transition logic in the new protocol.

        if max_seeded_view > self.current_view {
            self.current_view = max_seeded_view;
            self.current_epoch = if *self.epoch_height > 0 {
                Some(max_seeded_epoch)
            } else {
                None
            };
        }

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

new protocol always should have epochs

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 6, 2026

Nextest failures (1) in this run

Test Attempts Time (s) Main history
hotshot-testing::test_vid2_upgrade::test_vid2_upgrade::testtypes_::memoryimpl_::test_vid2_upgrade 1 34.85 passing

See the step summary for flaky tests and slowest tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant