validate reactor messages by melekes · Pull Request #2711 · tendermint/tendermint

melekes · 2018-10-26T14:20:50Z

~~Updated all relevant documentation in docs~~ no need(?)
Updated all code comments where relevant
Wrote tests
Updated CHANGELOG_PENDING.md

TODO

validate netAddr in pex reactor
check signatures size

codecov-io · 2018-10-26T16:55:22Z

Codecov Report

Merging #2711 into develop will decrease coverage by 0.28%.
The diff coverage is 31.37%.

@@             Coverage Diff             @@
##           develop    #2711      +/-   ##
===========================================
- Coverage    62.52%   62.23%   -0.29%     
===========================================
  Files          212      212              
  Lines        17074    17185     +111     
===========================================
+ Hits         10675    10695      +20     
- Misses        5532     5592      +60     
- Partials       867      898      +31

Impacted Files	Coverage Δ
crypto/crypto.go	`0% <ø> (ø)`	⬆️
config/toml.go	`53.19% <ø> (ø)`	⬆️
p2p/pex/errors.go	`0% <0%> (ø)`	⬆️
p2p/pex/addrbook.go	`69.24% <0%> (-0.34%)`	⬇️
consensus/replay.go	`56.33% <100%> (ø)`	⬆️
consensus/reactor.go	`66.66% <26.19%> (-4.27%)`	⬇️
evidence/reactor.go	`63% <33.33%> (-2.94%)`	⬇️
p2p/pex/pex_reactor.go	`73.05% <50%> (-1.29%)`	⬇️
state/validation.go	`65.81% <73.68%> (+0.18%)`	⬆️
blockchain/reactor.go	`35.78% <8.33%> (-3.67%)`	⬇️
... and 3 more

types/heartbeat.go

melekes · 2018-10-26T19:33:02Z

Also we should try and offload some tasks (like checking negative integers or rather using uint's) to compiler :)

blockchain/reactor.go

ebuchman

Woohooo!

blockchain/reactor.go

consensus/reactor.go

types/part_set.go

ebuchman · 2018-10-29T15:02:49Z

types/proposal.go

+	}
+	if p.Round < 0 {
+		return errors.New("Negative Round")
+	}


how should validate it? compare it to the local time with some delta?

Yeh, maybe plus or minus a year? Just to eliminate the crazy outliers. The decoder will have done alot of work for us here already actually. We can punt on it for now.

ebuchman · 2018-10-29T15:03:33Z

types/signable.go

+const (
+	// MaxSignatureSize is a maximum allowed signature size for the Heartbeat,
+	// Proposal and Vote.
+	MaxSignatureSize = 64


I think we need this to be the math.Max of values read from the crypto package incase we introduce bigger signatures

types/vote.go

ebuchman · 2018-10-29T15:04:24Z

types/vote.go

+	}
+	if err := vote.BlockID.ValidateBasic(); err != nil {
+		return fmt.Errorf("Wrong BlockID: %v", err)
+	}


timestamp and Type

types/evidence.go

ebuchman

Great work, thanks @melekes !

consensus/reactor.go

types/evidence.go

ebuchman · 2018-10-30T13:55:11Z

types/proposal.go

+	}
+	if p.Round < 0 {
+		return errors.New("Negative Round")
+	}


Yeh, maybe plus or minus a year? Just to eliminate the crazy outliers. The decoder will have done alot of work for us here already actually. We can punt on it for now.

ebuchman · 2018-10-30T15:45:38Z

types/proposal.go

 		return errors.New("Negative Round")
 	}
+	now := tmtime.Now()
+	oneYear := 8766 * time.Hour


let's make a ValidateTime(timestamp time.Time) error function for this?

hendrikhofstadt · 2018-10-30T15:57:19Z

types/block.go

 		)
 	}
+
 	return nil


We should also do ValidateBasic on the evidence here since otherwise the votes in the DuplicateVoteEvidence could still be nil when the block is further processed.

Suggested change

return nil

