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

fix(rpc): rpc crashes when a peer has no addresses #1507

Merged
merged 1 commit into from
Jan 26, 2023

Conversation

alrevuelta
Copy link
Contributor

@alrevuelta alrevuelta commented Jan 25, 2023

Closes #1505

Summary:

  • get_waku_v2_admin_v1_peers was crashing the node when a given peer contained no multiaddress ([0] out of bound)
  • Add test case reproducing the error.
  • Fix by checking the size to avoid accesing [0] in an empty seq.

@status-im-auto
Copy link
Collaborator

Jenkins Builds

Commit #️⃣ Finished (UTC) Duration Platform Result
e728032 #1 2023-01-25 23:05:21 ~18 min macos 📄log

Copy link
Contributor

@jm-clius jm-clius left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This API definitely needs a revision and dates back from when we had much cruder peer management. However, I'd split the API change from the necessary fix for crashing with no multiaddrs. Without changing the API (i.e. WakuPeer still only has a single protocol field), we could assume that all peers in the peerStore are WakuRelayCodec peers and that the peers in the service slots (post #1473) are for the protocols specified there even if we repeat them in the output.

Reasoning: the API change will require an RFC update, informing clients and changing some infra scripts, whereas we could first fix the bug.

Comment on lines 67 to 73
let connectedPeers = node.peerManager.peerStore.peers.filterIt(it.connectedness == Connectedness.Connected)
wPeers.insert(connectedPeers.mapIt(WakuPeer(multiaddr: constructMultiaddrStr(it),
identifyProtocols: it.protos,
connectedProtocol: "TODO",
connected: true)))

let notConnectedPeers = node.peerManager.peerStore.peers.filterIt(it.connectedness != Connectedness.Connected)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For interest's sake, why would this need to be split into two steps and not just map all peers from the peerStore.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yep indeed thanks!

@@ -29,7 +29,8 @@ type

WakuPeer* = object
multiaddr*: string
protocol*: string
identifyProtocols*: seq[string]
connectedProtocol*: string
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Presumably some peers could be connected for more than one protocol or would you return those peers twice?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right now we only allow 1 max connection per peer. So shouldnt be the case.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't we have multiple protocol streams per connection? How do Status go-waku nodes for example connect to fleet nodes as their main bootstrap relay connection, but use those same nodes for store, filter and lightpush (presumably without disconnecting each time)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mm yeah good point. guess they are multiplexed under the same connection so connectedProtocol didnt really made sense. should be more like []protocolStreams idk. But note that with the current public method/variables in nim-libp2p we can't really get that afaik.

@alrevuelta alrevuelta marked this pull request as ready for review January 26, 2023 09:47
@alrevuelta alrevuelta requested a review from jm-clius January 26, 2023 09:47
Copy link
Contributor

@jm-clius jm-clius left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@alrevuelta alrevuelta merged commit 93a07ba into master Jan 26, 2023
@alrevuelta alrevuelta deleted the fix-rpc-crash-peers branch January 26, 2023 11:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

bug: calling get_waku_v2_admin_v1_peers crashes nwaku
3 participants