libp2p / jvm-libp2p

a libp2p implementation for the JVM, written in Kotlin 🔥
https://libp2p.io
Apache License 2.0
260 stars 74 forks source link

circuit-relay-v2 #344

Closed ianopolous closed 7 months ago

ianopolous commented 9 months ago

Hi @Nashatyrev et al,

I'm working on an implementation of circuit-relay-v2 in another repo and would like to upstream it, if you are interested, when it is working. Is this something you'd like upstreamed?

I've got the relay working, and relaying connections. I'm just working on adding the security and muxer upgrade to relayed streams. I've implemented a new RelayTransport and made sure TcpTransport won't try and handle circuit addresses.

What are you thoughts on the direction of this: https://github.com/Peergos/nabu/compare/feat/circuit-relay-v2?expand=1

The important bit is HostBuilder::upgradeConnection for handling incoming relayed connections. Once this is done I'll work on dcutr.

Nashatyrev commented 9 months ago

Hey @ianopolous

What are you thoughts on the direction of this: https://github.com/Peergos/nabu/compare/feat/circuit-relay-v2?expand=1

That doesn't look like a lot of changes to the core classes there. I believe it's worth to upstream. @StefanBratanov what do you think?

ianopolous commented 9 months ago

@Nashatyrev It's looking like I might need more upstream changes than I was hoping. Any suggestions for how to get RelayTransport::upgradeStream working when we have a Stream not a Channel? It seems we fundamentally need a channel to set the remote peerid on for the security negotiation?

StefanBratanov commented 9 months ago

Hey @ianopolous

What are you thoughts on the direction of this: https://github.com/Peergos/nabu/compare/feat/circuit-relay-v2?expand=1

That doesn't look like a lot of changes to the core classes there. I believe it's worth to upstream. @StefanBratanov what do you think?

I think it's good addition especially since there is already a project which would require it and most of the code is already there.

ianopolous commented 9 months ago

@Nashatyrev I've got the code working upstream in jvm-libp2p, including listening in the transport itself here: https://github.com/libp2p/jvm-libp2p/compare/develop...Peergos:jvm-libp2p:feat/circuit-relay-v2?expand=1 However, the upgrade on the stream is still not working. Any help would be much appreciated.

It seems like the channelActive() of the Negotiator for the security protocol is never called. I've tried calling fireChannelActive in various places but no luck.

Nashatyrev commented 8 months ago

@ianopolous, sorry for delay again, I'm OOO currently I'll try to look into this in a week or so

Nashatyrev commented 8 months ago

Yeah, I believe it's expected that channelActive event is called on the first connection handler. Since we upgrade already existing and active netty channel ctx.fireChannelActive() needs to be called manually in the right place. The 'right place' should be somewhere after you removed relay protocol handlers and called upgradeStream().

BTW couldn't run the relay test due to io.libp2p.security.InvalidRemotePubKey

    Multiaddr relayAddr =
        new Multiaddr(
            "/ip4/23.95.209.108/tcp/4001/p2p/12D3KooWMjxFAyMR2STsgr8Qoxhg27K64uuBL7UZ8gDkdjEnEeya");

Is it some public relay node? Probably the node has changed its key? May be you could update the test to make it self-contained?

ianopolous commented 8 months ago

My hope is to test with a local relay once it's working. But wanted to rule out issues with the relay first. The normal way to find a relay is to try a bunch of nodes you've discovered, as it is on by default normally. We do this downstream in Nabu using our kademlia impl.

ianopolous commented 8 months ago

I've changed the test relay to another random node.

ianopolous commented 8 months ago

I've tried calling channel.pipeline().fireChannelActive() in a bunch of places including in upgradeStream after calling upgrader.establishSecureChannel(). None of it seems to have any effect.

ianopolous commented 8 months ago

I've added a local relay test which hits the same issue now.

Nashatyrev commented 8 months ago

Thanks for adding the standalone test 👍 I can see now that 'old' handlers are not removed from the stream. You need to remove the handlers processing the circuit protocol messages to make the stream channel raw again. After removing old handlers and adding new handlers from upgrader calling fireChannelActive() should work I believe

Nashatyrev commented 8 months ago

Just leave the circuit-v2 protocol spec bookmark here: https://github.com/libp2p/specs/blob/master/relay/circuit-v2.md

ianopolous commented 8 months ago

Thank you @Nashatyrev that did the trick. It's now working with a remote relay. Local relay is still broken though.

ianopolous commented 8 months ago

Okay it's working over a local relay now too. Any suggestions before I PR, @Nashatyrev ?