codecov-commenter
commented
1 year ago Codecov Report
Base: 79.07% // Head: 79.07% // No change to project coverage :thumbsup:
Coverage data is based on head (
34c22fd) compared to base (0ef32b0). Patch coverage: 100.00% of modified lines in pull request are covered.
Additional details and impacted files
```diff @@ Coverage Diff @@ ## develop #124 +/- ## ======================================== Coverage 79.07% 79.07% ======================================== Files 23 23 Lines 4880 4880 ======================================== Hits 3859 3859 Misses 1021 1021 ``` | Flag | Coverage Δ | | |---|---|---| | unittests | `79.07% <100.00%> (ø)` | | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=nervosnetwork#carryforward-flags-in-the-pull-request-comment) to find out more. | [Impacted Files](https://codecov.io/gh/nervosnetwork/ckb-light-client/pull/124?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=nervosnetwork) | Coverage Δ | | |---|---|---| | [src/protocols/light\_client/sampling.rs](https://codecov.io/gh/nervosnetwork/ckb-light-client/pull/124?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=nervosnetwork#diff-c3JjL3Byb3RvY29scy9saWdodF9jbGllbnQvc2FtcGxpbmcucnM=) | `97.77% <100.00%> (ø)` | | Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=nervosnetwork). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=nervosnetwork):umbrella: View full report at Codecov.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.
Issue
The bug is introduced since #121.
https://github.com/nervosnetwork/ckb-light-client/blob/0ef32b0bcca0a379a5c44ce1adbe1c77fda3e085/src/protocols/light_client/components/send_last_state_proof.rs#L603-L618
Let $b_{\text{start}}$ denote the start block in the request, it's also the previous last state.
Let $b_{\text{last}}$ denote the block in the last state in the request.
Let $B_{\text{all}}$ denote all blocks which requires verification.
Obviously,
The first block in $B{\text{all}}$ is $b{\text{start}}$.
The next block of $B{\text{all}}$ is $b{\text{last}}$.
Let $B_{\text{last}}$ denote the real last n blocks.
Obviously, $B{\text{all}}$ contains $B{\text{last}}$.
Let $B_{\text{sampled}}$ denote the sampled blocks.
Obviously, $B{\text{all}}$ contains $B{\text{sampled}}$.
Let $C{\text{all}}$ denote the count of $B{\text{all}}$.
Let $C{\text{last}}$ denote the count of $B{\text{last}}$.
Let $C{\text{sampled}}$ denote the count of $B{\text{sampled}}$.
Let $N_{\text{last}}$ denote the paramter
LAST_N_BLOCKS.https://github.com/nervosnetwork/ckb-light-client/blob/0ef32b0bcca0a379a5c44ce1adbe1c77fda3e085/src/protocols/mod.rs#L28
Before that PR #121, we only checked if $B_\text{last}$ are continuous, but didn't check:
whether $B{\text{last}}$ are continuous with $b{\text{last}}$.
whether $B{\text{last}}$ contain $b{\text{start}}$, when $C_{\text{all}}$ is small.
The bug happened in the check-2.
If $C{\text{all}} \le N{\text{last}}$, CKB node will send all blocks of $B_{\text{all}}$ to the client, so the check-1 and check-2 will be satisfied.
But it's a necessary and insufficient condition, its opposite is not true.
If $C{\text{all}} = N{\text{last}} + \beta \gt N{\text{last}}$, when $\beta$ is small enough, the $C{\text{sampled}}$ also could be $0$. So, even there are no sampled blocks in the response, the check-2 also could be NOT satisfied.
Solutions
There are 3 solutions:
Remove the check-2.
Update the check-2: remove the check when $C{\text{all}} = N{\text{last}}$, only do the check when $C{\text{all}} \lt N{\text{last}}$.
If the $C{\text{sampled}}$ which computed from the formula is $0$, but $C{\text{all}} \gt N{\text{last}}$, set $C{\text{sampled}}$ to $1$.
To make sure there always has at least sampled 1 block ( $C{\text{sampled}} \gt 0$ ) when $C{\text{all}} \gt N_{\text{last}}$.
Then no sampled blocks ( $C{\text{sampled}} = 0$ ) will only happened when $C{\text{all}} \le N_{\text{last}}$.
This PR choose the 3rd solution.