Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(share/discovery): Discard method #2207

Merged
merged 3 commits into from
May 16, 2023
Merged

Conversation

Wondertan
Copy link
Member

Fixes #2204

Regarding the case where we exhausted all the peers on the network and backed them all off. It can happen on the smaller network, and in the worst case, the node will wait 10 minutes(the default backoff time), which is acceptable considering peers from shrexsub #2105

@Wondertan Wondertan added area:shares Shares and samples kind:feat Attached to feature PRs labels May 15, 2023
@Wondertan Wondertan self-assigned this May 15, 2023
@walldiss
Copy link
Member

It seems like with current solution peer could be rediscovered right away. Could we move discarded peer to some set that will prevent it from be in discovery set?

@walldiss
Copy link
Member

Nvm, answer was in description. I'm a bit sleepy and skipped reading it

@Wondertan
Copy link
Member Author

...

=== RUN   TestIntegration/get_peer_from_discovery
    manager_test.go:446: 
        	Error Trace:	/home/runner/work/celestia-node/celestia-node/share/p2p/peers/manager_test.go:446
        	Error:      	Received unexpected error:
        	            	context deadline exceeded
        	Test:       	TestIntegration/get_peer_from_discovery

walldiss
walldiss previously approved these changes May 15, 2023
@distractedm1nd
Copy link
Member

For the integration it may be easier if the Discard API returns a boolean, if it was in discovery or not. This is because:

For removing a peer from the pool, we rely on discovery to call our callback, which calls pool.remove
If we call disc.Discard(peer), it will call pool.remove through the callback
But not all peers are from discovery, so if Discard didnt get rid of the peer we need to call pool.remove ourselves

Not ideal but its a solution

@Wondertan Wondertan enabled auto-merge (squash) May 16, 2023 10:54
@Wondertan Wondertan merged commit ac0f0e3 into main May 16, 2023
12 of 14 checks passed
@Wondertan Wondertan deleted the hlib/discovery-discard branch May 16, 2023 10:57
vgonkivs pushed a commit to vgonkivs/celestia-node that referenced this pull request May 22, 2023
Fixes celestiaorg#2204

Regarding the case where we exhausted all the peers on the network and backed them all off. It can happen on the smaller network, and in the worst case, the node will wait 10 minutes(the default backoff time), which is acceptable considering peers from shrexsub celestiaorg#2105
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:shares Shares and samples kind:feat Attached to feature PRs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

share/discovery: DiscardPeer method
4 participants