From 0dac36aa5d5797e54c2d9999b7ee5a3f192f5644 Mon Sep 17 00:00:00 2001 From: Jae Kwon Date: Wed, 31 Dec 2014 16:14:26 -0800 Subject: [PATCH] Do not panic when double-signing, return error. --- consensus/pol_test.go | 7 ++++++- consensus/state.go | 42 ++++++++++++++++++++++++++------------ consensus/state_test.go | 20 ++++++++++++++---- consensus/vote_set_test.go | 2 +- state/priv_validator.go | 32 ++++++++++++++++------------- 5 files changed, 70 insertions(+), 33 deletions(-) diff --git a/consensus/pol_test.go b/consensus/pol_test.go index 6c0f48178..a4a636406 100644 --- a/consensus/pol_test.go +++ b/consensus/pol_test.go @@ -16,8 +16,13 @@ import ( // Signs the vote and sets the POL's vote at the desired index // Returns the POLVoteSignature pointer, so you can modify it afterwards. func signAddPOLVoteSignature(val *state.PrivValidator, valSet *state.ValidatorSet, vote *Vote, pol *POL) *POLVoteSignature { + vote = vote.Copy() + err := val.SignVote(vote) + if err != nil { + panic(err) + } idx, _ := valSet.GetByAddress(val.Address) // now we have the index - pol.Votes[idx] = POLVoteSignature{vote.Round, val.SignVote(vote)} + pol.Votes[idx] = POLVoteSignature{vote.Round, vote.Signature} return &pol.Votes[idx] } diff --git a/consensus/state.go b/consensus/state.go index a4889487a..ad0ba764e 100644 --- a/consensus/state.go +++ b/consensus/state.go @@ -491,8 +491,13 @@ func (cs *ConsensusState) updateToState(state *state.State) { Address: cs.PrivValidator.Address, Height: cs.Height + 1, } - rebondTx.Signature = cs.PrivValidator.SignRebondTx(rebondTx) - cs.mempoolReactor.BroadcastTx(rebondTx) + err := cs.PrivValidator.SignRebondTx(rebondTx) + if err == nil { + log.Info("Signed and broadcast RebondTx", "height", cs.Height, "round", cs.Round, "tx", rebondTx) + cs.mempoolReactor.BroadcastTx(rebondTx) + } else { + log.Warn("Error signing RebondTx", "height", cs.Height, "round", cs.Round, "tx", rebondTx, "error", err) + } } } @@ -609,6 +614,7 @@ func (cs *ConsensusState) RunActionPropose(height uint, round uint) { // Set the block.Header.StateHash. // TODO: we could cache the resulting state to cs.stagedState. + // TODO: This is confusing, not clear that we're mutating block. cs.state.Copy().AppendBlock(block, PartSetHeader{}, false) blockParts = NewPartSetFromData(BinaryBytes(block)) @@ -621,14 +627,18 @@ func (cs *ConsensusState) RunActionPropose(height uint, round uint) { // Make proposal proposal := NewProposal(cs.Height, cs.Round, blockParts.Header(), polParts.Header()) - proposal.Signature = cs.PrivValidator.SignProposal(proposal) - - // Set fields - cs.Proposal = proposal - cs.ProposalBlock = block - cs.ProposalBlockParts = blockParts - cs.ProposalPOL = pol - cs.ProposalPOLParts = polParts + err := cs.PrivValidator.SignProposal(proposal) + if err == nil { + log.Info("Signed and set proposal", "height", cs.Height, "round", cs.Round, "proposal", proposal) + // Set fields + cs.Proposal = proposal + cs.ProposalBlock = block + cs.ProposalBlockParts = blockParts + cs.ProposalPOL = pol + cs.ProposalPOLParts = polParts + } else { + log.Warn("Error signing proposal", "height", cs.Height, "round", cs.Round, "error", err) + } } // Prevote for LockedBlock if we're locked, or ProposealBlock if valid. @@ -1015,9 +1025,15 @@ func (cs *ConsensusState) signAddVote(type_ byte, hash []byte, header PartSetHea BlockHash: hash, BlockParts: header, } - vote.Signature = cs.PrivValidator.SignVote(vote) - cs.addVote(cs.PrivValidator.Address, vote) - return vote + err := cs.PrivValidator.SignVote(vote) + if err == nil { + log.Info("Signed and added vote", "height", cs.Height, "round", cs.Round, "vote", vote) + cs.addVote(cs.PrivValidator.Address, vote) + return vote + } else { + log.Warn("Error signing vote", "height", cs.Height, "round", cs.Round, "vote", vote, "error", err) + return nil + } } func (cs *ConsensusState) saveCommitVoteBlock(block *Block, blockParts *PartSet) { diff --git a/consensus/state_test.go b/consensus/state_test.go index f7a333bc7..25dd9b1ec 100644 --- a/consensus/state_test.go +++ b/consensus/state_test.go @@ -15,7 +15,10 @@ func TestSetupRound(t *testing.T) { voteTypes := []byte{VoteTypePrevote, VoteTypePrecommit, VoteTypeCommit} for _, voteType := range voteTypes { vote := &Vote{Height: 1, Round: 0, Type: voteType} // nil vote - vote.Signature = val0.SignVote(vote) + err := val0.SignVote(vote) + if err != nil { + t.Error("Error signing vote: %v", err) + } cs.AddVote(val0.Address, vote) } @@ -120,7 +123,10 @@ func TestRunActionPrecommitCommitFinalize(t *testing.T) { BlockHash: cs.ProposalBlock.Hash(), BlockParts: cs.ProposalBlockParts.Header(), } - vote.Signature = privValidators[i].SignVote(vote) + err := privValidators[i].SignVote(vote) + if err != nil { + t.Error("Error signing vote: %v", err) + } cs.AddVote(privValidators[i].Address, vote) } @@ -147,7 +153,10 @@ func TestRunActionPrecommitCommitFinalize(t *testing.T) { BlockHash: cs.ProposalBlock.Hash(), BlockParts: cs.ProposalBlockParts.Header(), } - vote.Signature = privValidators[i].SignVote(vote) + err := privValidators[i].SignVote(vote) + if err != nil { + t.Error("Error signing vote: %v", err) + } added, _, err := cs.AddVote(privValidators[i].Address, vote) if !added || err != nil { t.Errorf("Error adding precommit: %v", err) @@ -182,7 +191,10 @@ func TestRunActionPrecommitCommitFinalize(t *testing.T) { BlockHash: cs.ProposalBlock.Hash(), BlockParts: cs.ProposalBlockParts.Header(), } - vote.Signature = privValidators[i].SignVote(vote) + err := privValidators[i].SignVote(vote) + if err != nil { + t.Error("Error signing vote: %v", err) + } added, _, err := cs.AddVote(privValidators[i].Address, vote) if !added || err != nil { t.Errorf("Error adding commit: %v", err) diff --git a/consensus/vote_set_test.go b/consensus/vote_set_test.go index 96e54d202..6ae240101 100644 --- a/consensus/vote_set_test.go +++ b/consensus/vote_set_test.go @@ -49,7 +49,7 @@ func withBlockParts(vote *Vote, blockParts PartSetHeader) *Vote { } func signAddVote(privVal *state.PrivValidator, vote *Vote, voteSet *VoteSet) (bool, error) { - vote.Signature = privVal.SignVoteUnsafe(vote) + privVal.SignVoteUnsafe(vote) added, _, err := voteSet.Add(privVal.Address, vote) return added, err } diff --git a/state/priv_validator.go b/state/priv_validator.go index 9e66e0647..06ffaa24e 100644 --- a/state/priv_validator.go +++ b/state/priv_validator.go @@ -6,6 +6,7 @@ import ( "bytes" "encoding/base64" "encoding/json" + "errors" "fmt" "io/ioutil" "math" @@ -151,27 +152,27 @@ func (privVal *PrivValidator) JSONBytes() []byte { } // TODO: test -func (privVal *PrivValidator) SignVote(vote *Vote) SignatureEd25519 { +func (privVal *PrivValidator) SignVote(vote *Vote) error { privVal.mtx.Lock() defer privVal.mtx.Unlock() // If height regression, panic if privVal.LastHeight > vote.Height { - panic("Height regression in SignVote") + return errors.New("Height regression in SignVote") } // More cases for when the height matches if privVal.LastHeight == vote.Height { // If attempting any sign after commit, panic if privVal.LastStep == stepCommit { - panic("SignVote on matching height after a commit") + return errors.New("SignVote on matching height after a commit") } // If round regression, panic if privVal.LastRound > vote.Round { - panic("Round regression in SignVote") + return errors.New("Round regression in SignVote") } // If step regression, panic if privVal.LastRound == vote.Round && privVal.LastStep > voteToStep(vote) { - panic("Step regression in SignVote") + return errors.New("Step regression in SignVote") } } @@ -182,14 +183,15 @@ func (privVal *PrivValidator) SignVote(vote *Vote) SignatureEd25519 { privVal.save() // Sign - return privVal.SignVoteUnsafe(vote) + privVal.SignVoteUnsafe(vote) + return nil } -func (privVal *PrivValidator) SignVoteUnsafe(vote *Vote) SignatureEd25519 { - return privVal.PrivKey.Sign(SignBytes(vote)).(SignatureEd25519) +func (privVal *PrivValidator) SignVoteUnsafe(vote *Vote) { + vote.Signature = privVal.PrivKey.Sign(SignBytes(vote)).(SignatureEd25519) } -func (privVal *PrivValidator) SignProposal(proposal *Proposal) SignatureEd25519 { +func (privVal *PrivValidator) SignProposal(proposal *Proposal) error { privVal.mtx.Lock() defer privVal.mtx.Unlock() if privVal.LastHeight < proposal.Height || @@ -203,13 +205,14 @@ func (privVal *PrivValidator) SignProposal(proposal *Proposal) SignatureEd25519 privVal.save() // Sign - return privVal.PrivKey.Sign(SignBytes(proposal)).(SignatureEd25519) + proposal.Signature = privVal.PrivKey.Sign(SignBytes(proposal)).(SignatureEd25519) + return nil } else { - panic(fmt.Sprintf("Attempt of duplicate signing of proposal: Height %v, Round %v", proposal.Height, proposal.Round)) + return errors.New(fmt.Sprintf("Attempt of duplicate signing of proposal: Height %v, Round %v", proposal.Height, proposal.Round)) } } -func (privVal *PrivValidator) SignRebondTx(rebondTx *RebondTx) SignatureEd25519 { +func (privVal *PrivValidator) SignRebondTx(rebondTx *RebondTx) error { privVal.mtx.Lock() defer privVal.mtx.Unlock() if privVal.LastHeight < rebondTx.Height { @@ -221,9 +224,10 @@ func (privVal *PrivValidator) SignRebondTx(rebondTx *RebondTx) SignatureEd25519 privVal.save() // Sign - return privVal.PrivKey.Sign(SignBytes(rebondTx)).(SignatureEd25519) + rebondTx.Signature = privVal.PrivKey.Sign(SignBytes(rebondTx)).(SignatureEd25519) + return nil } else { - panic(fmt.Sprintf("Attempt of duplicate signing of rebondTx: Height %v", rebondTx.Height)) + return errors.New(fmt.Sprintf("Attempt of duplicate signing of rebondTx: Height %v", rebondTx.Height)) } }