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

Review from Thomas Eizinger #97

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Review from Thomas Eizinger #97

wants to merge 1 commit into from

Conversation

Wiezzel
Copy link
Contributor

@Wiezzel Wiezzel commented Apr 22, 2024

No description provided.

@thomaseizinger
Copy link
Collaborator

Thanks! I'll try to answer questions regarding my comments, just tag me!

Copy link
Contributor Author

@Wiezzel Wiezzel left a comment

Choose a reason for hiding this comment

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

@thomaseizinger
I've added a bunch of follow-up questions/comments.

I also have one extra question that I'm struggling with: I have observed that quite often the DHT lookups for peers fail, but when I try to look up the same peerID by running libp2p-lookup, it succeeds. What could be the reason??

transport/src/transport.rs Show resolved Hide resolved
transport/src/bootnode.rs Show resolved Hide resolved
transport/src/transport.rs Show resolved Hide resolved
transport/src/transport.rs Show resolved Hide resolved
transport/src/transport.rs Show resolved Hide resolved
transport/src/transport.rs Show resolved Hide resolved
transport/src/transport.rs Show resolved Hide resolved
@thomaseizinger
Copy link
Collaborator

I also have one extra question that I'm struggling with: I have observed that quite often the DHT lookups for peers fail, but when I try to look up the same peerID by running libp2p-lookup, it succeeds. What could be the reason??

Is this in your own DHT or in the IPFS one? Like I commented, I would strongly recommend moving off the IPFS DHT. There are proposals for making it universally useful but until then, it would be better to create your own DHT.

@Wiezzel
Copy link
Contributor Author

Wiezzel commented Apr 24, 2024

@thomaseizinger

Is this in your own DHT or in the IPFS one? Like I commented, I would strongly recommend moving off the IPFS DHT. There are proposals for making it universally useful but until then, it would be better to create your own DHT.

Our network is isolated, so I believe it works as our own DHT, despite using the same protocol ID string.

@thomaseizinger
Copy link
Collaborator

@thomaseizinger

Is this in your own DHT or in the IPFS one? Like I commented, I would strongly recommend moving off the IPFS DHT. There are proposals for making it universally useful but until then, it would be better to create your own DHT.

Our network is isolated, so I believe it works as our own DHT, despite using the same protocol ID string.

Any logs on the nodes where the lookup fails?

@Wiezzel
Copy link
Contributor Author

Wiezzel commented Apr 25, 2024

@thomaseizinger

Is this in your own DHT or in the IPFS one? Like I commented, I would strongly recommend moving off the IPFS DHT. There are proposals for making it universally useful but until then, it would be better to create your own DHT.

Our network is isolated, so I believe it works as our own DHT, despite using the same protocol ID string.

Any logs on the nodes where the lookup fails?

Well, I just get the Peer 12D3Koo... not found from this line. But I can run it with debug-level logs from libp2p_kad to see more. I was also thinking it might be related to libp2p/rust-libp2p#5270. 🤔

@Wiezzel
Copy link
Contributor Author

Wiezzel commented Apr 25, 2024

@thomaseizinger One more architecture question:
Let's say I want to implement a behaviour that works like request-response, but it waits until the node address is known before making the dial attempt. I would include kademlia inside and start a lookup if there are no known address of a peer. But what if I'd like to have multiple instances of this behaviour (for different message protocols)? How to do this without having multiple kademlia instances? Could this behaviour communicate with kademlia using swarm events somehow? Or should I use a shared reference or a channel to pass calls to kademlia? What would be the cleanest solution in your opinion?

EDIT: Or maybe I shouldn't even use multiple request-response behaviours, but a single one with multiple protocols supported? Though the downside of this approach seems to be that all protocols would need to share the same response and return and response type, so I cannot have a separate enum per each protocol.

@thomaseizinger
Copy link
Collaborator

@thomaseizinger One more architecture question: Let's say I want to implement a behaviour that works like request-response, but it waits until the node address is known before making the dial attempt. I would include kademlia inside and start a lookup if there are no known address of a peer. But what if I'd like to have multiple instances of this behaviour (for different message protocols)? How to do this without having multiple kademlia instances? Could this behaviour communicate with kademlia using swarm events somehow? Or should I use a shared reference or a channel to pass calls to kademlia? What would be the cleanest solution in your opinion?

EDIT: Or maybe I shouldn't even use multiple request-response behaviours, but a single one with multiple protocols supported? Though the downside of this approach seems to be that all protocols would need to share the same response and return and response type, so I cannot have a separate enum per each protocol.

I wouldn't move kademlia inside. Instead I would:

  • Make a wrapping behaviour that:
    • first tries to send the message (which will attempt a dial and use all addresses that we know)
    • if it fails, queue the message and emit an event that we need to search for a peer
    • sends the message once the connection is established
    • has a timeout if a msg is queued for too long
  • use that event in the eventloop to trigger a kademlia lookup
  • Dial every node you find with kademlia

The result will be that a regardless of how the connection will be established, queued messages are always sent and your behaviour is independent of the discovery mechanism.

Does that work for you?

@Wiezzel
Copy link
Contributor Author

Wiezzel commented Apr 26, 2024

@thomaseizinger

Does that work for you?

Yeah, thank you.

Wiezzel added a commit that referenced this pull request May 8, 2024
* Apply code review suggestions from #97
* Transport refactor
* Feature flags for different actors
* Add peer_id to WorkerLogs, update rust version
* Ensure message signing
* Make libp2p metrics static
* Worker adjustments
* Fix logs_collected and dockerfile
* Revert keypair encoding change (not backwards compatible)
* Fix Wrapped::poll() + some trace logs
* Fix backwards compatibility issues
* Fix legacy messages
* Fix handling legacy workers in scheduler
* Add missing function in gateway
* Legacy gateway logs handling
* Observer actor
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.

2 participants