Subnet service

[dknopik]

Proposed Changes

Small service that gets notified when a validator gets added or removed and passes on instructions to subscribe/unsubscribe/look for peers

[diegomrsantos]

Thank you for starting this! I added some comments.

[diegomrsantos]

Interestingly, Lighthouse doesn't do that. There could be a check somewhere else (PeerManager?) that keeps the target number of peers for each subnet.

[dknopik]

Pushed a new design based on #117.

Also pulled out the logic from network into its own crate, but kept the full logic (mapping updates to subnets) in one place. The reasoning behind that is that I think we should avoid bothering the network create with events as much as possible to avoid computation in the network service if possible.

[diegomrsantos]

What is subnet_tracker doing? It feels a bit confusing to me, it's being called before the SubnetTrackerinstance is created right after.

[diegomrsantos]

Maybe this could be a start function called on a SubnetTracker instance?

[diegomrsantos]

Something like

pub struct SubnetTracker {
    db: watch::Receiver<NetworkState>,
    subnet_count: usize,
    events_tx: mpsc::Sender<SubnetEvent>,
    events_rx: mpsc::Receiver<SubnetEvent>,
}

impl SubnetTracker {
    pub fn new(db: watch::Receiver<NetworkState>, subnet_count: usize) -> Self {
        let (tx, rx) = mpsc::channel(1);
        Self {
            db,
            subnet_count,
            events_tx: tx,
            events_rx: rx,
        }
    }

    /// Spawn the background task that processes network state updates.
    pub fn start(&self, executor: &TaskExecutor) {
        let db = self.db.clone();          // watchers can be cloned
        let tx = self.events_tx.clone();
        let subnet_count = self.subnet_count;

        executor.spawn(async move {
            run_subnet_tracker_loop(tx, db, subnet_count).await;
        }, "subnet_tracker");
    }

    /// Receive subnet events from the background task.
    pub async fn recv(&mut self) -> Option<SubnetEvent> {
        self.events_rx.recv().await
    }
}

async fn run_subnet_tracker_loop(
    tx: mpsc::Sender<SubnetEvent>,
    mut db: watch::Receiver<NetworkState>,
    subnet_count: usize
) {
    // The same logic as the existing `subnet_tracker` function
}

Wdyt? It might make things easier to understand and test. Or it could just be my OOP mind.

[dknopik]

Some arguments against this:

• Holding a sender unnecessarily will prevent us from detecting that the actual subnet_tracker has exited.
• All the data in the struct (except for the receiver) is unnecessary once the subnet_tracker has started, and then still carried around uselessly.
• I argue that this actually makes testing harder and increases coupling. The test_tracker at the bottom of the file shows nicely that it is trivial with the current setup to create a mock that does not require a db: watch::Receiver<NetworkState>.

Some alternatives:

• Rename SubnetTracker to SubnetTrackerHandle or SubnetTrackingReceiver or something like that to make it a bit clearer
• No struct at all, just pass the mpsc::Receiver<SubnetEvent> around.

[diegomrsantos]

I agree, SubnetTracker isn't doing much currently, getting rid of it would probably make it simpler than my suggestion.