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

Light Client: Evidence reporting #336

Merged
merged 6 commits into from
Jun 19, 2020
Merged

Light Client: Evidence reporting #336

merged 6 commits into from
Jun 19, 2020

Conversation

romac
Copy link
Member

@romac romac commented Jun 17, 2020 •
edited
Loading

Closes #217

TODO

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

@romac romac added the light-client Issues/features which involve the light client label Jun 17, 2020
melekes
melekes reviewed Jun 18, 2020
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.

what's left here? writing tests?

light-client/src/fork_detector.rs
primary: light_block.clone(),
witness: witness_block,
}),
Err(e) if e.kind().has_expired() => {
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
Err(e) if e.kind().has_expired() => {
Err(e) if !e.kind().has_expired() => {

?

Copy link
Member Author

Choose a reason for hiding this comment

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

Could you please expand on that?

The spec currently describe the case where header expired as a potential fork: https://github.com/informalsystems/tendermint-rs/pull/287/files#diff-57dd6c1989dd85c55b47487b7e1f92d8R409-R414

Copy link
Contributor

Choose a reason for hiding this comment

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

Right. We report it even though we won't be able to punish. All good

@romac
Copy link
Member Author

romac commented Jun 18, 2020 •
edited
Loading

what's left here? writing tests?

  • Merge master in
  • Documentation
  • Tests
  • Match the spec more closely (eg. maintain a list of full nodes that haven't been contacted)
  • Add tags to the code for traceability
  • Add contracts wherever possible

@brapse
Copy link
Contributor

brapse commented Jun 18, 2020

Could this be exercised by #331 perhaps?

@romac
Copy link
Member Author

romac commented Jun 18, 2020

Could this be exercised by #331 perhaps?

You mean the whole fork detector? I guess that would exercise part of it, yes but not the case where the header verifies successfully.

@codecov-commenter
Copy link

Codecov Report

Merging #336 into master will decrease coverage by 0.3%.
The diff coverage is 0.0%.

Impacted file tree graph

@@           Coverage Diff            @@
##           master    #336     +/-   ##
========================================
- Coverage    27.2%   26.8%   -0.4%     
========================================
  Files         129     131      +2     
  Lines        5040    5112     +72     
  Branches     1541    1570     +29     
========================================
  Hits         1375    1375             
- Misses       2694    2766     +72     
  Partials      971     971
Impacted Files Coverage Δ
light-client/src/evidence.rs 0.0% <0.0%> (ø)
light-client/src/fork_detector.rs 0.0% <0.0%> (ø)
light-client/src/supervisor.rs 0.0% <0.0%> (ø)
rpc/src/client.rs 0.0% <0.0%> (ø)
rpc/src/endpoint/evidence.rs 0.0% <0.0%> (ø)
rpc/src/method.rs 0.0% <0.0%> (ø)
tendermint/src/evidence.rs 25.8% <0.0%> (-7.6%) ⬇️
tendermint/src/abci/transaction/hash.rs 19.0% <0.0%> (-6.8%) ⬇️
tendermint/src/vote.rs 33.9% <0.0%> (-2.0%) ⬇️
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update be25579...072e1a3. Read the comment docs.

brapse
brapse approved these changes Jun 19, 2020
View reviewed changes
Copy link
Contributor

@brapse brapse left a comment

Choose a reason for hiding this comment

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

:shipit: with follow up tests once the fixtures are ready.

@romac romac merged commit 06ed36e into master Jun 19, 2020
@romac romac deleted the anton/217-evidence-reporting branch June 19, 2020 15:47
liamsi
liamsi reviewed Jun 19, 2020
View reviewed changes
tendermint/src/evidence.rs
pub fn new(h1: SignedHeader, h2: SignedHeader) -> Self {
Self { h1, h2 }
}
}
Copy link
Member

Choose a reason for hiding this comment

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

same

@brapse brapse mentioned this pull request Jun 23, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@liamsi liamsi liamsi left review comments

@melekes melekes melekes left review comments

@brapse brapse brapse approved these changes

Assignees
No one assigned
Labels
light-client Issues/features which involve the light client
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[light-client] Evidence Reporting
5 participants
@romac @brapse @codecov-commenter @melekes @liamsi