limits number of nodes per IP address in Turbine #864
Conversation
| @@ -39,6 +40,9 @@ use { | |||
| const DATA_PLANE_FANOUT: usize = 200; | |||
| pub(crate) const MAX_NUM_TURBINE_HOPS: usize = 4; | |||
|
|
|||
| // Limit number of nodes per IP address. | |||
| const MAX_NUM_NODES_PER_IP_ADDRESS: usize = 10; | |||
There was a problem hiding this comment.
what's the usecase for more than one? local-cluster or something?
There was a problem hiding this comment.
Multiple nodes running behind NAT probably.
Currently I see several nodes on mainnet with the same IP address; in particular one IP address with 5 nodes.
There was a problem hiding this comment.
lemme guess... all with ~40kSOL stake. sfdp sybils...
There was a problem hiding this comment.
I have not checked their stake.
Either way, it is the status quo and we cannot break it with this commit because it was not disallowed before.
| || node | ||
| .contact_info() | ||
| .and_then(|node| node.tvu(Protocol::UDP).ok()) | ||
| .map(|addr| { | ||
| *counts | ||
| .entry(addr.ip()) | ||
| .and_modify(|count| *count += 1) | ||
| .or_insert(1) | ||
| }) |
There was a problem hiding this comment.
can we pop the query out to a closure for the sake of readability? nfc why fmt prefers something like this
There was a problem hiding this comment.
I get lifetime errors with an out-of-line closure.
There was a problem hiding this comment.
🤔
are you capturing counts instead of passing it in?
There was a problem hiding this comment.
Passing it might work but it definitely becomes a more verbose code.
I think the inline version is better anyways because it is explicit what is it doing and don't need to jump elsewhere to figure that out.
|
@t-nelson we probably need to ship this sooner. |
|
Backports to the beta branch are to be avoided unless absolutely necessary for fixing bugs, security issues, and perf regressions. Changes intended for backport should be structured such that a minimum effective diff can be committed separately from any refactoring, plumbing, cleanup, etc that are not strictly necessary to achieve the goal. Any of the latter should go only into master and ride the normal stabilization schedule. Exceptions include CI/metrics changes, CLI improvements and documentation updates on a case by case basis. |
(cherry picked from commit c87b830)
(cherry picked from commit c87b830)
This was erronously deemed unnecessary and removed in: anza-xyz#864 The commit partially reverts solana-labs#864 and adds back socket-addr dedup.
This was erronously deemed as unnecessary and removed in: anza-xyz#864 The commit partially reverts solana-labs#864 and adds back socket-addr dedup.
This was erronously deemed as unnecessary and removed in: anza-xyz#864 The commit partially reverts solana-labs#864 and adds back socket-addr dedup.
… of #1106) (#1225) reverts back in SocketAddr dedup in retransmit stage (#1106) This was erronously deemed as unnecessary and removed in: anza-xyz#864 The commit partially reverts #864 and adds back socket-addr dedup. (cherry picked from commit fbe1dbc) Co-authored-by: behzad nouri <behzadnouri@gmail.com>
…nza-xyz#864) (anza-xyz#978) limits number of nodes per IP address in Turbine (anza-xyz#864) (cherry picked from commit c87b830) Co-authored-by: behzad nouri <behzadnouri@gmail.com>
Problem
Nodes congestion.
Summary of Changes
Limit to max 10 nodes per IP address in Turbine.