Skip to content
This repository was archived by the owner on Nov 15, 2023. It is now read-only.

Conversation

@xlc
Copy link
Contributor

@xlc xlc commented Dec 6, 2018

A block request can result an empty response if the requested hight is too high, which shouldn't resulting the peer get banned just because it is behind

I have a 3 node network setup running for two days, today one of the node is lagging behind and it is not able to catch up. Some logs:

018-12-06 01:19:12 Starting consensus session on top of parent 0x88ddfc295c254e6f645e44a82dd11473fb8740775d99e0702b0518f2cb510648
2018-12-06 01:19:12 Rejected connection from disabled peer: PeerId(QmbADoJzP16EoAudVusknELEsbm4FAYQWgWkNjPuEYTWwz)
2018-12-06 01:19:12 Proposing block [number: 15518; hash: 0xab16…e845; parent_hash: 0x88dd…0648; extrinsics: [0xbd4e…e1f0, 0x7b4e…2d59]]
2018-12-06 01:19:12 Imported #15518 (0xeb2e…9fef)
2018-12-06 01:19:14 Rejected connection from disabled peer: PeerId(QmbADoJzP16EoAudVusknELEsbm4FAYQWgWkNjPuEYTWwz)
2018-12-06 01:19:14 Purposefully dropping 10513 ; reason: Bad("Peer is on different chain (our genesis: 0x23de…6e62 theirs: 0x03a1…8353)")
2018-12-06 01:19:14 Banned PeerId(QmXboRGHd5ZG51LMGwqGnbocXZGnTwbp3eoUkZqBJYaEfM)
2018-12-06 01:19:16 Rejected connection from disabled peer: PeerId(QmbADoJzP16EoAudVusknELEsbm4FAYQWgWkNjPuEYTWwz)
2018-12-06 01:19:16 Idle (3 peers), best: #15518 (0xeb2e…9fef)
2018-12-06 01:19:18 Rejected connection from disabled peer: PeerId(QmbADoJzP16EoAudVusknELEsbm4FAYQWgWkNjPuEYTWwz)
2018-12-06 01:19:21 Rejected connection from disabled peer: PeerId(QmbADoJzP16EoAudVusknELEsbm4FAYQWgWkNjPuEYTWwz)

A block request can result an empty response if the requested hight is too high, which shouldn't resulting the peer get banned just becuase it is behind
@parity-cla-bot
Copy link

It looks like @xlc signed our Contributor License Agreement. 👍

Many thanks,

Parity Technologies CLA Bot

@arkpar
Copy link
Member

arkpar commented Dec 6, 2018

Ideally peers should not be requesting past min(our_best, theirs_best) when starting ancestry search. The proposed fix can only help if the node that's behind somehow ended up on a fork. I wonder how this happened in the first place. Do you have full logs with -l sync=trace to share?

@xlc
Copy link
Contributor Author

xlc commented Dec 6, 2018

I only have standard logs, which doesn't contain much useful details.
I will add -l sync=trace to the nodes.
What is the recommend logging level for this kind of setups?

@xlc
Copy link
Contributor Author

xlc commented Dec 7, 2018

This does have an issue that is one of the peer is very behind (in erroneous state), it will keep trying to find common ancestor block with linear algorithm, which going to take very very long time (#1229)
The previous behavior is just ban the behind node, which don't have this issue.

@arkpar
Copy link
Member

arkpar commented Dec 7, 2018

Right, with the state pruning the node can only resolve forks up to a certain length. This needs to be checked before attempting ancestry search.

What is the recommend logging level for this kind of setups?

Logs are only needed if you run into issues. trace gives you the most details.

@gavofyork gavofyork requested a review from arkpar December 8, 2018 05:37
None => {
if n > As::sa(0) {
trace!(target:"sync", "Ancestry block not found for peer {}: block #{}", who, n);
let n = n - As::sa(1);
Copy link
Member

Choose a reason for hiding this comment

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

One::one() is more idiomatic.

Suggested change
let n = n - As::sa(1);
let n = n - One::one();

@arkpar
Copy link
Member

arkpar commented Dec 10, 2018

This should be fixed by #1235
Thank you for looking into it nevertheless.

@arkpar arkpar closed this Dec 10, 2018
@xlc xlc deleted the fix-syncing branch December 10, 2018 21:06
lamafab pushed a commit to lamafab/substrate that referenced this pull request Jun 16, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants