bitcoinj icon indicating copy to clipboard operation
bitcoinj copied to clipboard

Modernize peer communication network stack?

Open schildbach opened this issue 3 years ago • 6 comments

This is meant for discussion if and how we want to modernize our network stack (likely after 0.17).

Once upon a time, we used Netty and had only blocking I/O. Then, Matt Corallo came to the rescue and wrote our own stack, tailored to our needs of the time. Now we have blocking I/O and non-blocking I/O. However, this code is unmaintained and especially the integration-tests that use it (subclasses of TestWithNetworkConnections) are a bloody mess.

I have two questions to start the discussion:

  1. Do we still need blocking I/O? I heard it's needed for Tor, but I don't understand the details.
    • If it was only Orchid (which we removed) was not supporting NIO, then why not find a NIO-capable Tor client – if we want to return to natively supporting Tor connections at all.
    • Is it needed for communicating with (Tor) socket proxies?
  2. Should we return to using a library for the network layer?
    • Which one? Sometimes I'm eying at OkIo (used by the excellent OkHttp and Moshi) – it seems to be lightweight and extends on what the JDK already provides. Even Netty boasts itself with "asynchronous event-driven network application framework" these days.
    • If we rewrite our own or retrofit the one we have, how are we going to maintain it?

schildbach avatar Mar 11 '23 09:03 schildbach

One thing we should probably clarify is that by "our network stack" what we really mean is our library for Peer networking. This is the bulk of networking code in bitcoinj because of bitcoinj's historical focus on SPV wallet support. (There is also the o.b.net.discovery package which is used to find peer addresses, but does not actually talk to peers.)

I've also thought that as part of our refactoring-to-modules plan (see Issue #1874) we might at some point in the future consider a peer module that would contain Peer, PeerGroup, all the peer messages, the current net package, and other related functionality. (Alternatively, I suppose you could argue that is what the core module is.)

In 0.17 we are moving from ListenableFuture to CompletableFuture and I think this is an important first step in modernizing peer networking. Post 0.17 we could move to improve and simplify the current interfaces in the o.b.net module.

msgilligan avatar Mar 11 '23 17:03 msgilligan

To answer your two questions:

  1. Do we still need blocking I/O? I heard it's needed for Tor, but I don't understand the details.

I'm a little weak on the details, too. My big picture understanding is that the main reason we have not removed the blocking I/O implementation is because currently that is the only implementation that provides Proxy support. Proxy support is needed for Tor and for clients inside firewalls that require use of a proxy, etc.

  1. Should we return to using a library for the network layer?

I think we should create an (SPI-style, similar to what we're talking about for lower-level crypto) interface for our networking layer (probably using CompletableFuture) and then create 1 or more implementations of that interface using well-supported libraries. My recommendation would be two implementations:

  1. Okio: This is a well-regarded library that supports JDK 8+ and Android.
  2. java.net.http: This is also a well-regarded (and I have positive experience with it) but it requires JDK 11+ and I do not think it is available on Android.

Both Okio and java.net.http support proxies.

I think the hard part would be refactoring to the right interface. Once that work is done, I would expect writing both the java.net.http and Okio implementations would not be that difficult. Depending upon my future availability, I'd be interested in writing the java.net.http implementation. (But I'd be happy of course if someone else stepped up and delivered it!)

msgilligan avatar Mar 11 '23 17:03 msgilligan

…we have not removed the blocking I/O implementation is because currently that is the only implementation that provides Proxy support.

Does that mean we could implement proxy support for NIO, them remove the blocking I/O implementation?

I think we should create an (SPI-style, similar to what we're talking about for lower-level crypto) interface for our networking layer (probably using CompletableFuture) and then create 1 or more implementations of that interface using well-supported libraries.

Sound like a good plan!

java.net.http

Does this support plain (non-HTTP) socket I/O?

I'd be interested in writing the java.net.http implementation.

Thanks for the offer! I'd be interested in writing the Okio impl.

But as you said, finding the right interface is probably the hardest part. Though we could perhaps take a sneak peek at other projects…

schildbach avatar Mar 11 '23 22:03 schildbach

Does that mean we could implement proxy support for NIO, them remove the blocking I/O implementation?

I don't know. We'd have to look into it.

create an (SPI-style) interface for our networking layer … 1 or more implementations of that interface

Sound like a good plan!

java.net.http

Does this support plain (non-HTTP) socket I/O?

It doesn't. Oops, my (stupid) mistake.

I had looked into this before (PR #2101) without making that mistake.

I'd be interested in writing [a JDK-only] implementation.

Corrected, above.

msgilligan avatar Mar 13 '23 00:03 msgilligan

If we want to iterate on what we already have, perhaps we could follow this strategy:

  1. Rewrite NioClient(Manager) to use AsynchronousSocketChannel internally, or write a new implementation.
  2. Find or write an AsynchronousSocketChannel wrapper that implements the SOCKS5 protocol.
  3. Make sure all client code and tests uses the new NioClient.
  4. Remove BlockingClient(Manager) and the old NioClient (if it still exists) – this step perhaps after a deprecation cycle.
  5. We can then go wild on the abstraction, which currently seems to be based mainly on ClientConnectionManager, StreamConnection and MessageWriteTarget.

schildbach avatar Mar 13 '23 15:03 schildbach