julianknutsen / rust-lightning Goto Github PK

As a new dev to the transport layer, I want to quickly read about the objects, interfaces, and tests so I can understand the code and add features with minimal ramp up time.

Definition of Done:

• Add additional documentation about the separate transport layer, the interfaces, objects, and potential use cases in the future.

Tech Discussion:
This separate module could have uses outside of peers. Document it once the final code is in and list the potential use cases. Suggested here.

As a developer, I want to modify the event handling code in peers/ with the safety of unit tests and that have their dependencies minimized so that I have confidence in the changes and the features are easy to write.

Definition of Done:

• Unit tests are refactored from peers/handler.rs to a new module responsible for the event handling
• Unit tests do not require a full PeerManager/Transport (even test double versions)

Tech Discussion:
The current tests of this code include the full PeerManager which adds other dependent objects that aren’t really necessary for true unit tests. Instead, this behavior can be factored out into an event_handler module that just takes in an event and the required dependencies and verifies the correct state change.

This likely involves splitting out an interface for retrieving an initialized peer by node_id (InitializedPeerProvider?/PeerIterator?). The PeerHolder can implement this in the real code and tests can have a simple wrapper around a HashMap.

Most of the existing tests of this in handler.rs can be removed in favor of a few sanity tests to make sure any interface breakdowns are caught early.

End result will look something like this:

#[test]
Fn example_test() {
	// create dependencies:
	// channel handler spy, route handler stub, initialized node provider
	
	process_event(XYZ);

	// validate spys, stubs, and outbound queues of peers match the expected behavior
}

As a message handler, I want to forward certain messages to known initialized peers if the route_handler allows it.

Definition of Done:

• Unit tests are updated to include tests that validate message is forwarded to (non-source?) initialized peers for:
  • wire::Message::ChannelAnnouncement
  • wire::Message::NodeAnnouncement
  • wire::Message::ChannelUpdate
• Unit tests exist for the negative case as well when route_handler does not allow it.

Tech Discussion:
TODO from the codebase. Similar to the event path, if the route_handler allows it, broadcast the message to all known initialized peers.

Route handler stubs already exist so the testing should be straight forward. A little bit of work to create multiple initialized peers.

Recommend this is done AFTER the unit test work on message handling to get the testing in a better place. Will require a dependency update due to needing an InitializedPeerProvider instead of just the Peer, but should be straight forward.

As a developer, I want variable names to be consistent across the codebase so that it is easier to jump around and understand the code.

Definition of Done:

• decide on name (counterparty_id/counterparty_node_id?)
• do the rename

Tech Discussion:

As a developer, I want the Transport code to be in a separate folder so that the folder structure matches the dependencies (making scope creep harder) and it is easier to separate it out if/when it is needed for other use cases (watchtowers?).

Definition of Done:

• transport/ subfolder containing all items transport related (encryption/handshakes/etc)

Tech Discussion:
Move Transport code and all its dependencies to a sub-folder.
This can live under peer/ for now, but cleaning up the file structure to match the dependencies can make it easier to maintain.

Work backlog for the ln/peers/ module. Items are added in priority order and specific issues contain discussions on final acceptance criteria.

• [Peers] Testing: Add basic unit tests of PeerHandler

@ariard pointed out the BOLT spec is incomplete wrt byte-fragmentation concerns

The first step is validation of the issue as a problem and a move to design.

As a user of LDK, I want easy to follow documentation for the PeerManager so that I can integrate into my application seamlessly.

Definition of Done:

• Another pass at public docs for peers/handler.rs
• Optional: Update of tokio to make it more readable in reference public docs

Tech Discussion:
This can go hand-in-hand with another review and cleanup of the tokio code. Since the tokio code and tests are the reference implementation for how to use the PeerManager it makes sense for it to help drive the documentation.

As a Transport layer, I want to know which messages are optional and which messages are mandatory so I can make the right policy decision when my queue is full or the SocketDescriptor fails a full write.

Definition of Done:

• Optimal messages identified
• Policy decision made on if/when items should be dropped
• Unit tests updated to drop optional messages during flow control

Tech Discussion:
TODO from the codebase:
DoS vector by announcing channels on another node and flooding the OutboundQueue.

The current OutboundQueue allows the message path to push messages over the soft limit (the sync messages are the only things that care about the soft limit).

Seems like an update to the Transport API makes the most sense. Potentially adding a priority number or classification to each enqueue_message() where the Transport layer can make the policy decision to drop it if necessary.

The Transport unit tests are easily updated to test this behavior.

As a message handler, I want to attempt to send the inner message from ErrorAction::DisconnectPeer so that my peer could learn why I am disconnecting.

Definition of Done:

• Inner message is sent to the peer (best effort)

Tech Discussion:

TODO from the codebase. Similar to the event path, we should attempt to push the inner message through the SocketDescriptor before disconnecting.

The set of broadcast events don’t do anything special for the DisconnectPeer errors from the route handler in favor of just ignoring the event and not broadcasting messages. Is this the intended behavior?

As a developer, I want to modify the message handling code in peers/ with the safety of unit tests and that have their dependencies minimized so that I have confidence in the changes and the features are easy to write.

Definition of Done:

• Unit tests are refactored from peers/handler.rs to a new module responsible for the message handling
• Unit tests do not require a full PeerManager/Transport (even test double versions)

Tech Discussion:

The current tests of this code include the full PeerManager which adds other dependent objects that aren’t really necessary for true unit tests. Instead, this behavior can be factored out into a message_handler module that just takes in a message and the required dependencies and verifies the correct state change.

The existing tests don't need a PeerIterator/InitializedNodeProvider, but it will be required to implement the broadcast message features that are still TODO.

End result might look something like this:

#[test]
Fn example_test() {
	// create dependencies:
	// channel handler spy, route handler stub, Peer
	
	handle_message(XYZ);

	// validate spys, stubs, and outbound queues of peers match the expected behavior
}

The sync message generation and subsequent Peer state updates are done as a side effect of flushing the outbound queue.

This could use a cleaner design and is hard to test through the front door of the PeerManager.

Should the OutboundQueue be separated into a SocketDescriptorWriter object that handles the sync generation post-Init so it can be tested?

Should the Peer state updates that happen as a side-effect be replaced by turning InitSyncState into a real object that can be tested in isolation?

from @ariard
Just a side-note, but I think in the future we would like to move the sync status inside NetGraphMsgHandler. IMO this logic doesn't belong to the peer handler as it's already concerned with processing state.

It would be easier to implement more-agressive syncing logic like fetching valid node_announcement and start thinking with them.
lightningdevkit#692 (comment)

As a developer, I want to reduce the number of dependencies for tests that surround the PeerHolder behavior to gain confidence in the object and reduce development time for future features that interact with it.

Definition of Done:

• Unit tests exist for PeerHolder API
• Optional: PeerHolder is moved to its own file

Tech Discussion:

Refactoring unified a lot of the individual map accesses to a true API in front of the PeerHolder. Aside from code deduplication, one of the benefits was that it can now be tested in isolation.

The tests should be really straight forward using a fake Peer. The only other external dependency is the ChannelHandler callback for disconnect_peer and a spy already exists for this.

The current implementation doesn’t check the pong and always sends a bytes_len of 64.

What is the value of implementing this feature? Should we document anywhere that we are intentionally doing nothing?

As a decryption layer, I want to use non-zero initial ciphertext buffers so that chacha failures biasing towards zero aren't masked.

Definition of Done:

• Random data is used to fill the initial act buffers
• Optional: Buffers are set to 0 for tests to remove nondeterminism

Tech Discussion:

Per this discussion, fill with random data to help reduce masking decryption failures. I don't think the nondeterminism is really an issue for tests, but I've included it since it came up in the discussion.