-
Notifications
You must be signed in to change notification settings - Fork 5
Grandpa protocols #6
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
base: main
Are you sure you want to change the base?
Conversation
zdave-parity
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't comment on the actual message contents as I'm not familiar enough with GRANDPA; will have to trust you on that for now!
simple.md
Outdated
| Prevote = Header Hash ++ Slot | ||
| Precommit = Header Hash ++ Slot | ||
| PrimaryPropose = Header Hash ++ Slot | ||
| Message = Enum { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be consistent with how enums are defined elsewhere, I would write this like:
0 ++ Prevote OR 1 ++ Precommit OR 2 ++ PrimaryPropose
Would also be good to use a more specific name than Message. If these are votes, perhaps Vote would be a good name?
simple.md
Outdated
|
|
||
| Validator -> Validator | ||
|
|
||
| --> 0 ++ Round Number ++ Set Id ++ Signed Message |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The 0 ++ seems unnecessary, similar comment on the other protocols.
simple.md
Outdated
| <-- FIN | ||
| ``` | ||
|
|
||
| Catchup Response. This includes all votes required to catch up state to that of the responding voter. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Assuming this is a response to a previous catchup request, we should probably just return it on the request stream?
zdave-parity
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've read the GRANDPA paper and somewhat understand how it works now. It should be possible for someone to implement this protocol given only the GP, referenced papers (this includes the GRANDPA paper), and this document. I don't think there is enough detail here at the moment for that to be the case. I've commented on particular places where I think there is not enough detail, but these comments are probably not exhaustive...
|
|
||
| GRANDPA voter sets match validator sets for each epoch. | ||
| This is sent by each voting validator to all other voting validators. | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A summary of the vote types here and when they are sent would be good. Ideally it should be possible to implement this using only the GRANDPA paper and this document. So in particular, I think PrimaryPropose needs clarifying here, as I don't think that wording is used in the paper. I assume it is the block broadcast by the primary in step 2 of the protocol? We also need to define here how the primary is chosen as this is not defined in the paper.
simple.md
Outdated
|
|
||
| ``` | ||
| Message = 0 ++ Prevote OR 1 ++ Precommit OR 2 ++ PrimaryPropose | ||
| Signed Message = Message ++ Ed25519 Signature ++ Ed25519 Public |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably don't need the public key here given that it should match the peer ID?
|
|
||
| ### CE 147: GRANDPA Commit | ||
|
|
||
| This is sent by each voting validator to all other voting validators. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we need to be clearer about the sending behaviour here. The paper says that to reduce spam, we should:
- Delay broadcasting a commit by a random time from 0..1 seconds.
- Don't broadcast if we receive a commit from another validator.
I assume this is implemented in the Rust crate? Although it doesn't seem critical that all implementations do exactly the same thing here, we should probably still document the expected behaviour.
simple.md
Outdated
|
|
||
| ``` | ||
| Precommits = len++[Precommit] | ||
| Multi Auth Data = len++[Ed25519 Signature ++ Ed25519 Public] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume the nth signature applies to the nth precommit? This should be documented. Preferable to identify signers by their index rather than their public key here? Seems like this would cut message size by ~25%, and would be consistent with the other protocols and the GP.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the signatures are in order of precommit. I've removed this along with Compact Commit though.
|
|
||
| ### CE 148: GRANDPA State | ||
|
|
||
| This is sent by each voting validator to all other voting validators and informs them of the latest round it is participating in. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should clarify when this is sent. Is it sent once the validator "starts the round" as defined in the paper?
simple.md
Outdated
| Grandpa Justification = Round Number ++ Commit ++ Votes Ancestries | ||
|
|
||
| Justification Type = 0 OR 1 | ||
| Encoded Justification = len++[u8] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the reason for the double-encoding here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was for a generic finality justification system that would work with eg, Grandpa and Beefy. Replaced this with a simple Grandpa Justification protocol for now so the double encoding has gone.
simple.md
Outdated
|
|
||
| ``` | ||
| Votes Ancestries = len++[Header] | ||
| Grandpa Justification = Round Number ++ Commit ++ Votes Ancestries |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume this does not include a Set Id because that is implied or something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It does not strictly need Set Id as all the votes inside Commit are signed with the Set Id so the Justification will only validate with the correct Set Id.
I've decided to add Set Id to it though as I think that will make syncing and tracking set changes easier.
simple.md
Outdated
| Prevote = Header Hash ++ Slot | ||
| Precommit = Header Hash ++ Slot | ||
| PrimaryPropose = Header Hash ++ Slot | ||
| Signed Prevote = Prevote ++ Ed25519 Signature ++ Ed25519 Public |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does the signature cover? Presumably it should cover round number and set ID to avoid weird replay attacks? In any case I think this should be documented here given that it isn't covered in the GP. Also should document the prefix used for the signature.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I've clarified this now. The signature does cover round number and set ID.
|
|
||
| ``` | ||
| Round Number = u64 | ||
| Set Id = u32 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume Set Id refers to the validator/voter set? I think we need to be explicit here about what it actually is, eg is it an epoch index or something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Clarified this. It starts at 0 at genesis and increments after each set change block is finalized. So it normally increments with epoch but it is not strictly the same as epoch index.
simple.md
Outdated
|
|
||
| ### CE 149: GRANDPA CatchUp | ||
|
|
||
| Catchup Request. This is sent by a voting validator to another validator. The response includes all votes required to catch up state to that of the responding voter. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs more explanation, particularly as this isn't really covered by the GRANDPA paper AFAICT. eg:
- What are
Base HashandBase Header? - What do the round number and set ID in the request and response mean?
- You say "the response includes all votes required to catch up state to that of the responding voter", but it seems like only the votes for one round are returned? If the requester is multiple rounds behind, are they supposed to request votes for one round at a time or something? Given the protocol defined in the paper, it does not seem possible for an honest voter to skip rounds? But perhaps this is possible based on observed commits or something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've clarified this a bit. Base refers to a block that all vote targets are a descendent of. There is no longer a response set ID, just for the request. The responding validator only returns the votes for the last completed round from it's view.
…pa Justification protocol
Add posterior state root hash to the GRANDPA vote target as described in GP section 19.
No description provided.