feat: broadcast size should not be capped for newly connected neighbors #3367
Conversation
istae
requested review from
a team,
vladopajic and
metacertain
and removed request for
a team
istae
changed the title
There was a problem hiding this comment.
I don't think this is a good idea. This will exponentially increase the no. of protocol messages in the network. Each neighbor on getting the new peer will again be broadcasting to all the neighborhood. This will lead to a lot of duplicate broadcasts.
In order to test this, we should simulate peers connecting/disconnecting in the load test cluster and observe how much this affects the protocol messages for hive.
We have an out/in rate limiter in hive so it's okay :) Hive relative to the other protocols has a very small footprint. Plus neighborhoods are small so we should not limit the peer count when announcing them to the new neighbor. |
We need to test and verify all the assumptions here. We can have bunch of other problems even if we rate-limit like blocklisting/libp2p stream related problems etc. |
I feel that we are blowing out the proportions of this change significantly....we literally have an out limiter that blocks us from sending more than 128 peers a minute. I appreciate the cautious approach, but it's a bit crippling. |
So I dont think this is that simple of a change. This change affects the no of protocol messages nodes will exchange. This is very significant according to me as this can break the network if something goes wrong. Also, I dont see why we need this change. Kademlia build-up is not a problem, has never been. With the libp2p changes coming in, I would prefer if we are extra cautious. This is known to cause issues due to more streams etc which is what will happen. |
|
For correctness sake, a newly connected neighbor should receive a full list of connected neighbors. |
| t.Parallel() | ||
|
|
||
| defer func(p int) { | ||
| *kademlia.SaturationPeers = p | ||
| }(*kademlia.SaturationPeers) | ||
| *kademlia.SaturationPeers = 4 | ||
|
|
||
| defer func(p int) { | ||
| *kademlia.OverSaturationPeers = p | ||
| }(*kademlia.OverSaturationPeers) | ||
| *kademlia.OverSaturationPeers = 4 | ||
|
|
There was a problem hiding this comment.
Test which modify global variables shouldn't be executed in parallel because race condition may occur in which case one test function (executed in parallel) my affect expected variable of another test function.
PR #3360 fixes hacky usage of global variables in kadeemlia (which makes possible for test to execute in parallel).
There was a problem hiding this comment.
@vladopajic Let's merge this, then can you also take a look at this test since your PR takes care of the global variables.
Checklist
Description
Kademlia randomly picks 4 peers from each bin to broadcast when a new connection is made. If the connected peer is a neighbor, however, the broadcast peer size should not be capped.
Open API Spec Version Changes (if applicable)
Motivation and Context (Optional)
Related Issue (Optional)
Screenshots (if appropriate):
This change is