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.

Sign up for GitHub

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

Jump to bottom

[Libp2p] Add Libp2p module (part 4) #545

Merged
merged 11 commits into from
Mar 3, 2023
Merged

[Libp2p] Add Libp2p module (part 4) #545

merged 11 commits into from
Mar 3, 2023

Conversation

bryanchriswhite
Copy link
Contributor

@bryanchriswhite bryanchriswhite commented Feb 24, 2023 •
edited
Loading

Description

This is another of a series of PRs split out from #500. Here we add a new modules.P2PModule implementation which utilizes the typesP2P.Network implementation which was added in #540. It will be utilized together with the config changes introduced by #535 in forthcoming changes to the node and debug CLI.

Issue

#347

Type of change

Please mark the relevant option(s):

  • New feature, functionality or library
  • Bug fix
  • Code health or cleanup
  • Major breaking change
  • Documentation
  • Other

List of changes

  • Added a new modules.P2PModule implementation to the libp2p module directory

Testing

  • make develop_test
  • LocalNet w/ all of the steps outlined in the README

Required Checklist

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have tested my changes using the available tooling
  • I have updated the corresponding CHANGELOG

If Applicable Checklist

  • I have updated the corresponding README(s); local and/or global
  • I have added tests that prove my fix is effective or that my feature works
  • I have added, or updated, mermaid.js diagrams in the corresponding README(s)
  • I have added, or updated, documentation and mermaid.js diagrams in shared/docs/* if I updated shared/*README(s)

@bryanchriswhite bryanchriswhite added the p2p P2P specific changes label Feb 24, 2023
@bryanchriswhite bryanchriswhite self-assigned this Feb 24, 2023
@bryanchriswhite bryanchriswhite force-pushed the chore/libp2p-3 branch 2 times, most recently from 9af1fb5 to 4f9f13f Compare February 24, 2023 11:24
@bryanchriswhite bryanchriswhite marked this pull request as ready for review February 24, 2023 11:25
@bryanchriswhite
Copy link
Contributor Author

bryanchriswhite commented Feb 27, 2023 •
edited
Loading

This PR is based off of the chore/libp2p-2 branch but is also dependent on the chore/config branch. As a result tests and linting will fail until #535 is merged.

deblasis
deblasis reviewed Feb 28, 2023
View reviewed changes
libp2p/module.go Outdated Show resolved Hide resolved
deblasis
deblasis reviewed Feb 28, 2023
View reviewed changes
libp2p/module.go Outdated
Comment on lines 88 to 91
func (mod *libp2pModule) Create(bus modules.Bus, options ...modules.ModuleOption) (modules.Module, error) {
logger.Global.Print("Creating libp2p-backed network module")

*mod = libp2pModule{
cfg: bus.GetRuntimeMgr().GetConfig().P2P,
}
Copy link
Contributor

Choose a reason for hiding this comment

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

How about:

Suggested change
func (mod *libp2pModule) Create(bus modules.Bus, options ...modules.ModuleOption) (modules.Module, error) {
logger.Global.Print("Creating libp2p-backed network module")
*mod = libp2pModule{
cfg: bus.GetRuntimeMgr().GetConfig().P2P,
}
func (*libp2pModule) Create(bus modules.Bus, options ...modules.ModuleOption) (modules.Module, error) {
logger.Global.Debug().Msg("Creating libp2p-backed network module")
mod := &libp2pModule{
cfg: bus.GetRuntimeMgr().GetConfig().P2P,
logger: logger.Global.CreateLoggerForModule(modules.P2PModuleName),
}

?

More consistent with other modules.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

💯 agreed. We need to update the shared modules docs to reflect this new convention, as currently, it effectively says that logger initialization should happen in #Start():

When defining the start function for the module, it is essential to initialise a namespace logger as well:

Do you think this is covered by #509, specifically the first goal?:

Add godoc comments to the various interfaces, functions, methods involved in the module creation process

Copy link
Contributor

Choose a reason for hiding this comment

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

💯 agreed. We need to update the shared modules docs to reflect this new convention, as currently, it effectively says that logger initialization should happen in #Start():

When defining the start function for the module, it is essential to initialise a namespace logger as well:

Totally, thanks for pointing that out!

Do you think this is covered by #509, specifically the first goal?:

Add godoc comments to the various interfaces, functions, methods involved in the module creation process

Agreed, I have clarified the scope so I don't forget :) Thanks buddy!

image

deblasis
deblasis reviewed Feb 28, 2023
View reviewed changes
libp2p/module.go Outdated Show resolved Hide resolved
deblasis
deblasis reviewed Feb 28, 2023
View reviewed changes
libp2p/module.go Outdated Show resolved Hide resolved
deblasis
deblasis reviewed Feb 28, 2023
View reviewed changes
libp2p/module.go

// NB: time out if no data is sent to free resources.
if err := stream.SetReadDeadline(newReadStreamDeadline()); err != nil {
mod.logger.Error().Err(err).Msg("setting stream read deadline")
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems that we are settling on this pattern (Added the Bool(true)) for this kind of thing to help with 🪵 🔍

Suggested change
mod.logger.Error().Err(err).Msg("setting stream read deadline")
mod.logger.Error().Err(err).
Bool("TODO", true).
Msg("setting stream read deadline")

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@deblasis can you elaborate on (or link to) the thinking behind this?

Copy link
Contributor

Choose a reason for hiding this comment

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

Of course.

We have a bunch of places in our codebase where we log a message and we also attach a TODO: true

Example:
image

As we mature our observability stack, it could be useful to search for these and also to inform the users that something is still in development and the log entries are merely placeholders for future real functionality

Copy link
Member

Choose a reason for hiding this comment

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

We also have #478

deblasis
deblasis reviewed Feb 28, 2023
View reviewed changes
libp2p/module.go Outdated Show resolved Hide resolved
deblasis
deblasis suggested changes Feb 28, 2023
View reviewed changes
Copy link
Contributor

@deblasis deblasis left a comment

Choose a reason for hiding this comment

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

Good job @bryanchriswhite! 🚀
Some minor changes, nits, mostly for consistency

@bryanchriswhite bryanchriswhite linked an issue Mar 1, 2023 that may be closed by this pull request
[P2P] Integrate LibP2P #347
Closed
6 tasks
@bryanchriswhite bryanchriswhite requested a review from deblasis March 1, 2023 12:45
deblasis
deblasis approved these changes Mar 1, 2023
View reviewed changes
Copy link
Contributor

@deblasis deblasis left a comment

Choose a reason for hiding this comment

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

LGTM👍

@bryanchriswhite bryanchriswhite changed the base branch from chore/libp2p-2 to main March 3, 2023 12:30
@bryanchriswhite bryanchriswhite merged commit c80e630 into main Mar 3, 2023
@bryanchriswhite bryanchriswhite deleted the chore/libp2p-3 branch March 3, 2023 13:06
Olshansk
Olshansk reviewed Mar 10, 2023
View reviewed changes
Copy link
Member

@Olshansk Olshansk left a comment

Choose a reason for hiding this comment

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

Easy to understand, a big step forward and great documentation in terms of what we have and what's next. Great job Bryan 👏

p2p/module.go
@@ -154,6 +154,7 @@ func (m *p2pModule) Broadcast(msg *anypb.Any) error {
c := &messaging.PocketEnvelope{
Content: msg,
}
//TECHDEBT: use shared/codec for marshalling
Copy link
Member

Choose a reason for hiding this comment

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

Thank you for adding these

libp2p/module.go

var _ modules.P2PModule = &libp2pModule{}

type libp2pModule struct {
Copy link
Member

Choose a reason for hiding this comment

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

nice comments on the sattributes

libp2p/module.go
mod.listenAddrs = libp2p.NoListenAddrs
default:
return nil, fmt.Errorf(
// DISCUSS: rename to "transport protocol" instead.
Copy link
Member

Choose a reason for hiding this comment

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

Let's do it

libp2p/module.go
}

func (mod *libp2pModule) Start() error {
// IMPROVE: receive context in interface methods.
Copy link
Member

Choose a reason for hiding this comment

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

💯

libp2p/module.go

// NB: time out if no data is sent to free resources.
if err := stream.SetReadDeadline(newReadStreamDeadline()); err != nil {
mod.logger.Error().Err(err).Msg("setting stream read deadline")
Copy link
Member

Choose a reason for hiding this comment

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

We also have #478

@Jhoony12 Jhoony12 mentioned this pull request May 4, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Olshansk Olshansk Olshansk left review comments

@deblasis deblasis deblasis approved these changes

Assignees

@bryanchriswhite bryanchriswhite

Labels
p2p P2P specific changes
Projects
V1 Dashboard
Status: Done
Milestone
M1: Pocket PoS (Proof of Stake)
Development

Successfully merging this pull request may close these issues.

[P2P] Integrate LibP2P
3 participants
@bryanchriswhite @Olshansk @deblasis