From 4f2ef3670143e8bc46fc76e734be80416053cc16 Mon Sep 17 00:00:00 2001 From: Ethan Buchman Date: Fri, 8 Feb 2019 18:40:41 -0500 Subject: [PATCH] types.NewCommit (#3275) * types.NewCommit * use types.NewCommit everywhere * fix log in unsafe_reset * memoize height and round in constructor * notes about deprecating toVote * bring back memoizeHeightRound --- blockchain/reactor_test.go | 4 +-- blockchain/store_test.go | 7 ++-- .../commands/reset_priv_validator.go | 2 +- consensus/replay_test.go | 6 ++-- consensus/state.go | 2 +- consensus/types/round_state_test.go | 7 ++-- lite/helpers.go | 8 ++--- lite/proxy/validate_test.go | 8 ++--- node/node_test.go | 2 +- state/execution_test.go | 4 +-- types/block.go | 34 +++++++++++-------- types/validator_set_test.go | 10 ++---- types/vote_set.go | 5 +-- 13 files changed, 42 insertions(+), 57 deletions(-) diff --git a/blockchain/reactor_test.go b/blockchain/reactor_test.go index 138e16222..1e8666f12 100644 --- a/blockchain/reactor_test.go +++ b/blockchain/reactor_test.go @@ -95,13 +95,13 @@ func newBlockchainReactor(logger log.Logger, genDoc *types.GenesisDoc, privVals // let's add some blocks in for blockHeight := int64(1); blockHeight <= maxBlockHeight; blockHeight++ { - lastCommit := &types.Commit{} + lastCommit := types.NewCommit(types.BlockID{}, nil) if blockHeight > 1 { lastBlockMeta := blockStore.LoadBlockMeta(blockHeight - 1) lastBlock := blockStore.LoadBlock(blockHeight - 1) vote := makeVote(&lastBlock.Header, lastBlockMeta.BlockID, state.Validators, privVals[0]).CommitSig() - lastCommit = &types.Commit{Precommits: []*types.CommitSig{vote}, BlockID: lastBlockMeta.BlockID} + lastCommit = types.NewCommit(lastBlockMeta.BlockID, []*types.CommitSig{vote}) } thisBlock := makeBlock(blockHeight, state, lastCommit) diff --git a/blockchain/store_test.go b/blockchain/store_test.go index 9abc210b1..931faf6a7 100644 --- a/blockchain/store_test.go +++ b/blockchain/store_test.go @@ -23,11 +23,8 @@ import ( // make a Commit with a single vote containing just the height and a timestamp func makeTestCommit(height int64, timestamp time.Time) *types.Commit { - return &types.Commit{ - Precommits: []*types.CommitSig{ - {Height: height, Timestamp: timestamp}, - }, - } + commitSigs := []*types.CommitSig{{Height: height, Timestamp: timestamp}} + return types.NewCommit(types.BlockID{}, commitSigs) } func makeStateAndBlockStore(logger log.Logger) (sm.State, *BlockStore) { diff --git a/cmd/tendermint/commands/reset_priv_validator.go b/cmd/tendermint/commands/reset_priv_validator.go index 122c2a725..055a76c51 100644 --- a/cmd/tendermint/commands/reset_priv_validator.go +++ b/cmd/tendermint/commands/reset_priv_validator.go @@ -61,7 +61,7 @@ func resetFilePV(privValKeyFile, privValStateFile string, logger log.Logger) { } else { pv := privval.GenFilePV(privValKeyFile, privValStateFile) pv.Save() - logger.Info("Generated private validator file", "file", "keyFile", privValKeyFile, + logger.Info("Generated private validator file", "keyFile", privValKeyFile, "stateFile", privValStateFile) } } diff --git a/consensus/replay_test.go b/consensus/replay_test.go index e7269254c..297c13c3b 100644 --- a/consensus/replay_test.go +++ b/consensus/replay_test.go @@ -537,10 +537,8 @@ func makeBlockchainFromWAL(wal WAL) ([]*types.Block, []*types.Commit, error) { } case *types.Vote: if p.Type == types.PrecommitType { - thisBlockCommit = &types.Commit{ - BlockID: p.BlockID, - Precommits: []*types.CommitSig{p.CommitSig()}, - } + commitSigs := []*types.CommitSig{p.CommitSig()} + thisBlockCommit = types.NewCommit(p.BlockID, commitSigs) } } } diff --git a/consensus/state.go b/consensus/state.go index 74165801b..c6c49d875 100644 --- a/consensus/state.go +++ b/consensus/state.go @@ -954,7 +954,7 @@ func (cs *ConsensusState) createProposalBlock() (block *types.Block, blockParts if cs.Height == 1 { // We're creating a proposal for the first block. // The commit is empty, but not nil. - commit = &types.Commit{} + commit = types.NewCommit(types.BlockID{}, nil) } else if cs.LastCommit.HasTwoThirdsMajority() { // Make the commit from LastCommit commit = cs.LastCommit.MakeCommit() diff --git a/consensus/types/round_state_test.go b/consensus/types/round_state_test.go index cb16f939a..a9bc8e14b 100644 --- a/consensus/types/round_state_test.go +++ b/consensus/types/round_state_test.go @@ -53,11 +53,8 @@ func BenchmarkRoundStateDeepCopy(b *testing.B) { Data: types.Data{ Txs: txs, }, - Evidence: types.EvidenceData{}, - LastCommit: &types.Commit{ - BlockID: blockID, - Precommits: precommits, - }, + Evidence: types.EvidenceData{}, + LastCommit: types.NewCommit(blockID, precommits), } parts := block.MakePartSet(4096) // Random Proposal diff --git a/lite/helpers.go b/lite/helpers.go index 6b18b3514..119797f36 100644 --- a/lite/helpers.go +++ b/lite/helpers.go @@ -80,12 +80,8 @@ func (pkz privKeys) signHeader(header *types.Header, first, last int) *types.Com vote := makeVote(header, vset, pkz[i]) commitSigs[vote.ValidatorIndex] = vote.CommitSig() } - - res := &types.Commit{ - BlockID: types.BlockID{Hash: header.Hash()}, - Precommits: commitSigs, - } - return res + blockID := types.BlockID{Hash: header.Hash()} + return types.NewCommit(blockID, commitSigs) } func makeVote(header *types.Header, valset *types.ValidatorSet, key crypto.PrivKey) *types.Vote { diff --git a/lite/proxy/validate_test.go b/lite/proxy/validate_test.go index 1ce4d667e..dce177d7b 100644 --- a/lite/proxy/validate_test.go +++ b/lite/proxy/validate_test.go @@ -70,7 +70,7 @@ func TestValidateBlock(t *testing.T) { }, signedHeader: types.SignedHeader{ Header: &types.Header{Height: 11}, - Commit: &types.Commit{BlockID: types.BlockID{Hash: []byte("0xDEADBEEF")}}, + Commit: types.NewCommit(types.BlockID{Hash: []byte("0xDEADBEEF")}, nil), }, wantErr: "Data hash doesn't match header", }, @@ -81,7 +81,7 @@ func TestValidateBlock(t *testing.T) { }, signedHeader: types.SignedHeader{ Header: &types.Header{Height: 11}, - Commit: &types.Commit{BlockID: types.BlockID{Hash: []byte("DEADBEEF")}}, + Commit: types.NewCommit(types.BlockID{Hash: []byte("DEADBEEF")}, nil), }, }, // End Header.Data hash mismatch test @@ -169,7 +169,7 @@ func TestValidateBlockMeta(t *testing.T) { ValidatorsHash: []byte("Tendermint"), Time: testTime2, }, - Commit: &types.Commit{BlockID: types.BlockID{Hash: []byte("DEADBEEF")}}, + Commit: types.NewCommit(types.BlockID{Hash: []byte("DEADBEEF")}, nil), }, wantErr: "Headers don't match", }, @@ -188,7 +188,7 @@ func TestValidateBlockMeta(t *testing.T) { ValidatorsHash: []byte("Tendermint-x"), Time: testTime2, }, - Commit: &types.Commit{BlockID: types.BlockID{Hash: []byte("DEADBEEF")}}, + Commit: types.NewCommit(types.BlockID{Hash: []byte("DEADBEEF")}, nil), }, wantErr: "Headers don't match", }, diff --git a/node/node_test.go b/node/node_test.go index 4b4610e10..d7907e88a 100644 --- a/node/node_test.go +++ b/node/node_test.go @@ -260,7 +260,7 @@ func TestCreateProposalBlock(t *testing.T) { evidencePool, ) - commit := &types.Commit{} + commit := types.NewCommit(types.BlockID{}, nil) block, _ := blockExec.CreateProposalBlock( height, state, commit, diff --git a/state/execution_test.go b/state/execution_test.go index e0c9b4b9d..8cd90f963 100644 --- a/state/execution_test.go +++ b/state/execution_test.go @@ -79,7 +79,7 @@ func TestBeginBlockValidators(t *testing.T) { } for _, tc := range testCases { - lastCommit := &types.Commit{BlockID: prevBlockID, Precommits: tc.lastCommitPrecommits} + lastCommit := types.NewCommit(prevBlockID, tc.lastCommitPrecommits) // block for height 2 block, _ := state.MakeBlock(2, makeTxs(2), lastCommit, nil, state.Validators.GetProposer().Address) @@ -139,7 +139,7 @@ func TestBeginBlockByzantineValidators(t *testing.T) { commitSig0 := (&types.Vote{ValidatorIndex: 0, Timestamp: now, Type: types.PrecommitType}).CommitSig() commitSig1 := (&types.Vote{ValidatorIndex: 1, Timestamp: now}).CommitSig() commitSigs := []*types.CommitSig{commitSig0, commitSig1} - lastCommit := &types.Commit{BlockID: prevBlockID, Precommits: commitSigs} + lastCommit := types.NewCommit(prevBlockID, commitSigs) for _, tc := range testCases { block, _ := state.MakeBlock(10, makeTxs(2), lastCommit, nil, state.Validators.GetProposer().Address) diff --git a/types/block.go b/types/block.go index ec09fd449..bcaf0725c 100644 --- a/types/block.go +++ b/types/block.go @@ -490,8 +490,8 @@ func (cs *CommitSig) String() string { } // toVote converts the CommitSig to a vote. -// Once CommitSig has fewer fields than vote, -// converting to a Vote will require more information. +// TODO: deprecate for #1648. Converting to Vote will require +// access to ValidatorSet. func (cs *CommitSig) toVote() *Vote { if cs == nil { return nil @@ -509,18 +509,30 @@ type Commit struct { BlockID BlockID `json:"block_id"` Precommits []*CommitSig `json:"precommits"` - // Volatile + // memoized in first call to corresponding method + // NOTE: can't memoize in constructor because constructor + // isn't used for unmarshaling height int64 round int hash cmn.HexBytes bitArray *cmn.BitArray } +// NewCommit returns a new Commit with the given blockID and precommits. +// TODO: memoize ValidatorSet in constructor so votes can be easily reconstructed +// from CommitSig after #1648. +func NewCommit(blockID BlockID, precommits []*CommitSig) *Commit { + return &Commit{ + BlockID: blockID, + Precommits: precommits, + } +} + // VoteSignBytes constructs the SignBytes for the given CommitSig. // The only unique part of the SignBytes is the Timestamp - all other fields // signed over are otherwise the same for all validators. func (commit *Commit) VoteSignBytes(chainID string, cs *CommitSig) []byte { - return cs.toVote().SignBytes(chainID) + return commit.ToVote(cs).SignBytes(chainID) } // memoizeHeightRound memoizes the height and round of the commit using @@ -543,27 +555,20 @@ func (commit *Commit) memoizeHeightRound() { // ToVote converts a CommitSig to a Vote. // If the CommitSig is nil, the Vote will be nil. -// When CommitSig is reduced to contain fewer fields, -// this will need access to the ValidatorSet to properly -// reconstruct the vote. func (commit *Commit) ToVote(cs *CommitSig) *Vote { + // TODO: use commit.validatorSet to reconstruct vote + // and deprecate .toVote return cs.toVote() } // Height returns the height of the commit func (commit *Commit) Height() int64 { - if len(commit.Precommits) == 0 { - return 0 - } commit.memoizeHeightRound() return commit.height } // Round returns the round of the commit func (commit *Commit) Round() int { - if len(commit.Precommits) == 0 { - return 0 - } commit.memoizeHeightRound() return commit.round } @@ -595,9 +600,10 @@ func (commit *Commit) BitArray() *cmn.BitArray { } // GetByIndex returns the vote corresponding to a given validator index. +// Panics if `index >= commit.Size()`. // Implements VoteSetReader. func (commit *Commit) GetByIndex(index int) *Vote { - return commit.Precommits[index].toVote() + return commit.ToVote(commit.Precommits[index]) } // IsCommit returns true if there is at least one vote. diff --git a/types/validator_set_test.go b/types/validator_set_test.go index 04874c198..cdc04d459 100644 --- a/types/validator_set_test.go +++ b/types/validator_set_test.go @@ -563,18 +563,12 @@ func TestValidatorSetVerifyCommit(t *testing.T) { sig, err := privKey.Sign(vote.SignBytes(chainID)) assert.NoError(t, err) vote.Signature = sig - commit := &Commit{ - BlockID: blockID, - Precommits: []*CommitSig{vote.CommitSig()}, - } + commit := NewCommit(blockID, []*CommitSig{vote.CommitSig()}) badChainID := "notmychainID" badBlockID := BlockID{Hash: []byte("goodbye")} badHeight := height + 1 - badCommit := &Commit{ - BlockID: blockID, - Precommits: []*CommitSig{nil}, - } + badCommit := NewCommit(blockID, []*CommitSig{nil}) // test some error cases // TODO: test more cases! diff --git a/types/vote_set.go b/types/vote_set.go index 14930da4a..1cd0f2281 100644 --- a/types/vote_set.go +++ b/types/vote_set.go @@ -545,10 +545,7 @@ func (voteSet *VoteSet) MakeCommit() *Commit { for i, v := range voteSet.votes { commitSigs[i] = v.CommitSig() } - return &Commit{ - BlockID: *voteSet.maj23, - Precommits: commitSigs, - } + return NewCommit(*voteSet.maj23, commitSigs) } //--------------------------------------------------------------------------------