p2p: introduce Gossip peer capability #5935

algorandskiy · 2024-02-12T22:44:40Z

Summary

This PR enables p2p relays by introducing a new gossip capability and preferring such peers for connection.

Add DHT capability
Update peerstore methods (and tests) to support AddInfo/multiaddrs
Query Gossip peer from DHT
Prefer Gossip peers when dialing. Ensure DHT subsystem maintains own peers and can connect to such DHT peers.
Added relayMessages condition
Do not send TX traffic to NPN nodes

Test Plan

Unit tests for relayMessages
Unit tests for wantTXGossip and gossip sub.
Unit test for gossip capability

codecov · 2024-02-13T22:16:47Z

Codecov Report

Attention: Patch coverage is 77.05882% with 39 lines in your changes are missing coverage. Please review.

Project coverage is 55.88%. Comparing base (2df76fd) to head (4ee6bb2).
Report is 3 commits behind head on feature/p2p.

Files	Patch %	Lines
network/p2pNetwork.go	79.02%	24 Missing and 6 partials ⚠️
network/p2p/p2p.go	12.50%	7 Missing ⚠️
network/p2p/peerstore/peerstore.go	93.75%	1 Missing ⚠️
network/p2p/pubsub.go	0.00%	1 Missing ⚠️

Additional details and impacted files

@@               Coverage Diff               @@
##           feature/p2p    #5935      +/-   ##
===============================================
+ Coverage        55.84%   55.88%   +0.04%     
===============================================
  Files              490      490              
  Lines            68540    68628      +88     
===============================================
+ Hits             38273    38353      +80     
- Misses           27655    27665      +10     
+ Partials          2612     2610       -2

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

gmalouf

A few questions, but overall this makes a lot of sense.

gmalouf · 2024-02-21T21:05:08Z

network/p2p/p2p.go

