search

lightninglabs / neutrino

Privacy-Preserving Bitcoin Light Client
MIT License
894 stars 182 forks source link

refresh peer's rank on reconnect #280

Closed Crypt-iQ closed 7 months ago

Crypt-iQ commented 1 year ago

In peer_rank.go we never refresh a peer's rank if we reconnect. It will use the old ranking we had for it. If we try to query a tor node and penalize it due to high latency, we should allow reconnecting to refresh the rank: https://github.com/lightninglabs/neutrino/blob/cb61d7f9ff0381415acd88b841f74bfeb4b87e80/query/peer_rank.go#L57-L61

Chinwendu20 commented 11 months ago

I guess we can delete the peer address from.the map after it has been disconnected but disconnecting leads to punishment in the current design.

Crypt-iQ commented 11 months ago

I think it should just get refreshed on a reconnect, some people had problems with connecting to tor since the old ranking was used

Chinwendu20 commented 11 months ago

Old Ranking? Was there a different ranking before this? Just checked this PR: https://github.com/lightninglabs/neutrino/pull/179 and it seems this current ranking was introduced same time as the work dispatcher.

ellemouton commented 11 months ago

@Chinwendu20 - he means the rank value

Chinwendu20 commented 11 months ago

Thanks @ellemouton, we can fix this by deleting the peer from the ranking map when it disconnects, changing the current design. I can work on this but I do not know if the issue needs further triaging.

Crypt-iQ commented 11 months ago

That would work, but I think just resetting the value to defaultScore in AddPeer is a smaller patch. Maybe not as correct, but still should work

Chinwendu20 commented 11 months ago

Yeah that sounds great.

Chinwendu20 commented 11 months ago

Please I have a question about this, why is it important to fetch from this particular peer. If a peer has a bad score and other peers are present they can take on that task otherwise, the workmanager would still schedule the task to the bad scored peer if that peer is the only one on the list or all peers present have same score.

Crypt-iQ commented 11 months ago

Hmm, I don't remember anymore. @Roasbeef do you think this should be addressed given https://github.com/lightninglabs/neutrino/issues/280#issuecomment-1703914567

Roasbeef commented 8 months ago

. If a peer has a bad score and other peers are present they can take on that task otherwise, the workmanager would still schedule the task to the bad scored peer if that peer is the only one on the list or all peers present have same score.

Yeah @Chinwendu20 is right here imo, as is we still uniformly schedule tasks to all peers when we have a batch. If it's a series of one-off requests, then the highest ranking node will get the task. However if we have a batch of items, due to the way the initial allocation logic is setup, the load will be distributed to all peers.

Making a more specific issue with an implementation plan for improving the state of the art here.