for _, ev := range b.Evidence.Evidence {

if err := ev.ValidateBasic(); err != nil {

return err

}

}

return nil

@ebuchman @melekes This would be a quick fix. However b.Evidence is of the type EvidenceData which should also have a ValidateBasic function. That would also have to check whether EvidenceData.Evidence != nil since a nil slice could be passed which could cause another panic in a hypothetical scenario.

b.Evidence can't be nil since it's not a pointer. b.Evidence.Evidence could be nil because it's a slice, but here we just loop over it, so if its nil its no problem, the loop just won't run. The methods defined on EvidenceList similarly only loop over it, so if it's nil there should be no issue. Am I missing something? Or you're saying we should always ensure we have a zero-length array here and not nil ?

Refs #2683

Fix ``` grouped write of manifest, lock and vendor: failed to export github.com/tendermint/go-amino: fatal: failed to unpack tree object 6dcc6ddc143e116455c94b25c1004c99e0d0ca12 ``` by running `dep ensure -update`

ebuchman · 2018-10-31T13:04:29Z

Why were the test files deleted? p2p tests fail:

Successfully initialized 4 node directories
ERROR: open /go/src/github.com/tendermint/tendermint/test/p2p/data/mach0/config/node_key.json: no such file or directory
Exited with code 1

melekes · 2018-10-31T13:52:27Z

Why were the test files deleted? p2p tests fail:

