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

Vote accumulation and round-estimate logic #2

Merged
merged 11 commits into from
Aug 24, 2018
Merged

Conversation

rphmeier
Copy link
Contributor

@rphmeier rphmeier commented Aug 15, 2018

Specifically this section from the latest hackmd document. Can be found in c9d922c

We define E_r,v, v's estimate of what might have been finalised in round r, to be the last block in the chain with head g(V_r,v) that it is possible for C_r,r to have a supermajority for. If either E_r,v<g(V_r,v) or it is impossible for C_r,v to have a supermajority for any children of g(V_r,v), then we say that (v sees that) round r is completable. E_0,v is the genesis block (if we start at r=1).

Copy link
Contributor

@gnunicorn gnunicorn left a comment

Choose a reason for hiding this comment

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

Minor comment requests. Ready to merge on your own after.

src/round.rs Outdated
if occupied.get().0 != vote {
return Err(Equivocation {
round_number: 0,
identity: id,
Copy link
Contributor

Choose a reason for hiding this comment

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

the clone self.votes.entry(id.clone()) { above is unnecessary if you did here: identity: occupied.key().clone(). Then we'd only clone in case of of an equivocation.

src/round.rs Outdated

/// Stores data for a round.
pub struct Round<Id: Hash + Eq, H: Hash + Eq, Signature> {
graph: VoteGraph<H, VoteCount>,
Copy link
Contributor

Choose a reason for hiding this comment

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

pub struct fields should probably have some docs, no?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

can't hurt, but the fields aren't pub so my convention is not to add doc comments. in this case I will add a few because the code isn't easy to understand without

src/round.rs Outdated
}
}

/// Import a prevote. Has no effect on internal state if an equivocation.
Copy link
Contributor

Choose a reason for hiding this comment

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

  • "or if signature is not by any current voters".

Copy link

@AlistairStewart AlistairStewart left a comment

Choose a reason for hiding this comment

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

We need to be dealing with equivocations correctly. If some Byzantine guy v votes for both B_1 and B_2, then maybe some honest guy saw only one vote or the other before they vote/finalise blocks. Then the way to deal with this correctly is to count v's vote weight towards B_1, B_2 and all their ancestors. Which means we need logic to add vote weight to B_2's ancestors but only those that don't already get it from the B_1 vote.

src/round.rs Outdated
}
}

/// Import a prevote. Has no effect on internal state if an equivocation.

Choose a reason for hiding this comment

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

This also means that we need to a +self.faulty_weight in the expression for remaining_commit_votes since if we saw v vote for B_2, that doesn't rule out there being a vote for B_1 which could be used to finalise a block.

@rphmeier
Copy link
Contributor Author

rphmeier commented Aug 24, 2018

@AlistairStewart We will address that in a follow-up issue because this PR is fairly monumental already #4

@rphmeier rphmeier merged commit a5bf74e into master Aug 24, 2018
@rphmeier rphmeier deleted the rh-accumulator branch August 24, 2018 21:15
rphmeier added a commit that referenced this pull request Jan 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants