-
Notifications
You must be signed in to change notification settings - Fork 129
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(lib/grandpa): ensure catch-up logic works #2152
Conversation
- ensure catch-up logic works when interacting with substrate nodes - If round in the neighbour message is ahead of our current round by a threshold, send a catch up request - process the catch up response, if we can't process it at the moment, store it to process later. Closes #1531
got it working on devnet
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.
basically looks good, the main problem i see is that handling the response isn't thread safe, there is a very high chance we will be sending out multiple requests and getting multiple responses at handling them concurrently. you could add something to check if catch up is currently in progress already, and if so, then don't send out any more requests (see https://github.com/ChainSafe/gossamer/pull/1554/files) it would also be nice to split up the catch-up logic into its own file/module. it would also be nice to make catch up synchronous (the way block request-responses are), this would also prevent multiple requests/responses being sent out/handled at once unnecessarily
Co-authored-by: Eclésio Junior <eclesiomelo.1@gmail.com>
Co-authored-by: Quentin McGaw <quentin.mcgaw@gmail.com>
Co-authored-by: Quentin McGaw <quentin.mcgaw@gmail.com>
Co-authored-by: Quentin McGaw <quentin.mcgaw@gmail.com>
Co-authored-by: Quentin McGaw <quentin.mcgaw@gmail.com>
Co-authored-by: Quentin McGaw <quentin.mcgaw@gmail.com>
Co-authored-by: Quentin McGaw <quentin.mcgaw@gmail.com>
Co-authored-by: Quentin McGaw <quentin.mcgaw@gmail.com>
Co-authored-by: Quentin McGaw <quentin.mcgaw@gmail.com>
Co-authored-by: Quentin McGaw <quentin.mcgaw@gmail.com>
Co-authored-by: Quentin McGaw <quentin.mcgaw@gmail.com>
Co-authored-by: Quentin McGaw <quentin.mcgaw@gmail.com>
Co-authored-by: Quentin McGaw <quentin.mcgaw@gmail.com>
Co-authored-by: Quentin McGaw <quentin.mcgaw@gmail.com>
Co-authored-by: Quentin McGaw <quentin.mcgaw@gmail.com>
Co-authored-by: Quentin McGaw <quentin.mcgaw@gmail.com>
Co-authored-by: Quentin McGaw <quentin.mcgaw@gmail.com>
Co-authored-by: Quentin McGaw <quentin.mcgaw@gmail.com>
Co-authored-by: Quentin McGaw <quentin.mcgaw@gmail.com>
Co-authored-by: Quentin McGaw <quentin.mcgaw@gmail.com>
While, testing out a gossamer network of 4 node nodes, nodes stop with I tried eclecio's networking race condition PR with this branch (#2105), but that did not fix the problem. |
@kishansagathiya can you make this a draft PR if you don't intend to merge anytime soon? |
too old, closing this. |
Changes
threshold, send a catch up request
store it to process later.
Tests
Issues
Primary Reviewer