[P2P] [Tooling] Peer discovery `peer connections` subcommand by bryanchriswhite · Pull Request #801 · pokt-network/pocket

bryanchriswhite · 2023-06-02T13:20:30Z

@Reviewer

This PR may be more digestible / reviewable on a commit-by-commit basis. Commits are organized logically and any given line is only modified in a single commit, with few exceptions*.

*(In the interest of preserving the git-time-continuum 👮🚨, this applies in batches of commits between comments or reviews by humans, only once "in review")

Description

Adds the following subcommands:

Print open peer connections

Usage:
  p1 peer connections [flags]

Flags:
  -h, --help   help for connections

Global Flags:
  -a, --all                     operations apply to both staked & unstaked router peerstores (default)
      --config string           Path to config
      --data_dir string         Path to store pocket related data (keybase etc.) (default "/home/bwhite/.pocket")
  -l, --local                   operations apply to the local (CLI binary's) P2P module instead of being broadcast
      --non_interactive         if true skips the interactive prompts wherever possible (useful for scripting & automation)
      --remote_cli_url string   takes a remote endpoint in the form of <protocol>://<host>:<port> (uses RPC Port) (default "http://localhost:50832")
  -s, --staked                  operations only apply to staked router peerstore (i.e. raintree)
  -u, --unstaked                operations only apply to unstaked router peerstore (i.e. gossipsub)
      --verbose                 Show verbose output

Issue

[P2P] Peer discovery debug CLI sub-commands #730

Dependencies:

Type of change

Please mark the relevant option(s):

List of changes

Added peer connections subcommand
Added P2PModule#GetConnections() interface method & implementation

Testing

make develop_test; if any code changes were made
make test_e2e on k8s LocalNet; if any code changes were made
e2e-devnet-test passes tests on DevNet; if any code was changed
Docker Compose LocalNet; if any major functionality was changed or introduced
k8s LocalNet; if any infrastructure or configuration changes were made

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 added, or updated, godoc format comments on touched members (see: tip.golang.org/doc/comment)
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)

codecov · 2023-06-02T13:34:24Z

Codecov Report

❗ No coverage uploaded for pull request base (re/refactor/cli@a18c047). Click here to learn what that means.
Patch has no changes to coverable lines.

❗ Current head 10e00d0 differs from pull request most recent head ca2e971. Consider uploading reports for the commit ca2e971 to get more accurate results

Additional details and impacted files

@@                Coverage Diff                 @@
##             re/refactor/cli     #801   +/-   ##
==================================================
  Coverage                   ?   29.96%           
==================================================
  Files                      ?      108           
  Lines                      ?     8829           
  Branches                   ?        0           
==================================================
  Hits                       ?     2646           
  Misses                     ?     5873           
  Partials                   ?      310

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

Olshansk · 2023-07-13T22:12:24Z

p2p/debug.go

