lightning-net crate

[MaxFangX]

lightning-net

Motivation

@philip9 and I are working on an initiative to compile and run LDK within an SGX enclave using the Fortanix Enclave Development Platform (EDP). Among the many additional constraints imposed when trying to run Rust code inside SGX is that we cannot use Tokio, specifically none of the net features that are relied upon in the current Tokio-based implementation of the rust-lightning network stack lightning-net-tokio because those features rely on mio which in turn relies on epoll-like syscalls which are not available in the x86_64-fortanix-unknown-sgx (EDP) compilation target.

Description

This PR implements a new crate called lightning-net which essentially reimplements the functionality of lightning-net-tokio but without using Tokio, futures, async Rust, or anything else that will not compile to the x86_64-fortanix-unknown-sgx target.

As the code currently stands, the ldk-sample node could easily swap out all usages of lightning-net-tokio with their equivalent functions in lightning-net and function perfectly fine. I've tested this already in a private fork of ldk-sample running on testnet - let me know if you'd like me to make a public version so that you can play around with it as well.

Previous work

Prior to this pull request, @phlip9 and I iterated on this module on our own fork at lexe-tech/rust-lightning#1. We now feel it is ready for the next stage of review prior to being upstreamed.

Follow-on work

Currently, lightning-net does not yet implement any SGX-specific functionality. In particular, we need to implement an SGX-compatible socket that has a subset of the existing TcpStream features. Follow-on work will implement this concrete SgxTcpStream using either custom Fortanix Usercalls (or possibly via patches to Rust std), and modify the handle_connection() and initiate_outbound() functions to take in an impl of e.g. a new Socket trait. Since we are not yet settled on the final / best approach, and since this pull request is fairly beefy already at +1129 lines, we figure it would be best to upstream this code now and add in the generics / SGX functionality later.

Original commits prior to squash

Squashed commits in original PR at lexe-tech/rust-lightning#1, reviewed by @phlip9:

• Create empty lightning-net module
• SyncSocketDescriptor skeleton implementation
• Connection struct with read and write halves
• Implement shutdown() for TcpReader and TcpWriter
• Implement most of spawn_inbound_handler
• Add crossbeam and some associated channels
• Implement basic functionality of ConnectionWriter
• Pass a Arc into ConnectionReader
• Get a SyncSocketDescriptor into the ConnectionWriter
• Implement first half of pausing reads
• Renome write tx/rx to write_data tx/rx
• Rename ConnectionReader/Writer to Reader/Writer
• Create the Reader/Writer cmd channels
• Send ResumeRead command to the Reader
• Remove write_cmd, rename reader_cmd -> resume_read
• Reader can handle ResumeRead events
• Implement disconnecting from peer
• send_data() actually sends data now
• Allow send_data() to pause reads
• Get a Arc into Writer
• Refactor write_data tx/rx into writer_cmd tx/rx
• Give Reader/Writer a cmd tx for the other
• Implement all disconnecting except TcpDisconnectooor
• Remove the Arc<Mutex>
• Refactor Connection into setup()
• disconnect_socket() now calls into TcpDisconnectooor
• Get a SyncSocketDescriptor into Writer
• Call write_buffer_space_avail and socket_disconnected v2
• resume_read check should go before the early return
• Handle read() ErrorKind variants
• Implement handle_connection and initiate_outbound
• Finish writing doc comments, clean up
• Basic tests for lightning-net
• These is modeled exactly after the tests in lightning-net-tokio
• Update GitHub CI and dependabot for lightning-net
• Reduce the dependencies used
• Implementing PR review feedback from @phlip9
• Remove some comments about SGX
• Hide ID_COUNTER in a function, return JoinHandles
• Reader/Writer now send commands via the descriptor
• Extract do_recv(), do_try_recv(), do_read() fns v2
• Split WriteData command into its own channel v4
• Remove std::net limitations from EDP doc comment
• Implement ReaderState enum v2

[MaxFangX]

Looks like a bunch of stuff broke after rebasing, on it.

[codecov-commenter]

Codecov Report

Merging #1469 (05e9d34) into main (6592081) will decrease coverage by 0.16%.
The diff coverage is 70.48%.

@@            Coverage Diff             @@
##             main    #1469      +/-   ##
==========================================
- Coverage   90.92%   90.75%   -0.17%     
==========================================
  Files          75       78       +3     
  Lines       41842    42583     +741     
  Branches    41842    42583     +741     
==========================================
+ Hits        38043    38645     +602     
- Misses       3799     3938     +139     

Impacted Files	Coverage Δ
lightning/src/ln/msgs.rs	86.68% <66.66%> (+0.01%)	⬆️
lightning-net/src/lib.rs	70.49% <70.49%> (ø)
lightning-net-tokio/src/lib.rs	76.72% <100.00%> (+1.25%)	⬆️
lightning/src/util/test_utils.rs	77.96% <0.00%> (-4.88%)	⬇️
lightning/src/ln/peer_channel_encryptor.rs	94.73% <0.00%> (-0.25%)	⬇️
lightning/src/ln/functional_tests.rs	97.08% <0.00%> (-0.09%)	⬇️
lightning-invoice/src/utils.rs	96.77% <0.00%> (-0.08%)	⬇️
lightning/src/ln/channelmanager.rs	84.66% <0.00%> (-0.03%)	⬇️
lightning-invoice/src/lib.rs	87.39% <0.00%> (ø)
lightning/src/util/fairrwlock.rs	85.71% <0.00%> (ø)
... and 15 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6592081...05e9d34. Read the comment docs.

[TheBlueMatt]

Nice work! I think a good chunk of the complexity here could be cut back, especially the writer thread and some of the channels.

[TheBlueMatt]

rust-lightning should re-call with the remaining data if you only write part, I think you should be able to do the writer with no internal buffer at all. For that matter, I think you should be able to do the writer with no separate thread at all, either!

[MaxFangX]

I think you should be able to do the writer with no separate thread at all, either!

If I'm understanding you correctly, you mean to use TcpStream's set_nonblocking(true), and then do stream.write() directly within SyncSocketDescriptor::send_data()? That would work for the writer, but since the Reader and Writer share the same underlying TcpStream which is managed by the OS, this means that the reader would no longer be able to block on read(), and could only read in a busy loop. From the set_nonblocking() docs:

This will result in read, write, recv and send operations becoming nonblocking, i.e., immediately returning from their calls. If the IO operation is successful, Ok is returned and no further action is required. If the IO operation could not be completed and needs to be retried, an error with kind io::ErrorKind::WouldBlock is returned.

On Unix platforms, calling this method corresponds to calling fcntl FIONBIO. On Windows calling this method corresponds to calling ioctlsocket FIONBIO

[MaxFangX]

rust-lightning should re-call with the remaining data if you only write part

This requires send_data() to know immediately whether a call to write() wrote all of the bytes, some of the bytes, etc. But since we cannot use non-blocking writes due to the constraints on the Reader above, there needs to be a dedicated Writer thread that can block on write(). Which AFAICT necessitates a layer of indirection (i.e. the write_data channel) between send_data() and the Writer, but which unfortunately makes partial write()s undetectable to callers of send_data().

[MaxFangX]

I think you should be able to do the writer with no internal buffer at all

I could imagine making the write_data channel a zero-capacity channel, so that when the Writer recv()s data, it immediately turns around and passes the data into write(), instead of storing it in an internal buffer. But since send_data() cannot detect partial writes (as explored above), the Writer must continue trying to send the remaining data until it succeeds (or shuts down), since the call to send_data() reported to Rust-Lightning that the entire write was a success. And in order to do that, the remaining data must be stored in memory somewhere, thus the internal buffer.

