-
Notifications
You must be signed in to change notification settings - Fork 402
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
Fix sync peer disconnected when peer has no body #4267
Conversation
if (blocks == null || blocks.Length == 0) | ||
break; |
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.
Shouldn't we validate that all Roots of requested blocks are empty and if not disconnect the peer? So if we are requesting empty or not blocks?
That would check TxRoot, ReceiptsRoot and UncleHash.
Actually, we can do that before sending the request!
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 have no idea. I'm just replicating the logic of MergeBlockDownloader. I guess if all root is empty, then we need to create empty block to replace them? But what if the peer is just waiting to download block from one of it peers?
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.
We already have it here:
nethermind/src/Nethermind/Nethermind.Synchronization/Blocks/BlockDownloadContext.cs
Lines 62 to 72 in 82f331a
if (header.HasBody) | |
{ | |
Blocks[i - 1] = new Block(header); | |
_indexMapping.Add(currentBodyIndex, i - 1); | |
currentBodyIndex++; | |
NonEmptyBlockHashes.Add(header.Hash); | |
} | |
else | |
{ | |
Blocks[i - 1] = new Block(header, BlockBody.Empty); | |
} |
so why is that a problem? Some kind of edge case?
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.
If all roots are empty don't send the request and don't expect response
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.
Ok, I think the problem is that the headers returned is 1 instead of 2 which is the minimum.
914974a
to
dbacb69
Compare
if (headers.Length < 2) | ||
{ | ||
// Peer dont have new header | ||
break; | ||
} | ||
|
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.
Can you include more robust comment explanation of this scenario?
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.
What do you have in mind?
Fixes issue on kurtosis when peer return empty array on request body.
Types of changes
What types of changes does your code introduce?
Put an
x
in the boxes that applyTesting
Requires testing
In case you checked yes, did you write tests??
Comments about testing , should you have some (optional)
Further comments (optional)
If this is a relatively large or complex change, kick off the discussion by explaining why you chose the solution you did and what alternatives you considered, etc...