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

Kademlia protocol implemented #353

Merged
merged 4 commits into from Sep 2, 2019

Conversation

@limebell
Copy link
Contributor

limebell commented Jul 15, 2019

Swarm will not communicate with all the peers on the network. Choosing and connecting to the peers to communicate with is done by Kademlia protocol.
For this change, some message formats(BlockHashes, TxIds) and some public methods were changed.
Waiting for the reply from the dealer socket will not create block, it now uses NetMQPoller to handle messages.
SwarmTest modified due to swarm interface changes. ProtocolTest and some new tests in SwarmTest is added to check correctness of changed behaviour of swarm.

@limebell

This comment has been minimized.

Copy link
Contributor Author

limebell commented Jul 16, 2019

BootstrapAsync needs summary since it's public method provided to users of this library.

Copy link
Member

dahlia left a comment

  • Tests seem to fail.
  • Please keep past or present tense and avoid future tense in the changelog for consistency. See also existing changelog items.

I'm going to review in depth soon.

@limebell limebell force-pushed the limebell:kademlia-kademlia branch from 018665b to a53f1f9 Jul 16, 2019
@limebell

This comment has been minimized.

Copy link
Contributor Author

limebell commented Jul 16, 2019

Currently Kademlia protocol here does not implement refresh, which should be called every 30 minutes. Refresh looks up nearby nodes from itself, and from some random addresses. This should be implemented to ensure network stability.

@limebell limebell force-pushed the limebell:kademlia-kademlia branch 2 times, most recently from b7a07b8 to 5d23cc2 Jul 18, 2019
@limebell limebell force-pushed the limebell:kademlia-kademlia branch from ce961c7 to 61c660d Jul 29, 2019
@limebell

This comment has been minimized.

Copy link
Contributor Author

limebell commented Jul 29, 2019

The PR has some know issues which occurs occasionally:

  • Exception at TurnClient.ProcessMessage()
  • System.ObjectDisposedException: Safe handle has been closed
@limebell limebell force-pushed the limebell:kademlia-kademlia branch from 73affed to 1bad5da Jul 30, 2019
@codecov

This comment has been minimized.

Copy link

codecov bot commented Jul 30, 2019

Codecov Report

Merging #353 into master will decrease coverage by 0.72%.
The diff coverage is 80%.

@@            Coverage Diff             @@
##           master     #353      +/-   ##
==========================================
- Coverage   88.26%   87.54%   -0.73%     
==========================================
  Files         195      198       +3     
  Lines       13852    14208     +356     
==========================================
+ Hits        12227    12438     +211     
- Misses       1349     1500     +151     
+ Partials      276      270       -6
Impacted Files Coverage Δ
Libplanet/Net/Protocols/PeerNotFoundException.cs 0% <ø> (ø)
Libplanet/Net/DifferentProtocolVersionEventArgs.cs 100% <ø> (ø) ⬆️
Libplanet.Tests/Net/PeerTest.cs 100% <ø> (ø) ⬆️
...planet/Net/DifferentAppProtocolVersionException.cs 75% <ø> (ø) ⬆️
Libplanet/Net/Messages/Ping.cs 100% <100%> (ø) ⬆️
Libplanet/Net/Messages/Pong.cs 100% <100%> (ø) ⬆️
Libplanet/Net/Messages/FindPeer.cs 100% <100%> (ø)
Libplanet.Tests/Net/Messages/RecentStatesTest.cs 91.66% <100%> (+0.14%) ⬆️
Libplanet/Net/Messages/Message.cs 81.25% <53.33%> (-2.97%) ⬇️
Libplanet.Tests/Store/FileStoreFixture.cs 80.95% <55.55%> (-19.05%) ⬇️
... and 16 more
@limebell limebell force-pushed the limebell:kademlia-kademlia branch 2 times, most recently from 03f294b to a4f1841 Jul 30, 2019
@limebell limebell force-pushed the limebell:kademlia-kademlia branch 5 times, most recently from 96a4663 to 64b3ecc Aug 6, 2019
@limebell limebell closed this Aug 14, 2019
@limebell limebell force-pushed the limebell:kademlia-kademlia branch from 64b3ecc to 4334025 Aug 14, 2019
@limebell limebell reopened this Aug 14, 2019
@limebell limebell requested review from dahlia, longfin and earlbread Aug 16, 2019
Copy link
Member

longfin left a comment

Sorry for the late review. I'll review again.

