fix(i): Make peer connections deterministic #2888

nasdf · 2024-08-06T18:26:45Z

Relevant issue(s)

Resolves #2847
Resolves #1902

Description

This PR fixes an issue where peer connections were not deterministic within the test framework.

There's also a few other areas that were cleaned up:

Bootstrap has been replaced with https://github.com/ipfs/boxo/blob/main/bootstrap/bootstrap.go
Host setup logic was moved to net/host.go
Persistent peer store was replaced with in memory only version
Context reuse within the net package has been refactored
Removed unused peer event

Tasks

I made sure the code is well commented, particularly hard-to-understand areas.
I made sure the repository-held documentation is changed accordingly.
I made sure the pull request title adheres to the conventional commit style (the subset used in the project can be found in tools/configs/chglog/config.yml).
I made sure to discuss its limitations such as threats to validity, vulnerability to mistake and misuse, robustness to invalidation of assumptions, resource requirements, ...

How has this been tested?

make test

Specify the platform(s) on which this was tested:

MacOS

codecov · 2024-08-06T18:44:40Z

Codecov Report

Attention: Patch coverage is 74.78992% with 30 lines in your changes missing coverage. Please review.

Project coverage is 79.34%. Comparing base (495c456) to head (1654c0e).
Report is 1 commits behind head on develop.

Files	Patch %	Lines
net/peer.go	76.81%	11 Missing and 5 partials ⚠️
net/host.go	78.57%	3 Missing and 3 partials ⚠️
net/server.go	57.14%	5 Missing and 1 partial ⚠️
cli/start.go	0.00%	1 Missing ⚠️
node/node.go	0.00%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #2888      +/-   ##
===========================================
- Coverage    79.40%   79.34%   -0.06%     
===========================================
  Files          326      326              
  Lines        24870    24772      -98     
===========================================
- Hits         19747    19655      -92     
+ Misses        3716     3704      -12     
- Partials      1407     1413       +6

Flag	Coverage Δ
all-tests	`79.34% <74.79%> (-0.06%)`	⬇️

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

Files	Coverage Δ
event/event.go	`100.00% <ø> (ø)`
net/client.go	`100.00% <100.00%> (ø)`
net/config.go	`100.00% <100.00%> (ø)`
cli/start.go	`43.70% <0.00%> (+2.10%)`	⬆️
node/node.go	`53.01% <0.00%> (+1.36%)`	⬆️
net/host.go	`78.57% <78.57%> (ø)`
net/server.go	`76.19% <57.14%> (+0.48%)`	⬆️
net/peer.go	`81.09% <76.81%> (-1.40%)`	⬇️

... and 18 files with indirect coverage changes

Continue to review full report in Codecov by Sentry.

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

AndrewSisley · 2024-08-06T19:02:58Z

net/server.go

@@ -196,7 +195,7 @@ func (s *server) addPubSubTopic(topic string, subscribe bool) error {
 		}
 	}

-	t, err := rpc.NewTopic(s.peer.ctx, s.peer.ps, s.peer.host.ID(), topic, subscribe)
+	t, err := rpc.NewTopic(context.Background(), s.peer.ps, s.peer.host.ID(), topic, subscribe)


question: Are you sure this is an improvement? It does look like a suspicious change to me, I would have expected us to want to use the same context for all topics+peer

And the same goes for publishing to topics, and client.PushLog calls

I've always followed this rule from the Go docs:

Do not store Contexts inside a struct type; instead, pass a Context explicitly to each function that needs it.

Since there is no context to pass around in these functions they should be run with a background context..

In that case I really think we should ignore that suggestion here and use the context provided to the system. Please consider reverting this change a todo.

I don't think the context provided to the constructor makes sense either since it would outlive the function call. Would you be okay with creating a new background context that is then used for all publish / topic calls?

I don't think the context provided to the constructor makes sense either since it would outlive the function call.

Why does that not make sense? Any ~~context~~thing provided to a function outlives the function call

suggestion: If you are not happy about this, I suggest we discuss it in the standup.

I mean if I cancelled a context that I used to construct something, I'd want it to close all the things owned by that. I like that about the current setup.

If we didn't have explicit Start and Stop functions I would agree with this.

If we didn't have explicit Start and Stop functions I would agree with this.

I could rant for quite a while about how much I hate Start functions on objects with constructors... I don't see a Stop function though, although I could rant about that/those paired with Close functions too 😅.

Interesting discussion, I feel like we have discussed something similar before. While I do personally have a slight preference for Andy's approach. Since I read the Go guidelines regarding not storing contexts in structs I do tend to stick to those guidelines and am happy with this change. However I do wonder if it's possible to just pass the ctx to addPubSubTopic through a function parameter (most of the callers do have a ctx).

