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!: only consider backbone nodes for core protocols #2565

Merged
merged 57 commits into from
Nov 4, 2021
Merged

Conversation

istae
Copy link
Member

@istae istae commented Oct 4, 2021

fixes #2555
fixes #2550

This change is Reviewable

Copy link
Member

@acud acud left a comment

Choose a reason for hiding this comment

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

Nice :) I have a few comments

pkg/reacher/reacher.go Outdated Show resolved Hide resolved
pkg/reacher/reacher.go Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented Oct 4, 2021

Codecov Report

Merging #2565 (4e41e2f) into master (e53b36c) will increase coverage by 0.26%.
The diff coverage is 87.26%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2565      +/-   ##
==========================================
+ Coverage   63.54%   63.81%   +0.26%     
==========================================
  Files         232      235       +3     
  Lines       25937    26222     +285     
==========================================
+ Hits        16482    16733     +251     
- Misses       7958     7987      +29     
- Partials     1497     1502       +5     
Impacted Files Coverage Δ
pkg/puller/puller.go 67.68% <ø> (ø)
pkg/pullsync/pullsync.go 48.33% <ø> (ø)
pkg/retrieval/retrieval.go 64.06% <ø> (-0.32%) ⬇️
pkg/p2p/libp2p/libp2p.go 61.60% <70.00%> (-0.03%) ⬇️
pkg/topology/kademlia/kademlia.go 75.54% <80.76%> (-0.54%) ⬇️
pkg/p2p/libp2p/internal/reacher/metrics.go 89.47% <89.47%> (ø)
pkg/p2p/libp2p/internal/reacher/reacher.go 92.15% <92.15%> (ø)
pkg/chainsyncer/chainsyncer.go 81.15% <100.00%> (+2.17%) ⬆️
pkg/p2p/p2p.go 100.00% <100.00%> (ø)
pkg/pushsync/pushsync.go 66.37% <100.00%> (+0.07%) ⬆️
... and 23 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 e53b36c...4e41e2f. Read the comment docs.

Copy link
Member

@acud acud left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 3 files at r1, 2 of 2 files at r2, 2 of 2 files at r3, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @istae)


pkg/blocker/blocker.go, line 91 at r3 (raw file):

	defer b.mux.Unlock()

	_, ok := b.peers[addr.ByteString()]

if _,ok := b.peers[addr.ByteString()]; !ok { b.peers[addr] = &peer.....}


pkg/reacher/reacher.go, line 1 at r3 (raw file):

package reacher

copyright missing

Copy link
Contributor

@mrekucci mrekucci left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 2 files at r3, 2 of 3 files at r4, 2 of 4 files at r7, 1 of 1 files at r8, 1 of 1 files at r9, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @acud and @istae)


pkg/reacher/metrics.go, line 25 at r9 (raw file):

			Subsystem: subsystem,
			Name:      "pings",
			Help:      "Ping counter.",

I'd suggest using phrases as: "The number of pings made."


pkg/reacher/metrics.go, line 30 at r9 (raw file):

			Namespace: m.Namespace,
			Subsystem: subsystem,
			Name:      "ping_timer",

I'd suggest using ping_duration


pkg/reacher/metrics.go, line 31 at r9 (raw file):

			Subsystem: subsystem,
			Name:      "ping_timer",
			Help:      "Ping timer.",

I'd suggest using phrases as: "The duration of a ping".

@acud
Copy link
Member

acud commented Oct 5, 2021

few comments:

  • reacher package should be internal to libp2p package
  • reacher.New should be called when SetPickyNotifier is called on libp2p with the kademlia instance (right now it is not wired at all)
  • interface in the p2p package should include both notifiers:
type PickyNotifier interface {
	Picker
	Notifier
	ReachabilityUpdater
	ReachableNotifier
}

Copy link
Contributor

@mrekucci mrekucci left a comment

Choose a reason for hiding this comment

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

Reviewed 9 of 20 files at r15, 8 of 14 files at r16, 6 of 6 files at r17, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @acud and @istae)

pkg/p2p/libp2p/internal/reacher/reacher.go Outdated Show resolved Hide resolved
pkg/topology/kademlia/kademlia.go Outdated Show resolved Hide resolved
@sonarqubecloud
Copy link

sonarqubecloud bot commented Nov 4, 2021

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 2 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@istae istae merged commit 554ab3e into master Nov 4, 2021
@istae istae deleted the reacher branch November 4, 2021 10:24
@ldeffenb
Copy link
Collaborator

ldeffenb commented Nov 4, 2021

@istae Just to be clear, since this PR has been merged into master, then master builds now contain the protocol version changes which makes it a breaking change and no longer compatible with the current testnet and mainnet swarms?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Monitor node reachability status Expose NAT status
7 participants