I suppose that making write_data a zero-capacity channel would prevent the channel itself from behaving as a second buffer (where the first buffer is the Writer's self.buf). This eliminates one of the buffers, but still doesn't eliminate the Writer's internal buffer.

I don't see a way around these constraints - any suggestions? The constraints are subtle but have wide-reaching implications regarding the design of this crate.

[TheBlueMatt]

Can you not set the write timeout to 1 microsecond? I'm not 100% sure how the platform-specific bits there work, so we'd need to test it on win/linux/bsd but I'd imagine it may work.

[TheBlueMatt]

I'm also really curious if there's not a way to somehow money-patch in a select call, or at a minimum ask Fortanix if they'd be interested in a minimal select-like API. They write in their ABI doc that one of their goals is to "support network services" but without select I would suggest it really doesn't. Before we stick with all the overhead here maybe it makes sense to ask (okay, beg) them to add select?

[TheBlueMatt]

If you're already thinking of adding custom usercalls, I'm not sure about the approach of running the TCP socket handling inside the enclave - I wrote this above, but it may have gotten missed in all the messages

we could also consider running the actual net logic outside of the enclave - it doesn't change the security model to make the syscalls outside of the enclave, we just pass the stream of read/write_buffer_space_avail events and write calls over some IPC. I'd bet we could do all that with less total LoC :/

[MaxFangX]

To rethink this somewhat further, instead of jumping through hoops here, we could also consider running the actual net logic outside of the enclave - it doesn't change the security model to make the syscalls outside of the enclave, we just pass the stream of read/write_buffer_space_avail events and write calls over some IPC. I'd bet we could do all that with less total LoC :/

That's essentially what we will be doing once the usercalls are implemented: making the syscalls outside of the enclave. Maybe take a quick look at the EDP architecture (it's short)

[TheBlueMatt]

We could then think about (a) a nice and performant ldk-net crate here (b) a little "ldk-net <-> PeerManager" shim trait/API here, and then (c) you could hook that shim to actually pass the events through to the enclave, but other users could hook it directly up to the PeerManager.

[TheBlueMatt]

That's essentially what we will be doing once the usercalls are implemented: making the syscalls outside of the enclave. Maybe take a quick look at the EDP architecture (it's short)

Right, it was not at all clear to me from that doc if the usercall API was designed to let you add your own calls or if it was designed only to let you use their small number of syscalls.

[MaxFangX]

After the review by @TheBlueMatt, I think this module needs more comprehensive tests. A future contributor is likely not to be as familiar with all the constraints around threads blocking on recv, read, and write, and may easily introduce a bug that prevents a reader or writer thread from shutting down while reads are paused, the writer is writing, etc. I'll work on covering those cases next week.

[TheBlueMatt]

Before you go toooo far down the rabbit hole of adding a ton of tests and documentation maybe lets figure out if we can remove the channels (and second thread) entirely first. I'm pretty confident we can which should massively reduce the total code surface here.

[MaxFangX]

Please excuse the delay; I got my computer repaired during the last week while I was with family.

I still don't think the writer thread can be eliminated since there would be no way to call write_buffer_space_avail, but I did manage to replace all the crossbeam channels with shared state + condition variables. If the changes look good to you I can start writing some more comprehensive tests and docs @TheBlueMatt.

FYI I will be on a trip and have limited bandwidth to implement things from the 21st through the 29th, so no rush.

[TheBlueMatt]

I still don't think the writer thread can be eliminated since there would be no way to call write_buffer_space_avail

Right, I'm still somewhat curious as to the exact SGX programming environment here - are you able to define your own "usercall"s, and if so, can we define a select usercall? Alternatively, does it make more sense to run the network stack directly in normal user mode, with the in-SGX just handling a stream of read/write/writeable events? In either model we could run all the networking stuff for an arbitrary number of connections with one or two threads and then have the SGX stuff just pass the network bytes through to the PeerManager.

[MaxFangX]

Right, I'm still somewhat curious as to the exact SGX programming environment here - are you able to define your own "usercall"s, and if so, can we define a select usercall? Alternatively, does it make more sense to run the network stack directly in normal user mode, with the in-SGX just handling a stream of read/write/writeable events? In either model we could run all the networking stuff for an arbitrary number of connections with one or two threads and then have the SGX stuff just pass the network bytes through to the PeerManager.

The custom usercalls are fairly limited. The main interface is the UsercallExtension trait defined here which exposes connect_stream and bind_stream which one can use to override the normal behavior of TcpStream::connect().

Essentially, I'd have to implement an RPC service from scratch. The enclave would call UsercallExtension::connect_stream("rpc", ...) to obtain a client that can send bytes to the RPC service running on the host. The client would need to serialize all Read / Write events to bytes before sending it to the RPC service, and the RPC service would need to deserialize again before passing the actual Lightning data bytes to the peer address specified in the Read / Write event. The RPC service runs outside of the enclave and can use Tokio, so there would only need to be a one or two threads for handling all of the connections.

