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

eth/downloader: not fetch older peer #74

Open
wants to merge 1 commit into
base: develop/5.0.x
Choose a base branch
from

Conversation

zhiqiangxu
Copy link
Contributor

Seems there's no point trying to fetch a peer whose td is smaller.

@fanbsb
Copy link
Member

fanbsb commented Oct 14, 2022

Not sure the change is needed or not. Because before fetch the part, the downloader already check the peer's TD, if less than local, it will drop the peers.

BTW, the eth/downloader module was forked from ethereum, I didn't see the same logic on the their side.

@zhiqiangxu
Copy link
Contributor Author

Not sure the change is needed or not. Because before fetch the part, the downloader already check the peer's TD, if less than local, it will drop the peers.

BTW, the eth/downloader module was forked from ethereum, I didn't see the same logic on the their side.

Hopefully this PR will fix an issue I met: I set up two nodes, one(call it A) only connects to the other(call it B), while B connects to others via bootstrap nodes. I saw A often disconnected by B when chosen as an idle peer to fetch from.

@fanbsb
Copy link
Member

fanbsb commented Oct 14, 2022

I will try to reproduce the issue later, see if the issue fixed by this pr or not

@fanbsb fanbsb added the question Further information is requested label Nov 3, 2022
@fanbsb
Copy link
Member

fanbsb commented Nov 3, 2022

Have you tried to add the Node B as a static node in Node A's peer list?

@zhiqiangxu
Copy link
Contributor Author

Have you tried to add the Node B as a static node in Node A's peer list?

Yes,A has B in static peer list, and disabled discovery so it only connects B.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants