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.

Sign up for GitHub

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

Jump to bottom

Enforce Ordering in DuplicateVoteEvidence #4151

Merged
merged 6 commits into from
Nov 19, 2019
Merged

Enforce Ordering in DuplicateVoteEvidence #4151

merged 6 commits into from
Nov 19, 2019

Conversation

AdityaSripal
Copy link
Contributor

@AdityaSripal AdityaSripal commented Nov 16, 2019 •
edited
Loading

Fixes issue described in: #4143

  • Referenced an issue explaining the need for the change
  • Updated all relevant documentation in docs
  • Updated all code comments where relevant
  • Wrote tests
  • Updated CHANGELOG_PENDING.md

melekes
melekes reviewed Nov 16, 2019
View reviewed changes
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.

💪 🏋 🥇

types/evidence.go Outdated Show resolved Hide resolved
@melekes
Copy link
Contributor

melekes commented Nov 16, 2019

This also needs a CHANGELOG_PENDING.md entry.

@AdityaSripal
Copy link
Contributor Author

@melekes Would this be considered a breaking change? I don't believe it is, since the submission of evidence is taken care of completely inside tendermint

@melekes
Copy link
Contributor

melekes commented Nov 16, 2019

@melekes Would this be considered a breaking change? I don't believe it is, since the submission of evidence is taken care of completely inside tendermint

no, just a bugfix

@codecov-io
Copy link

codecov-io commented Nov 16, 2019 •
edited
Loading

Codecov Report

Merging #4151 into master will increase coverage by 0.02%.
The diff coverage is 88.88%.

@@            Coverage Diff             @@
##           master    #4151      +/-   ##
==========================================
+ Coverage   66.66%   66.69%   +0.02%     
==========================================
  Files         247      247              
  Lines       21221    21233      +12     
==========================================
+ Hits        14148    14161      +13     
+ Misses       6014     6012       -2     
- Partials     1059     1060       +1
Impacted Files Coverage Δ
types/vote.go 88.4% <100%> (-0.64%) ⬇️
types/evidence.go 60.58% <87.5%> (+3.55%) ⬆️
blockchain/v2/reactor.go 50.87% <0%> (-7.02%) ⬇️
consensus/reactor.go 76.99% <0%> (-1.28%) ⬇️
blockchain/v0/reactor.go 78.77% <0%> (-0.95%) ⬇️
consensus/state.go 77.74% <0%> (-0.22%) ⬇️
p2p/pex/addrbook.go 68.34% <0%> (+0.5%) ⬆️
p2p/pex/pex_reactor.go 85.01% <0%> (+1.72%) ⬆️
privval/signer_listener_endpoint.go 89.13% <0%> (+2.17%) ⬆️
consensus/ticker.go 95.83% <0%> (+4.16%) ⬆️
... and 3 more

melekes
melekes reviewed Nov 19, 2019
View reviewed changes
types/evidence.go
@@ -209,6 +231,10 @@ func (dve *DuplicateVoteEvidence) ValidateBasic() error {
if err := dve.VoteB.ValidateBasic(); err != nil {
return fmt.Errorf("Invalid VoteB: %v", err)
}
// Enforce Votes are lexicographically sorted on blockID
if strings.Compare(dve.VoteA.BlockID.Key(), dve.VoteB.BlockID.Key()) >= 0 {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is actually breaking since old evidence may not be in correct order.

@melekes melekes added the T:breaking Type: Breaking Change label Nov 19, 2019
@melekes melekes merged commit 8a878c1 into master Nov 19, 2019
@melekes melekes deleted the aditya/fix-swap-evidence branch November 19, 2019 06:54
@ebuchman
Copy link
Contributor

We should update the spec for this here: https://github.com/tendermint/tendermint/blob/master/docs/spec/blockchain/blockchain.md#evidence-1

@melekes
Copy link
Contributor

melekes commented Nov 20, 2019

@ebuchman thanks for pointing this out!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@melekes melekes melekes approved these changes

@ebuchman ebuchman Awaiting requested review from ebuchman

@tessr tessr Awaiting requested review from tessr

Assignees
No one assigned
Labels
T:breaking Type: Breaking Change
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@AdityaSripal @melekes @codecov-io @ebuchman