refactor(px): refactor peer exchange + tests #1527

alrevuelta · 2023-02-01T14:54:09Z

Last item to close #1463

Some refactoring in the px protocol + more tests + moving the "what to do with the peers we got via px" logic to the application. Some discussions started in #1463 and this prepares the ground for #1521.

Summary of changes:

Improve error handling using Result and catch exceptions (mainly readLp and writeLp since can raise an Exception if the stream is prematurely closed)
No longer handle the px response on the protocol. The response should be handled by the application, which now adds the peers to the peerstore intead of trying to connect to them.
Changes respond proc, reusing the existing connection instead of attempting to create a new one.
Fixes possible vulnerability here, since one could inject unsolicitatede responses to a node that never requested px. In other words, you could send a PeerExchangeResponse to a px node without that node ever requesting anything from you.
Add new tests for all request proc variants both succesfull and not.

Test:

Once approved manually test it.

End to end test

Node that discovers peers and runs peer exchange:

./build/wakunode2 \
--dns-discovery=true \
--dns-discovery-url=enrtree://AOGECG2SPND25EEFMAJ5WF3KSGJNSGV356DSTL2YVLLZWIV6SAYBM@test.waku.nodes.status.im \
--discv5-discovery=true \
--discv5-enr-auto-update=True \
--nodekey=161c2836197e8918d7eb1039758423795f0dc697a018ed55d5931aa9b0e5efb3 \
--peer-exchange

Node that sets the above one as px node

./build/wakunode2 \
--tcp-port=30305 \
--ports-shift=1 \
--peer-exchange-node=/ip4/127.0.0.1/tcp/60000/p2p/16Uiu2HAm2ZuRHGZUzG6JGJeY1nGKHXLGrQYnZ7mZToymHtFfYFFf

Peers are succesfuly retrieved in the second node and added to the peerstore.

status-im-auto · 2023-02-01T23:08:51Z

Jenkins Builds

Click to see older builds (3)

❔	Commit	#️⃣	Finished (UTC)	Duration	Platform	Result
✔️	`69c94bd`	#1	2023-02-01 23:08:50	~21 min	`macos`	📦`bin`

✔️	`3aee8df`	#2	2023-02-02 15:32:54	~22 min	`macos`	📦`bin`

✔️	`3708e06`	#3	2023-02-03 15:02:42	~15 min	`macos`	📦`bin`

❔	Commit	#️⃣	Finished (UTC)	Duration	Platform	Result
✔️	`1233fa9`	#4	2023-02-06 15:03:32	~16 min	`macos`	📦`bin`

❌	`19bab05`	#5	2023-02-07 15:05:14	~17 min	`macos`	📄`log`

LNSD

Waku peer-exchange wire formats must be added to the vacp2p/waku repository.

Please add them before proceeding with modifying the implementation. Flaws in the wire format and the RPC types should be fixed before moving forward.

waku/v2/protocol/waku_peer_exchange/protocol.nim

LNSD · 2023-02-06T14:02:46Z

waku/v2/protocol/waku_peer_exchange/protocol.nim

-    # handle peer exchange request
-    if rpc.request != PeerExchangeRequest():
+    # If we got a request (request field is not empty)
+    if rpc.request != default(PeerExchangeRequest):


While I know this is in other places in the code, we should avoid doing it. This is a defect in this protocol's RPC codec and RPC types:

If the field is mandatory, return a "required field missing error"

If the field is optional, the codec should return an Option; here, you should check if it is present (or not) and act in consequence.

Yep, I agree. But since the rpc doesn't allow us to add an error message, this would involve changing the px proto, changing the spec, and perhaps bumping the px version? All of this beyond the PR.

This is not correct.

I am not asking to change the protocol. I am concerned about the implementation and the way this equality comparison uses the default(...) to detect if the field was missing:

if rpc.request != default(PeerExchangeRequest):