CHANGES.md Outdated Show resolved Hide resolved
CHANGES.md Outdated Show resolved Hide resolved
CHANGES.md Outdated Show resolved Hide resolved
CHANGES.md Outdated Show resolved Hide resolved
CHANGES.md Outdated Show resolved Hide resolved
Libplanet/Net/Protocols/KademliaProtocol.cs Outdated Show resolved Hide resolved
Libplanet/Net/Protocols/KademliaProtocol.cs Outdated Show resolved Hide resolved
Libplanet/Net/Protocols/KademliaProtocol.cs Outdated Show resolved Hide resolved
Libplanet/Net/Protocols/RoutingTable.cs Outdated Show resolved Hide resolved
Libplanet/Net/Protocols/RoutingTable.cs Show resolved Hide resolved
@limebell limebell force-pushed the limebell:kademlia-kademlia branch 4 times, most recently from 1a566a3 to dec9842 Aug 21, 2019
@longfin longfin added this to the 0.6.0 milestone Aug 22, 2019
@limebell limebell force-pushed the limebell:kademlia-kademlia branch from dec9842 to 10a7841 Aug 22, 2019
@limebell limebell force-pushed the limebell:kademlia-kademlia branch from d1cb297 to 1e43ecf Aug 26, 2019
CHANGES.md Outdated Show resolved Hide resolved
CHANGES.md Outdated Show resolved Hide resolved
CHANGES.md Outdated Show resolved Hide resolved
Libplanet/Net/Messages/FindPeer.cs Outdated Show resolved Hide resolved
Libplanet/Net/Messages/FindPeer.cs Outdated Show resolved Hide resolved
Libplanet/Net/Protocols/KBucket.cs Outdated Show resolved Hide resolved
Libplanet/Net/Protocols/KBucket.cs Outdated Show resolved Hide resolved
Libplanet/Net/Protocols/RoutingTable.cs Outdated Show resolved Hide resolved
Libplanet/Net/Protocols/RoutingTable.cs Outdated Show resolved Hide resolved
Libplanet/Net/Protocols/RoutingTable.cs Outdated Show resolved Hide resolved
@limebell limebell force-pushed the limebell:kademlia-kademlia branch from 1e43ecf to 297c791 Aug 26, 2019
@codecov

This comment has been minimized.

Copy link

codecov bot commented Aug 26, 2019

Codecov Report

Merging #353 into master will decrease coverage by 0.7%.
The diff coverage is 83.62%.

@@            Coverage Diff             @@
##           master     #353      +/-   ##
==========================================
- Coverage   87.97%   87.26%   -0.71%     
==========================================
  Files         195      199       +4     
  Lines       13675    14359     +684     
==========================================
+ Hits        12030    12531     +501     
- Misses       1332     1526     +194     
+ Partials      313      302      -11
Impacted Files Coverage Δ
Libplanet/Net/Messages/FindPeer.cs 100% <100%> (ø)
Libplanet.Tests/Net/Messages/RecentStatesTest.cs 91.66% <100%> (+0.14%) ⬆️
Libplanet.Tests/Net/ProtocolTest.cs 100% <100%> (ø)
Libplanet/Net/Messages/Pong.cs 100% <100%> (ø) ⬆️
Libplanet/Net/Protocols/KademliaProtocol.cs 63.29% <63.29%> (ø)
Libplanet/Net/Swarm.cs 75.17% <78.12%> (-4.63%) ⬇️
Libplanet/Net/Messages/Message.cs 85.71% <78.57%> (+1.5%) ⬆️
Libplanet/Net/Protocols/KBucket.cs 83.67% <83.67%> (ø)
Libplanet/Net/Protocols/RoutingTable.cs 84.46% <84.46%> (ø)
Libplanet/Net/Messages/Neighbours.cs 84.61% <84.61%> (ø)
... and 12 more
@codecov

This comment has been minimized.

Copy link

codecov bot commented Aug 26, 2019

Codecov Report

Merging #353 into master will decrease coverage by 0.89%.
The diff coverage is 81.93%.

@@            Coverage Diff            @@
##           master     #353     +/-   ##
=========================================
- Coverage   88.44%   87.55%   -0.9%     
=========================================
  Files         195      200      +5     
  Lines       13779    14487    +708     
