From fc1c37c4ac9a93ac94029adabf27e37c34211e4f Mon Sep 17 00:00:00 2001 From: Ethan Buchman Date: Wed, 11 Sep 2019 10:10:07 -0400 Subject: [PATCH] Update ADR-025 and mark it as Accepted (#3958) * update adr-025, mark accepted * minor update * Update docs/architecture/adr-025-commit.md Co-Authored-By: Anton Kaliaev --- docs/architecture/adr-025-commit.md | 91 +++++++++++++++++++++++------ 1 file changed, 73 insertions(+), 18 deletions(-) diff --git a/docs/architecture/adr-025-commit.md b/docs/architecture/adr-025-commit.md index 6db039d43..8f68662a8 100644 --- a/docs/architecture/adr-025-commit.md +++ b/docs/architecture/adr-025-commit.md @@ -5,7 +5,8 @@ Currently the `Commit` structure contains a lot of potentially redundant or unnecessary data. It contains a list of precommits from every validator, where the precommit includes the whole `Vote` structure. Thus each of the commit height, round, -type, and blockID are repeated for every validator, and could be deduplicated. +type, and blockID are repeated for every validator, and could be deduplicated, +leading to very significant savings in block size. ``` type Commit struct { @@ -24,21 +25,40 @@ type Vote struct { Signature []byte `json:"signature"` } ``` -References: -[#1648](https://github.com/tendermint/tendermint/issues/1648) -[#2179](https://github.com/tendermint/tendermint/issues/2179) -[#2226](https://github.com/tendermint/tendermint/issues/2226) -## Proposed Solution +The original tracking issue for this is [#1648](https://github.com/tendermint/tendermint/issues/1648). +We have discussed replacing the `Vote` type in `Commit` with a new `CommitSig` +type, which includes at minimum the vote signature. The `Vote` type will +continue to be used in the consensus reactor and elsewhere. -We can improve efficiency by replacing the usage of the `Vote` struct with a subset of each vote, and by storing the constant values (`Height`, `Round`, `BlockID`) in the Commit itself. +A primary question is what should be included in the `CommitSig` beyond the +signature. One current constraint is that we must include a timestamp, since +this is how we calculuate BFT time, though we may be able to change this [in the +future](https://github.com/tendermint/tendermint/issues/2840). + +Other concerns here include: + +- Validator Address [#3596](https://github.com/tendermint/tendermint/issues/3596) - + Should the CommitSig include the validator address? It is very convenient to + do so, but likely not necessary. This was also discussed in [#2226](https://github.com/tendermint/tendermint/issues/2226). +- Absent Votes [#3591](https://github.com/tendermint/tendermint/issues/3591) - + How to represent absent votes? Currently they are just present as `nil` in the + Precommits list, which is actually problematic for serialization +- Other BlockIDs [#3485](https://github.com/tendermint/tendermint/issues/3485) - + How to represent votes for nil and for other block IDs? We currently allow + votes for nil and votes for alternative block ids, but just ignore them + + +## Decision + +Deduplicate the fields and introduce `CommitSig`: ``` type Commit struct { Height int64 Round int BlockID BlockID `json:"block_id"` - Precommits []*CommitSig `json:"precommits"` + Precommits []CommitSig `json:"precommits"` } type CommitSig struct { @@ -60,19 +80,54 @@ const ( ``` -Note the need for an extra byte to indicate whether the signature is for the BlockID or for nil. -This byte can also be used to indicate an absent vote, rather than using a nil object like we currently do, -which has been [problematic for compatibility between Amino and proto3](https://github.com/tendermint/go-amino/issues/260). - -Note we also continue to store the `ValidatorAddress` in the `CommitSig`. -While this still takes 20-bytes per signature, it ensures that the Commit has all -information necessary to reconstruct Vote, which simplifies mapping between Commit and Vote objects -and with debugging. It also may be necessary for the light-client to know which address a signature corresponds to if -it is trying to verify a current commit with an older validtor set. +Re the concerns outlined in the context: + +**Timestamp**: Leave the timestamp for now. Removing it and switching to +proposer based time will take more analysis and work, and will be left for a +future breaking change. In the meantime, the concerns with the current approach to +BFT time [can be +mitigated](https://github.com/tendermint/tendermint/issues/2840#issuecomment-529122431). + +**ValidatorAddress**: we include it in the `CommitSig` for now. While this +does increase the block size unecessarily (20-bytes per validator), it has some ergonomic and debugging advantages: + +- `Commit` contains everything necessary to reconstruct `[]Vote`, and doesn't depend on additional access to a `ValidatorSet` +- Lite clients can check if they know the validators in a commit without + re-downloading the validator set +- Easy to see directly in a commit which validators signed what without having + to fetch the validator set + +If and when we change the `CommitSig` again, for instance to remove the timestamp, +we can reconsider whether the ValidatorAddress should be removed. + +**Absent Votes**: we include absent votes explicitly with no Signature or +Timestamp but with the ValidatorAddress. This should resolve the serialization +issues and make it easy to see which validator's votes failed to be included. + +**Other BlockIDs**: We use a single byte to indicate which blockID a `CommitSig` +is for. The only options are: + - `Absent` - no vote received from the this validator, so no signature + - `Nil` - validator voted Nil - meaning they did not see a polka in time + - `Commit` - validator voted for this block + +Note this means we don't allow votes for any other blockIDs. If a signature is +included in a commit, it is either for nil or the correct blockID. According to +the Tendermint protocol and assumptions, there is no way for a correct validator to +precommit for a conflicting blockID in the same round an actual commit was +created. This was the consensus from +[#3485](https://github.com/tendermint/tendermint/issues/3485) + +We may want to consider supporting other blockIDs later, as a way to capture +evidence that might be helpful. We should clarify if/when/how doing so would +actually help first. To implement it, we could change the `Commit.BlockID` +field to a slice, where the first entry is the correct block ID and the other +entries are other BlockIDs that validators precommited before. The BlockIDFlag +enum can be extended to represent these additional block IDs on a per block +basis. ## Status -Proposed +Accepted ## Consequences