Skip to content

lite: modified bisection to loop#4400

Merged
melekes merged 14 commits intomasterfrom
callum/non-recursive-bisection
Feb 19, 2020
Merged

lite: modified bisection to loop#4400
melekes merged 14 commits intomasterfrom
callum/non-recursive-bisection

Conversation

@cmwaters
Copy link
Contributor

closes #4329

As opposed to using recursion to implement the bisection method of verifying a header, which could have problems with memory allocation (especially for smaller devices), the bisection algorithm now uses a for loop.

  • 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

@tac0turtle tac0turtle added the C:light Component: Light label Feb 13, 2020
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
Copy link
Contributor Author

looks good from the first glance 💛

have you seen the spec https://github.com/tendermint/spec/blob/zm_non-recursive-verification/spec/consensus/light-client/non-recursive-verification.md btw?

Yes, I've had a look at it. The code there seems a little bit bloated but I think my implementation matches it

@cmwaters cmwaters requested a review from melekes February 14, 2020 17:20
@codecov-io
Copy link

codecov-io commented Feb 14, 2020

Codecov Report

Merging #4400 into master will increase coverage by 0.25%.
The diff coverage is 0%.

@@            Coverage Diff             @@
##           master    #4400      +/-   ##
==========================================
+ Coverage   65.69%   65.95%   +0.25%     
==========================================
  Files         226      226              
  Lines       19992    19997       +5     
==========================================
+ Hits        13134    13189      +55     
+ Misses       5799     5759      -40     
+ Partials     1059     1049      -10
Impacted Files Coverage Δ
rpc/core/tx.go 0% <0%> (ø) ⬆️
blockchain/v0/reactor.go 78.77% <0%> (-0.95%) ⬇️
blockchain/v0/pool.go 78.84% <0%> (-0.33%) ⬇️
privval/signer_server.go 95.65% <0%> (ø) ⬆️
p2p/pex/pex_reactor.go 84.18% <0%> (+0.56%) ⬆️
state/store.go 66.88% <0%> (+0.68%) ⬆️
consensus/replay.go 72.54% <0%> (+0.78%) ⬆️
consensus/state.go 79.39% <0%> (+1.65%) ⬆️
consensus/reactor.go 79.58% <0%> (+1.84%) ⬆️
privval/signer_listener_endpoint.go 89.13% <0%> (+2.17%) ⬆️
... and 3 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.

We need to write more tests. If I am correct, current code may lead to infinite loops under certain conditions...

@melekes
Copy link
Contributor

melekes commented Feb 19, 2020

I am going to write tests now.

@melekes melekes merged commit 1874a97 into master Feb 19, 2020
@melekes melekes deleted the callum/non-recursive-bisection branch February 19, 2020 14:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

C:light Component: Light

Projects

None yet

Development

Successfully merging this pull request may close these issues.

lite2: align with the newer version of the spec

5 participants