I think this is totally unnecessary though. On my lowest tier SGX Azure instance, the default maximum number of threads supported by the operating system is 31368. With bitcoind running and nothing else, there are 162 threads available. Assuming each lightning node requires 1 main thread and 2 threads per peer, this single instance can support 10,000 nodes with a single peer running simultaneously, or it can support a single node with 15,000 P2P connections. Since the nodes really don't need to be running in the enclave all the time - they only need to be loaded in when receiving payments, executing user-initiated commands, etc - the scenarios where a single cloud instance is running 10,000 nodes simultaneously or a single node is maintaining 15,000 P2P connections sounds absurd to me, even for the beefiest of node operators. Two threads per P2P connection is not a lot of overhead on modern infrastructures.

Making the existing implementation work has already stalled our project by a few weeks. The easiest, lowest resistance path forward I see is to simply move ahead with the current implementation that uses two threads per connection, and implement a usercall to handle TcpStream::shutdown(). I think anyone who needs to run 20,000 nodes simultaneously is perfectly capable of using lightning-net-tokio, or creating their own bespoke implementation of the network stack.

[TheBlueMatt]

The custom usercalls are fairly limited. The main interface is the UsercallExtension trait defined here which exposes connect_stream and bind_stream which one can use to override the normal behavior of

Ah, that's quite dissapointing. I guess it makes sense, but frustrating.

I think this is totally unnecessary though. On my lowest tier SGX Azure instance, the default maximum number of threads supported by the operating system is 31368.

Right, my questions aren't strictly about thread count - each thread carries (by default) 8MB of stack dedicated to it, context switch latency when we go to do writes, etc. My poor little RPi with 35 channels would eat 560MB (1/4-1/3 of its available memory) just on threads dedicated to handling network tasks (assuming those threads use stack, in theory it could be left unallocated, though some per thread would always be allocated, and that's not counting OS/kernel overhead of threads).

Making the existing implementation work has already stalled our project by a few weeks. The easiest, lowest resistance path forward I see is to simply move ahead with the current implementation that uses two threads per connection, and implement a usercall to handle TcpStream::shutdown()

Sorry to hear that, I think that's a pretty clear indication we need to provide a better higher-level network interface here. And of course there's no reason you can't use this crate for your own networking logic! It just may not make sense to take it upstream as something we encourage people to use (at least outside of the rather narrow use-case of SGX).

[MaxFangX]

https://doc.rust-lang.org/std/thread/struct.Builder.html#method.stack_size

Thoughts on setting the thread stack size to eg 4KB? The biggest local variable is the 8kb buffer for the Reader, which can be heap allocated by using a Vec instead.

[TheBlueMatt]

Thoughts on setting the thread stack size to eg 4KB? The biggest local variable is the 8kb buffer for the Reader, which can be heap allocated by using a Vec instead.

Stack size isn't just the maximum of any single function, its the total stack allocations of every function in an entire call tree. For example, if I set the stack limit to 4KB (ulimit -s 4) cargo test immediately segfaults.

I'd be willing to bet the maximum stack depth for calling LDK functions is at least 256K-512K, at least once you count OS thread setup (likely more in your case for SGX?) and syscalls. In general, though, I don't think there's a reason to reduce the stack size - the stack size is effectively a "maximum", which if we overrun it will cause (in the best case) a crash. Linux, however, can avoid actually allocating the stack space, or at least swap it out, until its used. Of course once its used you don't get the memory back.

More generally, this also doesn't consider the OS overhead of threads, context switches, etc, which, sure, aren't much on modern machines, but its also not free. For running in SGX on a beefy machine its probably totally fine, but it may not be the direction we want to go for a general lightning-net crate.

[MaxFangX]

I'd be willing to bet the maximum stack depth for calling LDK functions is at least 256K-512K, at least once you count OS thread setup (likely more in your case for SGX?) and syscalls. In general, though, I don't think there's a reason to reduce the stack size - the stack size is effectively a "maximum", which if we overrun it will cause (in the best case) a crash. Linux, however, can avoid actually allocating the stack space, or at least swap it out, until its used. Of course once its used you don't get the memory back.