-	case messaging.DebugMessageAction_DEBUG_P2P_PEER_LIST:
+	case messaging.DebugMessageAction_DEBUG_P2P_PEER_LIST,
+		messaging.DebugMessageAction_DEBUG_P2P_PEER_CONNECTIONS:
 		if !m.cfg.EnablePeerDiscoveryDebugRpc {


I feel like we can skip the first switch statement and just return (with a debug log or error) if if !m.cfg.EnablePeerDiscoveryDebugRpc {

Olshansk · 2023-07-13T23:15:01Z

charts/pocket/values.yaml

    use_rain_tree: true
    is_empty_connection_type: false
    private_key: "" # @ignored This value is needed but ignored - use privateKeySecretKeyRef instead
+    # TODO: I think this has been renamed to `max_nonces`


Cna you please update it everywhere else as well?

Olshansk · 2023-07-13T23:20:36Z

app/client/cli/peer/connections.go

+)
+
+var (
+	connectionsCmd = &cobra.Command{


Everywhere else we have a function that returns a new instance of cobra.Command.

I think we should either:

Stay consisstent

Stay consistent + add a TODO to change the others

Change all of them

Olshansk · 2023-07-13T23:22:21Z

p2p/debug/connections.go

@@ -0,0 +1,149 @@
+package debug


Should we add a debug build tag?

Olshansk · 2023-07-13T23:23:37Z

p2p/debug/connections.go

+	return nil
+}
+
+func PrintConnectionsTable(conns []libp2pNetwork.Conn) error {


does PrintConnectionsTable need to be publically exposed given that we use PrintPeerConnections in the cli?

Olshansk · 2023-07-13T23:29:01Z

p2p/debug/connections.go

+func peerConnsRowConsumerFactory(conns []libp2pNetwork.Conn) utils.RowConsumer {
+	return func(provideRow utils.RowProvider) error {
+		for _, conn := range conns {
+			if err := provideRow(


what happens if we don't return an error when provideRow errors? Is a partial list not an option?

Olshansk · 2023-07-13T23:29:33Z

app/client/cli/peer/connections.go

+		Use:   "connections",
+		Short: "Print open peer connections",


Suggested change

Use: "connections",

Short: "Print open peer connections",

Use: "Connections",

Short: "Print open peer connections",

Long: "Prints ..." // list out printConnectionsHeader

Aliases: []string{"conns"},

Olshansk · 2023-07-13T23:33:34Z

app/client/cli/peer/connections.go

+	}
+
+	switch {
+	case stakedFlag:


OPTIONAL: The way I would've done this is by having a smaller private helper function like below that returns the router typ:

func foo() routerType { switch case stakedFlag && !unstakedFlag && !allFlag { return debug.StakeRouterType } case unstakedFlag && !stakedFlag && !allFlag { return debug.UnstakedRouterType } case stakedFlag || unstakedFlag { return ErrRouterType } // even if `allFlag` is false, we still want to print all connections default: return debug.AllRouterTypes } }

Wdyt?

Olshansk · 2023-07-13T23:34:57Z

app/client/cli/peer/connections.go

+
+	// TECHDEBT(#810, #811): will need to wait for DHT bootstrapping to complete before
+	// p2p broadcast can be used with to reach unstaked actors.
+	// CONSIDERATION: add the peer commands to the interactive CLI as the P2P module


So why don't we do this?

Olshansk · 2023-07-13T23:35:32Z

app/client/cli/peer/connections.go

+		return fmt.Errorf("creating anypb from debug message: %w", err)
+	}
+
+	if localFlag {


So if this is false we don't print anything?

I don't fully understand its use. Is it like a "verbose" flag?

This localFlag determines whether we print locally ie to the terminal that ran the command or on the node itself AFAIK. With -l we will see the output in our terminal, otherwise it will be logged by the node

Olshansk · 2023-07-28T20:20:01Z

@bryanchriswhite Can you pick this up when you're back?

bryanchriswhite added the p2p P2P specific changes label Jun 2, 2023

bryanchriswhite added this to the M1: Pocket PoS (Proof of Stake) milestone Jun 2, 2023

bryanchriswhite self-assigned this Jun 2, 2023

reviewpad bot added the large Pull request is large label Jun 2, 2023

bryanchriswhite force-pushed the feat/peer-discovery-debug-cli branch from 3784b14 to 1c23be5 Compare June 5, 2023 08:04

reviewpad bot added medium Pull request is medium and removed large Pull request is large labels Jun 5, 2023

bryanchriswhite force-pushed the feat/peer-discovery-debug-cli branch from ce1b1bd to 80aec49 Compare June 5, 2023 08:12

bryanchriswhite changed the base branch from feat/kad-peer-discovery to refactor/peerstore-provider June 5, 2023 08:14

bryanchriswhite added the do not merge Prevent merging even with sufficient approvals label Jun 5, 2023

bryanchriswhite force-pushed the refactor/peerstore-provider branch from 9209358 to 968be5f Compare June 5, 2023 11:01

bryanchriswhite force-pushed the feat/peer-discovery-debug-cli branch from 0cdc40d to c78193d Compare June 5, 2023 11:18

bryanchriswhite changed the base branch from refactor/peerstore-provider to re/refactor/peerstore-provider June 5, 2023 11:23

bryanchriswhite force-pushed the feat/peer-discovery-debug-cli branch from c6e8447 to 39a2f3e Compare June 6, 2023 07:21

reviewpad bot added large Pull request is large and removed medium Pull request is medium labels Jun 6, 2023

bryanchriswhite force-pushed the feat/peer-discovery-debug-cli branch from 592eee8 to e56bc6e Compare June 6, 2023 08:56

bryanchriswhite changed the base branch from re/refactor/peerstore-provider to re/refactor/cli June 6, 2023 13:51

bryanchriswhite force-pushed the feat/peer-discovery-debug-cli branch from 6ba979e to 4716bf7 Compare June 6, 2023 13:51

reviewpad bot added medium Pull request is medium and removed large Pull request is large labels Jun 6, 2023

bryanchriswhite linked an issue Jun 6, 2023 that may be closed by this pull request

[P2P] Peer discovery debug CLI sub-commands #730

Open

10 tasks

bryanchriswhite force-pushed the re/refactor/cli branch from 5eea086 to a18c047 Compare June 6, 2023 14:29

bryanchriswhite force-pushed the feat/peer-discovery-debug-cli branch from f498196 to 10e00d0 Compare June 6, 2023 14:30

reviewpad bot added large Pull request is large and removed medium Pull request is medium labels Jun 6, 2023

bryanchriswhite added the tooling tooling to support development, testing et al label Jun 6, 2023

bryanchriswhite changed the title ~~[P2P] Peer discovery CLI commands~~ [P2P] [Tooling] Peer discovery CLI commands Jun 6, 2023

bryanchriswhite changed the base branch from re/refactor/cli to chore/introduce-submodule July 6, 2023 10:07

bryanchriswhite added push-image Build and push a container image on non-main builds e2e-devnet-test Runs E2E tests on devnet labels Jul 13, 2023

bryanchriswhite force-pushed the feat/peer-discovery-debug-cli branch from 8213d38 to a4b12df Compare July 13, 2023 15:09

reviewpad bot added large Pull request is large and removed small Pull request is small labels Jul 13, 2023

bryanchriswhite force-pushed the feat/peer-discovery-list branch from 0d9d930 to 380e7d5 Compare July 13, 2023 15:10

reviewpad bot added medium Pull request is medium and removed large Pull request is large labels Jul 13, 2023

bryanchriswhite force-pushed the feat/peer-discovery-list branch from ed9ed31 to 1bbad38 Compare July 13, 2023 15:26

reviewpad bot added large Pull request is large and removed medium Pull request is medium labels Jul 13, 2023

bryanchriswhite force-pushed the feat/peer-discovery-debug-cli branch from b64c8f4 to 59449b0 Compare July 13, 2023 15:37

reviewpad bot added medium Pull request is medium and removed large Pull request is large labels Jul 13, 2023

bryanchriswhite force-pushed the feat/peer-discovery-debug-cli branch 2 times, most recently from ea50366 to 2bd789c Compare July 13, 2023 15:43

bryanchriswhite marked this pull request as ready for review July 13, 2023 15:43

bryanchriswhite requested a review from Olshansk July 13, 2023 15:43

reviewpad bot added the waiting-for-review label Jul 13, 2023

bryanchriswhite requested review from dylanlott and red-0ne July 13, 2023 15:43

bryanchriswhite marked this pull request as draft July 13, 2023 15:46

bryanchriswhite added 2 commits July 13, 2023 17:52

feat: add peer connections subcommand

561bc82

feat: add P2PModule#GetConnections() interface method and impl.

4df2da8

bryanchriswhite force-pushed the feat/peer-discovery-debug-cli branch from 2bd789c to 4df2da8 Compare July 13, 2023 15:52

bryanchriswhite marked this pull request as ready for review July 13, 2023 15:52

Olshansk requested changes Jul 13, 2023

View reviewed changes

Olshansk mentioned this pull request Jul 14, 2023

[P2P] [Tooling] Peer discovery peer list subcommand #892

Open

20 tasks

Olshansk assigned h5law Jul 17, 2023

Comments

Conversation

bryanchriswhite commented Jun 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Issue

Type of change

List of changes

Testing

Required Checklist

If Applicable Checklist

Uh oh!

codecov bot commented Jun 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Olshansk commented Jul 28, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

bryanchriswhite commented Jun 2, 2023 •

edited

Loading

codecov bot commented Jun 2, 2023 •

edited

Loading