Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change ContextuallyVerifiedBlockWithTrees into an enum to correctly represent verification statuses #6912

Closed
teor2345 opened this issue Jun 11, 2023 · 1 comment · Fixed by #7035
Assignees
Labels
A-consensus Area: Consensus rule updates A-state Area: State / database changes C-audit Category: Issues arising from audit findings C-bug Category: This is a bug C-tech-debt Category: Code maintainability issues

Comments

@teor2345
Copy link
Contributor

teor2345 commented Jun 11, 2023

Motivation

ContextuallyVerifiedBlockWithTrees actually represents two different types with two different verification statuses. This is a maintainability issue.

Design Background

Blocks from the checkpoint verifier are checkpoint-verified: they are semantically verified, but their transaction authorizing data isn't bound to their header.

Blocks from the non-finalized state are contextually verified: their transaction authorizing data is bound to their header. (And their trees have been calculated.)

Complex Code or Requirements

The consensus code is correct, but the names and types we're using could cause bugs during future changes.

Design

The current code is:

pub struct ContextuallyVerifiedBlockWithTrees {
/// A block ready to be committed.
pub checkpoint_verified: CheckpointVerifiedBlock,
/// The tresstate associated with the block.
pub treestate: Option<Treestate>,
}

The different verification statuses can be represented using an enum:

pub enum FinalizableBlock {
    Checkpoint {
        checkpoint_verified: CheckpointVerifiedBlock
    },
    Contextual {
        contextually_verified: ContextuallyVerifiedBlock,
        treestate: Treestate,
    }
}

Here are some example code changes that we could do:

In the match statement in commit_finalized_direct():

  • FinalizableBlock::Checkpoint { checkpoint_verified } replaces ContextuallyVerifiedBlockWithTrees { checkpoint_verified, treestate: None }
  • FinalizableBlock::Contextual { contextually_verified, treestate } replaces ContextuallyVerifiedBlockWithTrees { checkpoint_verified, treestate: Some(treestate) }

The Checkpoint case can construct a ContextuallyVerifiedBlockWithTrees from the checkpoint block and the trees in commit_finalized_direct(). Then the rest of the finalized state can use ContextuallyVerifiedBlockWithTrees instead of passing separate finalized, history_tree, note_commitment_trees arguments.

Testing

Our existing tests should cover this change.

Related Work

This is part of:

@teor2345 teor2345 added C-bug Category: This is a bug A-consensus Area: Consensus rule updates S-needs-triage Status: A bug report needs triage P-Medium ⚡ A-state Area: State / database changes C-audit Category: Issues arising from audit findings C-tech-debt Category: Code maintainability issues labels Jun 11, 2023
@mpguerra mpguerra added this to Zebra Jun 11, 2023
@github-project-automation github-project-automation bot moved this to 🆕 New in Zebra Jun 11, 2023
@teor2345 teor2345 changed the title Turn ContextuallyVerifiedBlockWithTrees into an enum Change ContextuallyVerifiedBlockWithTrees into an enum to correctly represent verification statuses Jun 11, 2023
@upbqdn upbqdn changed the title Change ContextuallyVerifiedBlockWithTrees into an enum to correctly represent verification statuses Change ContextuallyVerifiedBlockWithTrees into an enum to correctly represent verification statuses Jun 14, 2023
@mpguerra
Copy link
Contributor

@upbqdn can you please add an estimate here?

@mpguerra mpguerra linked a pull request Jun 20, 2023 that will close this issue
6 tasks
@upbqdn upbqdn removed the S-needs-triage Status: A bug report needs triage label Jun 21, 2023
@mpguerra mpguerra linked a pull request Jun 21, 2023 that will close this issue
6 tasks
@mergify mergify bot closed this as completed in #7035 Jun 27, 2023
@github-project-automation github-project-automation bot moved this from 🆕 New to ✅ Done in Zebra Jun 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-consensus Area: Consensus rule updates A-state Area: State / database changes C-audit Category: Issues arising from audit findings C-bug Category: This is a bug C-tech-debt Category: Code maintainability issues
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

3 participants