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

T2.1.1: Unnecessary double-commit in VoteData #1772

Closed
Tracked by #1757
ggutoski opened this issue Sep 18, 2023 · 1 comment · Fixed by #1777
Closed
Tracked by #1757

T2.1.1: Unnecessary double-commit in VoteData #1772

ggutoski opened this issue Sep 18, 2023 · 1 comment · Fixed by #1777
Assignees

Comments

@ggutoski
Copy link
Contributor

Currently we are needlessly computing commitments of commitments in the impl of Committable for VoteData.

All variants of VoteData are of type Commitment<COMMITTABLE>. In other words: they are already commitments.

pub enum VoteData<COMMITTABLE: Committable + Serialize + Clone> {
/// Vote to provide availability for a block.
DA(Commitment<COMMITTABLE>),

However, the impl of Committable computes another commitment of the commitment:

impl<COMMITTABLE: Committable + Serialize + Clone> Committable for VoteData<COMMITTABLE> {
fn commit(&self) -> Commitment<Self> {
match self {
VoteData::DA(block_commitment) => {
commit::RawCommitmentBuilder::new("DA BlockPayload Commit")
.field("block_commitment", *block_commitment)
.finalize()
}

Seems that VoteData should not impl Committable. Or, at the very least, its impl of Committable should just return the commitment that's already stored in VoteData.

@ggutoski
Copy link
Contributor Author

ggutoski commented Sep 18, 2023

I think we should remove the Committable impl.

A reasonable impl of Committable might look like this:

fn commit(&self) -> Commitment<Self> {
    match self {
        DA(c) | Yes(c) | No(c) | Timeout(c) | ViewSyncPreCommit(c) | ViewSyncCommit(c)
        | ViewSyncFinalize(c) => *c,
    }
}

However, the return type of the above code is Commitment<COMMITTABLE>. That might seem right, but any impl of Committable for VoteData<COMMITTABLE> must have return type Commitment<VoteData<COMMITTABLE>>. In other words, it must be a commitment of a commitment. We should not have anything with that type.

@ggutoski ggutoski changed the title Unnecessary double-commit in VoteData T2.1.1: Unnecessary double-commit in VoteData Sep 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant