From 4f79930c127c7d6f0ad62f977afaba163f7a0ca7 Mon Sep 17 00:00:00 2001 From: Callum Waters Date: Mon, 28 Sep 2020 17:02:46 +0200 Subject: [PATCH] blockchain: remove duplication of validate basic (#5418) --- blockchain/msgs.go | 6 ++---- blockchain/v0/reactor.go | 1 + blockchain/v1/reactor.go | 1 + blockchain/v2/reactor.go | 1 + blockchain/v2/scheduler_test.go | 19 ++++++++++++------- types/validator_set.go | 11 ++++++++++- 6 files changed, 27 insertions(+), 12 deletions(-) diff --git a/blockchain/msgs.go b/blockchain/msgs.go index 35868830b..cd5ef977f 100644 --- a/blockchain/msgs.go +++ b/blockchain/msgs.go @@ -83,10 +83,8 @@ func ValidateMsg(pb proto.Message) error { return errors.New("negative Height") } case *bcproto.BlockResponse: - _, err := types.BlockFromProto(msg.Block) - if err != nil { - return err - } + // validate basic is called later when converting from proto + return nil case *bcproto.NoBlockResponse: if msg.Height < 0 { return errors.New("negative Height") diff --git a/blockchain/v0/reactor.go b/blockchain/v0/reactor.go index 093e08241..9ae6b1798 100644 --- a/blockchain/v0/reactor.go +++ b/blockchain/v0/reactor.go @@ -227,6 +227,7 @@ func (bcR *BlockchainReactor) Receive(chID byte, src p2p.Peer, msgBytes []byte) bi, err := types.BlockFromProto(msg.Block) if err != nil { bcR.Logger.Error("Block content is invalid", "err", err) + bcR.Switch.StopPeerForError(src, err) return } bcR.pool.AddBlock(src.ID(), bi, len(msgBytes)) diff --git a/blockchain/v1/reactor.go b/blockchain/v1/reactor.go index 30bd4dd8a..b43be3d3a 100644 --- a/blockchain/v1/reactor.go +++ b/blockchain/v1/reactor.go @@ -285,6 +285,7 @@ func (bcR *BlockchainReactor) Receive(chID byte, src p2p.Peer, msgBytes []byte) bi, err := types.BlockFromProto(msg.Block) if err != nil { bcR.Logger.Error("error transition block from protobuf", "err", err) + _ = bcR.swReporter.Report(behaviour.BadMessage(src.ID(), err.Error())) return } msgForFSM := bcReactorMessage{ diff --git a/blockchain/v2/reactor.go b/blockchain/v2/reactor.go index 6aee5078e..b9d8bab27 100644 --- a/blockchain/v2/reactor.go +++ b/blockchain/v2/reactor.go @@ -469,6 +469,7 @@ func (r *BlockchainReactor) Receive(chID byte, src p2p.Peer, msgBytes []byte) { bi, err := types.BlockFromProto(msg.Block) if err != nil { r.logger.Error("error transitioning block from protobuf", "err", err) + _ = r.reporter.Report(behaviour.BadMessage(src.ID(), err.Error())) return } if r.events != nil { diff --git a/blockchain/v2/scheduler_test.go b/blockchain/v2/scheduler_test.go index a4636d954..a2b9c5963 100644 --- a/blockchain/v2/scheduler_test.go +++ b/blockchain/v2/scheduler_test.go @@ -1377,6 +1377,9 @@ func checkScResults(t *testing.T, wantErr bool, err error, wantEvent Event, even t.Errorf("error = %v, wantErr %v", err, wantErr) return } + if !assert.IsType(t, wantEvent, event) { + t.Log(fmt.Sprintf("Wrong type received, got: %v", event)) + } switch wantEvent := wantEvent.(type) { case scPeerError: assert.Equal(t, wantEvent.peerID, event.(scPeerError).peerID) @@ -1460,7 +1463,7 @@ func TestScHandleBlockResponse(t *testing.T) { pendingTime: map[int64]time.Time{6: now}, }, args: args{event: block6FromP1}, - wantEvent: scBlockReceived{peerID: "P1", block: makeScBlock(6)}, + wantEvent: scBlockReceived{peerID: "P1", block: block6FromP1.block}, }, } @@ -2047,6 +2050,8 @@ func TestScHandle(t *testing.T) { priorityNormal } + block1, block2, block3 := makeScBlock(1), makeScBlock(2), makeScBlock(3) + t0 := time.Now() tick := make([]time.Time, 100) for i := range tick { @@ -2135,8 +2140,8 @@ func TestScHandle(t *testing.T) { }, }, { // block response 1 - args: args{event: bcBlockResponse{peerID: "P1", time: tick[4], size: 100, block: makeScBlock(1)}}, - wantEvent: scBlockReceived{peerID: "P1", block: makeScBlock(1)}, + args: args{event: bcBlockResponse{peerID: "P1", time: tick[4], size: 100, block: block1}}, + wantEvent: scBlockReceived{peerID: "P1", block: block1}, wantSc: &scTestParams{ startTime: now, peers: map[string]*scPeer{"P1": {height: 3, state: peerStateReady, lastTouched: tick[4]}}, @@ -2148,8 +2153,8 @@ func TestScHandle(t *testing.T) { }, }, { // block response 2 - args: args{event: bcBlockResponse{peerID: "P1", time: tick[5], size: 100, block: makeScBlock(2)}}, - wantEvent: scBlockReceived{peerID: "P1", block: makeScBlock(2)}, + args: args{event: bcBlockResponse{peerID: "P1", time: tick[5], size: 100, block: block2}}, + wantEvent: scBlockReceived{peerID: "P1", block: block2}, wantSc: &scTestParams{ startTime: now, peers: map[string]*scPeer{"P1": {height: 3, state: peerStateReady, lastTouched: tick[5]}}, @@ -2161,8 +2166,8 @@ func TestScHandle(t *testing.T) { }, }, { // block response 3 - args: args{event: bcBlockResponse{peerID: "P1", time: tick[6], size: 100, block: makeScBlock(3)}}, - wantEvent: scBlockReceived{peerID: "P1", block: makeScBlock(3)}, + args: args{event: bcBlockResponse{peerID: "P1", time: tick[6], size: 100, block: block3}}, + wantEvent: scBlockReceived{peerID: "P1", block: block3}, wantSc: &scTestParams{ startTime: now, peers: map[string]*scPeer{"P1": {height: 3, state: peerStateReady, lastTouched: tick[6]}}, diff --git a/types/validator_set.go b/types/validator_set.go index cdd748476..744d11b73 100644 --- a/types/validator_set.go +++ b/types/validator_set.go @@ -661,6 +661,9 @@ func (vals *ValidatorSet) UpdateWithChangeSet(changes []*Validator) error { // with a bonus for including more than +2/3 of the signatures. func (vals *ValidatorSet) VerifyCommit(chainID string, blockID BlockID, height int64, commit *Commit) error { + if commit == nil { + return errors.New("nil commit") + } if vals.Size() != len(commit.Signatures) { return NewErrInvalidCommitSignatures(vals.Size(), len(commit.Signatures)) @@ -718,6 +721,9 @@ func (vals *ValidatorSet) VerifyCommit(chainID string, blockID BlockID, // signatures. func (vals *ValidatorSet) VerifyCommitLight(chainID string, blockID BlockID, height int64, commit *Commit) error { + if commit == nil { + return errors.New("nil commit") + } if vals.Size() != len(commit.Signatures) { return NewErrInvalidCommitSignatures(vals.Size(), len(commit.Signatures)) @@ -770,10 +776,13 @@ func (vals *ValidatorSet) VerifyCommitLight(chainID string, blockID BlockID, // This method is primarily used by the light client and does not check all the // signatures. func (vals *ValidatorSet) VerifyCommitLightTrusting(chainID string, commit *Commit, trustLevel tmmath.Fraction) error { - // sanity check + // sanity checks if trustLevel.Denominator == 0 { return errors.New("trustLevel has zero Denominator") } + if commit == nil { + return errors.New("nil commit") + } var ( talliedVotingPower int64