we now use tendermint testnet cmd to generate them (that way we won't need to update them every time we change genesis/node_key/priv_validator files). Tests pass for me locally. I am not sure why they're failing on circle.

Refs #2737

test/p2p/peer.sh

melekes · 2018-10-31T18:07:35Z

Done here.

some ValidateBasic updates

* 'master' of https://github.com/tendermint/tendermint: (330 commits) Release/v0.26.1 (tendermint#2803) fix amino overhead computation for Tx (tendermint#2792) p2p: re-check after sleeps (tendermint#2664) check the result of `ps.peer.Send` before calling `ps.setHasVote` (tendermint#2787) p2p: AddressBook requires addresses to have IDs; Do not close conn immediately after sending pex addrs in seed mode (tendermint#2797) test AutoFile#Size (happy path) [autofile/group] do not panic when checking size openFile creates a file if not exist => ErrNotExist is not possible use our logger in autofile/group Add tests for ValidateBasic methods (tendermint#2754) [docs] improve organization of ABCI docs & fix links (tendermint#2749) p2p: peer-id -> peer_id (tendermint#2771) mempool: print postCheck error (tendermint#2762) Fix crypto/merkle ProofOperators.Verify to check bounds on keypath pa… (tendermint#2756) Mempool WAL is still created by default in home directory, leads to permission errors (tendermint#2758) mempool: ErrPreCheck and more log info (tendermint#2724) Release/v0.26.0 (tendermint#2726) [ADR] [DRAFT] pubsub 2.0 (tendermint#2532) validate reactor messages (tendermint#2711) TMHASH is 32 bytes. Closes tendermint#1990 (tendermint#2732) ...

melekes requested review from ebuchman and xla as code owners October 26, 2018 14:20

melekes force-pushed the 2683-validate-reactor-messages branch from 09591ab to a12664e Compare October 26, 2018 16:47

melekes force-pushed the 2683-validate-reactor-messages branch from a12664e to ce9b642 Compare October 26, 2018 17:46

melekes commented Oct 26, 2018

View reviewed changes

types/heartbeat.go Outdated Show resolved Hide resolved

melekes commented Oct 29, 2018

View reviewed changes

blockchain/reactor.go Show resolved Hide resolved

melekes changed the title ~~[WIP] validate reactor messages~~ validate reactor messages Oct 29, 2018

ebuchman mentioned this pull request Oct 29, 2018

Add a checked integer type #2692

Closed

ebuchman reviewed Oct 29, 2018

View reviewed changes

types/evidence.go Show resolved Hide resolved

melekes force-pushed the 2683-validate-reactor-messages branch from c8cf780 to 45a71b6 Compare October 30, 2018 13:13

ebuchman approved these changes Oct 30, 2018

View reviewed changes

tendermint deleted a comment from ebuchman Oct 30, 2018

ebuchman reviewed Oct 30, 2018

View reviewed changes

hendrikhofstadt suggested changes Oct 30, 2018

View reviewed changes

melekes force-pushed the 2683-validate-reactor-messages branch from b306b55 to 7f919df Compare October 30, 2018 18:03

melekes added 12 commits October 31, 2018 08:54

validate reactor messages

e7ca52b

Refs #2683

validate blockchain messages

69ea74e

Refs #2683

validate evidence messages

4572b92

Refs #2683

todo

db699f6

check ProposalPOL and signature sizes

53a4660

add a changelog entry

fc5f868

check addr is valid when we add it to the addrbook

57163e9

validate incoming netAddr (not just nil check!)

8bf215f

fixes after Bucky's review

866af23

check timestamps

8f8aae5

beef up block#ValidateBasic

daa6bb9

move some checks into bcBlockResponseMessage

e6aaed4

melekes force-pushed the 2683-validate-reactor-messages branch from 916216e to e6aaed4 Compare October 31, 2018 08:49

melekes added 4 commits October 31, 2018 10:13

update Gopkg.lock

3cc3c8e

Fix ``` grouped write of manifest, lock and vendor: failed to export github.com/tendermint/go-amino: fatal: failed to unpack tree object 6dcc6ddc143e116455c94b25c1004c99e0d0ca12 ``` by running `dep ensure -update`

bump year since now we check it

22e7333

generate test/p2p/data on the fly using tendermint testnet

ceabe99

allow sync chains older than 1 year

e46b35b

melekes added 2 commits October 31, 2018 15:06

use full path when creating a testnet

015a33d

move testnet gen to test/docker/Dockerfile

9baaa4b

melekes mentioned this pull request Oct 31, 2018

LastCommitRound can be -1 for heights > 1 #2737

Closed

melekes and others added 3 commits October 31, 2018 16:44

relax LastCommitRound check

24f4e1e

Refs #2737

Merge branch 'develop' into 2683-validate-reactor-messages

8aa18f8

fix conflicts after merge

c4817c7

melekes commented Oct 31, 2018

View reviewed changes

test/p2p/peer.sh Show resolved Hide resolved

add small comment

43f3d14

ebuchman mentioned this pull request Oct 31, 2018

some ValidateBasic updates #2738

Merged

ebuchman added 4 commits October 31, 2018 18:01

some ValidateBasic updates

0a3d52e

fixes

1c5088e

Merge pull request #2738 from tendermint/bucky/validate-basic

46388ae

some ValidateBasic updates

AppHash length is not fixed

20149a4

ebuchman merged commit fb91ef7 into develop Nov 1, 2018

ebuchman deleted the 2683-validate-reactor-messages branch November 1, 2018 06:07

This was referenced Nov 1, 2018

Write more tests for ValidateBasic methods #2740

Closed

Validate all reactor messages #2683

Closed

liamsi mentioned this pull request Nov 22, 2018

Validate message types tendermint/tmkms#81

Closed

Conversation

melekes commented Oct 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-io commented Oct 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

melekes commented Oct 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

ebuchman left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ebuchman left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hendrikhofstadt Oct 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hendrikhofstadt Oct 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hendrikhofstadt Oct 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ebuchman commented Oct 31, 2018

Uh oh!

melekes commented Oct 31, 2018

Uh oh!

Uh oh!

melekes commented Oct 31, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

melekes commented Oct 26, 2018 •

edited

Loading

codecov-io commented Oct 26, 2018 •

edited

Loading

melekes commented Oct 26, 2018 •

edited

Loading

hendrikhofstadt Oct 30, 2018 •

edited

Loading

hendrikhofstadt Oct 30, 2018 •

edited

Loading

hendrikhofstadt Oct 30, 2018 •

edited

Loading