From cd875c8a2c9c8ff2577433b6a20d2e8b11f1a686 Mon Sep 17 00:00:00 2001 From: Jasmina Malicevic Date: Mon, 7 Feb 2022 13:57:52 +0100 Subject: [PATCH] types: remove nested evidence field from block (#7765) * types: replaced EvidenceData in block structure with EvidenceList * types: introduced toProto, fromProto functions to EvidenceList * updated Changelog * Removed comments from tests --- CHANGELOG_PENDING.md | 2 + internal/consensus/byzantine_test.go | 4 +- internal/consensus/reactor_test.go | 2 +- internal/consensus/state.go | 2 +- internal/evidence/pool_test.go | 2 +- internal/state/execution.go | 10 +-- internal/state/execution_test.go | 2 +- node/node_test.go | 2 +- test/e2e/tests/evidence_test.go | 4 +- types/block.go | 99 ++-------------------------- types/block_test.go | 48 ++------------ types/evidence.go | 68 +++++++++++++++++++ types/evidence_test.go | 38 +++++++++++ 13 files changed, 130 insertions(+), 153 deletions(-) diff --git a/CHANGELOG_PENDING.md b/CHANGELOG_PENDING.md index 8dccb8dd3..0e0638d64 100644 --- a/CHANGELOG_PENDING.md +++ b/CHANGELOG_PENDING.md @@ -62,10 +62,12 @@ Special thanks to external contributors on this release: - [internal/protoio] \#7325 Optimized `MarshalDelimited` by inlining the common case and using a `sync.Pool` in the worst case. (@odeke-em) - [consensus] \#6969 remove logic to 'unlock' a locked block. +- [evidence] \#7700 Evidence messages contain single Evidence instead of EvidenceList (@jmalicevic) - [pubsub] \#7319 Performance improvements for the event query API (@creachadair) - [node] \#7521 Define concrete type for seed node implementation (@spacech1mp) - [rpc] \#7612 paginate mempool /unconfirmed_txs rpc endpoint (@spacech1mp) - [light] [\#7536](https://github.com/tendermint/tendermint/pull/7536) rpc /status call returns info about the light client (@jmalicevic) +- [types] \#7765 Replace EvidenceData with EvidenceList to avoid unnecessary nesting of evidence fields within a block. (@jmalicevic) ### BUG FIXES diff --git a/internal/consensus/byzantine_test.go b/internal/consensus/byzantine_test.go index 81813e948..50b4ffc30 100644 --- a/internal/consensus/byzantine_test.go +++ b/internal/consensus/byzantine_test.go @@ -265,8 +265,8 @@ func TestByzantinePrevoteEquivocation(t *testing.T) { require.NotNil(t, msg) block := msg.Data().(types.EventDataNewBlock).Block - if len(block.Evidence.Evidence) != 0 { - evidenceFromEachValidator[j] = block.Evidence.Evidence[0] + if len(block.Evidence) != 0 { + evidenceFromEachValidator[j] = block.Evidence[0] return } } diff --git a/internal/consensus/reactor_test.go b/internal/consensus/reactor_test.go index 3e1c930d9..e8c2df745 100644 --- a/internal/consensus/reactor_test.go +++ b/internal/consensus/reactor_test.go @@ -471,7 +471,7 @@ func TestReactorWithEvidence(t *testing.T) { } block := msg.Data().(types.EventDataNewBlock).Block - require.Len(t, block.Evidence.Evidence, 1) + require.Len(t, block.Evidence, 1) }(sub) } diff --git a/internal/consensus/state.go b/internal/consensus/state.go index 4772b23d5..d4a14e7ef 100644 --- a/internal/consensus/state.go +++ b/internal/consensus/state.go @@ -1960,7 +1960,7 @@ func (cs *State) RecordMetrics(height int64, block *types.Block) { byzantineValidatorsCount int64 ) - for _, ev := range block.Evidence.Evidence { + for _, ev := range block.Evidence { if dve, ok := ev.(*types.DuplicateVoteEvidence); ok { if _, val := cs.Validators.GetByAddress(dve.VoteA.ValidatorAddress); val != nil { byzantineValidatorsCount++ diff --git a/internal/evidence/pool_test.go b/internal/evidence/pool_test.go index 36431d49d..9f0c99435 100644 --- a/internal/evidence/pool_test.go +++ b/internal/evidence/pool_test.go @@ -241,7 +241,7 @@ func TestEvidencePoolUpdate(t *testing.T) { require.Equal(t, uint32(3), pool.Size()) - pool.Update(state, block.Evidence.Evidence) + pool.Update(state, block.Evidence) // a) Update marks evidence as committed so pending evidence should be empty evList, _ = pool.PendingEvidence(defaultEvidenceMaxBytes) diff --git a/internal/state/execution.go b/internal/state/execution.go index 5cd0c718b..161d3ec54 100644 --- a/internal/state/execution.go +++ b/internal/state/execution.go @@ -163,7 +163,7 @@ func (blockExec *BlockExecutor) ValidateBlock(state State, block *types.Block) e return err } - err = blockExec.evpool.CheckEvidence(block.Evidence.Evidence) + err = blockExec.evpool.CheckEvidence(block.Evidence) if err != nil { return err } @@ -233,7 +233,7 @@ func (blockExec *BlockExecutor) ApplyBlock( } // Update evpool with the latest state. - blockExec.evpool.Update(state, block.Evidence.Evidence) + blockExec.evpool.Update(state, block.Evidence) // Update the app hash and save the state. state.AppHash = appHash @@ -365,7 +365,7 @@ func execBlockOnProxyApp( commitInfo := getBeginBlockValidatorInfo(block, store, initialHeight) byzVals := make([]abci.Evidence, 0) - for _, evidence := range block.Evidence.Evidence { + for _, evidence := range block.Evidence { byzVals = append(byzVals, evidence.ABCI()...) } @@ -578,8 +578,8 @@ func fireEvents( logger.Error("failed publishing new block header", "err", err) } - if len(block.Evidence.Evidence) != 0 { - for _, ev := range block.Evidence.Evidence { + if len(block.Evidence) != 0 { + for _, ev := range block.Evidence { if err := eventBus.PublishEventNewEvidence(ctx, types.EventDataNewEvidence{ Evidence: ev, Height: block.Height, diff --git a/internal/state/execution_test.go b/internal/state/execution_test.go index 96b92d9bd..da8dc1fbc 100644 --- a/internal/state/execution_test.go +++ b/internal/state/execution_test.go @@ -225,7 +225,7 @@ func TestBeginBlockByzantineValidators(t *testing.T) { block, err := sf.MakeBlock(state, 1, new(types.Commit)) require.NoError(t, err) - block.Evidence = types.EvidenceData{Evidence: ev} + block.Evidence = ev block.Header.EvidenceHash = block.Evidence.Hash() bps, err := block.MakePartSet(testPartSize) require.NoError(t, err) diff --git a/node/node_test.go b/node/node_test.go index 514fab1e6..eafc5ebdd 100644 --- a/node/node_test.go +++ b/node/node_test.go @@ -313,7 +313,7 @@ func TestCreateProposalBlock(t *testing.T) { evList, size := evidencePool.PendingEvidence(state.ConsensusParams.Evidence.MaxBytes) require.Less(t, size, state.ConsensusParams.Evidence.MaxBytes+1) - evData := &types.EvidenceData{Evidence: evList} + evData := types.EvidenceList(evList) require.EqualValues(t, size, evData.ByteSize()) // fill the mempool with more txs diff --git a/test/e2e/tests/evidence_test.go b/test/e2e/tests/evidence_test.go index 9d0a1d255..a757c8550 100644 --- a/test/e2e/tests/evidence_test.go +++ b/test/e2e/tests/evidence_test.go @@ -17,8 +17,8 @@ func TestEvidence_Misbehavior(t *testing.T) { testnet := loadTestnet(t) seenEvidence := 0 for _, block := range blocks { - if len(block.Evidence.Evidence) != 0 { - seenEvidence += len(block.Evidence.Evidence) + if len(block.Evidence) != 0 { + seenEvidence += len(block.Evidence) } } require.Equal(t, testnet.Evidence, seenEvidence, diff --git a/types/block.go b/types/block.go index aef752847..a5e9d05d5 100644 --- a/types/block.go +++ b/types/block.go @@ -44,7 +44,7 @@ type Block struct { Header `json:"header"` Data `json:"data"` - Evidence EvidenceData `json:"evidence"` + Evidence EvidenceList `json:"evidence"` LastCommit *Commit `json:"last_commit"` } @@ -80,8 +80,8 @@ func (b *Block) ValidateBasic() error { return fmt.Errorf("wrong Header.DataHash. Expected %X, got %X", w, g) } - // NOTE: b.Evidence.Evidence may be nil, but we're just looping. - for i, ev := range b.Evidence.Evidence { + // NOTE: b.Evidence may be nil, but we're just looping. + for i, ev := range b.Evidence { if err := ev.ValidateBasic(); err != nil { return fmt.Errorf("invalid evidence (#%d): %v", i, err) } @@ -316,7 +316,7 @@ func MakeBlock(height int64, txs []Tx, lastCommit *Commit, evidence []Evidence) Data: Data{ Txs: txs, }, - Evidence: EvidenceData{Evidence: evidence}, + Evidence: evidence, LastCommit: lastCommit, } block.fillHeader() @@ -1083,97 +1083,6 @@ func DataFromProto(dp *tmproto.Data) (Data, error) { return *data, nil } -//----------------------------------------------------------------------------- - -// EvidenceData contains any evidence of malicious wrong-doing by validators -type EvidenceData struct { - Evidence EvidenceList `json:"evidence"` - - // Volatile. Used as cache - hash tmbytes.HexBytes - byteSize int64 -} - -// Hash returns the hash of the data. -func (data *EvidenceData) Hash() tmbytes.HexBytes { - if data.hash == nil { - data.hash = data.Evidence.Hash() - } - return data.hash -} - -// ByteSize returns the total byte size of all the evidence -func (data *EvidenceData) ByteSize() int64 { - if data.byteSize == 0 && len(data.Evidence) != 0 { - pb, err := data.ToProto() - if err != nil { - panic(err) - } - data.byteSize = int64(pb.Size()) - } - return data.byteSize -} - -// StringIndented returns a string representation of the evidence. -func (data *EvidenceData) StringIndented(indent string) string { - if data == nil { - return "nil-Evidence" - } - evStrings := make([]string, tmmath.MinInt(len(data.Evidence), 21)) - for i, ev := range data.Evidence { - if i == 20 { - evStrings[i] = fmt.Sprintf("... (%v total)", len(data.Evidence)) - break - } - evStrings[i] = fmt.Sprintf("Evidence:%v", ev) - } - return fmt.Sprintf(`EvidenceData{ -%s %v -%s}#%v`, - indent, strings.Join(evStrings, "\n"+indent+" "), - indent, data.hash) -} - -// ToProto converts EvidenceData to protobuf -func (data *EvidenceData) ToProto() (*tmproto.EvidenceList, error) { - if data == nil { - return nil, errors.New("nil evidence data") - } - - evi := new(tmproto.EvidenceList) - eviBzs := make([]tmproto.Evidence, len(data.Evidence)) - for i := range data.Evidence { - protoEvi, err := EvidenceToProto(data.Evidence[i]) - if err != nil { - return nil, err - } - eviBzs[i] = *protoEvi - } - evi.Evidence = eviBzs - - return evi, nil -} - -// FromProto sets a protobuf EvidenceData to the given pointer. -func (data *EvidenceData) FromProto(eviData *tmproto.EvidenceList) error { - if eviData == nil { - return errors.New("nil evidenceData") - } - - eviBzs := make(EvidenceList, len(eviData.Evidence)) - for i := range eviData.Evidence { - evi, err := EvidenceFromProto(&eviData.Evidence[i]) - if err != nil { - return err - } - eviBzs[i] = evi - } - data.Evidence = eviBzs - data.byteSize = int64(eviData.Size()) - - return nil -} - //-------------------------------------------------------------------------------- // BlockID diff --git a/types/block_test.go b/types/block_test.go index 54292463a..4ed47dd9d 100644 --- a/types/block_test.go +++ b/types/block_test.go @@ -52,7 +52,7 @@ func TestBlockAddEvidence(t *testing.T) { block := MakeBlock(h, txs, commit, evList) require.NotNil(t, block) - require.Equal(t, 1, len(block.Evidence.Evidence)) + require.Equal(t, 1, len(block.Evidence)) require.NotNil(t, block.EvidenceHash) } @@ -109,7 +109,7 @@ func TestBlockValidateBasic(t *testing.T) { }, true}, {"Invalid Evidence", func(blk *Block) { emptyEv := &DuplicateVoteEvidence{} - blk.Evidence = EvidenceData{Evidence: []Evidence{emptyEv}} + blk.Evidence = []Evidence{emptyEv} }, true}, } for i, tc := range testCases { @@ -700,7 +700,7 @@ func TestBlockProtoBuf(t *testing.T) { evidenceTime := time.Date(2019, 1, 1, 0, 0, 0, 0, time.UTC) evi, err := NewMockDuplicateVoteEvidence(ctx, h, evidenceTime, "block-test-chain") require.NoError(t, err) - b2.Evidence = EvidenceData{Evidence: EvidenceList{evi}} + b2.Evidence = EvidenceList{evi} b2.EvidenceHash = b2.Evidence.Hash() b3 := MakeBlock(h, []Tx{}, c1, []Evidence{}) @@ -729,7 +729,7 @@ func TestBlockProtoBuf(t *testing.T) { require.NoError(t, err, tc.msg) require.EqualValues(t, tc.b1.Header, block.Header, tc.msg) require.EqualValues(t, tc.b1.Data, block.Data, tc.msg) - require.EqualValues(t, tc.b1.Evidence.Evidence, block.Evidence.Evidence, tc.msg) + require.EqualValues(t, tc.b1.Evidence, block.Evidence, tc.msg) require.EqualValues(t, *tc.b1.LastCommit, *block.LastCommit, tc.msg) } else { require.Error(t, err, tc.msg) @@ -760,46 +760,6 @@ func TestDataProtoBuf(t *testing.T) { } } -// TestEvidenceDataProtoBuf ensures parity in converting to and from proto. -func TestEvidenceDataProtoBuf(t *testing.T) { - ctx, cancel := context.WithCancel(context.Background()) - defer cancel() - - const chainID = "mychain" - ev, err := NewMockDuplicateVoteEvidence(ctx, math.MaxInt64, time.Now(), chainID) - require.NoError(t, err) - data := &EvidenceData{Evidence: EvidenceList{ev}} - _ = data.ByteSize() - testCases := []struct { - msg string - data1 *EvidenceData - expPass1 bool - expPass2 bool - }{ - {"success", data, true, true}, - {"empty evidenceData", &EvidenceData{Evidence: EvidenceList{}}, true, true}, - {"fail nil Data", nil, false, false}, - } - - for _, tc := range testCases { - protoData, err := tc.data1.ToProto() - if tc.expPass1 { - require.NoError(t, err, tc.msg) - } else { - require.Error(t, err, tc.msg) - } - - eviD := new(EvidenceData) - err = eviD.FromProto(protoData) - if tc.expPass2 { - require.NoError(t, err, tc.msg) - require.Equal(t, tc.data1, eviD, tc.msg) - } else { - require.Error(t, err, tc.msg) - } - } -} - // exposed for testing func MakeRandHeader() Header { chainID := "test" diff --git a/types/evidence.go b/types/evidence.go index c9a60786a..264e0957b 100644 --- a/types/evidence.go +++ b/types/evidence.go @@ -15,6 +15,7 @@ import ( "github.com/tendermint/tendermint/crypto/merkle" "github.com/tendermint/tendermint/crypto/tmhash" "github.com/tendermint/tendermint/internal/jsontypes" + tmmath "github.com/tendermint/tendermint/libs/math" tmrand "github.com/tendermint/tendermint/libs/rand" tmproto "github.com/tendermint/tendermint/proto/tendermint/types" ) @@ -554,6 +555,73 @@ func LightClientAttackEvidenceFromProto(lpb *tmproto.LightClientAttackEvidence) // EvidenceList is a list of Evidence. Evidences is not a word. type EvidenceList []Evidence +// StringIndented returns a string representation of the evidence. +func (evl EvidenceList) StringIndented(indent string) string { + if evl == nil { + return "nil-Evidence" + } + evStrings := make([]string, tmmath.MinInt(len(evl), 21)) + for i, ev := range evl { + if i == 20 { + evStrings[i] = fmt.Sprintf("... (%v total)", len(evl)) + break + } + evStrings[i] = fmt.Sprintf("Evidence:%v", ev) + } + return fmt.Sprintf(`EvidenceList{ +%s %v +%s}#%v`, + indent, strings.Join(evStrings, "\n"+indent+" "), + indent, evl.Hash()) +} + +// ByteSize returns the total byte size of all the evidence +func (evl EvidenceList) ByteSize() int64 { + if len(evl) != 0 { + pb, err := evl.ToProto() + if err != nil { + panic(err) + } + return int64(pb.Size()) + } + return 0 +} + +// FromProto sets a protobuf EvidenceList to the given pointer. +func (evl *EvidenceList) FromProto(eviList *tmproto.EvidenceList) error { + if eviList == nil { + return errors.New("nil evidence list") + } + + eviBzs := make(EvidenceList, len(eviList.Evidence)) + for i := range eviList.Evidence { + evi, err := EvidenceFromProto(&eviList.Evidence[i]) + if err != nil { + return err + } + eviBzs[i] = evi + } + *evl = eviBzs + return nil +} + +// ToProto converts EvidenceList to protobuf +func (evl *EvidenceList) ToProto() (*tmproto.EvidenceList, error) { + if evl == nil { + return nil, errors.New("nil evidence list") + } + + eviBzs := make([]tmproto.Evidence, len(*evl)) + for i, v := range *evl { + protoEvi, err := EvidenceToProto(v) + if err != nil { + return nil, err + } + eviBzs[i] = *protoEvi + } + return &tmproto.EvidenceList{Evidence: eviBzs}, nil +} + func (evl EvidenceList) MarshalJSON() ([]byte, error) { lst := make([]json.RawMessage, len(evl)) for i, ev := range evl { diff --git a/types/evidence_test.go b/types/evidence_test.go index 440531aa9..dd3b54d08 100644 --- a/types/evidence_test.go +++ b/types/evidence_test.go @@ -33,6 +33,44 @@ func TestEvidenceList(t *testing.T) { assert.False(t, evl.Has(&DuplicateVoteEvidence{})) } +// TestEvidenceListProtoBuf to ensure parity in protobuf output and input +func TestEvidenceListProtoBuf(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + const chainID = "mychain" + ev, err := NewMockDuplicateVoteEvidence(ctx, math.MaxInt64, time.Now(), chainID) + require.NoError(t, err) + data := EvidenceList{ev} + testCases := []struct { + msg string + data1 *EvidenceList + expPass1 bool + expPass2 bool + }{ + {"success", &data, true, true}, + {"empty evidenceData", &EvidenceList{}, true, true}, + {"fail nil Data", nil, false, false}, + } + + for _, tc := range testCases { + protoData, err := tc.data1.ToProto() + if tc.expPass1 { + require.NoError(t, err, tc.msg) + } else { + require.Error(t, err, tc.msg) + } + + eviD := new(EvidenceList) + err = eviD.FromProto(protoData) + if tc.expPass2 { + require.NoError(t, err, tc.msg) + require.Equal(t, tc.data1, eviD, tc.msg) + } else { + require.Error(t, err, tc.msg) + } + } +} func randomDuplicateVoteEvidence(ctx context.Context, t *testing.T) *DuplicateVoteEvidence { t.Helper() val := NewMockPV()