[Peers] Tech Debt: Revisit InitialSyncState behavior and object relationships

[julianknutsen]

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.
#692 (comment)

[TheBlueMatt]

Hmm, do you have a concrete design in mind? We don't get a lot of feedback from the user (by design - the API mirrors exactly what the user can get from the system in a cross-platform manner - roughly POSIX), so about the only thing we know is when the write buffer is empty.

[julianknutsen]

I had to look at this part of the code to track down some existing bugs as part of #714 and I think I came up with a reasonable separation that cleans it up.

There are two code smells that the design should address:

• Sync code knows the size of the queue
• Sync code knows how many items are returned from route handler callbacks

These result in code that knows too much about implementation details while filling the queue to the soft limit.

A cleaner design might look something like this:

// Interface used by the PeerHandler to fill outbound queue with sync messages
trait SyncMessageProvider {
	fn has_next() -> bool;
	fn drain_one() -> Option<Message>;
	
	
	// Or an iterator api that drains which is a bit non-standard
	fn has_next() -> bool;
	fn next() -> Option<Message>;
	
	// Or something that looks a bit like a linked list
	fn front() -> Option<&Message>;
	fn pop_front(); 
}

This trait would be implemented by a new InitSyncTracker object that encapsulates the existing InitSyncTracker enum and state transitions.

The InitSyncTracker object would likely prefetch the next item that needs to be sent (or 3 items in the case of channel announcements) and cache them for retrieval one at a time by the PeerHandler.

This could be paired with OutboundQueue::is_full() to make the fill_outbound_queue_with_sync() have a lot less implementation specifics in it and address the code smells. As a result OutboundQueue::queue_space() can be removed.

fn fill_outbound_queue_with_sync<Q: PayloadQueuer + SocketDescriptorFlusher>(
	&self,
	sync_status: &mut InitSyncTracker,
	message_queuer: &mut impl MessageQueuer,
	outbound_queue: &mut Q) {

	if !queue.is_full() {
		if let Some(msg) = init_sync_tracker.drain_one() {
			message_queuer.enqueue_message(msg, outbound_queue, &*self.logger);
		}
	}
	

A design like this also addresses the testability of the InitSyncTracker enum which is in a rough place right now. But abstracting it into an object and using RoutingMessageHandler test stubs all of the transitions and prefetching can be easily tested in unit tests.

This object can also be moved into the NetGraphMsgHandler as suggested by @ariard as the PeerManager now only cares about processing the messages and retrieving the next sync message to send.

[TheBlueMatt]

Ah, you're suggesting basically just abstract out the state into some object instead of forcing a certain API - sure, that sounds good too, as long as its just as easy for users to implement (and hard to mis-use, which is the main feature of the current design that something abstract doesn't capture - the "obvious" way to implement would be a copy of the state which just DoS'es yourself).

[TheBlueMatt]

This would be nice, but its probably never going to be a priority enough for anyone to get to.