Skip to content
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

evidence: check evidence hasn't already been stored #4632

Merged
merged 2 commits into from
Apr 7, 2020

Conversation

cmwaters
Copy link
Contributor

@cmwaters cmwaters commented Apr 2, 2020

Closes: #4614

Description

Uses the Has() function from the DB store interface to first confirm that a piece of received evidence is not already stored before commencing verification


For contributor use:

  • Wrote tests
  • Updated CHANGELOG_PENDING.md
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
  • Updated relevant documentation (docs/) and code comments
  • Re-reviewed Files changed in the Github PR explorer

@cmwaters cmwaters self-assigned this Apr 2, 2020
@cmwaters cmwaters added the C:evidence Component: Evidence label Apr 2, 2020
Copy link
Contributor

@melekes melekes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cool!

don't forget to add CHANGELOG_PENDING.md entry and maybe write a test

evidence/reactor.go Outdated Show resolved Hide resolved
evidence/reactor.go Outdated Show resolved Hide resolved
"github.com/tendermint/tendermint/types"
)

type ErrInvalidEvidence struct {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why expose?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, perhaps I need a better understanding of when errors should be exposed or not because almost all the errors in all other packages are exposed, so I have just been copying them

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think in general it's just good practice. Unless, however, you know with 100% certainty, the error will never be used/needed by the outside (package) world.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we'll be creating a typed error every time we return an error in public function, we will end up with pile of extra code, which is not needed in reality

evidence/errors.go Outdated Show resolved Hide resolved
evidence/errors.go Outdated Show resolved Hide resolved
evidence/pool.go Outdated Show resolved Hide resolved
evidence/pool.go Outdated Show resolved Hide resolved
@cmwaters
Copy link
Contributor Author

cmwaters commented Apr 3, 2020

Previously the evidence reactor would StopPeerForError when any error was returned from AddEvidence. Now, I've changed it such that only when the validation fails that it disconnects the peer. It doesn't seem necessary to disconnect because it has already received the evidence. Is this okay? Do you think we should further punish the peer by banning them?

Also is it okay that AddNewEvidence ignores any error that the DB might return?

@cmwaters cmwaters requested a review from melekes April 3, 2020 14:17
evidence/errors.go Outdated Show resolved Hide resolved
evidence/pool.go Show resolved Hide resolved
Copy link
Contributor

@alexanderbez alexanderbez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

"github.com/tendermint/tendermint/types"
)

type ErrInvalidEvidence struct {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think in general it's just good practice. Unless, however, you know with 100% certainty, the error will never be used/needed by the outside (package) world.

evidence/store.go Outdated Show resolved Hide resolved
@codecov-io
Copy link

codecov-io commented Apr 7, 2020

Codecov Report

Merging #4632 into master will increase coverage by 0.00%.
The diff coverage is 46.15%.

@@           Coverage Diff           @@
##           master    #4632   +/-   ##
=======================================
  Coverage   66.00%   66.01%           
=======================================
  Files         220      221    +1     
  Lines       19766    19800   +34     
=======================================
+ Hits        13047    13070   +23     
- Misses       5676     5686   +10     
- Partials     1043     1044    +1     
Impacted Files Coverage Δ
evidence/errors.go 0.00% <0.00%> (ø)
rpc/core/evidence.go 0.00% <0.00%> (ø)
evidence/reactor.go 61.01% <40.00%> (-0.81%) ⬇️
evidence/store.go 82.60% <62.50%> (-5.63%) ⬇️
evidence/pool.go 76.00% <66.66%> (-2.09%) ⬇️
blockchain/v0/reactor.go 73.91% <0.00%> (-1.74%) ⬇️
blockchain/v0/pool.go 78.98% <0.00%> (+0.31%) ⬆️
blockchain/v1/peer.go 95.91% <0.00%> (+0.62%) ⬆️
consensus/replay.go 73.64% <0.00%> (+0.77%) ⬆️
... and 3 more

evidence/errors.go Show resolved Hide resolved
evidence/errors.go Show resolved Hide resolved
evidence/reactor.go Show resolved Hide resolved
evidence/reactor.go Outdated Show resolved Hide resolved
type ErrEvidenceAlreadyStored struct{}

func (e ErrEvidenceAlreadyStored) Error() string {
return fmt.Sprint("evidence is already stored")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
return fmt.Sprint("evidence is already stored")
return "evidence is already stored"

Add Has function, create better handling of errors when adding evidence, usage of error types.
@cmwaters cmwaters force-pushed the callum/check-inbound-evidence branch from 3f517ae to c69b1f7 Compare April 7, 2020 08:31
@cmwaters cmwaters added the S:automerge Automatically merge PR when requirements pass label Apr 7, 2020
@mergify mergify bot merged commit 88d7007 into master Apr 7, 2020
@mergify mergify bot deleted the callum/check-inbound-evidence branch April 7, 2020 08:42
@melekes
Copy link
Contributor

melekes commented Apr 14, 2020

Previously the evidence reactor would StopPeerForError when any error was returned from AddEvidence. Now, I've changed it such that only when the validation fails that it disconnects the peer. It doesn't seem necessary to disconnect because it has already received the evidence. Is this okay? Do you think we should further punish the peer by banning them?

It's not necessary to disconnect, agree. we should ban peers who send us invalid evidence.

Also is it okay that AddNewEvidence ignores any error that the DB might return?

probably not

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C:evidence Component: Evidence S:automerge Automatically merge PR when requirements pass
Projects
None yet
Development

Successfully merging this pull request may close these issues.

evidence: create a cache in pool to prevent DoS attacks
4 participants