feat(connection-limit): set bypass rules for connections #5720

drHuangMHT · 2024-12-06T08:00:36Z

Description

Add BypassRules for connection_limit::Behaviour to allow bypasses.
May close #5605

Notes & open questions

This implememtation does not distingush between local addresses and remote addresses, will that be a problem?

Change checklist

I have performed a self-review of my own code
I have made corresponding changes to the documentation
I have added tests that prove my fix is effective or that my feature works
A changelog entry has been made in the appropriate crates

drHuangMHT · 2024-12-06T08:05:20Z

I also found that you can get a mutable reference to the ConnectionLimits at runtime but since all fields are private and all methods require ownership, you need to do clone and replace to actually modifiy it. Is that intentional?

elenaf9

I think adding bypass rules for a PeerId makes sense. I am not sure about having them for a MultiAddr as well.
When would one know the address of a trusted peer, but not it's PeerId?

drHuangMHT · 2024-12-06T10:12:44Z

I think adding bypass rules for a PeerId makes sense. I am not sure about having them for a MultiAddr as well. When would one know the address of a trusted peer, but not it's PeerId?

I don't know, but trusting an address also kind of makes sense? Though this behaviour is not primarily used for managing trusts between peers.
Also this is brought up by the issue itself, @dariusc93 what do you think?

drHuangMHT · 2024-12-09T06:54:38Z

I think adding bypass rules for a PeerId makes sense. I am not sure about having them for a MultiAddr as well. When would one know the address of a trusted peer, but not it's PeerId?

Also you can use the rule to allow a range of peers from an address, for example behind a load balancer and such, or a domain(I guess it only works for dialing). Allowing a range of addresses will grant even greater flexibility but it will be a bit difficult to implement.
We can also allow all connections from specific listeners.

elenaf9

We can also allow all connections from specific listeners.

Not sure I understand what you mean. Won't using the listeners PeerId do exactly that?

I still have a slight preference to only allow bypassing of connection limits based on PeerIds. In allow-block-list we also only operated on PeerIds and not on multiaddresses.
But I don't feel strongly about it, so if from a user perspective it's useful and needed I am okay with also bypassing based on addresses.

misc/connection-limits/src/lib.rs

dariusc93 · 2025-01-09T12:37:07Z

Sorry for the late response. Looking at the comments and review, I think what we can do for now is exclude the address portion of the code and allow it based on PeerId alone and have a later discussion on if we should allow Multiaddr to be used as well. My train of thought in my issue #5605 was to not only allow PeerId but also specific Multiaddr (i.e if the local node wish to only allow connections from a specific relay, to allow connections from a specific address from the peer for content discovery, etc), but as i think about it more, I can imagine it getting more complex down the road. I could be wrong though on that and I may be overthinking that use-case, but in all I am not against just sticking with PeerId in this PR for now. Thoughts?

CC @drHuangMHT @elenaf9

fix: bypassed pending connections still take up slots

elenaf9 · 2025-01-26T09:06:53Z

misc/connection-limits/src/lib.rs

+type BypassFn = fn(
+    remote_peer: Option<&PeerId>,
+    remote_addresses: &[Multiaddr],
+    local_addresses: Option<&Multiaddr>,
+) -> bool;
+


Given that this is function pointer type, an implementation won't be able to capture an environment. So it could only hardcode here a list of allowed peer-ids or multiaddresses, right?
I am not sure this brings any advantage over the former bypass_multiaddr and is more complex. Do you have an example use-case for it?

No example for this unfortunately. It only grants more flexibility so that users can mix and match rules. Of course it can be a Box<dyn Fn>.

I agree with @elenaf9 that allowing a closure here would open up many nice use-cases.

We could also instead do a trait BypassRule instead and add some default implementations, for example:

HashSet<PeerId> to allow the whitelisting you implemented,

FnMut(Option<&PeerId>, &[Multiaddr], Option<&Multiaddr>) for convenience,

Vec<B> to compose multiple rules,

struct NoBypass; as default to allow no bypasses.

...

and then make Behaviour generic over BypassRule. But I am not sure if that would add to much complexity for too little added value.

Interesting idea!

But I am a bit torn about it. As you also noted, I am unsure if we really need the complexity.
I would expect that you usually know the PeerId of trusted peers. Thus I think most use-cases can be simply satisfied with the one bypass_peer function, and the rest is for now only theoretical.

As mentioned before (and I think @dariusc93) agrees, I would prefer if we for now just add bypass rules for PeerIds, and extend the API and it's complexity only when the need arises.

jxs · 2025-01-31T16:14:25Z

misc/connection-limits/src/lib.rs

