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

Add clearer warning when failing to resolve seedNodes via DNS #752

Open
tegefaulkes opened this issue Jun 28, 2024 · 6 comments
Open

Add clearer warning when failing to resolve seedNodes via DNS #752

tegefaulkes opened this issue Jun 28, 2024 · 6 comments
Assignees
Labels
development Standard development r&d:polykey:core activity 4 End to End Networking behind Consumer NAT Devices

Comments

@tegefaulkes
Copy link
Contributor

Specification

During testing we found the warning WARN:polykey:No seednodes could be found for for mainnet.polykey.com This comes from resolveSeednodes src/nodes/utils.ts:427.

cmcdragonkai ➜ matrix-framework-13-ryzen-7040  ➜ ~/Projects/Polykey-CLI
 $ node ./dist/polykey.js agent start
✔ Please enter the password … ******
WARN:polykey:No seednodes could be found for for mainnet.polykey.com
(node:103397) [DEP0112] DeprecationWarning: Socket.prototype._handle is deprecated
(Use `node --trace-deprecation ...` to show where the warning was created)
pid           103397
nodeId        vcj3ufun2506jn44q7rr9bc1hv4c6eebicusfj1bl9sr27ookknlg
clientHost    ::1
clientPort    39329
agentHost     ::
agentPort     43009
WARN:polykey.PolykeyAgent.task v0pjgubeia5o0182k4d99n8vep4:Failed - Reason: ErrorUtilsUndefinedBehaviour

This was a result of the tailscale magic DNS acting up and preventing the ability to resolve the seed node addresses via DNS. As far as the behaviour goes, this is working as intended. However the messaging can be improved.

We need to be clearer with the following details

  1. The failure was due to DNS failing to resolve.
  2. Failing to resolve any seed nodes without proving a list of the directly with the seedNodes error will cause the syncNodeGraph logic to skip entirely. So we wont get any warnings about a failure at that stage.
  3. Connections can still be made automatically using details in the NodeGraph. These will trigger in the background.

We can also look into actively using the nodeGraph as a fallback for entering the network. We can use either the closest nodes to us as the starting set, Nodes we've succeeded via direct connections in the past. Or keep better track of seed nodes in the graph and use them. Needs some discussion.

Tasks

  1. Modify the warning to be much clearer why things failed. Include details about the error that caused it.
  2. Add an info message that shares details when resolving via DNS succeeds.
  3. Look into using the nodeGraph entries as a fallback seednode list if resolving fails.
@tegefaulkes tegefaulkes added the development Standard development label Jun 28, 2024
Copy link

linear bot commented Jun 28, 2024

@tegefaulkes
Copy link
Contributor Author

@aryanjassal when you feel more comfortable with the source code this might be a good issue to start with.

Copy link
Contributor

I want to clarify the requirements of this issue.

For the first task, I need to include additional details, like a custom error message stating that DNS resolution failed, correct?

For the second task, success of this function would happen just before the return statement in the function, right? If that is the case, then I just have to send a message stating that node resolution using DNS succeeded, right?

I'm also not sure how to approach the task of using nodeGraph entries as fallback seednode list if resolving fails.

@tegefaulkes
Copy link
Contributor Author

  1. Right now when resolving DNS fails, it just returns no results. From the outside this looks like a failure to sync the node graph. But there is few ways that can fail with DNS being one of them. So the message No seednodes could be found for for mainnet.polykey.com is too vague here. It needs to be clear that we failed to resolve them by DNS with a descriptive message. Just to distinguish it from say, failing to connect to any seed nodes.
  2. Yes, so inside resolveSeednodes function we need to call logger.info with a message stating that resolving the seed nodes succeeded with some details about it. But it's a utility function and we don't usually pass a logger into it. So the info message will likely need to be done where resolveSeednodes is called.
  3. If DNS resolution fails then we don't end up with a list of addresses to connect to. Check the node graph for either records relating to the nodes we wanted to resolve in the first place. Or get a list of nodes to use instead of seed nodes. I don't know if the node graph tracks if a node had a public IP but if it does then prefer those.

Don't get too stuck into this if you're struggling. The main goal is to get a top level overview of Polykey's structure for now. It will take too long to get intimate knowledge of all it's parts for now.

@CMCDragonkai CMCDragonkai added the r&d:polykey:core activity 4 End to End Networking behind Consumer NAT Devices label Aug 15, 2024
@CMCDragonkai
Copy link
Member

Related to #729?

@tegefaulkes
Copy link
Contributor Author

Yeah, this is related but on top of the warning this issue wants to add using the NodeGraph as a fallback for entering the network f we fail to resolve the seed nodes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
development Standard development r&d:polykey:core activity 4 End to End Networking behind Consumer NAT Devices
Development

No branches or pull requests

3 participants