You can return the this "required field missing" error from the rpc_codec the same way other protocols do, and this check can be removed from the protocol handler because if the field is not present, the message will be considered invalid.

This doesn't require any protocol modification. It is an implementation detail, and it should be in the scope of the PR, given that this PR aims to improve the implementation. This technical debt cannot be left out of the scope.

Should be fixed in 7cb5433

and it should be in the scope of the PR

note thought that the initial goal of this PR was to close #1463 and some refactoring was done along the way, but the priority was to decouple discovery from connection.

As a general note, PRs can and should have a limited scope. This PR had a clear focus listed in the description and addressed those items.

Not approving a PR generally means that there is a critical bug/regression/technical debt introduced (not existing prior to PR) or a critical question which needs answering. Technical debt which existed before the PR can be pointed out, but to speed up PR process should not stop approval.

alrevuelta

Waku peer-exchange wire formats must be added to the vacp2p/waku repository.

Please add them before proceeding with modifying the implementation. Flaws in the wire format and the RPC types should be fixed before moving forward.

Sure, wouldnt consider it a blocker for this, but this should cover it:
waku-org/waku-proto#12
vacp2p/rfc#570

waku/v2/protocol/waku_peer_exchange/protocol.nim

alrevuelta · 2023-02-07T08:59:21Z

waku/v2/protocol/waku_peer_exchange/protocol.nim

-    # handle peer exchange request
-    if rpc.request != PeerExchangeRequest():
+    # If we got a request (request field is not empty)
+    if rpc.request != default(PeerExchangeRequest):


Yep, I agree. But since the rpc doesn't allow us to add an error message, this would involve changing the px proto, changing the spec, and perhaps bumping the px version? All of this beyond the PR.

jm-clius

Agree with the general approach of:

decoupling peer exchange response from connecting to peers
what to do with peers in peer exchange response not being a concern of the protocol but the "application", but think we can have WakuNode be setup as a basic peer exchange client application: e.g. calling request once on init, similar to DNS lookup being done once when configured.

jm-clius · 2023-02-07T13:31:54Z

apps/wakunode2/wakunode2.nim

+    let pxPeersRes = await node.wakuPeerExchange.request(desiredOutDegree)
+    if pxPeersRes.isOk:
+      var record: enr.Record
+      var validPeers = 0
+      for pi in pxPeersRes.get().peerInfos:
+        if enr.fromBytes(record, pi.enr):
+          node.peerManager.addPeer(record.toRemotePeerInfo().get, WakuRelayCodec)
+          validPeers += 1
+      info "Retrieved peer info via peer exchange protocol", validPeers = validPeers
+    else:
+      warn "Failed to retrieve peer info via peer exchange protocol"


This is a bit weird for me. While I agree with removing this from the protocol module, why not into waku_node when set up as a peer exchange client? It may make sense during node init to call some function that sets up peer exchange as a client and for now does only a single request. Basically we init and start all other discovery mechanisms when initiating the WakuNode, so this belongs there IMO.

yes indeed, not sure what i was thinking. Addressed in 9083938

Added also a new test which also showcases how this is expected to be used.

nwaku/tests/v2/test_wakunode.nim

Lines 293 to 317 in 9083938

asyncTest "Function fetchPeerExchangePeers succesfully exchanges px peers":

let

node1 = WakuNode.new(generateKey(), ValidIpAddress.init("0.0.0.0"), Port(0))

node2 = WakuNode.new(generateKey(), ValidIpAddress.init("0.0.0.0"), Port(0))

# Start and mount peer exchange

await allFutures([node1.start(), node2.start()])

await allFutures([node1.mountPeerExchange(), node2.mountPeerExchange()])

# Mock that we discovered a node (to avoid running discv5)

var enr = enr.Record()

