-
Notifications
You must be signed in to change notification settings - Fork 971
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
feat(share/p2p/peer-manager): use shrexSub peers as full nodes #2105
Conversation
1e3c701
to
b0ce602
Compare
There was a problem hiding this 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 ?
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 |
All tests failing here @walldiss |
There was a problem hiding this 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.
I second what @Wondertan said, lets convert to draft pls @walldiss 🙏🏻 |
@Wondertan @renaynay Are you worried about any potential negative effects that may arise from the changes that have been implemented? |
@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. |
dfb6355
to
f1f2af1
Compare
Let's rebase this |
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 |
f1f2af1
to
5d740c9
Compare
eccffb8
to
c952803
Compare
…g peer manager cooldown issue
…g peer manager cooldown issue
There was a problem hiding this 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
✅ LGTM! Can't approve |
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
…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 <[email protected]> Co-authored-by: Hlib Kanunnikov <[email protected]>
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.