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: fix bug with hashes #6375

Merged
merged 16 commits into from
Apr 21, 2021
Merged

Conversation

cmwaters
Copy link
Contributor

@cmwaters cmwaters commented Apr 20, 2021

This is a non-breaking solution to #6342 where the hashes remain the same.

Specifically, I have removed fastCheck and modified the CheckEvidence function so that the bytes of the evidence are not modified.

I have also added some more tests on top of this

Closes: #6326
Closes: #6312
Addresses: #5942

@cmwaters cmwaters changed the title evidences: fix bug with hashes evidence: fix bug with hashes Apr 20, 2021
Copy link
Contributor Author

@cmwaters cmwaters left a comment

Choose a reason for hiding this comment

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

@marbar3778 can you confirm that this makes sense :)

Comment on lines +1031 to +1033
// NOTE: Sometimes we use the bytes of the proto form as a hash. This means that we need to
// be consistent with cached data
vp.TotalVotingPower = 0
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can you confirm that this doesn't break proto

types/validator_set.go Outdated Show resolved Hide resolved
@cmwaters cmwaters added the S:backport-to-v0.34.x Tell mergify to backport the PR to v0.34.x label Apr 21, 2021
@codecov
Copy link

codecov bot commented Apr 21, 2021

Codecov Report

Merging #6375 (2e93858) into master (d36a590) will increase coverage by 0.01%.
The diff coverage is 54.34%.

@@            Coverage Diff             @@
##           master    #6375      +/-   ##
==========================================
+ Coverage   60.78%   60.80%   +0.01%     
==========================================
  Files         282      282              
  Lines       26910    26868      -42     
==========================================
- Hits        16358    16336      -22     
+ Misses       8839     8824      -15     
+ Partials     1713     1708       -5     
Impacted Files Coverage Δ
p2p/shim.go 55.63% <0.00%> (ø)
evidence/verify.go 70.80% <28.00%> (+5.86%) ⬆️
evidence/pool.go 64.55% <60.00%> (+3.24%) ⬆️
types/evidence.go 56.57% <100.00%> (+2.25%) ⬆️
types/light.go 72.52% <100.00%> (ø)
types/validator_set.go 83.17% <100.00%> (ø)
privval/signer_server.go 89.47% <0.00%> (-5.27%) ⬇️
types/tx.go 82.97% <0.00%> (-4.26%) ⬇️
consensus/reactor.go 66.56% <0.00%> (-3.57%) ⬇️
consensus/peer_state.go 77.63% <0.00%> (-3.51%) ⬇️
... and 16 more

@cmwaters cmwaters merged commit 5bafedf into master Apr 21, 2021
@cmwaters cmwaters deleted the callum/evidence-hashes-non-breaking branch April 21, 2021 16:50
mergify bot pushed a commit that referenced this pull request Apr 21, 2021
(cherry picked from commit 5bafedf)

# Conflicts:
#	CHANGELOG_PENDING.md
#	evidence/pool.go
#	evidence/pool_test.go
#	evidence/verify.go
#	evidence/verify_test.go
#	p2p/shim.go
#	test/e2e/networks/ci.toml
#	test/e2e/pkg/testnet.go
#	test/e2e/runner/evidence.go
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S:backport-to-v0.34.x Tell mergify to backport the PR to v0.34.x
Projects
None yet
Development

Successfully merging this pull request may close these issues.

e2e: evidence data generates two different hashes e2e: same evidence verified twice
3 participants