(3/4) Conduit fixups from 494 by julianknutsen · Pull Request #4 · arik-so/rust-lightning

julianknutsen · 2020-09-08T22:36:02Z

Cleans up the remaining feedback regarding the Conduit behavior in the original lightningdevkit#494 review.

New commits start @ 41c7374 as this is a continuation of (2/4)

Decryption has been moved to the read() path to remove the complexity required to handle iterators that can error
The Decryptor will now decrypt directly from the input slice if there is no existing data in the read buffer. This should technically improve performance in some cases.
The Conduit object has been destructured into Encryptor & Decryptor.
process_act() now returns a simple container structure for the Encryptor/Decryptor/Remote Static Public Key to clean up the return value.

/// Container for the information returned from a successfully completed handshake
pub struct CompletedHandshakeInfo {
pub decryptor: Decryptor,
pub encryptor: Encryptor,
pub their_node_id: PublicKey,
}

Add unit tests against the PeerHandshake public interface prior to changing any of the implementation details. This is a solid stand-alone patch, even if the subsequent refactor and design changes are not merged. A future patch uses the test vectors from the RFC to minimize the code under test.

The implementation logic is duplicated from handshake/mod.rs so that it can be tested and refactored independent of the current implementation. This uses enum dispatch coupled with separately 'moved' state objects that allow for simple testing, easy state inspection, and understandable transition logic between states. This pattern also removes the need for match statements inside implementation logic in favor of the instance representing the current state and available data. Every state transition is implemented in a next() function and is a straight forward translation from an object representing the existing known data and an input act message (bytes). Example usage: let (act_data, next_state) = cur_state.next(input_data); This state machine will eventually be moved into place inside the PeerHandshake which will act as a simple wrapper that just maintains the state machine and marshals the data between the peer_handler and states.

PeerHandshake is now a simple object around the new state machine that simply marshals data between the callers and state machine states. For now, keep the APIs identical to make the patches easier to read. The only interesting piece to note here is the handling of get_remote_pubkey() to keep parity with the existing implementation. Future patches will remove this separate function in favor of callers taking ownership of it once the handshake is complete.

Introduce the sha256!() and concat!() macros that are used to improve readability of the noise exchanges.

Small inconvenience with the HandshakeHash return value that will be taken care of by the end of this patch stack.

Fix inconsistent use of concat vs. Sha256::engine() input. These can be deduplicated at the end.

Identify potential issue regarding the invalid case where the read_buffer has data after completion.

All needed public keys are calculated during the first state and passed through as needed. Limits the call sites of private_key_to_public and makes the initial states for the initiator and responder more symmetric.

Create an alias type to make the code more readable and change all usages to moves.

Push buffer read into helper function Combines static length values of ActOne and ActTwo structs

All consumers immediately serialize the struct so just do it for them and hide the implementation details of the handshake.

Now that final process_act() returns (conduit, remote_pubkey), the callers can just receive it when the handshake is complete.

Now that the remote_pubkey is returned from process_act(), the logic can be more straightforward. Each branch is responsible for returning the next PeerState and PeerDataProcessingDecision. Remove Option from CompleteHandshake now that the PublicKey is guaranteed to exist

Clean up loose threads (wrong docs, variable names) now that this code is in a more stable state.

Now that the implementation is complete and swapped over, remove the duplicated tests for process_act(). At this layer, all that needs to be tested is: * The new state in PeerHandshake is set to the state returned from state_machine.next() * Any error generated from state_machine.next() is returned * Any calls to process_next() after an error was returned panic These tests are a good use case for a mocking library, but for now just leverage implementation details to trigger the proper errors and states.

The previous concat!() macro and the variadic sha256!() had the same functionality due to the way sha.input() works. Reduce to a common code path that now uses concat_then_sha256() in all places and reduces the number of potential copies. Where appropriate, use the actual Sha256 type. The full type usage is hard due to the usages of hkdf::derive() in other parts of the code, but this is a good start on the path to cleaner type usage.

Add documentation and a unit test against the relevant test vector from the RFC.

Not using IHandshakeState slipped through the cracks until this point, but using it makes the enum dispatch follow a more standardized pattern.

This uses hard-coded test vectors to minimize the code under test as well as ensures full coverage. One additional test was added to account for an Act3 bad rs value.