require enr.fromUri("enr:-Iu4QGNuTvNRulF3A4Kb9YHiIXLr0z_CpvWkWjWKU-o95zUPR_In02AWek4nsSk7G_-YDcaT4bDRPzt5JIWvFqkXSNcBgmlkgnY0gmlwhE0WsGeJc2VjcDI1NmsxoQKp9VzU2FAh7fwOwSpg1M_Ekz4zzl0Fpbg6po2ZwgVwQYN0Y3CC6mCFd2FrdTIB")

node2.wakuPeerExchange.enrCache.add(enr)

# Set node2 as service peer (default one) for px protocol

node1.peerManager.addServicePeer(node2.peerInfo.toRemotePeerInfo(), WakuPeerExchangeCodec)

# Request 1 peer from peer exchange protocol

await node1.fetchPeerExchangePeers(1)

# Check that the peer ended up in the peerstore

let rpInfo = enr.toRemotePeerInfo.get()

check:

node1.peerManager.peerStore.peers.anyIt(it.peerId == rpInfo.peerId)

node1.peerManager.peerStore.peers.anyIt(it.addrs == rpInfo.addrs)

waku/v2/protocol/waku_peer_exchange/protocol.nim

jm-clius

LGTM! Thanks. I'm still uneasy about the wakunode2 performing the API call, but I think this is clearly beyond the scope of this PR. :)

alrevuelta · 2023-02-09T07:25:36Z

@LNSD I addresses your comments (rfc update and proto updates) which are already merged, and replied. Is there anything left? Would like to merge this today unless you need more time.

LNSD · 2023-02-09T14:20:23Z

Please, check my comments on the default(...) comparison.

alrevuelta · 2023-02-09T15:19:14Z

@LNSD thanks, addresses the changes.

LNSD

I see some details that do not follow the agreed style guide, but you can merge it if you want ✅

alrevuelta force-pushed the refactor-px branch 3 times, most recently from c2ca40e to 3708e06 Compare February 2, 2023 16:04

alrevuelta mentioned this pull request Feb 2, 2023

Networking MVP: Refactor + extend functionality #1353

Closed

17 tasks

alrevuelta marked this pull request as ready for review February 2, 2023 16:21

alrevuelta mentioned this pull request Feb 2, 2023

chore(networking): decouple discovery, add peers and connect to them #1463

Closed

3 tasks

alrevuelta requested review from kaiserd, LNSD and jm-clius and removed request for LNSD February 2, 2023 16:23

alrevuelta force-pushed the refactor-px branch from 3708e06 to 1233fa9 Compare February 6, 2023 13:43

LNSD suggested changes Feb 6, 2023

View reviewed changes

alrevuelta commented Feb 7, 2023

View reviewed changes

jm-clius requested changes Feb 7, 2023

View reviewed changes

alrevuelta force-pushed the refactor-px branch from 19bab05 to 9083938 Compare February 8, 2023 09:11

alrevuelta requested review from jm-clius and LNSD February 8, 2023 09:15

jm-clius approved these changes Feb 8, 2023

View reviewed changes

alrevuelta added 5 commits February 9, 2023 08:55

refactor(px): refactor px + add tests

059d744

refactor(px): add tests and handle error gracefully

8bf3832

refactoro(px): minor improvements and cleanups

d6cdfbd

add new test case

e5e4398

refactor(px): move logic to waku_node + test

d33294d

alrevuelta force-pushed the refactor-px branch from 9083938 to d33294d Compare February 9, 2023 08:02

Fix issue where ok enrs were parsed as nok

1af3a0d

Fix proto comments

7cb5433

LNSD approved these changes Feb 9, 2023

View reviewed changes

alrevuelta merged commit 9c3e5f5 into master Feb 9, 2023

alrevuelta deleted the refactor-px branch February 9, 2023 15:59

chaitanyaprem mentioned this pull request Aug 17, 2023

Introduce Peer Management waku-org/go-waku#594

Closed

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(px): refactor peer exchange + tests #1527

refactor(px): refactor peer exchange + tests #1527

alrevuelta commented Feb 1, 2023 •

edited

Loading

status-im-auto commented Feb 1, 2023 •

edited

Loading

