privval: Switch to amino encoding in SignBytes

CHANGELOG_PENDING.md

@@ -18,6 +18,8 @@ BREAKING CHANGES:
     * int.go: Uint64Slice, all put/get int64 methods
 
 * Blockchain Protocol
+  * [types] \#2459 `Vote`/`Proposal`/`Heartbeat` use amino encoding instead of JSON in `SignBytes`.
+  * [privval] \#2459 Split `SocketPVMsg`s implementations into Request and Response, where the Response may contain a error message (returned by the remote signer). 
 
 * P2P Protocol
 

cmd/tendermint/commands/init.go

@@ -59,8 +59,8 @@ func initFilesWithConfig(config *cfg.Config) error {
 		}
 		genDoc.Validators = []types.GenesisValidator{{
 			Address: pv.GetPubKey().Address(),
-			PubKey: pv.GetPubKey(),
-			Power:  10,
+			PubKey:  pv.GetPubKey(),
+			Power:   10,
 		}}
 
 		if err := genDoc.SaveAs(genFile); err != nil {

cmd/tendermint/commands/testnet.go

@@ -92,9 +92,9 @@ func testnetFiles(cmd *cobra.Command, args []string) error {
 		pv := privval.LoadFilePV(pvFile)
 		genVals[i] = types.GenesisValidator{
 			Address: pv.GetPubKey().Address(),
-			PubKey: pv.GetPubKey(),
-			Power:  1,
-			Name:   nodeDirName,
+			PubKey:  pv.GetPubKey(),
+			Power:   1,
+			Name:    nodeDirName,
 		}
 	}
 

consensus/metrics.go

@@ -85,7 +85,6 @@ func PrometheusMetrics() *Metrics {
 			Help:      "Total power of the byzantine validators.",
 		}, []string{}),
 
