search

libp2p / jvm-libp2p

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

Upstream new protocols (Yamux, TLS, Quic) #272

Open ianopolous opened 1 year ago

ianopolous commented 1 year ago

Hi we're hoping to upstream a bunch of new protocols we've implemented based on this. Specifically Yamux, TLS, multiaddr fixes and soon Quic. We also have Kademlia and bitswap in Java on top of this here: https://github.com/Peergos/nabu These are all tested against kubo for interop.

The only potential difficulties we see are

Is there interest in up-streaming our work? What are your thoughts?

The fork is https://github.com/peergos/jvm-libp2p

mxinden commented 1 year ago

Friendly ping @Nashatyrev.

Nashatyrev commented 1 year ago

Hey, that sounds exciting! However I believe that should be discussed more closely with the Teku team, since the changes could potentially impose the risk of regression bugs in the Teku networking layer.

In the meanwhile let me ask you some initial questions on the items you listed:

our removal of a bunch of dependencies like guava, log4j, apache commons

What's the reasons behind removing those dependencies? Are you using other logging API? (I think I've seen Java Logging?) Don't you have plans to switch?

our minimum Java 17 requirement (required for Ed25519 TLS handling)

Teku is still on Java 11, so I'm not sure how we could handle this at the moment. As an option: postpone TLS module until Teku upgrade?

the ~1 commit after we forked where this repo was refactored and a lot of stuff was moved around.

Are you referring to #267 ? I believe it shouldn't be too problematic to adopt, since there were mostly structural changes and the code was not affected mostly

ianopolous commented 1 year ago

Thanks @Nashatyrev , yep that makes sense to discuss with Teku.

We try to minimise dependencies for security and maintainability reasons mainly, but also compiled jar size. For logging in particular we've never needed log4j in various companies over the years and always manage with the built-in logging.

I'd love to remove the Java 17 requirement too. It might be possible with some vendoring of bouncy castle components. Quic also depends on TLS though, so that would also have to be post-poned.

Good to hear #267 shouldn't be difficult to rebase off.

Nashatyrev commented 1 year ago

our removal of a bunch of dependencies like guava, log4j, apache commons

ianopolous commented 1 year ago

Yep we're happy to include slf4j if that helps.

ajsutton commented 1 year ago

For the record, log4j-api works just like slf4j these days and can work with multiple backends, not just log4j (including funnily enough slf4j from memory). It's more common to use slf4j for that kind of thing though so not unreasonable to switch to that. Teku already has some dependencies that use slf4j so needs to ensure it has a mapping to log4j in place anyway.

StefanBratanov commented 1 year ago

Hi @ianopolous , we (Teku) will migrate to Java 17 in a couple of releases, so that shouldn't be a problem. Wonder if it is an idea to implement the changes on a branch (v1.x for example) and potentially release 1.0.0 and then we can start testing with it and when happy can merge these changes to develop and move from there. It is unlikely we would do any significant changes in the meantime to the library. What do you think @Nashatyrev ?

Nashatyrev commented 1 year ago

Wonder if it is an idea to implement the changes on a branch (v1.x for example)

@StefanBratanov , sounds like a good plan 👍 And it would be indeed a good candidate for version 1.0.0

Just a note regarding Java 17. I believe we would also like to keep some compatibility with Android. jvm-libp2p have android-chatter example (which is not yet included to the build #270) which is presumably functional. I'll try to sort it out. CCing @raulk who was the primary person interested in Android Libp2p

Nashatyrev commented 1 year ago

Related PR #265 which was closed Also the previous discussion is here: #262

ianopolous commented 1 year ago

Cool, so it sounds like we've got the green light to rebase, add slf4j and submit a PR, and then we can discuss the details there?

Nashatyrev commented 1 year ago

Ok, I've created new branch as discussed : https://github.com/libp2p/jvm-libp2p/tree/v1.0.0 The first PR here is including Android sample as promised earlier: #275

I'm looking forward to integrate all the changes step-by-step with minimal atomic PRs. I would suggest to start with the following sequence:

Nashatyrev commented 1 year ago

@ianopolous just found out that you have excluded guava dependency but copied some of its classes to the project. What's the point of this?

ianopolous commented 1 year ago

We care about binary size. So this is ~3MiB saving. They are mostly random util classes.

Nashatyrev commented 1 year ago

Got it. Didn't you consider using kind of 'minifiers' for that? As far as know they are able to safely exclude unused classes from the distribution. I would then suggest to postpone that decision because I'm not really sure this is the solution we would like to stick with.

Nashatyrev commented 1 year ago

BTW I've managed to remove Apache commons and use Base32 from bouncycastle: #277

ianopolous commented 1 year ago

Nope but we can probably use one of those.

ianopolous commented 1 year ago

I think we used the Base32 from java-multibase which is itself a vendored version of apache commons.

ianopolous commented 1 year ago

Ok, how does this sound:

~PR 1: switch to slf4j~ ~PR 2: multiaddr fixes - https://github.com/libp2p/jvm-libp2p/pull/280~ ~PR 3: yamux - https://github.com/libp2p/jvm-libp2p/pull/281~ PR 4: TLS + early muxer negotiation - https://github.com/libp2p/jvm-libp2p/pull/283 PR 5: WIP quic transport (and security and muxer)

We can do the jar minifier downstream.

Nashatyrev commented 1 year ago

@ianopolous Sounds good! I've already prepared Slf4j changes as well :) There is a queue of PRs to v1.0.0 at the moment. Calling @StefanBratanov for review help

StefanBratanov commented 1 year ago

@ianopolous Sounds good! I've already prepared Slf4j changes as well :) There is a queue of PRs to v1.0.0 at the moment. Calling @StefanBratanov for review help

Hi @Nashatyrev approved your PRs

Nashatyrev commented 1 year ago

Ok, preparatory PRs are all merged. Sorry again for Slf4j mess

ianopolous commented 1 year ago

TLS PR is ready for review: https://github.com/libp2p/jvm-libp2p/pull/283

Nashatyrev commented 1 year ago

@ianopolous could you please review #285 . Yamux cases in particular.

ianopolous commented 1 year ago

All that's left now that TLS is merged is:

Nashatyrev commented 1 year ago

@ianopolous I will be on vacation for the next week, but then I would like to sort out the EventLoop need. More details on that are highly appreciated.

If you think it makes sense we may connect on Telegram: https://t.me/nashatyrev_a

ianopolous commented 1 year ago

Let's upgrade to email (I don't have telegram), and we can upgrade from there. I'm ian at peergos dot org.

ianopolous commented 1 year ago

There are two more PRs to fix a yamux and a TLS regression:

Nashatyrev commented 1 year ago

@ianopolous I'm updating README for v1.0.0 here https://github.com/libp2p/jvm-libp2p/pull/301 Should we add your project in the section 'Notable users'?

ianopolous commented 1 year ago

Sure, that'd be great, you can add https://github.com/peergos/nabu and https:///github.com/peergos/peergos if you want.

ianopolous commented 1 year ago

We also have implementations of kademlia, bitswap, autonat, circuit-relay-v2 (WIP) in Nabu.