=========================================
+ Hits        12187    12684    +497     
- Misses       1306     1526    +220     
+ Partials      286      277      -9
Impacted Files Coverage Δ
Libplanet/Net/Messages/BlockHashes.cs 100% <100%> (ø) ⬆️
Libplanet.Tests/Net/PeerTest.cs 100% <100%> (ø) ⬆️
Libplanet/Net/Messages/Pong.cs 100% <100%> (ø) ⬆️
Libplanet/Net/Messages/Neighbors.cs 100% <100%> (ø)
Libplanet/Net/Messages/FindNeighbors.cs 100% <100%> (ø)
Libplanet.Tests/Net/Messages/RecentStatesTest.cs 91.66% <100%> (+0.14%) ⬆️
Libplanet.Tests/Net/ProtocolTest.cs 100% <100%> (ø)
Libplanet/Net/Peer.cs 93.75% <100%> (+1.02%) ⬆️
Libplanet/Net/Protocols/KademliaProtocol.cs 60.23% <60.23%> (ø)
Libplanet/Net/Swarm.cs 76.92% <73.84%> (-4.77%) ⬇️
... and 18 more
@limebell limebell force-pushed the limebell:kademlia-kademlia branch 5 times, most recently from 6f83a56 to 487bad0 Aug 26, 2019
Libplanet/Net/Swarm.cs Outdated Show resolved Hide resolved
Libplanet/Net/Swarm.cs Show resolved Hide resolved
Libplanet/Net/Swarm.cs Outdated Show resolved Hide resolved
Libplanet/Net/Swarm.cs Outdated Show resolved Hide resolved
Libplanet/Net/Swarm.cs Show resolved Hide resolved
@limebell limebell force-pushed the limebell:kademlia-kademlia branch from 9c3dc3b to 592ad01 Aug 28, 2019
CHANGES.md Outdated Show resolved Hide resolved
CHANGES.md Outdated Show resolved Hide resolved
Libplanet/Net/BoundPeer.cs Outdated Show resolved Hide resolved
Libplanet/Net/UnboundPeer.cs Outdated Show resolved Hide resolved
Libplanet/Net/BoundPeer.cs Outdated Show resolved Hide resolved
Libplanet/Net/Peer.cs Outdated Show resolved Hide resolved
Libplanet/Net/UnboundPeer.cs Outdated Show resolved Hide resolved
@limebell limebell force-pushed the limebell:kademlia-kademlia branch 5 times, most recently from 3f60261 to 2bf2bde Aug 28, 2019
@longfin

This comment has been minimized.

Copy link
Member

longfin commented Aug 29, 2019

It may be quite trivial, but I believe it would be better if Peer remains abstract for distinguishing UnboundPeer and BoundPeer as below.

UnboundPeer unboundPeer;
BoundPeer boundPeer;

Assert.True(unboundPeer is Peer);
Assert.True(boundPeer is Peer);

Assert.False(unboundPeer is BoundPeer);
Assert.False(boundPeer is UnboundPeer);
@limebell limebell force-pushed the limebell:kademlia-kademlia branch 2 times, most recently from cfebcf2 to 17f3218 Aug 30, 2019
limebell added 4 commits Aug 14, 2019
Does not Initialize NetMQ instances at constructor
@limebell limebell force-pushed the limebell:kademlia-kademlia branch from 17f3218 to 29cf204 Sep 2, 2019
@dahlia
dahlia approved these changes Sep 2, 2019
@longfin
longfin approved these changes Sep 2, 2019
@longfin longfin merged commit c0882cd into planetarium:master Sep 2, 2019
16 of 17 checks passed
16 of 17 checks passed
codecov/patch 81.93% of diff hit (target 88.44%)
Details
WIP Ready for review
Details
changelog This contains self-describing changelog.
Details
codecov/project 87.55% (-0.9%) compared to 97cd0dd
Details
docs Libplanet docs generated by DocFX
Details
license/cla Contributor License Agreement is signed.
Details
planetarium.libplanet Build #20190902.5 succeeded
Details
planetarium.libplanet (Linux_Mono) Linux_Mono succeeded
Details
planetarium.libplanet (Linux_NETCore) Linux_NETCore succeeded
Details
planetarium.libplanet (Windows_Mono) Windows_Mono succeeded
Details
planetarium.libplanet (Windows_NETCore) Windows_NETCore succeeded
Details
planetarium.libplanet (Windows_NETCore_Benchmark) Windows_NETCore_Benchmark succeeded
Details
planetarium.libplanet (Windows_NETCore_coverage) Windows_NETCore_coverage succeeded
Details
planetarium.libplanet (Windows_NETFramework) Windows_NETFramework succeeded
Details
planetarium.libplanet (macOS_Mono) macOS_Mono succeeded
Details
planetarium.libplanet (macOS_NETCore) macOS_NETCore succeeded
Details
planetarium.libplanet (macOS_Unity) macOS_Unity succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.