However I do wonder if it's possible to just pass the ctx to addPubSubTopic through a function parameter (most of the callers do have a ctx).

I'm open to this as long as that context is not the same as the constructor context.

AndrewSisley · 2024-08-06T19:12:59Z

net/config.go

@@ -24,6 +24,7 @@ type Options struct {
 	EnableRelay       bool
 	GRPCServerOptions []grpc.ServerOption
 	GRPCDialOptions   []grpc.DialOption
+	BootstrapPeers    []string


question: Sorry if I've misread, but I can't spot where this is actually read - is it dead code?

It's used here:

defradb/net/peer.go

Line 84 in f8032b3

peers := make([]peer.AddrInfo, len(options.BootstrapPeers))

shahzadlone · 2024-08-06T21:12:13Z

net/peer.go

+	for i, p := range options.BootstrapPeers {
+		addr, err := peer.AddrInfoFromString(p)
 		if err != nil {
 			return nil, err
 		}
-		options.PrivateKey = key
+		peers[i] = *addr
 	}


question: This for-loop is never entered in any test, is it possible to test this easily?

added a test for the non error case

shahzadlone · 2024-08-06T21:14:29Z

net/config.go

+// WithBootstrapPeers sets the bootstrap peer addresses to attempt to connect to.
+func WithBootstrapPeers(peers ...string) NodeOpt {
+	return func(opt *Options) {
+		opt.BootstrapPeers = peers
+	}
+}


suggestion: All other config functions are tested in this file except this one. Please add a test if not too much hassle.

added a test here

shahzadlone

Thanks for adding the requested tests, before merge please don't forget to handle the tidy check (just need to do make tidy) it's not required yet but will be nice to maintain a tidy state. Also do resolve the ctx discussion before merging.

move host setup. refactor bootstrap. add reconnectPeers to tests

f8032b3

AndrewSisley reviewed Aug 6, 2024

View reviewed changes

nasdf requested a review from a team August 6, 2024 19:10

nasdf self-assigned this Aug 6, 2024

nasdf added area/testing Related to any test or testing suite area/p2p Related to the p2p networking system labels Aug 6, 2024

nasdf added this to the DefraDB v0.13 milestone Aug 6, 2024

AndrewSisley reviewed Aug 6, 2024

View reviewed changes

nasdf requested a review from a team August 6, 2024 20:27

shahzadlone reviewed Aug 6, 2024

View reviewed changes

add more net tests

93da8b7

shahzadlone approved these changes Aug 7, 2024

View reviewed changes

nasdf added 2 commits August 13, 2024 10:42

restore peer context

5ef80c6

make tidy

4447678

nasdf force-pushed the nasdf/fix/peer-connections branch from 13254e8 to bf38b6e Compare August 14, 2024 16:56

fix failing net test

1baf383

nasdf force-pushed the nasdf/fix/peer-connections branch from bf38b6e to 1baf383 Compare August 15, 2024 16:07

nasdf added 2 commits August 15, 2024 09:24

Merge branch 'develop' into nasdf/fix/peer-connections

48c998a

Merge branch 'develop' into nasdf/fix/peer-connections

1654c0e

nasdf merged commit 9a228d2 into sourcenetwork:develop Aug 26, 2024
40 of 42 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(i): Make peer connections deterministic #2888

fix(i): Make peer connections deterministic #2888

nasdf commented Aug 6, 2024

codecov bot commented Aug 6, 2024 •

edited

Loading

AndrewSisley Aug 6, 2024 •

edited

Loading

nasdf Aug 6, 2024

AndrewSisley Aug 6, 2024

nasdf Aug 6, 2024

AndrewSisley Aug 6, 2024 •

edited

Loading

AndrewSisley Aug 6, 2024

nasdf Aug 6, 2024

AndrewSisley Aug 6, 2024

shahzadlone Aug 6, 2024

nasdf Aug 6, 2024

AndrewSisley Aug 6, 2024

nasdf Aug 6, 2024

AndrewSisley Aug 6, 2024

shahzadlone Aug 6, 2024

nasdf Aug 6, 2024

shahzadlone Aug 6, 2024

nasdf Aug 6, 2024

shahzadlone left a comment

fix(i): Make peer connections deterministic #2888

fix(i): Make peer connections deterministic #2888

Conversation

nasdf commented Aug 6, 2024

Relevant issue(s)

Description

Tasks

How has this been tested?

codecov bot commented Aug 6, 2024 • edited Loading

Codecov Report

AndrewSisley Aug 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndrewSisley Aug 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shahzadlone left a comment

Choose a reason for hiding this comment

codecov bot commented Aug 6, 2024 •

edited

Loading

AndrewSisley Aug 6, 2024 •

edited

Loading

AndrewSisley Aug 6, 2024 •

edited

Loading