Add peer discovery by tzdybal · Pull Request #17 · evstack/ev-node

tzdybal · 2021-03-04T20:58:36Z

Resolves#2.

In default ('auto') mode, peers are not advertising as DHT servers, and peer discovery is filtering them out during peer traversal.

codecov-io · 2021-03-13T12:23:46Z

Codecov Report

Merging #17 (e97f4bf) into main (cd64a50) will increase coverage by 5.40%.
The diff coverage is 72.46%.

@@ Coverage Diff @@## main #17 +/- ##
==========================================
+ Coverage 66.66% 72.07% +5.40% 
==========================================
Files 3 3 Lines 63 111 +48 ==========================================
+ Hits 42 80 +38 - Misses 12 16 +4 - Partials 9 15 +6

Impacted Files	Coverage Δ
p2p/client.go	`69.47% <72.46%> (+9.89%)`	⬆️

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 cd64a50...e97f4bf. Read the comment docs.

Wondertan · 2021-03-18T11:51:50Z

+func (c *Client) Close() error {
+	dhtErr := c.dht.Close()
+	if dhtErr != nil {
+ c.logger.Error("failed to close DHT", "error", dhtErr)


Why not return it? We can either:
call both Closes and return the error from the first/second one
use lightweight error combiner with decent api

Currently it's the first option ;) dhtErr is returned in line 99.
I just want to log both errors, in case only one of them is returned.

Wondertan · 2021-03-18T11:59:17Z

 func (c *Client) listen() error {
-// TODO(tzdybal): consider requiring listen address in multiaddress format
-	maddr, err := GetMultiAddr(c.conf.ListenAddress)
+var err error


Maybe declare var within func body(err error)?

You mean named return value?

Wondertan · 2021-03-18T12:00:17Z

+	}
+
+	var err error
+	c.dht, err = dht.New(c.ctx, c.host, dht.Mode(dht.ModeServer), dht.BootstrapPeers(seedNodes...))


Why ModeServer if this is a Client?

Found the answer in the commit. Can you please point to the place in the code where filtering of non-Server nodes happens?

Any updates? I think that this setting at least have to be configured

Here, is the filtering: https://github.com/libp2p/go-libp2p-kad-dht/blob/master/subscriber_notifee.go#L148
To ensure, that we're support DHT protocols, we need to go to server mode: https://github.com/libp2p/go-libp2p-kad-dht/blob/master/dht.go#L727

Wondertan · 2021-03-18T12:28:45Z

+	}
+}
+
+func (c *Client) getSeedAddrInfo(seedStr string) []peer.AddrInfo {


Why about having seeds as an array and not a single string?

cosmos-sdk is providing a single, comma-separated string anyways, so converting from string to slice is required.
But it may fit better in the conv package, as this logic is not client-specific.

After re-thinking I'll leave it as is. I tried to:
only split string into slice in conv; but it makes code more complicated and actual parsing is located in two packages
move entire method to conv; but error handling is client-specific - I can't think of other use of "parse seed nodes and only log errors if necessary, without returning errors" function ;)

Can you pls provide an example of such a string with multiple seeds? How is that related to Cosmos? I was thinking that this is a multiaddr string

cosmos-sdk app, will use optimint as a drop-in replacement of tendermint (see master-optimint branch on our fork).
For better user experience, we reuse configurations, command line options, etc from cosmos-sdk. And cosmos-sdk apps have a config/option Seeds which is a comma separated string. The actual change is that we expect multiaddr format, not cosmos-sdk format.
Example is: /ip4/192.168.1.1/tcp/1234/p2p/seed1_id,/ip4/192.168.1.2/tcp/4321/p2p/seed2_id
Ref: #16

Wondertan · 2021-03-18T12:44:15Z

 }

