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

fix:dot/network: fix grandpa protocol ID #1466

Merged
merged 6 commits into from
Mar 16, 2021
Merged

Conversation

noot
Copy link
Contributor

@noot noot commented Mar 15, 2021

Changes

  • previously all stream protocols were prefixed with the "base" protocol ID for example /ksmcc3, however the grandpa protocol is not prefixed, the protocol ID is just /paritytech/grandpa/1
  • this will be updated in the future but for now it is not correct, see Change the name of the GrandPa notifications protocol paritytech/substrate#7252
  • this PR allows notifications protocols to be registered with a protocol ID that "overwrites" ie. doesn't prepend the base protocol
  • updates network functions to use the full protocol ID

Tests

go test ./dot/network -short

Issues

@noot noot self-assigned this Mar 15, 2021
@codecov
Copy link

codecov bot commented Mar 15, 2021

Codecov Report

Merging #1466 (167ba30) into development (d8aa6ab) will increase coverage by 0.14%.
The diff coverage is 59.64%.

Impacted file tree graph

@@               Coverage Diff               @@
##           development    #1466      +/-   ##
===============================================
+ Coverage        57.70%   57.85%   +0.14%     
===============================================
  Files              152      152              
  Lines            15315    15226      -89     
===============================================
- Hits              8838     8809      -29     
+ Misses            4931     4873      -58     
+ Partials          1546     1544       -2     
Flag Coverage Δ
unit-tests 57.85% <59.64%> (+0.14%) ⬆️

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

Impacted Files Coverage Δ
dot/sync/syncer.go 44.86% <ø> (ø)
lib/runtime/life/resolver.go 70.33% <ø> (+4.43%) ⬆️
lib/runtime/types.go 0.00% <ø> (ø)
lib/runtime/wasmer/imports.go 43.87% <25.00%> (-0.48%) ⬇️
dot/network/notifications.go 57.69% <38.88%> (ø)
dot/network/service.go 58.10% <58.33%> (+0.17%) ⬆️
dot/network/host.go 69.59% <63.63%> (+3.14%) ⬆️
dot/state/service.go 46.18% <100.00%> (ø)
lib/grandpa/network.go 53.84% <100.00%> (+1.21%) ⬆️
lib/runtime/storage/trie.go 45.00% <100.00%> (+6.11%) ⬆️
... and 2 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 b384038...167ba30. Read the comment docs.

Comment on lines -280 to -300
func (h *host) broadcast(msg Message) {
for _, p := range h.peers() {
err := h.send(p, "", msg)
if err != nil {
logger.Error("Failed to broadcast message to peer", "peer", p, "error", err)
}
}
}

// broadcastExcluding sends a message to each connected peer except specified peer
func (h *host) broadcastExcluding(msg Message, peer peer.ID) { //nolint
for _, p := range h.peers() {
if p != peer {
err := h.send(p, "", msg)
if err != nil {
logger.Error("Failed to send message during broadcast", "peer", p, "err", err)
}
}
}
}

Copy link
Member

Choose a reason for hiding this comment

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

I'm assuming these are being removed because they're not needed, is there no longer a need to broadcast messages?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, they weren't actually being used anywhere. notifications messages are still being "broadcast" (gossiped) in the broadcastExcluding function in notifications.go

@noot noot merged commit 34befe0 into development Mar 16, 2021
@noot noot deleted the noot/fix-finality-protocol branch March 16, 2021 21:36
github-actions bot pushed a commit that referenced this pull request Mar 16, 2021
noot: fix:dot/network: fix grandpa protocol ID (#1466)
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.

None yet

4 participants