The previous implementation used implementation details of the act byte returns from process_next_act() to determine whether or not to send an init message, but the Peer already has that information in it's outbound field. Use it instead to clean up the layering.

The fact that we have to call next() to generate act1 is an implementation detail of the handshake. Rename the old constructor to indicate that it creates and initializes the state and fix up the uses.

Use the slice refs from the act array instead.

Write a new fuzz test for the PeerHandshake module that is able to generate failure paths that occur after a partial handshake sequence has been completed. To enable this, a new testing object FuzzGen has been introduced that can deterministically generate bytes and bools based on the random fuzz input. These building blocks are enough for the test code to generates different execution paths and complete partial phases of the handshake protocol before generating an error. When going through a test cycle where the handshake completes, it will also verify that the initiator and responder can communicate successfully through the returned conduits sending variable length data and validating the contents.

Instead of returning a subslice of input for the caller to process, just return the number of bytes consume and let them do the accounting.

To enable better testing, put the code to enqueue data and flush it to a SocketDescriptor behind an object. This patch starts to implement traits for the various separate interface that each object needs. The idea is the consumer of an interface defines it and the dependency objects implement it. This will be useful when creating test doubles for unit tests and helps make the dependencies of each function more clear. Additional uses of trait-based contracts will be more clear in the next patches that start to add more testing.

Start moving the code that implements the Bolt8 transport layer into a separate testable module. This patch makes use of Rust's supported mocking pattern to create test doubles and leverage them to write simple unit tests for the public API of Transport. The end goal is a mockable Transport layer for easier testing of the PeerManager.

This patch ontinues to separate state the exists before NOISE is complete and after it is complete to unlock future refactoring. Most callers immediately unwrapped the value from Peer and can just call Transport::get_their_node_id(). The duplicate connection disconnect path has been rewritten to determine whether or not to remove & send a disconnect event without needing to use a None value for Option<PublicKey> All other users are in contexts where they either exit early or continue if !transport.is_connected() so it is also safe to call Transport::get_their_node_id()

Use the newly abstracted Transport layer to write unit tests for most of the PeerManager functionality. This uses dependency inversion with the PeerManager to pass in a Transport test double that is configured to exercise the interesting code paths. To enable this, a new struct PeerManagerImpl has been created that has all of the original functionality of PeerManager, but includes a type parameter for the Transport implementation that should be used. To keep this test-feature hidden from public docs, the PeerManager struct is now just a shim that delegates all the calls to PeerManagerImpl. This provides a nice balance of a clean public interface with the features needed to create isolated tests. The tests make use of the Spy and Stub test patterns to control input state and validate the output state.

Pass in all the in/out parameters individually instead of taking everything out of peer. The important thing to notice here is that the pending_outbound_buffer parameter MUST be an OutboundQueue and CANNOT be an 'impl PayloadQueuer'. This demonstrates that the read_event() function has the ability to flush() since it can use the SocketDescriptorFlusher interface. This should be illegal due to the invariant that read_event() never calls back into the SocketDescriptor. This is one of the motivations of using trait bounds in argument position as a defensive programing technique to prohibit functions from taking actions that shouldn't be allowed even if they have access to the object. Future patches will fix this bug and ensure the type system enforces this bug from happening again. This is a short-lived destructure that will be recombined once the dependencies are cleaned up.

Per the external documentation, there is an invariant that read_event() will not call back into the SocketDescriptor, but the actual code flushed the outbound queue. Fix the bug and use the type system to guarantee this behavior in the future. By changing the function parameters to use `impl PayloadQueuer` the function will only have access to functions on that interface throughout the execution, but still be able to pass the OutboundQueue object since it satisfies the trait bounds. Functions such as enqueue_message() also take a `impl PayloadQueuer` so they can be called from that context, but functions that need the SocketDescriptorFlusher interface will fail at compile-time and point to the issue before any tests need to be run. This also fixes up tests and the tokio implementation that did not implement the proper behavior and relied on read_event() calling send_data().

Introduce the MessageQueuer interface used by Transport to queue messages for send and use it in all the existing places where messages are queued.

Many of the is_connected() checks were duplicative due to the macro continuing out of the match statement if the peer referenced by the node_id had not seen an Init message yet. Remove the macro in favor of a function on PeerHolder that will return an Option<> and use it instead. Split out a new helper function Peer::is_initialized() that will return true if the peer has seen an Init message.