+// Close gently stops Client.
+func (c *Client) Close() error {


You need to cancel the ctx within the Client on Close, otherwise the routine created by discovery.Advertise leaks.

Fixed in e97f4bf.

tzdybal

Thanks for review. I'll address those issues

tzdybal · 2021-03-18T15:10:54Z

+	chainID string
 	privKey crypto.PrivKey

+	ctx context.Context


As you said, I kind-of mimicked libp2p. But maybe it's worth fixing this; the sooner the better.

tzdybal · 2021-03-18T15:51:20Z

+func (c *Client) Close() error {
+	dhtErr := c.dht.Close()
+	if dhtErr != nil {
+ c.logger.Error("failed to close DHT", "error", dhtErr)


Currently it's the first option ;) dhtErr is returned in line 99.
I just want to log both errors, in case only one of them is returned.

tzdybal · 2021-03-18T15:54:53Z

+	}
+}
+
+func (c *Client) getSeedAddrInfo(seedStr string) []peer.AddrInfo {


cosmos-sdk is providing a single, comma-separated string anyways, so converting from string to slice is required.
But it may fit better in the conv package, as this logic is not client-specific.

tzdybal · 2021-03-18T15:58:22Z

 func (c *Client) listen() error {
-// TODO(tzdybal): consider requiring listen address in multiaddress format
-	maddr, err := GetMultiAddr(c.conf.ListenAddress)
+var err error


You mean named return value?

Wondertan · 2021-03-23T09:17:14Z

+
+func (c *Client) findPeers(ctx context.Context) error {
+	// TODO(tzdybal): add configuration parameter for max peers
+	peerCh, err := c.disc.FindPeers(ctx, c.getNamespace(), cdiscovery.Limit(60))


Extract to a constant like reAdveritse?

Wondertan · 2021-03-23T09:19:25Z

@liamsi, can I have write access?

tzdybal · 2021-03-23T09:22:17Z

@liamsi, can I have write access?

Granted!

tzdybal mentioned this pull request Mar 5, 2021
P2P bootstrapping #14
Merged

tzdybal force-pushed the tzdybal/p2p/bootstrapping branch from 63118f1 to bb1da59Compare March 9, 2021 13:28

tzdybal changed the base branch from tzdybal/p2p/bootstrapping to mainMarch 9, 2021 13:30

tzdybal force-pushed the tzdybal/p2p/discovery branch from b50405d to fc66088Compare March 9, 2021 13:35

tzdybal added 3 commits March 9, 2021 15:04

Add context to client
636483e

Not working, DHT routing as option + active peer dicovery
579442d

Use DHT in server mode
f924497
In default ('auto') mode, peers are not advertising as DHT servers, and peer discovery is filtering them out during peer traversal.

tzdybal force-pushed the tzdybal/p2p/discovery branch from fc66088 to f924497Compare March 9, 2021 14:08

tzdybal added 4 commits March 10, 2021 12:15

WIP
64f86ce

Add default configuration values.
9645437

Use DHT for bootstrapping and peer discovery
ce095e1

Add unit test of seeds string parsing function
a4944ef

tzdybal added 4 commits March 15, 2021 11:28

Remove outdated comment
0dffaff

Add DHT-based active peer discovery
0a672c7

Improved advertising logic and error handling
34ec982

Simple test of peer discovery
a560ff6

tzdybal marked this pull request as ready for review March 17, 2021 12:28

tzdybal requested review from Wondertan and liamsi March 17, 2021 12:29

tzdybal mentioned this pull request Mar 18, 2021
Add context to client #20
Closed

tzdybal added 2 commits March 18, 2021 12:57

Merge tracking branch 'main' into tzdybal/p2p/discovery
406e411

Extract const for hardcoded duration
7f56650

Wondertan reviewed Mar 18, 2021
View reviewed changes

tzdybal commented Mar 18, 2021
View reviewed changes

tzdybal mentioned this pull request Mar 19, 2021
Use libp2p mocked network in tests #26
Closed

tzdybal requested a review from Wondertan March 19, 2021 15:35

tzdybal mentioned this pull request Mar 22, 2021
Transaction gossiping #29
Merged

Wondertan approved these changes Mar 23, 2021
View reviewed changes

tzdybal added 5 commits March 23, 2021 10:27

Pass context instead of storing it in struct
f60014a

Change default listen address to 0.0.0.0
4fe1062

Wrap libp2p host with RoutedHost
2d4c64a

go mod tidy
504b75e

Ensure that context passed to libp2p is cancelled
7777439

tzdybal force-pushed the tzdybal/p2p/discovery branch from e97f4bf to 7777439Compare March 23, 2021 09:27

tzdybal merged commit 567292e into mainMar 23, 2021

tzdybal deleted the tzdybal/p2p/discovery branch March 23, 2021 09:34

tzdybal restored the tzdybal/p2p/discovery branch October 9, 2021 13:35

tzdybal deleted the tzdybal/p2p/discovery branch October 9, 2021 13:37

claudeBot mentioned this pull request Nov 5, 2025
feat: forced inclusion #2797
Merged
11 tasks

claudeBot mentioned this pull request Nov 18, 2025
feat: High availabilty via RAFT #2836
Closed

claudeBot mentioned this pull request Dec 15, 2025
feat: DA Client remove interface part 3, replace types with new code #2910
Merged

claudeBot mentioned this pull request Jan 26, 2026
feat: High availabilty via RAFT #2987
Merged

This was referenced Feb 3, 2026
feat: DA hints in p2p #2891
Merged
refactor(cache): skip empty data in pending data #3046
Merged
feat: block Pruning #2984
Merged

Uh oh!

Conversation

tzdybal commented Mar 4, 2021

Uh oh!

codecov-io commented Mar 13, 2021• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tzdybalMar 19, 2021• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tzdybal left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Wondertan commented Mar 23, 2021

Uh oh!

tzdybal commented Mar 23, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov-io commented Mar 13, 2021•
edited
Loading

tzdybalMar 19, 2021•
edited
Loading