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

light tests: remove assertions about number of bisections #295

Merged
merged 1 commit into from
Jun 3, 2020

Conversation

liamsi
Copy link
Member

@liamsi liamsi commented Jun 3, 2020

Quickfix that closes #276 (I had this change uncommited in my workspace and just pushed it)

  • 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

 - they are an implementation detail and the go and rust implementation seem to divert
@liamsi liamsi requested review from greg-szabo and romac June 3, 2020 08:30
@codecov-commenter
Copy link

Codecov Report

Merging #295 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff           @@
##           master    #295   +/-   ##
======================================
  Coverage    30.8%   30.8%           
======================================
  Files         129     129           
  Lines        4893    4893           
  Branches     1549    1549           
======================================
  Hits         1511    1511           
  Misses       2297    2297           
  Partials     1085    1085           

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 b881142...547d714. Read the comment docs.

@romac
Copy link
Member

romac commented Jun 3, 2020

Should we remove the field from the JSON as well?

@liamsi
Copy link
Member Author

liamsi commented Jun 3, 2020

Should we remove the field from the JSON as well?

The JSON is generated from the go-code. I wouldn't manually remove it here but let @Shivani912 change the generator code and re-generate the JSON files instead (which is kinda orthogonal to this PR).

Copy link
Member

@greg-szabo greg-szabo left a comment

Choose a reason for hiding this comment

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

I've got to learn more about bisections and why different implementations would require such different number of bisections but this change in itself is fine. You're removing unused code.

@romac
Copy link
Member

romac commented Jun 3, 2020

@greg-szabo: I've got to learn more about bisections and why different implementations would require such different number of bisections but this change in itself is fine. You're removing unused code.

@josef-widder touched on that in the verification spec:

Schedule decides which height to try to verify next. We keep this underspecified as different implementations (currently in Goland and Rust) may implement different optimizations here. We just provide necessary conditions on how the height may evolve.

As an example, an implementation of Schedule could realize that, even if the optimal block to bisect to is of height H, there is already a block of height H+N (for a small N) in the store, and decide to pick this one to avoid fetching block H over the network. This could lead to a different number of bisections.

@brapse brapse merged commit 4382f16 into master Jun 3, 2020
@brapse brapse deleted the ismail/remove_num_bisections branch June 3, 2020 14:37
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.

Light client testing: remove implementation specific assertions about number of bisections
5 participants