-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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(net): improve chain inventory generating logic #5393
Conversation
LinkedList<BlockId> ids = getBlockIds(unForkId.getNum()); | ||
|
||
if (ids.isEmpty() || !unForkId.equals(ids.peekFirst())) { | ||
unForkId = getUnForkId(blockIds); |
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 I'm not mistaken, is it just a retry?
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.
Yes, it can be solved by retrying.
@@ -64,6 +65,21 @@ public void testProcessMessage() throws Exception { | |||
method.setAccessible(true); | |||
boolean f = (boolean)method.invoke(handler, peer, message); | |||
Assert.assertTrue(!f); | |||
|
|||
Method method1 = handler.getClass().getDeclaredMethod( |
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.
Is it possible to reproduce this scenario without using reflection?
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.
Okay, further optimization.
It only happened with the forks which are not finalized? If so, all the blocks on both(or more) forks are stored in memory? If all the above is right, just sending the corresponding blocks can solve this thoroughly? |
yes
This problem will only involve non-solidified blocks.
After the chain switch is completed, the generated inventory is without any problems. |
I mean generating the inventory by iterating the specified fork instead of using |
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.
Approved, @wubin01 should do a double check
@@ -87,6 +87,18 @@ private boolean check(PeerConnection peer, SyncBlockChainMessage msg) throws P2p | |||
|
|||
private LinkedList<BlockId> getLostBlockIds(List<BlockId> blockIds) throws P2pException { |
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 does this PR do?
Retrying inventory generating if first blockId does not match the chain summary.
Why are these changes required?
This PR has been tested by:
Follow up
Extra details