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/p2p/peer-manager): use shrexSub peers as full nodes #2105

Merged

Conversation

walldiss
Copy link
Member

@walldiss walldiss commented Apr 19, 2023

Problem

#2107

Overview

Could hotfix the problem by allowing peer manager to save nodes discovered through shrexSub to be used in full nodes pool.
The solution would also benefit shrex performance by allowing distribution of request to more peers.

@walldiss walldiss added area:shares Shares and samples area:p2p labels Apr 19, 2023
@walldiss walldiss self-assigned this Apr 19, 2023
@walldiss walldiss force-pushed the use_shrex_peer_for_full_nodes branch 2 times, most recently from 1e3c701 to b0ce602 Compare April 19, 2023 16:41
Copy link
Member

@renaynay renaynay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do you want to convert this to draft for now @walldiss ?

share/getters/shrex.go Outdated Show resolved Hide resolved
@Wondertan
Copy link
Member

Wondertan commented Apr 20, 2023

Let's add more description as to why we are doing this. It's pretty important protocol change and while we don't have an issue, we should document the change on the PR

@walldiss walldiss requested a review from renaynay April 20, 2023 14:40
share/getters/shrex.go Outdated Show resolved Hide resolved
share/p2p/peers/manager.go Outdated Show resolved Hide resolved
@walldiss walldiss requested a review from renaynay April 21, 2023 11:00
@renaynay
Copy link
Member

All tests failing here @walldiss

@renaynay renaynay added the kind:feat Attached to feature PRs label Apr 25, 2023
share/getters/shrex.go Outdated Show resolved Hide resolved
Copy link
Member

@Wondertan Wondertan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the discovery fix will be enough to address the original issue we faced. I want to release without this solution first, see the results and reanalyze if we need this.

@renaynay
Copy link
Member

I second what @Wondertan said, lets convert to draft pls @walldiss 🙏🏻

@walldiss
Copy link
Member Author

@Wondertan @renaynay Are you worried about any potential negative effects that may arise from the changes that have been implemented?

@walldiss walldiss marked this pull request as draft April 26, 2023 11:48
@Wondertan
Copy link
Member

@walldiss, I am worried about the added complexity of something that is optional and is not part of the original design. We should add if it's proven to be beneficial.

@Wondertan
Copy link
Member

Let's rebase this

@Wondertan
Copy link
Member

A thought. I am still worried that we have two places where we keep the current state of FN we rely on. This PR makes this discrepancy even worse. Discovery will have its own peer set and PeerManager, but this time they are not just copied but different. The constancies seem to me obvious, but I can explain if needed.

I think that during future refactors, we should design shrex in a monolithic way. Where it has multiple coupled and internalized(via internal pkg) components, but still, everyone manages their own state, so it would have a single FullNodeSet allowing multiple discovery mechanisms to contribute to it, and everyone can access it.

share/getters/shrex.go Outdated Show resolved Hide resolved
share/p2p/peers/manager.go Show resolved Hide resolved
share/getters/shrex.go Outdated Show resolved Hide resolved
share/p2p/peers/manager.go Outdated Show resolved Hide resolved
share/p2p/peers/manager.go Show resolved Hide resolved
share/p2p/peers/pool.go Outdated Show resolved Hide resolved
Wondertan
Wondertan previously approved these changes May 16, 2023
share/getters/shrex.go Outdated Show resolved Hide resolved
share/p2p/peers/manager.go Outdated Show resolved Hide resolved
Wondertan added a commit that referenced this pull request May 16, 2023
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
share/p2p/peers/manager.go Show resolved Hide resolved
share/p2p/peers/manager.go Outdated Show resolved Hide resolved
@walldiss walldiss marked this pull request as ready for review May 17, 2023 15:05
share/p2p/peers/manager.go Outdated Show resolved Hide resolved
share/p2p/peers/manager.go Outdated Show resolved Hide resolved
share/availability/discovery/discovery.go Outdated Show resolved Hide resolved
share/p2p/peers/manager.go Show resolved Hide resolved
share/p2p/peers/manager.go Outdated Show resolved Hide resolved
share/p2p/peers/manager.go Outdated Show resolved Hide resolved
Copy link
Member Author

@walldiss walldiss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, just few small comments

share/p2p/peers/manager.go Outdated Show resolved Hide resolved
share/p2p/peers/manager.go Outdated Show resolved Hide resolved
share/p2p/peers/manager.go Outdated Show resolved Hide resolved
share/p2p/peers/manager.go Outdated Show resolved Hide resolved
renaynay
renaynay previously approved these changes May 19, 2023
share/p2p/peers/manager.go Show resolved Hide resolved
share/p2p/peers/manager.go Outdated Show resolved Hide resolved
@walldiss
Copy link
Member Author

✅ LGTM! Can't approve

@distractedm1nd distractedm1nd merged commit d4e70e2 into celestiaorg:main May 22, 2023
12 of 14 checks passed
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
vgonkivs pushed a commit to vgonkivs/celestia-node that referenced this pull request May 22, 2023
…tiaorg#2105)

## Problem
celestiaorg#2107
## Overview
Could hotfix the problem by allowing peer manager to save nodes
discovered through shrexSub to be used in full nodes pool.
The solution would also benefit shrex performance by allowing
distribution of request to more peers.

---------

Co-authored-by: Ryan <ryan@celestia.org>
Co-authored-by: Hlib Kanunnikov <hlibwondertan@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:p2p area:shares Shares and samples kind:feat Attached to feature PRs
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

5 participants