feat(bridge): census should use affected_by_radius

[morph-dev]

Followup to #1839

What was wrong?

The census should use distance and radius when selecting peers only if that's relevant for the provided content.

How was it fixed?

Added logic that uses affected_by_radius function

To-Do

• Clean up commit history and use conventional commits.

[KolbyML]

I don't get the reason for this change. In the context of ephemeral history. We are only sending series of ephemeral keys, in my PR I am working on I just added a function called random_peers() I don't think it makes sense to use select_peers() for ephemeral history, maybe in the beacon bootstrap usecase you can use this, is that the justification for this?

bridge's census should respect this when selecting peers (PR ready: #1841 )

For ephemeral keys it doesn't make sense to provide the select_peers() function with a key or use the function. There isn't a case where we will offer single ephemeral history content offers.

[morph-dev]

You are right about ephemeral headers. I didn't think about details regarding it

Still, I think it's cleaner and better if select_peers respects the affected_by_radius, even if we don't use it for ephemeral headers.

Do you think we shouldn't do it in general case? Or that there is some downside if we do it?

[KolbyML]

You are right about ephemeral headers. I didn't think about details regarding it

Still, I think it's cleaner and better if select_peers respects the affected_by_radius, even if we don't use it for ephemeral headers.

Do you think we shouldn't do it in general case? Or that there is some downside if we do it?

I think if you intend to use this functionality for the beacon network I see value in this PR. I can't think of a reason we would need this in History or State. Ideally we aren't maintaining deadcode paths is all, if we aren't using this functionality or intend to use it, why have it etc. Do we intend to use it for Beacon?

[morph-dev]

I think if you intend to use this functionality for the beacon network I see value in this PR.

There is no immediate use of it for beacon network as all beacon nodes should have 100% radius.
But as Kim said, storing all Bootstrap data on every node can be a bit too much, with very little value. On top of it, we can't backfill old bootstrap (we can only gossip/offer the one that matches finalized header), and he has an idea on how to fix that as well. If we change the spec to fix these things, then we will definitely have to use this PR for beacon.

We will use this (or some variation of this) for head state, as that one uses different distance metric as well (and doesn't use content_id in the same way).

Ideally we aren't maintaining deadcode paths is all, if we aren't using this functionality or intend to use it, why have it etc.

It's hardly any deadcode path. Instead of passing content_id, we are now passing content_key (expect at the very end, to prevent generating content_id every time).

I also think that conceptually, this is the correct code! We shouldn't filter peers based on their distance from content, if that particular content is dependent on radius and distance.

[KolbyML]

I think if you intend to use this functionality for the beacon network I see value in this PR.

There is no immediate use of it for beacon network as all beacon nodes should have 100% radius. But as Kim said, storing all Bootstrap data on every node can be a bit too much, with very little value. On top of it, we can't backfill old bootstrap (we can only gossip/offer the one that matches finalized header), and he has an idea on how to fix that as well. If we change the spec to fix these things, then we will definitely have to use this PR for beacon.

If we will use this PR for Beacon then I am for it

We will use this (or some variation of this) for head state, as that one uses different distance metric as well (and doesn't use content_id in the same way).

Ideally we aren't maintaining deadcode paths is all, if we aren't using this functionality or intend to use it, why have it etc.

It's hardly any deadcode path. Instead of passing content_id, we are now passing content_key (expect at the very end, to prevent generating content_id every time).

We will be generating a content_id every time though no? There isn't a case when we wouldn't. select_peers should be explicitly not callable for EphemeralHistory There isn't a usecase for

• EphemeralHeaderByOffer as we don't offer singles of these
• EphemeralHeaderByFindContent this can't be offered to begin with

So if anything I would expect the function to ensure!() that the content_key is radius dependent and fail early

I also think that conceptually, this is the correct code! We shouldn't filter peers based on their distance from content, if that particular content is dependent on radius and distance.

There isn't a valid reason to call this function unless the content is dependent on radius and distance

[KolbyML]

I think we should ideally avoid passing Generics through multiple layers of structures as well. If we want the interface of select_peers(content_key) the generic should only be passed to top layer, then we would use non-generic functions on the way done. I can't think of a good reason why the generic needs to be passed down so many layers.

Generics can be abused, and I think this is kind of it.

[KolbyML]

looks good. Milos and I had a call where we talked about how he planes to use some of this in the Beacon and State HEAD code.

Instead of doing generic functions, he updated content_key to instead take an implimentation of a trait, which I think is a lot cleaner and easier to read. It is also consistent with other parts of the code

    pub fn select_peers<'a>(
        &self,
        content_key: &impl OverlayContentKey,
        peers: impl IntoIterator<Item = &'a Peer>,
    ) -> Vec<PeerInfo> {

For example ^ this function already did this for peers.

So it is pretty nice!

[KolbyML]

If you rebase it will fix the tests