Alan's (first time) review on go-libp2p-pubsub

[daviddias]

Hi everyone! I've had asked @alanshaw to review the go-libp2p-pubsub repo and respective hardening PR #263.

My initial goal was for @alanshaw to apply is magic and make the repo and respective codebase really welcoming, but given that some code is still in flux, we dialed down the goal of the first review to be just reviewing and share observations, rather than take action. We still plan to do a future review when the WIP PRs have been merged.

I've took Alan's notes and translated them to:

• ❗️ Info/Observation points to take in consideration
• [ ] Action items
• ❓ Questions

Then I separated them into categories (Modularization, Testing, Onboarding, Refactoring & Misc)

All things below :)

Notes from @alanshaw first review on go-libp2p-pubsub. These are notes from PoV of new contributor to the code base.

Modularization

• ❗️ When looking at the API docs there's lots to take in and no separation between which types/functions belong to which implementation.
• ❗️ https://github.com/libp2p/go-libp2p-pubsub#in-this-repo-you-will-find is really helpful, but if these files were just in separate packages (or repos?) it would probably also be obvious to anyone who cloned the repo and didn't read the README

• Separate each implementation into their own package

Testing

• ❗️ The build is currently failing on the hardening branch - we should verify we have coverage for the added harding features (ah I see this is a TODO in the PR description nvm)
• 61% coverage could do with a little improvement!
  • Suggestion, increase to >=80%

Onboarding

⚠️ Peer exchange on PRUNE relies on signed peer records spec, which is currently an unmerged PR

❓ I don't yet understand the reasons behind for deprecating pubsub.Subscribe and pusub.Publish? Wouldn't make it difficult for two separate concerns to subscribe/publish to the same topic without them knowing about each other somehow?

• Should have at least one usage example https://github.com/libp2p/go-libp2p-pubsub#examples
• D needs defining - please link to the spec or describe it here! https://github.com/libp2p/go-libp2p-pubsub/blob/master/gossipsub.go#L26

Refactoring

❓ comm.go has receiver methods for a type defined in pubsub.go is that usual? Is unexpected to me, I would prefer to know all receiver methods are defined in the same file as the type...

• gossipsub.go is nearly 1,000 LoC! It needs to be split up into logical components that are easier to digest. Big files can be indicative of tighly coupled components and insufficient separation of concerns.
• Pull functional options into an opts/options package so "With..." is unnecessary - opts.MessageSigning(true) insetad of WithMessageSigning(true). It also reduces the total LoC in a single file making it easier to read & understand.

Misc

❓ It may be my golang unexperience talking here but I don't understand what use done has here

go-libp2p-pubsub/topic.go

Lines 53 to 71 in 357f291

	done := make(chan struct{}, 1)
	select {
	case t.p.eval <- func() {
	tmap := t.p.topics[t.topic]
	for p := range tmap {
	h.evtLog[p] = PeerJoin
	}
	t.evtHandlerMux.Lock()
	t.evtHandlers[h] = struct{}{}
	t.evtHandlerMux.Unlock()
	done <- struct{}{}
	}:
	case <-t.p.ctx.Done():
	return nil, t.p.ctx.Err()
	}
	<-done

only one thing is ever written to it and because it has a buffer of 1 it won't block.

• We made the decision in js-libp2p to avoid the term "blacklist" and instead use "denylist" in case of any negative connotations refactor: rename blacklist to denylist js-libp2p-switch#341

[aschmahmann]

It may be my golang unexperience talking here but I don't understand what use done has here

Done is used to indicate the the function being run in the eval loop is completed. Sending the func() into the eval loop does not mean it's completed so in order for the EventHandler() function to be blocking it needs a way to wait for the internal func() to complete, hence using the done channel

I don't yet understand the reasons behind for deprecating pubsub.Subscribe and pusub.Publish? Wouldn't make it difficult for two separate concerns to subscribe/publish to the same topic without them knowing about each other somehow?

Three reasons that I or @vyzo could go into more in depth:

1. pubsub.Publish and pubsub.Subscribe have the "wrong" function declaration (e.g. Publish is missing a context), we didn't want to break the interface here so we deprecated it and gave another one
2. It should be easier now for pubsub publishers and subscribers using the same instance to not know about each other if desired, which is generally intended Topic handles and ownership #198, even if we have some bugs add ValidatorData field to Message #231 (comment).
3. We were implicitly using the name of the topic as the reference to the topic, but having a struct makes extending the topic with additional properties a whole lot easier