Ah, I forgot the calls into PeerManager - good point.

More generally, this also doesn't consider the OS overhead of threads, context switches, etc, which, sure, aren't much on modern machines, but its also not free. For running in SGX on a beefy machine its probably totally fine, but it may not be the direction we want to go for a general lightning-net crate.

Noted. I did some digging on a select! for TcpStream read / write, and ended up at rust-lang/rust#6842 (8 years old) which was superceded by rust-lang/rfcs/issues/1081 in 2016. The conclusion, essentially, was that the solution for non-blocking IO is to use async/await, and to conduct async IO using crates external to rust std, such as mio. One reason for that is that Rust std needs to be maximally portable, and crates such as mio tend to have problems even on some of the more common platforms like Windows. Which comes back to why I ended up writing lightning-net in the first place - lightning-net-tokio was insufficient for my platform due to its dependence on mio, so I had to reimplement the network stack using only technologies that were available 8 years ago.

My point is - if this crate is to be upstreamed, I think the primary selling point should be that it is maximally portable with minimal reliance on any platform-specific APIs, rather than that it can match the performance of lightning-net-tokio. Maybe it can be renamed to lightning-net-sync (or similar) to reflect this posture, so that LDK users don't think that lightning-net is the primary, recommended crate instead of lightning-net-tokio. There could also be more notes on the performance implications of using this crate in the crate-level doc comments which discuss context switches, stack allocations, 2 threads per peer, etc. Additionally, after iterating on this together we've managed to produce a crate that has 0 dependencies outside of std (most importantly not using tokio which is massive), so this crate could also be useful for the especially security-conscious.

As for me and Philip's SGX project, we could certainly remove the overhead of dedicated threads by implementing a usercall extension that uses futures / polling / similar outside the enclave, but then the implementation would be specific to the SGX platform and it wouldn't make any sense to upstream it. The EDP people did mention the possibility of supporting TcpStream::shutdown() a few years ago in fortanix/rust-sgx#110 - presently I hope to push that forward and implement the usercall for shutdown() via a patch to Rust std rather than by introducing a Socket trait and SgxTcpStream in the lightning-net-sync crate. The result is that everything in lightning-net-sync is general purpose, with no mention of SGX or of a Socket trait that would make using the crate more unwieldy, and thus could be worthwhile to make available for anyone to use as a part of LDK.

[TheBlueMatt]

The conclusion, essentially, was that the solution for non-blocking IO is to use async/await, and to conduct async IO using crates external to rust std, such as mio

Sometimes I really really hate Rust. We have no-std and alloc separated out for a reason, std requires filesystem and TCP already, select is probably more broadly supported than filesystem access...

My point is - if this crate is to be upstreamed, I think the primary selling point should be that it is maximally portable with minimal reliance on any platform-specific APIs, rather than that it can match the performance of lightning-net-tokio.

select is not "platform-specific" in any material way - its standard POSIX, supported even by Windows just fine, I'm not aware of an OS that supports TCP and doesn't expose the same select interface as POSIX specifies. Sure, pselect, poll, etc are all easier to use, but select is just always available.

More generally, I'd argue the SGX platform you're using already doesn't "support std" given it mis-implements a number of std features in ways that would no doubt break many applications, certainly any doing nontrivial TCP read/writes (like this crate!).

hen the implementation would be specific to the SGX platform and it wouldn't make any sense to upstream it.

Ah, I think you may have missed my suggestion above - we can add a shim interface between the ldk-net crate and PeerManager that exposes a basic stream of "writeable/read" events and "write" calls. That could then be hooked and turned trivially into a usercall stream. If its not hooked, it'd be trivial to just pass it straight through to the PeerManager without any overhead.

All that said, given you indicated you're behind where you want to be, I suggest we leave this for now. We can revisit an architecture that's easier to hook in the future when we have more time in the future.

[MaxFangX]

All understood, and good points. Thanks for being welcoming to new contributors and taking the time to iterate with me on this @TheBlueMatt; it was a pleasure, and I learned a lot :) Will go ahead and close this PR.