@@ -92,6 +97,11 @@ impl Behaviour {
    pub fn limits_mut(&mut self) -> &mut ConnectionLimits {
        &mut self.limits
    }
+
+    /// Returns a mutable reference to [`BypassRules`].
+    pub fn bypass_rules_mut(&mut self) -> &mut BypassRules {


what is the advantage of having an external structure (BypassRules) instead of having these methods in the main Behaviour?

dariusc93

Left a couple of comments. Can we also add a test for this feature?

dariusc93 · 2025-01-31T16:22:32Z

misc/connection-limits/CHANGELOG.md

@@ -1,3 +1,9 @@
+## 0.5.1


Since there is a breaking change to the API, we should bump the minor version

Suggested change

## 0.5.1

## 0.6.0

Why is it a breaking change? We just add new methods no? https://doc.rust-lang.org/cargo/reference/semver.html#item-new

Because we added bypass_rules to Behaviour::new, which would then require a minor bump to satisfy semver. See #5720 (comment) :)

dariusc93 · 2025-01-31T16:23:03Z

misc/connection-limits/src/lib.rs

@@ -76,9 +80,10 @@ pub struct Behaviour {
 }

 impl Behaviour {
-    pub fn new(limits: ConnectionLimits) -> Self {
+    pub fn new(limits: ConnectionLimits, bypass_rules: BypassRules) -> Self {


I wonder if we should remove bypass_rules from here and have it be empty internally by default, or have a separate function to supply it when constructing the behaviour. This would then allow us to keep this as a patch release instead of bumping the minor version of the crate due to it being a breaking change. Thoughts?

elenaf9

Thank you for the follow-ups @drHuangMHT! LGTM.

dariusc93

LGTM. Can you update the versions in both Cargo.toml (crate and workpace)? :)

set bypass rules

d78a671

drHuangMHT changed the title ~~feat(connection-limit): set bypass rules for connection by PeerId or Multiaddr~~ feat(connection-limit): set bypass rules for connections Dec 6, 2024

drHuangMHT marked this pull request as draft December 6, 2024 08:06

elenaf9 reviewed Dec 6, 2024

View reviewed changes

dariusc93 and others added 4 commits December 15, 2024 19:54

Merge branch 'master' into connection-limit-whitelist

05c757f

Merge branch 'master' into connection-limit-whitelist

cbf009c

Merge branch 'master' into connection-limit-whitelist

399fd34

changelog and documentation

639c3d2

elenaf9 reviewed Jan 4, 2025

View reviewed changes

misc/connection-limits/src/lib.rs Outdated Show resolved Hide resolved

misc/connection-limits/src/lib.rs Outdated Show resolved Hide resolved

drHuangMHT added 3 commits January 10, 2025 12:13

replace multiaddr bypass with filter

227b062

Merge branch 'master' into connection-limit-whitelist

d9901ee

reduce diff

efa83b1

fix: bypassed pending connections still take up slots

drHuangMHT requested a review from elenaf9 January 23, 2025 12:43

fix incorrect condition

47b81c3

elenaf9 reviewed Jan 26, 2025

View reviewed changes

bypass by PeerId only

aa01b26

jxs reviewed Jan 31, 2025

View reviewed changes

dariusc93 reviewed Jan 31, 2025

View reviewed changes

drHuangMHT added 2 commits February 1, 2025 15:44

move bypass list into Behaviour

092f313

minor version bump

2836212

elenaf9 approved these changes Feb 1, 2025

View reviewed changes

Merge branch 'master' into connection-limit-whitelist

368313c

dariusc93 reviewed Feb 1, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(connection-limit): set bypass rules for connections #5720

feat(connection-limit): set bypass rules for connections #5720

drHuangMHT commented Dec 6, 2024 •

edited

Loading

drHuangMHT commented Dec 6, 2024

elenaf9 left a comment

drHuangMHT commented Dec 6, 2024

drHuangMHT commented Dec 9, 2024

elenaf9 left a comment

dariusc93 commented Jan 9, 2025

elenaf9 Jan 26, 2025

drHuangMHT Jan 27, 2025

dknopik Jan 30, 2025

elenaf9 Jan 31, 2025

jxs Jan 31, 2025

dariusc93 left a comment

dariusc93 Jan 31, 2025

elenaf9 Feb 1, 2025

dariusc93 Feb 1, 2025

dariusc93 Jan 31, 2025

elenaf9 left a comment

dariusc93 left a comment

feat(connection-limit): set bypass rules for connections #5720

Are you sure you want to change the base?

feat(connection-limit): set bypass rules for connections #5720

Conversation

drHuangMHT commented Dec 6, 2024 • edited Loading

Description

Notes & open questions

Change checklist

drHuangMHT commented Dec 6, 2024

elenaf9 left a comment

Choose a reason for hiding this comment

drHuangMHT commented Dec 6, 2024

drHuangMHT commented Dec 9, 2024

elenaf9 left a comment

Choose a reason for hiding this comment

dariusc93 commented Jan 9, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dariusc93 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elenaf9 left a comment

Choose a reason for hiding this comment

dariusc93 left a comment

Choose a reason for hiding this comment

drHuangMHT commented Dec 6, 2024 •

edited

Loading