-
 		BlockIntervalSeconds: prometheus.NewGaugeFrom(stdprometheus.GaugeOpts{
 			Subsystem: "consensus",
 			Name:      "block_interval_seconds",

consensus/state.go

@@ -961,7 +961,6 @@ func (cs *ConsensusState) createProposalBlock() (block *types.Block, blockParts
 	return block, parts
 }
 
-
 // Enter: `timeoutPropose` after entering Propose.
 // Enter: proposal block and POL is ready.
 // Enter: any +2/3 prevotes for future round.

docs/spec/blockchain/blockchain.md

@@ -401,14 +401,22 @@ must be greater than 2/3 of the total voting power of the complete validator set
 
 A vote is a signed message broadcast in the consensus for a particular block at a particular height and round.
 When stored in the blockchain or propagated over the network, votes are encoded in Amino.
-For signing, votes are encoded in JSON, and the ChainID is included, in the form of the `CanonicalSignBytes`.
+For signing, votes are represented via `CanonicalVote` and also encoded using amino (protobuf compatible) via 
+`Vote.SignBytes` which includes the `ChainID`.
 
-We define a method `Verify` that returns `true` if the signature verifies against the pubkey for the CanonicalSignBytes
+We define a method `Verify` that returns `true` if the signature verifies against the pubkey for the `SignBytes`
 using the given ChainID:
 
 ```go
-func (v Vote) Verify(chainID string, pubKey PubKey) bool {
-    return pubKey.Verify(v.Signature, CanonicalSignBytes(chainID, v))
+func (vote *Vote) Verify(chainID string, pubKey crypto.PubKey) error {
+	if !bytes.Equal(pubKey.Address(), vote.ValidatorAddress) {
+		return ErrVoteInvalidValidatorAddress
+	}
+
+	if !pubKey.VerifyBytes(vote.SignBytes(chainID), vote.Signature) {
+		return ErrVoteInvalidSignature
+	}
+	return nil
 }
 ```
 

privval/priv_validator.go

@@ -311,21 +311,18 @@ func (pv *FilePV) String() string {
 // returns the timestamp from the lastSignBytes.
 // returns true if the only difference in the votes is their timestamp.
 func checkVotesOnlyDifferByTimestamp(lastSignBytes, newSignBytes []byte) (time.Time, bool) {
-	var lastVote, newVote types.CanonicalJSONVote
-	if err := cdc.UnmarshalJSON(lastSignBytes, &lastVote); err != nil {
+	var lastVote, newVote types.CanonicalVote
+	if err := cdc.UnmarshalBinary(lastSignBytes, &lastVote); err != nil {
 		panic(fmt.Sprintf("LastSignBytes cannot be unmarshalled into vote: %v", err))
 	}
-	if err := cdc.UnmarshalJSON(newSignBytes, &newVote); err != nil {
+	if err := cdc.UnmarshalBinary(newSignBytes, &newVote); err != nil {
 		panic(fmt.Sprintf("signBytes cannot be unmarshalled into vote: %v", err))
 	}
 
-	lastTime, err := time.Parse(types.TimeFormat, lastVote.Timestamp)
-	if err != nil {
-		panic(err)
-	}
+	lastTime := lastVote.Timestamp
 
 	// set the times to the same value and check equality
-	now := types.CanonicalTime(tmtime.Now())
+	now := tmtime.Now()
 	lastVote.Timestamp = now
 	newVote.Timestamp = now
 	lastVoteBytes, _ := cdc.MarshalJSON(lastVote)
@@ -337,25 +334,21 @@ func checkVotesOnlyDifferByTimestamp(lastSignBytes, newSignBytes []byte) (time.T
 // returns the timestamp from the lastSignBytes.
 // returns true if the only difference in the proposals is their timestamp
 func checkProposalsOnlyDifferByTimestamp(lastSignBytes, newSignBytes []byte) (time.Time, bool) {
-	var lastProposal, newProposal types.CanonicalJSONProposal
-	if err := cdc.UnmarshalJSON(lastSignBytes, &lastProposal); err != nil {
+	var lastProposal, newProposal types.CanonicalProposal
+	if err := cdc.UnmarshalBinary(lastSignBytes, &lastProposal); err != nil {
 		panic(fmt.Sprintf("LastSignBytes cannot be unmarshalled into proposal: %v", err))
 	}
-	if err := cdc.UnmarshalJSON(newSignBytes, &newProposal); err != nil {
+	if err := cdc.UnmarshalBinary(newSignBytes, &newProposal); err != nil {
 		panic(fmt.Sprintf("signBytes cannot be unmarshalled into proposal: %v", err))
 	}
 
-	lastTime, err := time.Parse(types.TimeFormat, lastProposal.Timestamp)
-	if err != nil {
-		panic(err)
-	}
-
+	lastTime := lastProposal.Timestamp
 	// set the times to the same value and check equality
-	now := types.CanonicalTime(tmtime.Now())
+	now := tmtime.Now()
 	lastProposal.Timestamp = now
 	newProposal.Timestamp = now
-	lastProposalBytes, _ := cdc.MarshalJSON(lastProposal)
-	newProposalBytes, _ := cdc.MarshalJSON(newProposal)
+	lastProposalBytes, _ := cdc.MarshalBinary(lastProposal)
+	newProposalBytes, _ := cdc.MarshalBinary(newProposal)
 
 	return lastTime, bytes.Equal(newProposalBytes, lastProposalBytes)
 }

privval/socket.go

@@ -7,7 +7,7 @@ import (
 	"net"
 	"time"
 
-	amino "github.com/tendermint/go-amino"
+	"github.com/tendermint/go-amino"
 
 	"github.com/tendermint/tendermint/crypto"
 	"github.com/tendermint/tendermint/crypto/ed25519"
@@ -27,9 +27,10 @@ const (
 
 // Socket errors.
 var (
-	ErrDialRetryMax    = errors.New("dialed maximum retries")
-	ErrConnWaitTimeout = errors.New("waited for remote signer for too long")
-	ErrConnTimeout     = errors.New("remote signer timed out")
+	ErrDialRetryMax       = errors.New("dialed maximum retries")
+	ErrConnWaitTimeout    = errors.New("waited for remote signer for too long")
+	ErrConnTimeout        = errors.New("remote signer timed out")
+	ErrUnexpectedResponse = errors.New("received unexpected response")
 )
 
 var (
@@ -150,7 +151,7 @@ func (sc *SocketPV) getPubKey() (crypto.PubKey, error) {
 
 // SignVote implements PrivValidator.
 func (sc *SocketPV) SignVote(chainID string, vote *types.Vote) error {
-	err := writeMsg(sc.conn, &SignVoteMsg{Vote: vote})
+	err := writeMsg(sc.conn, &SignVoteRequest{Vote: vote})
 	if err != nil {
 		return err
 	}
@@ -160,7 +161,16 @@ func (sc *SocketPV) SignVote(chainID string, vote *types.Vote) error {
 		return err
 	}
 
-	*vote = *res.(*SignVoteMsg).Vote
+	resp, ok := res.(*SignedVoteResponse)
+	if !ok {
+		return ErrUnexpectedResponse
+	}
+	if resp.Error != nil {
+		return fmt.Errorf("remote error occurred: code: %v, description: %s",
+			resp.Error.Code,
+			resp.Error.Description)
+	}
+	*vote = *resp.Vote
 
 	return nil
 }
@@ -170,7 +180,7 @@ func (sc *SocketPV) SignProposal(
 	chainID string,
 	proposal *types.Proposal,
 ) error {
-	err := writeMsg(sc.conn, &SignProposalMsg{Proposal: proposal})
+	err := writeMsg(sc.conn, &SignProposalRequest{Proposal: proposal})
 	if err != nil {
 		return err
 	}
@@ -179,8 +189,16 @@ func (sc *SocketPV) SignProposal(
 	if err != nil {
 		return err
 	}
-
-	*proposal = *res.(*SignProposalMsg).Proposal
+	resp, ok := res.(*SignedProposalResponse)
+	if !ok {
+		return ErrUnexpectedResponse
+	}
+	if resp.Error != nil {
+		return fmt.Errorf("remote error occurred: code: %v, description: %s",
+			resp.Error.Code,
+			resp.Error.Description)
+	}
+	*proposal = *resp.Proposal
 
 	return nil
 }
@@ -190,7 +208,7 @@ func (sc *SocketPV) SignHeartbeat(
 	chainID string,
 	heartbeat *types.Heartbeat,
 ) error {
-	err := writeMsg(sc.conn, &SignHeartbeatMsg{Heartbeat: heartbeat})
+	err := writeMsg(sc.conn, &SignHeartbeatRequest{Heartbeat: heartbeat})
 	if err != nil {
 		return err
 	}
@@ -199,8 +217,16 @@ func (sc *SocketPV) SignHeartbeat(
 	if err != nil {
 		return err
 	}
-
-	*heartbeat = *res.(*SignHeartbeatMsg).Heartbeat
+	resp, ok := res.(*SignedHeartbeatResponse)
+	if !ok {
+		return ErrUnexpectedResponse
+	}
+	if resp.Error != nil {
+		return fmt.Errorf("remote error occurred: code: %v, description: %s",
+			resp.Error.Code,
+			resp.Error.Description)
+	}
+	*heartbeat = *resp.Heartbeat
 
 	return nil
 }
@@ -462,22 +488,34 @@ func (rs *RemoteSigner) handleConnection(conn net.Conn) {
 			var p crypto.PubKey
 			p = rs.privVal.GetPubKey()
 			res = &PubKeyMsg{p}
-		case *SignVoteMsg:
+		case *SignVoteRequest:
 			err = rs.privVal.SignVote(rs.chainID, r.Vote)
-			res = &SignVoteMsg{r.Vote}
-		case *SignProposalMsg:
+			if err != nil {
+				res = &SignedVoteResponse{nil, &RemoteSignerError{0, err.Error()}}
+			} else {
+				res = &SignedVoteResponse{r.Vote, nil}
+			}
+		case *SignProposalRequest:
 			err = rs.privVal.SignProposal(rs.chainID, r.Proposal)
-			res = &SignProposalMsg{r.Proposal}
-		case *SignHeartbeatMsg:
+			if err != nil {
+				res = &SignedProposalResponse{nil, &RemoteSignerError{0, err.Error()}}
+			} else {
+				res = &SignedProposalResponse{r.Proposal, nil}
+			}
+		case *SignHeartbeatRequest:
 			err = rs.privVal.SignHeartbeat(rs.chainID, r.Heartbeat)
-			res = &SignHeartbeatMsg{r.Heartbeat}
+			if err != nil {
+				res = &SignedHeartbeatResponse{nil, &RemoteSignerError{0, err.Error()}}
+			} else {
+				res = &SignedHeartbeatResponse{r.Heartbeat, nil}
+			}
 		default:
 			err = fmt.Errorf("unknown msg: %v", r)
 		}
 
 		if err != nil {
+			// only log the error; we'll reply with an error in res
 			rs.Logger.Error("handleConnection", "err", err)
-			return
 		}
 
 		err = writeMsg(conn, res)
@@ -496,31 +534,57 @@ type SocketPVMsg interface{}
 func RegisterSocketPVMsg(cdc *amino.Codec) {
 	cdc.RegisterInterface((*SocketPVMsg)(nil), nil)
 	cdc.RegisterConcrete(&PubKeyMsg{}, "tendermint/socketpv/PubKeyMsg", nil)
-	cdc.RegisterConcrete(&SignVoteMsg{}, "tendermint/socketpv/SignVoteMsg", nil)
-	cdc.RegisterConcrete(&SignProposalMsg{}, "tendermint/socketpv/SignProposalMsg", nil)
-	cdc.RegisterConcrete(&SignHeartbeatMsg{}, "tendermint/socketpv/SignHeartbeatMsg", nil)
+	cdc.RegisterConcrete(&SignVoteRequest{}, "tendermint/socketpv/SignVoteRequest", nil)
+	cdc.RegisterConcrete(&SignedVoteResponse{}, "tendermint/socketpv/SignedVoteResponse", nil)
+	cdc.RegisterConcrete(&SignProposalRequest{}, "tendermint/socketpv/SignProposalRequest", nil)
+	cdc.RegisterConcrete(&SignedProposalResponse{}, "tendermint/socketpv/SignedProposalResponse", nil)
+	cdc.RegisterConcrete(&SignHeartbeatRequest{}, "tendermint/socketpv/SignHeartbeatRequest", nil)
+	cdc.RegisterConcrete(&SignedHeartbeatResponse{}, "tendermint/socketpv/SignedHeartbeatResponse", nil)
 }
 
 // PubKeyMsg is a PrivValidatorSocket message containing the public key.
 type PubKeyMsg struct {
 	PubKey crypto.PubKey
 }
 
-// SignVoteMsg is a PrivValidatorSocket message containing a vote.
-type SignVoteMsg struct {
+// SignVoteRequest is a PrivValidatorSocket message containing a vote.
+type SignVoteRequest struct {
 	Vote *types.Vote
 }
 
-// SignProposalMsg is a PrivValidatorSocket message containing a Proposal.
-type SignProposalMsg struct {
+// SignVoteReply is a PrivValidatorSocket message containing a signed vote along with a potenial error message.
+type SignedVoteResponse struct {
+	Vote  *types.Vote
+	Error *RemoteSignerError
+}
+
+// SignProposalRequest is a PrivValidatorSocket message containing a Proposal.
+type SignProposalRequest struct {
+	Proposal *types.Proposal
+}
+
+type SignedProposalResponse struct {
 	Proposal *types.Proposal
+	Error    *RemoteSignerError
 }
 
-// SignHeartbeatMsg is a PrivValidatorSocket message containing a Heartbeat.
-type SignHeartbeatMsg struct {
+// SignHeartbeatRequest is a PrivValidatorSocket message containing a Heartbeat.
+type SignHeartbeatRequest struct {
 	Heartbeat *types.Heartbeat
 }
 
+type SignedHeartbeatResponse struct {
+	Heartbeat *types.Heartbeat
+	Error     *RemoteSignerError
+}
+
+// RemoteSignerError allows (remote) validators to include meaningful error descriptions in their reply.
+type RemoteSignerError struct {
+	// TODO(ismail): create an enum of known errors
+	Code        int
+	Description string
+}
+
 func readMsg(r io.Reader) (msg SocketPVMsg, err error) {
 	const maxSocketPVMsgSize = 1024 * 10
 	_, err = cdc.UnmarshalBinaryReader(r, &msg, maxSocketPVMsgSize)