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: introduction of LightClientAttackEvidence and refactor of evidence lifecycle #5361

Merged
merged 13 commits into from
Sep 22, 2020

Conversation

cmwaters
Copy link
Contributor

Description

For more description see ADR-059

@codecov
Copy link

codecov bot commented Sep 15, 2020

Codecov Report

Merging #5361 into master will increase coverage by 0.56%.
The diff coverage is 73.68%.

@@            Coverage Diff             @@
##           master    #5361      +/-   ##
==========================================
+ Coverage   60.90%   61.46%   +0.56%     
==========================================
  Files         258      259       +1     
  Lines       23080    23346     +266     
==========================================
+ Hits        14056    14349     +293     
+ Misses       7588     7536      -52     
- Partials     1436     1461      +25     
Impacted Files Coverage Δ
consensus/replay_file.go 0.00% <0.00%> (ø)
consensus/replay_stubs.go 57.57% <ø> (+2.57%) ⬆️
state/store.go 50.00% <ø> (ø)
types/protobuf.go 54.68% <0.00%> (-2.67%) ⬇️
evidence/reactor.go 57.00% <50.00%> (+1.66%) ⬆️
state/services.go 50.00% <50.00%> (ø)
types/evidence.go 70.12% <58.62%> (-4.57%) ⬇️
light/client.go 68.09% <59.25%> (-1.06%) ⬇️
evidence/pool.go 69.96% <71.82%> (+2.06%) ⬆️
light/errors.go 83.33% <75.00%> (+56.66%) ⬆️
... and 23 more

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.

15/55 files viewed

consensus/byzantine_test.go Outdated Show resolved Hide resolved
consensus/byzantine_test.go Outdated Show resolved Hide resolved
consensus/byzantine_test.go Outdated Show resolved Hide resolved
consensus/reactor_test.go Outdated Show resolved Hide resolved
consensus/state.go Show resolved Hide resolved
evidence/pool.go Outdated Show resolved Hide resolved
evidence/pool.go Show resolved Hide resolved
evidence/pool.go Outdated Show resolved Hide resolved
evidence/pool.go Show resolved Hide resolved
evidence/pool.go Outdated Show resolved Hide resolved
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.

51/55 files reviewed

evidence/pool_test.go Outdated Show resolved Hide resolved
evidence/pool_test.go Outdated Show resolved Hide resolved
evidence/pool_test.go Show resolved Hide resolved
evidence/verify.go Outdated Show resolved Hide resolved
evidence/verify.go Show resolved Hide resolved
light/detector.go Show resolved Hide resolved
light/detector.go Outdated Show resolved Hide resolved
light/detector.go Show resolved Hide resolved
light/detector.go Show resolved Hide resolved
light/detector_test.go Show resolved Hide resolved
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.

55/55 files reviewed

bz := make([]byte, tmhash.Size+n)
copy(bz[:tmhash.Size-1], l.ConflictingBlock.Hash().Bytes())
copy(bz[tmhash.Size:], buf)
return tmhash.Sum(bz)
Copy link
Contributor

Choose a reason for hiding this comment

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

do we want to memoize the hash to avoid recomputing it several times?

same question for duplicate vote evidence

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll check how many times we compute the hash. I actually don't think we do it that often

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So we compute the hash at most three times when evidence passes through the evidence pool. Do you thing that warrants saving the hash?

Copy link
Contributor

Choose a reason for hiding this comment

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

yes

types/evidence.go Show resolved Hide resolved
types/evidence.go Outdated Show resolved Hide resolved
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.

@cmwaters cmwaters merged commit ed002ce into master Sep 22, 2020
@cmwaters cmwaters deleted the callum/evidence branch September 22, 2020 08:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants