fix: pushsync disabled for bootnode #3616
Conversation
vladopajic
requested review from
a team,
aloknerurkar,
notanatol and
istae
and removed request for
a team
pkg/node/node.go
Outdated
| @@ -921,7 +921,8 @@ func NewBee(interrupt chan struct{}, sysInterrupt chan os.Signal, addr string, p | |||
|
|
|||
| pinningService := pinning.NewService(storer, stateStore, traversalService) | |||
|
|
|||
| pushSyncProtocol := pushsync.New(swarmAddress, nonce, p2ps, storer, kad, tagService, o.FullNodeMode, pssService.TryUnwrap, validStamp, logger, acc, pricer, signer, tracer, warmupTime) | |||
There was a problem hiding this comment.
I wonder if we can get away with not even registering the protocol for bootnodes 🤔
There was a problem hiding this comment.
@aloknerurkar @mrekucci @notanatol wdyt?
There was a problem hiding this comment.
@aloknerurkar @mrekucci @notanatol wdyt?
we could do that but I feel that having a switch in place is a more "fine grained" solution
There was a problem hiding this comment.
Sorry for not getting back!
So @istae is right. This will only disable storing on pushsync. However we will still see pushsync protocol traffic. I think the intention of this issue is to not have that as well. I think it would be better to disable the protocol itself, similar to how we do for pullsync i.e. not register the protocol on the libp2p host.
@janos Could there be any issues if we dont register this protocol? We might start seeing more errors for pushsync? Or would the nodes understand that this peer doesnt have the protocol and not send messages?
There was a problem hiding this comment.
While @notanatol worked on the lightnode mode implementation, p2p.WithBlocklistStreams function is implemented exactly for this purpose for more control on libp2p stream disabling, where a lot of new (unregstered) stream attempts could be performed by the peer and to be able to protect against that situation.
There was a problem hiding this comment.
@vladopajic let's go with p2p.WithBlocklistStreams
|
Since pushsync is now disabled for bootnodes, integration tests in beekeeper should be updated not to include bootnodes. |
Checklist
Closes: #3610
This change is