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

feat(lib/grandpa): update grandpa catch-up process; add submodule, use NeighbourMessages #1554

Closed
wants to merge 67 commits into from

Conversation

noot
Copy link
Contributor

@noot noot commented May 3, 2021

Changes

  • add catchUp submodule to track status of catch up and deal with sending out catch-up requests and handling responses
  • update grandpa message handler to use NeighbourMessages to trigger catch-up process if needed
  • TODO: still need to test this w/ polkadot (seems we aren't getting responses, probably continue this is another PR)
  • TODO: need to add test cases and update stress test

Tests

go test ./lib/grandpa -short

Issues

@noot noot self-assigned this May 3, 2021
@codecov
Copy link

codecov bot commented May 3, 2021

Codecov Report

Merging #1554 (a190d03) into development (2f9f80c) will decrease coverage by 1.67%.
The diff coverage is 33.71%.

❗ Current head a190d03 differs from pull request most recent head 550a625. Consider uploading reports for the commit 550a625 to get more accurate results
Impacted file tree graph

@@               Coverage Diff               @@
##           development    #1554      +/-   ##
===============================================
- Coverage        60.62%   58.94%   -1.68%     
===============================================
  Files              202      185      -17     
  Lines            27286    19404    -7882     
===============================================
- Hits             16541    11438    -5103     
+ Misses            8845     5980    -2865     
- Partials          1900     1986      +86     
Flag Coverage Δ
unit-tests 58.94% <33.71%> (-1.68%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
dot/network/service.go 69.12% <0.00%> (+11.15%) ⬆️
lib/grandpa/network.go 52.03% <0.00%> (+5.26%) ⬆️
lib/grandpa/catch_up.go 31.06% <31.06%> (ø)
dot/network/notifications.go 65.47% <37.50%> (-0.38%) ⬇️
lib/grandpa/message_handler.go 69.37% <61.53%> (+4.94%) ⬆️
lib/grandpa/grandpa.go 59.87% <100.00%> (+1.38%) ⬆️
dot/network/gossip.go 37.50% <0.00%> (-62.50%) ⬇️
dot/network/pool.go 50.00% <0.00%> (-50.00%) ⬇️
dot/state/transaction.go 58.33% <0.00%> (-21.36%) ⬇️
dot/types/consensus_digest.go 0.00% <0.00%> (-17.86%) ⬇️
... and 228 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2f9f80c...550a625. Read the comment docs.

@danforbes
Copy link
Contributor

It seems that the GRANDPA catch-up mechanism was implemented before request/response protocols were defined. I believe that these messages should be sent as "request notifications" and "response notifications".

@danforbes danforbes removed the wip label Mar 15, 2022
@danforbes danforbes closed this Aug 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

lib/grandpa: ensure catch-up logic works when interacting with substrate nodes
3 participants