Encapsulate the Peer information that is only valid after an Init message has been seen. This makes the accesses to sync_status, ping information, and their_features cleaner w.r.t. the Peer initialization state.

The previous implementation would send a peer_disconnect callback for any error after the NOISE handshake completed, but only send a peer_connected callback after the Init message was received. This could lead to a dangling peer_disconnected callback that didn't have a paired peer_connected. This patch cleans up the state differentiation to match the following cases: unconnected (!transport.is_connected() && peer.post_init_info.is_none()) * Know about the peer, but are still in the process of the NOISE handshake connected (transport.is_connected() && peer.post_init_info.is_none()) * NOISE handshake completed, but haven't received an Init message initialized (transport.is_connected() && peer.post_init_info.is_some()) * The NOISE handshake has completed and the Init message has been received and processed With the 3 conceptual states, the read_event() path now only inserts a Peer into the node_id_to_descriptor map after a Peer enters the initialized state. This fixes the asymmetry between peer_disconnected & peer_connected as well as simplifies the disconnect code.

Now that the Init handling has been moved and the dependencies are more clear, deduplicate the parameters of the read_event() and helper functions to make use of Peer. The overall pattern remains the same, read_event() does the locking and passes in the separate items (peer, peers_needing_send, node_id_to_descriptor) to do_read_event(). The handle_message() path is also cleaned up now that post_init_state is guaranteed to be valid at that point.

Now that the locking and destructuring is done in read_event(), the workarounds for the pre-NLL borrow checker can go away.

This patch expands the PeerHolder API allowing for encapsulation of the peer state from the code that needs to iterate over initialized peers. This cleans up peer iteration/removal allowing for a much simpler design. 1) Introduce new APIs for PeerHolder: * initialized_peers_mut() * initialized_peer_node_ids() * remove_peer_by_descriptor() 2) Clean up event handler iteration using new APIs 3) Unify the timer_tick and DisconnectPeer event disconnect path using new APIs 4) Convert get_peer_node_ids() to use new API

The broadcast event handling code did not catch Ok(false) returned from the route handler when deciding whether or not to broadcast messages. Instead, it only checked if the return value was an Error. Fix it up and enable the regression tests.

Use a separate lock to generate the SecretKey instead of overloading the PeerHolder mutex. Refactoring PeerManager removed this hidden constraint and this should make it more robust in the future.

s/fill_message_queue_with_sync/fill_outbound_queue_with_sync/ s/queue_init_message/enqueue_init_message/

Motivated by lightningdevkit#456, remove the peers_needing_send set in favor of just scanning the peers during process_events() and attempting to send data for those peers that have items in their outbound queue or pending sync items to be sent out.

This gets rid of the complexity required to handle Iterators that return errors and makes way for fewer copies in the decryption path.

Previously, all input data was written to the read buffer before any decryption was attempted. This patch will use the input data solely in the event that there is no previous data in the internal read buffer. This also converts all tests to use the public interface instead of the previously used decrypt_single_message This was design feedback from the original 494 review.

This makes the process_act() interface a bit cleaner on a successfully completed handshake.

The public interface was in a half-state with some callers referencing the Decryptor directly for message iteration and others using the public interface for the encryption path. This patch moves everything to using the Encryptor and Decryptor directly. This is motivated by feedback from 494, that recommended the objects are split up but still have common functions for the key rotation.

Now that the CompletedHandshakeInfo exists to pass the relevant pieces out of the handshake code and all users go to the Encryptor and Decryptor directly, this is no longer needed.

Clean up the loose comments, test names, and variables names that still referred to conduit now that it has been destructured.

julianknutsen · 2020-09-08T22:36:02Z

Cleans up the remaining feedback regarding the Conduit behavior in the original lightningdevkit#494 review.

New commits start @ 41c7374 as this is a continuation of (2/4)

Decryption has been moved to the read() path to remove the complexity required to handle iterators that can error
The Decryptor will now decrypt directly from the input slice if there is no existing data in the read buffer. This should technically improve performance in some cases.
The Conduit object has been destructured into Encryptor & Decryptor.
process_act() now returns a simple container structure for the Encryptor/Decryptor/Remote Static Public Key to clean up the return value.

/// Container for the information returned from a successfully completed handshake
pub struct CompletedHandshakeInfo {
pub decryptor: Decryptor,
pub encryptor: Encryptor,
pub their_node_id: PublicKey,
}