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 <anton.kalyaev@gmail.com>

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.
+Re the concerns outlined in the context:
-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`.
+**Timestamp**: Leave the timestamp for now. Removing it and switching to
-While this still takes 20-bytes per signature, it ensures that the Commit has all
+proposer based time will take more analysis and work, and will be left for a
-information necessary to reconstruct Vote, which simplifies mapping between Commit and Vote objects
+future breaking change. In the meantime, the concerns with the current approach to
-and with debugging. It also may be necessary for the light-client to know which address a signature corresponds to if
+BFT time [can be
-it is trying to verify a current commit with an older validtor set.
+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