New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
validate reactor messages #2711
Conversation
09591ab
to
a12664e
Compare
Codecov Report
@@ 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
|
a12664e
to
ce9b642
Compare
Also we should try and offload some tasks (like checking negative integers or rather using uint's) to compiler :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Woohooo!
} | ||
if p.Round < 0 { | ||
return errors.New("Negative Round") | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
timestamp?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how should validate it? compare it to the local time with some delta?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
types/signable.go
Outdated
const ( | ||
// MaxSignatureSize is a maximum allowed signature size for the Heartbeat, | ||
// Proposal and Vote. | ||
MaxSignatureSize = 64 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we need this to be the math.Max
of values read from the crypto package incase we introduce bigger signatures
} | ||
if err := vote.BlockID.ValidateBasic(); err != nil { | ||
return fmt.Errorf("Wrong BlockID: %v", err) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
timestamp and Type
c8cf780
to
45a71b6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work, thanks @melekes !
} | ||
if p.Round < 0 { | ||
return errors.New("Negative Round") | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
types/proposal.go
Outdated
@@ -50,6 +50,11 @@ func (p *Proposal) ValidateBasic() error { | |||
if p.Round < 0 { | |||
return errors.New("Negative Round") | |||
} | |||
now := tmtime.Now() | |||
oneYear := 8766 * time.Hour |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's make a ValidateTime(timestamp time.Time) error
function for this?
if !bytes.Equal(b.EvidenceHash, b.Evidence.Hash()) { | ||
return fmt.Errorf( | ||
"Wrong Block.Header.EvidenceHash. Expected %v, got %v", | ||
b.EvidenceHash, | ||
b.Evidence.Hash(), | ||
) | ||
} | ||
|
||
return nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return nil | |
for _, ev := range b.Evidence.Evidence { | |
if err := ev.ValidateBasic(); err != nil { | |
return err | |
} | |
} | |
return nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 ?
b306b55
to
7f919df
Compare
916216e
to
e6aaed4
Compare
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`
Why were the test files deleted? p2p tests fail:
|
we now use |
Done here. |
* '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) ...
Refs #2683
Updated all relevant documentation in docsno need(?)TODO