Exponse block/unblock functionality in da service

[pradovic]

1. What does this PR implement?

Exponse block/unblock functionality in da service. This is the PR that enables the follow up which is adding http block/unblock functionality.

2. Does the code have enough context to be clearly understood?

Yes.

3. Who are the specification authors and who is accountable for this PR?

@bacv, @pradovic.

4. Is the specification accurate and complete?

Yes, but still debating.

5. Does the implementation introduce changes in the specification?

No.

Checklist

Warning

Do not merge the PR if any of the following is missing:

• 1. Description added.
• 2. Context and links to Specification document(s) added.
• 3. Main contact(s) (developers and specification authors) added
• 4. Implementation and Specification are 100% in sync including changes. This is critical.
• 5. Link PR to a specific milestone.

[danielSanchezQ]

@bacv is this for testing purposes? If so it should not be exposed in builds.

[bacv]

@bacv is this for testing purposes? If so it should not be exposed in builds.

No, this is to block unblock unwanted peers in DA network (malicious for example).

[danielSanchezQ]

@bacv is this for testing purposes? If so it should not be exposed in builds.

No, this is to block unblock unwanted peers in DA network (malicious for example).

why http api then? 🤔

[bacv]

why http api then?

At the moment temporal (with cooldown) or permantent (until unblocked) blocking is performed by DA Monitor - we want user to allow to modify this behaviour manually.

[danielSanchezQ]

why http api then?

At the moment temporal (with cooldown) or permantent (until unblocked) blocking is performed by DA Monitor - we want user to allow to modify this behaviour manually.

I'm not sure this should happen manually. We can add a the blacklist/whitelist behaviour instead.

[pradovic]

why http api then?

At the moment temporal (with cooldown) or permantent (until unblocked) blocking is performed by DA Monitor - we want user to allow to modify this behaviour manually.

I'm not sure this should happen manually. We can add a the blacklist/whitelist behaviour instead.

Happy to change if needed. @bacv, @danielSanchezQ If you guys like we could also discuss it and spec out what needs to be done.
From what I see connection monitor has a list of malicious peers, which is kind of a blacklist. I don't have enough context to know if we need to distinguish between malicious and blacklisted peers, but they both seem to be blocked until unblocked.

[bacv]

I'm not sure this should happen manually. We can add a the blacklist/whitelist behaviour instead.

How will you remove blacklisted peers if you change your mind?

[bacv]

From what I see connection monitor has a list of malicious peers, which is kind of a blacklist

Yes monitor behaviour is already based on the linked behaviour - it is actually a superset, because it has temporary blocking.

[danielSanchezQ]

I'm not sure this should happen manually. We can add a the blacklist/whitelist behaviour instead.

How will you remove blacklisted peers if you change your mind?

Maybe I'm missing something.I thought blacklisting was done through the manager policies. If a misbehaving peer is detected maybe is blacklisted right? If it is added automatically, we should have an automated policy for removing them as well.

[danielSanchezQ]

From what I see connection monitor has a list of malicious peers, which is kind of a blacklist

Yes monitor behaviour is already based on the linked behaviour - it is actually a superset, because it has temporary blocking.

Ok, so monitor behaviour does track that. I thought it was the connection manager, hence my confusion.

[bacv]

I thought blacklisting was done through the manager policies.

Yes, the decisions are made through the Policy - based on policy:

• Balancer decides how many connections should be per subnetwork at a given time.
• Monitor decides if a peer is unhealthy or malicious based on error rate coming through the connection.

Even if the policy would have a way to determine when the peer is considered not malicious again, I think a user should have a way to block/unblock chosen peers, lets say because some new information about bad actors behind couple of nodes was uncovered.

[bacv]

The PR looks good, one command seems worth adding - a command to get the list of currently blocked peers - what do you think @danielSanchezQ @pradovic ? Could it be added here or maybe not required.

[danielSanchezQ]

The PR looks good, one seems worth adding - a command to get the list of currently blocked peers - what do you think @danielSanchezQ @pradovic ? Could it be added here or maybe not required.

Yep, seems good as other services may need of that. Btw, As this command are growing, better to nest them.

pub enum ManagePeer {
    BlockPeer {
        peer_id: PeerId,
        response_sender: oneshot::Sender<bool>,
    },
    UnblockPeer {
        peer_id: PeerId,
        response_sender: oneshot::Sender<bool>,
    },
    BlacklistedPeers {
        response_sender: oneshot::Sender<Vec<PeerId>>
    }
}

/// Message that the backend replies to
#[derive(Debug)]
pub enum DaNetworkMessage {
    /// Kickstart a network sapling
    RequestSample {
        subnetwork_id: SubnetworkId,
        blob_id: BlobId,
    },
  ManagePeer(ManagePeer)
}

Names are orientative...

[pradovic]

@bacv, @danielSanchezQ Thanks for the input! Everything should be implemented now. I am not sure do we need a call for temporary block as well, and should temporary blocked peers be part of blacklisted call. It is similar functionality, but not quite the same, as @bacv already mentioned.

[bacv]

Just idea, could this be MonitorCommand as it goes directly to the monitor behaviour?

[pradovic]

Makes sense, fixed!

[bacv]

Thanks for the input! Everything should be implemented now. I am not sure do we need a call for temporary block

Temporal block is to disconnect from unhealty peers with a hope that after some time has passed their operation will go back to normal. The cooldown interval is set in the config, and unless it's set to minutes, not seconds, then the user shouldn't be required to unblock them. Having unhealthy peer unblocked wouldn't change the outcome - if it fails to respond to the messages or refuses the connection, I don't see the reason to keep it unblocked manually.