Add protocol describing the semantics of the consensus network messages #66
Conversation
matan-starkware
force-pushed
the
matan/consensus_protocol_spec
branch
3 times, most recently
from
313b0f1 to
761cc69
Compare
p2p/proto/consensus/consensus.proto
Outdated
| @@ -30,68 +29,52 @@ message Vote { | |||
| // messages, to make sure the data, and therefore the signatures, are unambiguous between | |||
| // Prevote and Precommit. | |||
| VoteType vote_type = 1; | |||
| uint64 fork_id = 2; | |||
| uint64 block_number = 3; | |||
There was a problem hiding this comment.
We call this height in consensus. In particular because consensus does not know the concept of "block".
There was a problem hiding this comment.
Discussed in slack, height will refer to (block_number, fork_id)
p2p/proto/consensus/consensus.proto
Outdated
| // This is optional since a vote can be NIL. | ||
| optional Hash block_hash = 6; | ||
| optional Hash block_hash = 5; |
There was a problem hiding this comment.
We call this value_id in consensus, namely id(v).
There was a problem hiding this comment.
I think we prefer the term proposal_commitment as that seems to be more self explanatory to people who don't have a theoretical basis in consensus. It actually describes what we are voting on.
p2p/proto/consensus/consensus.proto
Outdated
| optional uint32 valid_round = 3; | ||
| Address proposer = 4; | ||
| uint64 fork_id = 1; | ||
| uint64 height = 2; |
There was a problem hiding this comment.
We are using height here and block_number for votes.
If this is the message that maps into the PROPOSAL message in the paper, the name should be the same.
There was a problem hiding this comment.
aligned so that we always have fork_id and block_number as the fields. height is the combination of the 2.
p2p/proto/consensus/consensus.proto
Outdated
|
|
||
| message ProposalCommitment { | ||
| uint64 block_number = 1; | ||
| uint64 fork_id = 2; |
There was a problem hiding this comment.
Do we really need that many numbers for fork IDs?
There was a problem hiding this comment.
The reason I did it this way is that fork_id and height progress across time indefinitely.
Round on the other hand is reset for each height. My thinking is that if somehow rounds take only 100ms (implying good network connectivity) but no decision is reached, it will still take over 10 years to reach round 2**32, in which case the network is dead anyways.
matan-starkware
force-pushed
the
matan/consensus_protocol_spec
branch
from
761cc69 to
f7514f7
Compare
matan-starkware
requested review from
cason and
dan-starkware
and removed request for
cason
matan-starkware
force-pushed
the
matan/consensus_protocol_spec
branch
from
f7514f7 to
b294c68
Compare
There was a problem hiding this comment.
Thank you for the changes, I left another couple of comments.
I would like to discuss with you how to render the consensus proto and documentation more self-contained and how we align with the pseudo-code or generic concepts from Tendermint.
Probably a different proto file with Starnet internals content would be the way to go.
p2p/proto/consensus/protocol.md
Outdated
| 3. ProposalCommitment - once | ||
| 4. ProposalFin | ||
|
|
||
| #### Empty Proposals |
There was a problem hiding this comment.
Are empty proposals really empty? Namely, don't they have the height, the hash of the previous block etc.?
There was a problem hiding this comment.
Good point I added the ProposalCommitment for an empty block and what the fields should be.
p2p/proto/consensus/protocol.md
Outdated
| A proposer may not be able to offer a valid proposal. If so, the height can be agreed to be empty. | ||
| In this case the proposer will send ProposalInit immediately followed by ProposalFin. | ||
|
|
||
| #### Known Proposals |
There was a problem hiding this comment.
Here I assume that the content of the proposal is nonetheless broadcast.
Which would be a problem, since the Fin sequence number is considered to be the last valid one.
But this is right now a proposed approach, without details.
There was a problem hiding this comment.
The ProposalFin and the StreamMessage Fin are not the same thing. The streaming protocol below is indifferent about which messages are sent at which sequence_number
p2p/proto/consensus/consensus.proto
Outdated
| uint64 fork_id = 4; | ||
| uint32 round = 5; | ||
| VoteType vote_type = 1; | ||
| uint64 fork_id = 2; |
There was a problem hiding this comment.
You could define a Height type consisting of the pair fork_id and block_number. This would be more self-explanatory.
There was a problem hiding this comment.
I tend to like the flat struct, so I'll let @dan-starkware tie break on this
matan-starkware
force-pushed
the
matan/consensus_protocol_spec
branch
2 times, most recently
from
9d7df9b to
9509e1d
Compare
matan-starkware
force-pushed
the
matan/consensus_protocol_spec
branch
from
9509e1d to
a75602f
Compare
No description provided.