-	peerIDs := s.host.Peerstore().Peers()
-	for _, peerID := range peerIDs {
+	ps := s.host.Peerstore().(*pstore.PeerStore)
+	peerIDs := ps.GetAddresses(targetConnCount, phonebook.PhoneBookEntryRelayRole)


this is connecting via libp2p to known relays?

This connects to peerstore relays that were either set via phonebook, DNS bootstrap, or discovered via DHT.

gmalouf · 2024-02-21T21:33:28Z

network/p2p/peerstore/peerstore_test.go

-	ph.ReplacePeerList(relaysSet, "default", PhoneBookEntryRelayRole)
-	ph.ReplacePeerList(archiverSet, "default", PhoneBookEntryArchiverRole)
+	ph.ReplacePeerList(infoRelaySet, "default", PhoneBookEntryRelayRole)
+	ph.ReplacePeerList(infoArchiverSet, "default", PhoneBookEntryArchiverRole)


I'm going to be killing off the archiver role in next PR.

gmalouf · 2024-02-21T21:35:13Z

network/p2pNetwork.go

+		var err error
+		peers, err = n.capabilitiesDiscovery.PeersForCapability(p2p.Gossip, n.config.GossipFanout)
+		if err != nil {
+			n.log.Warnf("Error getting relay nodes from capabilities discovery: %v", err)


Shouldn't we be willing to connect to anyone, not just relays?

This is to discuss. But there is no point to connect peers without gossip except to receive peers via peer exchange. Maybe it should be like 50/50 gossip/any

gmalouf · 2024-02-21T21:39:52Z

network/p2pNetwork.go

@@ -560,7 +605,19 @@ func (n *P2PNetwork) GetHTTPClient(address string) (*http.Client, error) {
 // this is the only indication that we have that we haven't formed a clique, where all incoming messages
 // arrive very quickly, but might be missing some votes. The usage of this call is expected to have similar
 // characteristics as with a watchdog timer.
-func (n *P2PNetwork) OnNetworkAdvance() {}
+func (n *P2PNetwork) OnNetworkAdvance() {


Am I understanding right, that we are checking based on something that we want to be connected to a participating node explicitly?

this checks subscribes to gossipsub TX traffic if the node is participating in consensus

gmalouf · 2024-02-21T21:53:26Z

network/p2pNetwork_test.go

+		return netA.hasPeers() && netB.hasPeers() && netC.hasPeers()
+	}, 2*time.Second, 50*time.Millisecond)
+
+	time.Sleep(time.Second) // give time for peers to connect.


should the eventually cover this?

This is the same in all 3 gossip tests. I tried to figure out why CI race build fails without it but no success.

jasonpaulos · 2024-02-22T23:28:26Z

network/p2pNetwork.go

+	}
+	var result []peer.AddrInfo
+	for _, addr := range unique {
+		result = append(result, *addr)


nit: could use maps.Keys

jasonpaulos · 2024-02-22T23:41:33Z

network/p2pNetwork_test.go

+		50*time.Millisecond,
+	)
+
+	// check netB did not receive the messages


Suggested change

// check netB did not receive the messages

// check netC did not receive the messages

right?

jasonpaulos · 2024-02-22T23:47:12Z

network/p2pNetwork_test.go

+
+			r2 := mergeP2PAddrInfoResolvedAddresses(info1, info2)
+			if len(r2) != tt.expected {
+				t.Errorf("Expected  %d addresses, got %d", tt.expected, len(r2))


nit: require.Len would be useful here

jasonpaulos · 2024-02-22T23:51:10Z

network/p2pNetwork.go

@@ -735,6 +851,13 @@ func (n *P2PNetwork) txTopicHandleLoop() {
 		// from gossipsub's point of view, it's just waiting to hear back from the validator,
 		// and txHandler does all its work in the validator, so we don't need to do anything here
 		_ = msg
+
+		// participation or configuration change, cancel subscription and quit
+		if !n.wantTXGossip.Load() {


Hm, I'm not sure how often OnNetworkAdvance can be called where it changes n.wantTXGossip. If this runs less often than OnNetworkAdvance, it's possible this routine won't notice swaps to n.wantTXGossip and multiple routines could be listening. That could be addressed by storing the "config" version as an int and bailing out here if that version isn't what the routine was started with.

But in reality I'm assuming it will be very rare for n.wantTXGossip to change?

algorandskiy added Enhancement p2p Work related to the p2p project labels Feb 12, 2024

algorandskiy requested review from gmalouf and jasonpaulos February 12, 2024 22:44

algorandskiy self-assigned this Feb 12, 2024

p2p: introduce Gossip peer capability

f2a05a6

algorandskiy force-pushed the pavel/p2p-relay-dht branch from b1f874d to f2a05a6 Compare February 13, 2024 21:51

algorandskiy added 3 commits February 13, 2024 17:32

p2p: connect to gossip nodes only

773a8ed

add relayMessages condition and tests

4959d39

unsubscribe non-part nodes from TX traffic

d05816c

algorandskiy force-pushed the pavel/p2p-relay-dht branch from 628024a to d05816c Compare February 15, 2024 23:18

algorandskiy marked this pull request as ready for review February 15, 2024 23:18

algorandskiy added 3 commits February 16, 2024 11:19

fix unit tests

cb48b0f

self review fixes

5b28821

debug CI failures

07bb5ce

algorandskiy force-pushed the pavel/p2p-relay-dht branch from 86d0be1 to 07bb5ce Compare February 16, 2024 20:15

algorandskiy added 4 commits February 16, 2024 17:41

remove debugging

391ac5e

DHT relays test

c9a7433

revert time.sleep removal

84c6f13

fix linter

f5b6fa9

algorandskiy mentioned this pull request Feb 21, 2024

Run fast catchup using peerstore instead of phonebook #5666

Closed

gmalouf reviewed Feb 21, 2024

View reviewed changes

algorandskiy added 3 commits February 22, 2024 15:55

meshThread: check DNS in addition to DHT

ef77db3

fix race condition

824fbad

extend tests

4ee6bb2

jasonpaulos reviewed Feb 22, 2024

View reviewed changes

algorandskiy merged commit ef82b2b into algorand:feature/p2p Feb 23, 2024
16 checks passed

cce mentioned this pull request Jun 14, 2024

network: p2p traffic exchange for algorand node #5939

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

p2p: introduce Gossip peer capability #5935

p2p: introduce Gossip peer capability #5935

algorandskiy commented Feb 12, 2024 •

edited

Loading

codecov bot commented Feb 13, 2024 •

edited

Loading

gmalouf left a comment

gmalouf Feb 21, 2024

algorandskiy Feb 21, 2024

gmalouf Feb 21, 2024

gmalouf Feb 21, 2024

algorandskiy Feb 21, 2024

gmalouf Feb 21, 2024

algorandskiy Feb 21, 2024

gmalouf Feb 21, 2024

algorandskiy Feb 21, 2024

jasonpaulos Feb 22, 2024

jasonpaulos Feb 22, 2024

jasonpaulos Feb 22, 2024

jasonpaulos Feb 22, 2024

	// check netB did not receive the messages
	// check netC did not receive the messages

p2p: introduce Gossip peer capability #5935

p2p: introduce Gossip peer capability #5935

Conversation

algorandskiy commented Feb 12, 2024 • edited Loading

Summary

Test Plan

codecov bot commented Feb 13, 2024 • edited Loading

Codecov Report

gmalouf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

algorandskiy commented Feb 12, 2024 •

edited

Loading

codecov bot commented Feb 13, 2024 •

edited

Loading