fix(net): reset block demand when not changed #1167
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
moreal
changed the title
moreal
force-pushed
the
bugfix/network/reset-block-demand-on-correct-time
branch
3 times, most recently
from
90b1c05
to
81d7c28
Compare
Codecov Report
@@ Coverage Diff @@
## main #1167 +/- ##
==========================================
- Coverage 87.77% 87.67% -0.10%
==========================================
Files 355 355
Lines 30812 30840 +28
==========================================
- Hits 27044 27039 -5
- Misses 2057 2088 +31
- Partials 1711 1713 +2
|
moreal
force-pushed
the
bugfix/network/reset-block-demand-on-correct-time
branch
2 times, most recently
from
51404b2
to
5d70c66
Compare
moreal
changed the title
auto-assign
bot
requested review from
dahlia,
limebell,
longfin and
riemannulus
dahlia
reviewed
Jan 22, 2021
longfin
previously approved these changes
Jan 25, 2021
moreal
force-pushed
the
bugfix/network/reset-block-demand-on-correct-time
branch
2 times, most recently
from
7b93570
to
61dcf01
Compare
@dahlia I tried to write the test in 61dcf01. As a comment, I felt those events should be reorganized once. For example, the |
longfin
previously approved these changes
Jan 25, 2021
limebell
previously approved these changes
Jan 25, 2021
Libplanet/Net/Swarm.cs
Outdated
| @@ -1758,9 +1761,14 @@ CancellationToken cancellationToken | |||
| { | |||
| // FIXME: Should only reset when BlockDemand has not changed | |||
moreal
force-pushed
the
bugfix/network/reset-block-demand-on-correct-time
branch
from
61dcf01
to
596683e
Compare
moreal
commented
Jan 25, 2021
Comment on lines
-1759
to
-1777
| // FIXME: Should only reset when BlockDemand has not changed | ||
| // from the beginning of this operation. |
There was a problem hiding this comment.
@limebell I removed the lines also 🙏
longfin
previously approved these changes
Jan 25, 2021
dahlia
previously approved these changes
Jan 25, 2021
limebell
previously approved these changes
Jan 25, 2021
moreal
force-pushed
the
bugfix/network/reset-block-demand-on-correct-time
branch
from
596683e
to
a2f1f96
Compare
[changelog skip]
This commit checks the situation like below steps - Received `BlockHeader` 19 and `BlockDemand` 19 block. - `ProcessFillBlocks()` starts to download the 19 blocks. - Received `BlockHeader` 20 and `BlockDemand` 20 block. - `ProcessFillBlocks()` finished to download the 19 blocks and reset `Swarm<T>.BlockDemand` as `null`. - Though `BlockHeader` 101 was received, it will be ignored. [changelog skip]
moreal
force-pushed
the
bugfix/network/reset-block-demand-on-correct-time
branch
from
a2f1f96
to
e14af51
Compare
|
I rebased this pull request, PTAL @planetarium/libplanet 🙏 |
dahlia
approved these changes
Jan 26, 2021
limebell
approved these changes
Jan 27, 2021
longfin
pushed a commit
that referenced
this pull request
Jan 29, 2021
…d-on-correct-time fix(net): reset block demand when not changed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This fixes an issue where it can reset block demand at an incorrect time. For instance:
BlockHeader100 andBlockDemand100 block.ProcessFillBlocks()starts to download the 100 blocks.BlockHeader101 andBlockDemand101 block.ProcessFillBlocks()finished to download the 100 blocks and resetSwarm<T>.BlockDemandasnull.BlockHeader101 was received, it was ignored.I tried to fix the problem by resetting the block demand as
nullwhen startingSyncPreviousBlocksAsync()after moving the block demand as the local variable, but it makes many code changes and I felt hard to build the situation. So I fixed the problem with the current changes simply. Also, I skipped the changelog becauseSwarm<T>.BlockDemandwas not released yet. If you think it needs to be recorded in the changelog or have other suggestions, please leave your opinions free.