LNSD left a comment

LNSD Feb 6, 2023

alrevuelta Feb 7, 2023

LNSD Feb 9, 2023 •

edited

Loading

alrevuelta Feb 9, 2023

jm-clius Feb 9, 2023

alrevuelta left a comment •

edited

Loading

alrevuelta Feb 7, 2023

jm-clius left a comment

jm-clius Feb 7, 2023

alrevuelta Feb 8, 2023

jm-clius left a comment

alrevuelta commented Feb 9, 2023

LNSD commented Feb 9, 2023

alrevuelta commented Feb 9, 2023

LNSD left a comment

	asyncTest "Function fetchPeerExchangePeers succesfully exchanges px peers":
	let
	node1 = WakuNode.new(generateKey(), ValidIpAddress.init("0.0.0.0"), Port(0))
	node2 = WakuNode.new(generateKey(), ValidIpAddress.init("0.0.0.0"), Port(0))

	# Start and mount peer exchange
	await allFutures([node1.start(), node2.start()])
	await allFutures([node1.mountPeerExchange(), node2.mountPeerExchange()])

	# Mock that we discovered a node (to avoid running discv5)
	var enr = enr.Record()
	require enr.fromUri("enr:-Iu4QGNuTvNRulF3A4Kb9YHiIXLr0z_CpvWkWjWKU-o95zUPR_In02AWek4nsSk7G_-YDcaT4bDRPzt5JIWvFqkXSNcBgmlkgnY0gmlwhE0WsGeJc2VjcDI1NmsxoQKp9VzU2FAh7fwOwSpg1M_Ekz4zzl0Fpbg6po2ZwgVwQYN0Y3CC6mCFd2FrdTIB")
	node2.wakuPeerExchange.enrCache.add(enr)

	# Set node2 as service peer (default one) for px protocol
	node1.peerManager.addServicePeer(node2.peerInfo.toRemotePeerInfo(), WakuPeerExchangeCodec)

	# Request 1 peer from peer exchange protocol
	await node1.fetchPeerExchangePeers(1)

	# Check that the peer ended up in the peerstore
	let rpInfo = enr.toRemotePeerInfo.get()
	check:
	node1.peerManager.peerStore.peers.anyIt(it.peerId == rpInfo.peerId)
	node1.peerManager.peerStore.peers.anyIt(it.addrs == rpInfo.addrs)

refactor(px): refactor peer exchange + tests #1527

refactor(px): refactor peer exchange + tests #1527

Conversation

alrevuelta commented Feb 1, 2023 • edited Loading

End to end test

status-im-auto commented Feb 1, 2023 • edited Loading

Jenkins Builds

LNSD left a comment

Choose a reason for hiding this comment

LNSD Feb 6, 2023

Choose a reason for hiding this comment

alrevuelta Feb 7, 2023

Choose a reason for hiding this comment

LNSD Feb 9, 2023 • edited Loading

Choose a reason for hiding this comment

alrevuelta Feb 9, 2023

Choose a reason for hiding this comment

jm-clius Feb 9, 2023

Choose a reason for hiding this comment

alrevuelta left a comment • edited Loading

Choose a reason for hiding this comment

alrevuelta Feb 7, 2023

Choose a reason for hiding this comment

jm-clius left a comment

Choose a reason for hiding this comment

jm-clius Feb 7, 2023

Choose a reason for hiding this comment

alrevuelta Feb 8, 2023

Choose a reason for hiding this comment

jm-clius left a comment

Choose a reason for hiding this comment

alrevuelta commented Feb 9, 2023

LNSD commented Feb 9, 2023

alrevuelta commented Feb 9, 2023

LNSD left a comment

Choose a reason for hiding this comment

alrevuelta commented Feb 1, 2023 •

edited

Loading

status-im-auto commented Feb 1, 2023 •

edited

Loading

LNSD Feb 9, 2023 •

edited

Loading

alrevuelta left a comment •

edited

Loading