[ADR][DRAFT] 024: SignBytes and validator types in privval (#2445)

* first draft for ADR summarizing discussion from:
https://github.com/tendermint/tendermint/issues/1622

* fix link and add comment about pub-key per message and fix link

* fix link and add comment about pub-key per message; also:
 - fix link
 - add little diagram
 - fix typo

* Add a slightly different approach

* typo and ADR number

docs/architecture/adr-024-sign-bytes.md

@@ -0,0 +1,234 @@
+# ADR 024: SignBytes and validator types in privval
+
+## Context
+
+Currently, the messages exchanged between tendermint and a (potentially remote) signer/validator, 
+namely votes, proposals, and heartbeats, are encoded as a JSON string 
+(e.g., via `Vote.SignBytes(...)`) and then 
+signed . JSON encoding is sub-optimal for both, hardware wallets 
+and for usage in ethereum smart contracts. Both is laid down in detail in [issue#1622].  
+
+Also, there are currently no differences between sign-request and -replies. Also, there is no possibility 
+for a remote signer to include an error code or message in case something went wrong.
+The messages exchanged between tendermint and a remote signer currently live in 
+[privval/socket.go] and encapsulate the corresponding types in [types].
+
+
+[privval/socket.go]: https://github.com/tendermint/tendermint/blob/d419fffe18531317c28c29a292ad7d253f6cafdf/privval/socket.go#L496-L502
+[issue#1622]: https://github.com/tendermint/tendermint/issues/1622
+[types]: https://github.com/tendermint/tendermint/tree/master/types
+ 
+
+## Decision
+
+- restructure vote, proposal, and heartbeat such that their encoding is easily parseable by 
+hardware devices and smart contracts using a  binary encoding format ([amino] in this case)
+- split up the messages exchanged between tendermint and remote signers into requests and 
+responses (see details below)
+- include an error type in responses
+
+### Overview
+```
++--------------+                      +----------------+
+|              |     SignXRequest     |                |
+|Remote signer |<---------------------+  tendermint    |
+| (e.g. KMS)   |                      |                |
+|              +--------------------->|                |
++--------------+    SignedXReply      +----------------+
+
+
+SignXRequest {
+    x: X
+}
+
+SignedXReply {
+    x: X
+  sig: Signature // []byte
+  err: Error{ 
+    code: int
+    desc: string
+  }
+}
+```
+
+TODO: Alternatively, the type `X` might directly include the signature. A lot of places expect a vote with a 
+signature and do not necessarily deal with "Replies".
+Still exploring what would work best here. 
+This would look like (exemplified using X = Vote):
+```
+Vote {
+    // all fields besides signature
+}
+
+SignedVote {
+ Vote Vote
+ Signature []byte
+}
+
+SignVoteRequest {
+   Vote Vote
+}
+
+SignedVoteReply {
+    Vote SignedVote
+    Err  Error
+}
+```
+
+**Note:** There was a related discussion around including a fingerprint of, or, the whole public-key 
+into each sign-request to tell the signer which corresponding private-key to 
+use to sign the message. This is particularly relevant in the context of the KMS
+but is currently not considered in this ADR. 
+
+
+[amino]: https://github.com/tendermint/go-amino/
+
+### Vote
+
+As explained in [issue#1622] `Vote` will be changed to contain the following fields 
+(notation in protobuf-like syntax for easy readability):
+
+```proto
+// vanilla protobuf / amino encoded
+message Vote {
+    Version       fixed32                      
+    Height        sfixed64       
+    Round         sfixed32
+    VoteType      fixed32
+    Timestamp     Timestamp         // << using protobuf definition
+    BlockID       BlockID           // << as already defined 
+    ChainID       string            // at the end because length could vary a lot
+}
+
+// this is an amino registered type; like currently privval.SignVoteMsg: 
+// registered with "tendermint/socketpv/SignVoteRequest"
+message SignVoteRequest {
+   Vote vote
+}
+
+//  amino registered type
+// registered with "tendermint/socketpv/SignedVoteReply"
+message SignedVoteReply { 
+   Vote      Vote
+   Signature Signature 
+   Err       Error
+}
+
+// we will use this type everywhere below
+message Error {
+  Type        uint  // error code
+  Description string  // optional description
+}
+
+```
+
+The `ChainID` gets moved into the vote message directly. Previously, it was injected 
+using the [Signable] interface method `SignBytes(chainID string) []byte`. Also, the 
+signature won't be included directly, only in the corresponding `SignedVoteReply` message.
+
+[Signable]: https://github.com/tendermint/tendermint/blob/d419fffe18531317c28c29a292ad7d253f6cafdf/types/signable.go#L9-L11
+ 
+### Proposal
+
+```proto
+// vanilla protobuf / amino encoded
+message Proposal {                      
+    Height            sfixed64       
+    Round             sfixed32
+    Timestamp         Timestamp         // << using protobuf definition
+    BlockPartsHeader  PartSetHeader     // as already defined
+    POLRound          sfixed32
+    POLBlockID        BlockID           // << as already defined    
+}
+ 
+// amino registered with "tendermint/socketpv/SignProposalRequest"
+message SignProposalRequest {
+   Proposal proposal
+}
+
+// amino registered with "tendermint/socketpv/SignProposalReply"
+message SignProposalReply { 
+   Prop   Proposal
+   Sig    Signature 
+   Err    Error     // as defined above
+}
+```
+
+### Heartbeat
+
+**TODO**: clarify if heartbeat also needs a fixed offset and update the fields accordingly: 
+
+```proto
+message Heartbeat {
+	ValidatorAddress Address 
+	ValidatorIndex   int     
+	Height           int64   
+	Round            int     
+	Sequence         int     
+}
+// amino registered with "tendermint/socketpv/SignHeartbeatRequest"
+message SignHeartbeatRequest {
+   Hb Heartbeat
+}
+
+// amino registered with "tendermint/socketpv/SignHeartbeatReply"
+message SignHeartbeatReply { 
+   Hb     Heartbeat
+   Sig    Signature 
+   Err    Error     // as defined above
+}
+
+```
+
+## PubKey
+
+TBA -  this needs further thoughts: e.g. what todo like in the case of the KMS which holds
+several keys? How does it know with which key to reply?
+
+## SignBytes
+`SignBytes` will not require a `ChainID` parameter:
+
+```golang
+type Signable interface {
+	SignBytes() []byte
+}
+
+```
+And the implementation for vote, heartbeat, proposal will look like:
+```golang
+// type T is one of vote, sign, proposal
+func (tp *T) SignBytes() []byte {
+	bz, err := cdc.MarshalBinary(tp)
+	if err != nil {
+		panic(err)
+	}
+	return bz
+}
+```
+
+## Status
+
+DRAFT
+
+## Consequences
+
+
+
+### Positive
+
+The most relevant positive effect is that the signing bytes can easily be parsed by a 
+hardware module and a smart contract. Besides that:
+ 
+- clearer separation between requests and responses
+- added error messages enable better error handling 
+
+
+### Negative
+
+- relatively huge change / refactoring touching quite some code
+- lot's of places assume a `Vote` with a signature included -> they will need to 
+- need to modify some interfaces 
+
+### Neutral
+
+not even the swiss are neutral