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

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

Open
wants to merge 43 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
43 commits
Select commit Hold shift + click to select a range
4badf3a
chore: simplify debug message broadcasting
bryanchriswhite Jun 13, 2023
2b83d32
refactor: CLI config parsing
bryanchriswhite Jul 7, 2023
eac7695
refactor: common CLI helpers
bryanchriswhite Jul 7, 2023
bca000b
chore: add `GetBusFromCmd()` CLI helper
bryanchriswhite Jul 11, 2023
5e963be
chore: consistent debug CLI identity
bryanchriswhite Jul 11, 2023
a883f08
chore: add `enable_peer_discovery_debug_rpc` to P2P config
bryanchriswhite Jul 6, 2023
83c3604
chore: add P2P debug message handling support
bryanchriswhite Jul 6, 2023
6ecca53
feat: add `peer` subcommand
bryanchriswhite Jun 6, 2023
d570b35
chore: add peer `--local` persistent flag
bryanchriswhite Jul 6, 2023
e952365
feat: add `peer list` subcommand
bryanchriswhite Jun 6, 2023
e80843c
chore: implement `PeerstoreProvider#GetUnstakedPeerstore()`
bryanchriswhite Jun 5, 2023
8f90e22
chore: add `PeerstoreProvider#GetStakedPeerstoreAtCurrentHeight()`
bryanchriswhite Jul 13, 2023
6e691cd
chore: interim bootstrapping changes (pre-#859)
bryanchriswhite Jul 11, 2023
430db08
fix: gofmt
bryanchriswhite Jul 12, 2023
440b59a
chore: ensure flag and config parsing
bryanchriswhite Jul 7, 2023
fcfa837
chore: add `GetBusFromCmd()` CLI helper
bryanchriswhite Jul 11, 2023
04dc0aa
chore: consistent debug CLI identity
bryanchriswhite Jul 11, 2023
3925c71
Merge branch 'refactor/cli' into feat/peer-discovery-list
bryanchriswhite Jul 13, 2023
1bbad38
fixup: add `peer list` subcommand
bryanchriswhite Jul 13, 2023
64abbc0
add generated helm docs
invalid-email-address Jul 13, 2023
d8b6296
squash: merge refactor/cli with main
h5law Jul 25, 2023
9ecc9e5
chore: address review comments
h5law Jul 25, 2023
1cbc249
Merge branch 'main' into refactor/cli
h5law Jul 25, 2023
ffbc539
fix merge error
h5law Jul 25, 2023
0cff1d2
Merge branch 'refactor/cli' into feat/peer-discovery-list
h5law Jul 25, 2023
39af37c
revert change
h5law Jul 25, 2023
c22011c
address comments
h5law Jul 25, 2023
1fc2bb4
chore: address comments
h5law Jul 25, 2023
764e171
clarify unstaked description
h5law Jul 25, 2023
9eb5a7e
Merge branch 'main' into feat/peer-discovery-list
h5law Jul 26, 2023
7380260
chore: up retry attempts and wait time, clarify error messages
h5law Jul 26, 2023
c03aa27
Merge branch 'main' into feat/peer-discovery-list
Olshansk Jul 26, 2023
da62de1
Fixed case messaging.DebugMessageEventType
Olshansk Jul 26, 2023
12e22e1
clairfy error log messages
h5law Jul 27, 2023
f8f5da5
fix waitgroup recovery in tests
h5law Jul 27, 2023
66a1347
chore: move comment line
h5law Jul 27, 2023
870805f
add NewListCommand function to fit CLI pattern
h5law Jul 27, 2023
64ec990
clarify errors
h5law Jul 27, 2023
ccec195
remove unneeded error casting as sync panics a string
h5law Jul 27, 2023
90385f0
tidy cli
h5law Jul 28, 2023
6d0d300
remove #810 from comment as merged
h5law Jul 28, 2023
c6488a5
shortern retries
h5law Jul 28, 2023
2317d42
bug: comment out buggy lines
h5law Jul 31, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 9 additions & 13 deletions app/client/cli/cmd.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import (
"github.com/spf13/viper"

"github.com/pokt-network/pocket/app/client/cli/flags"
"github.com/pokt-network/pocket/runtime/configs"
"github.com/pokt-network/pocket/app/client/cli/peer"
"github.com/pokt-network/pocket/runtime/defaults"
)

Expand All @@ -17,8 +17,6 @@ const (
flagBindErrFormat = "could not bind flag %q: %v"
)

var cfg *configs.Config

func init() {
rootCmd.PersistentFlags().StringVar(&flags.RemoteCLIURL, "remote_cli_url", defaults.DefaultRemoteCLIURL, "takes a remote endpoint in the form of <protocol>://<host>:<port> (uses RPC Port)")
// ensure that this flag can be overridden by the respective viper-conventional environment variable (i.e. `POCKET_REMOTE_CLI_URL`)
Expand All @@ -39,19 +37,17 @@ func init() {
if err := viper.BindPFlag("verbose", rootCmd.PersistentFlags().Lookup("verbose")); err != nil {
log.Fatalf(flagBindErrFormat, "verbose", err)
}

// Add subdir commands
// DISCUSS: Should we put the peer commands as the other commands are so we dont have to do this?
rootCmd.AddCommand(peer.NewPeerCommand())
}

var rootCmd = &cobra.Command{
Use: cliExecutableName,
Short: "Pocket Network Command Line Interface (CLI)",
Long: "The CLI is meant to be an user but also a machine friendly way for interacting with Pocket Network.",
PersistentPreRunE: func(cmd *cobra.Command, args []string) error {
// by this time, the config path should be set
cfg = configs.ParseConfig(flags.ConfigPath)
// set final `remote_cli_url` value; order of precedence: flag > env var > config > default
flags.RemoteCLIURL = viper.GetString("remote_cli_url")
return nil
},
Use: cliExecutableName,
Short: "Pocket Network Command Line Interface (CLI)",
Long: "The CLI is meant to be an user but also a machine friendly way for interacting with Pocket Network.",
PersistentPreRunE: flags.ParseConfigAndFlags,
}

func ExecuteContext(ctx context.Context) error {
Expand Down
19 changes: 19 additions & 0 deletions app/client/cli/flags/config.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
package flags

import (
"github.com/spf13/cobra"
"github.com/spf13/viper"

"github.com/pokt-network/pocket/runtime/configs"
)

var Cfg *configs.Config

func ParseConfigAndFlags(_ *cobra.Command, _ []string) error {
// by this time, the config path should be set
Cfg = configs.ParseConfig(ConfigPath)

// set final `remote_cli_url` value; order of precedence: flag > env var > config > default
RemoteCLIURL = viper.GetString("remote_cli_url")
Copy link
Member

Choose a reason for hiding this comment

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

Why don't we do this for the other flags.go?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I intended to revert this change. This is the direction I started when investigating #891 (comment), I thought I had reverted it but apparently failed.

To answer your question, the reason for this is because viper's only integration with flags is to support setting a viper key based on the flag, but not the other way around. I.e: Viper won't update the flag value. This only applies to bound flags

This means that we have to do one of the following consistently for bound flags:

  • call viper.GetString("<flag key>") (or a helper containing it) anywhere we need the value
  • update the flag consistently and reference the flag value anywhere we need it

I opted for the latter option as I felt it was more conventional and easier to read and maintain.

return nil
}
9 changes: 4 additions & 5 deletions app/client/cli/helpers/common.go
Original file line number Diff line number Diff line change
Expand Up @@ -23,15 +23,14 @@ func FetchPeerstore(cmd *cobra.Command) (types.Peerstore, error) {
if err != nil {
return nil, err
}
// TECHDEBT(#811): use `bus.GetPeerstoreProvider()` after peerstore provider
// is retrievable as a proper submodule
pstoreProvider, err := bus.GetModulesRegistry().GetModule(peerstore_provider.PeerstoreProviderSubmoduleName)
// TECHDEBT(#811): Remove type casting once Peerstore is available as a submodule
pstoreProvider, err := peerstore_provider.GetPeerstoreProvider(bus)
if err != nil {
return nil, errors.New("retrieving peerstore provider")
return nil, err
}
currentHeightProvider := bus.GetCurrentHeightProvider()
height := currentHeightProvider.CurrentHeight()
pstore, err := pstoreProvider.(peerstore_provider.PeerstoreProvider).GetStakedPeerstoreAtHeight(height)
pstore, err := pstoreProvider.GetStakedPeerstoreAtHeight(height)
if err != nil {
return nil, fmt.Errorf("retrieving peerstore at height %d", height)
}
Expand Down
97 changes: 97 additions & 0 deletions app/client/cli/peer/list.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,97 @@
package peer

import (
"fmt"

"github.com/spf13/cobra"
"google.golang.org/protobuf/types/known/anypb"

"github.com/pokt-network/pocket/app/client/cli/helpers"
"github.com/pokt-network/pocket/logger"
"github.com/pokt-network/pocket/p2p/debug"
"github.com/pokt-network/pocket/shared/messaging"
)

var ErrRouterType = fmt.Errorf("must specify one of --staked, --unstaked, or --all")

func NewListCommand() *cobra.Command {
return &cobra.Command{
Use: "List",
Short: "List the known peers",
Long: "Prints a table of the Peer ID, Pokt Address and Service URL of the known peers",
Aliases: []string{"list", "ls"},
RunE: listRunE,
}
}

func listRunE(cmd *cobra.Command, _ []string) error {
var routerType debug.RouterType

bus, err := helpers.GetBusFromCmd(cmd)
if err != nil {
return err
}

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

debugMsg := &messaging.DebugMessage{
Action: messaging.DebugMessageAction_DEBUG_P2P_PRINT_PEER_LIST,
Type: messaging.DebugMessageRoutingType_DEBUG_MESSAGE_TYPE_BROADCAST,
Message: &anypb.Any{
Value: []byte(routerType),
},
}
debugMsgAny, err := anypb.New(debugMsg)
if err != nil {
return fmt.Errorf("error creating anypb from debug message: %w", err)
}

if localFlag {
Copy link
Member

Choose a reason for hiding this comment

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

See my comments in #801. They're similar to this as well

Copy link
Contributor

Choose a reason for hiding this comment

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

@Olshansk can you clarify which comment you are referring to? And what you think should change here?

Copy link
Member

@Olshansk Olshansk Jul 25, 2023

Choose a reason for hiding this comment

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

E.g. #801 (comment)

@h5law Can you take a stab at #801 first? I (accidently) reviewed it before this one so I avoid repeating some of the stylistic recommendations here.

Copy link
Contributor

Choose a reason for hiding this comment

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

@Olshansk the reason I am tackling this first is that this is the base of #801 as such I feel it would be difficult to address #801 when the changes may need to be downwards propagated. Will go through #801 alongside from now on

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. This one is on me

Copy link
Contributor

Choose a reason for hiding this comment

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

Have gone through #801 and I think I have already addressed most of the relavent comments in this PR as is

if err := debug.PrintPeerList(bus, routerType); err != nil {
return fmt.Errorf("error printing peer list: %w", err)
}
return nil
}

// TECHDEBT(#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
// instance could persist between commands. Other interactive CLI commands which
// rely on unstaked actor router broadcast are working as expected.

// TECHDEBT(#811): use broadcast instead to reach all peers.
return sendToStakedPeers(cmd, debugMsgAny)
}

func sendToStakedPeers(cmd *cobra.Command, debugMsgAny *anypb.Any) error {
bus, err := helpers.GetBusFromCmd(cmd)
if err != nil {
return err
}

pstore, err := helpers.FetchPeerstore(cmd)
if err != nil {
logger.Global.Fatal().Err(err).Msg("unable to retrieve the pstore")
}

if pstore.Size() == 0 {
logger.Global.Fatal().Msg("no validators found")
}

for _, peer := range pstore.GetPeerList() {
if err := bus.GetP2PModule().Send(peer.GetAddress(), debugMsgAny); err != nil {
logger.Global.Error().Err(err).Msg("failed to send debug message")
}
}
return nil
}
55 changes: 55 additions & 0 deletions app/client/cli/peer/peer.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
package peer

import (
"github.com/spf13/cobra"

"github.com/pokt-network/pocket/app/client/cli/helpers"
)

var allFlag,
stakedFlag,
unstakedFlag,
localFlag bool

func NewPeerCommand() *cobra.Command {
cmd := &cobra.Command{
Use: "Peer",
Short: "Manage peers",
Aliases: []string{"peer"},
PersistentPreRunE: helpers.P2PDependenciesPreRunE,
}

cmd.PersistentFlags().
BoolVarP(
&allFlag,
"all", "a",
false,
"operations apply to both staked & unstaked router peerstores (default)",
)
cmd.PersistentFlags().
BoolVarP(
&stakedFlag,
"staked", "s",
false,
"operations only apply to staked router peerstore (i.e. raintree)",
)
cmd.PersistentFlags().
BoolVarP(
&unstakedFlag,
"unstaked", "u",
false,
"operations only apply to unstaked (including staked as a subset) router peerstore (i.e. gossipsub)",
)
cmd.PersistentFlags().
BoolVarP(
&localFlag,
"local", "l",
false,
"operations apply to the local (CLI binary's) P2P module instead of being broadcast",
)

// Add subcommands
cmd.AddCommand(NewListCommand())

return cmd
}
2 changes: 1 addition & 1 deletion app/client/cli/utils.go
Original file line number Diff line number Diff line change
Expand Up @@ -311,7 +311,7 @@ func attachKeybaseFlagsToSubcommands() []cmdOption {
}

func keybaseForCLI() (keybase.Keybase, error) {
return keybase.NewKeybase(cfg.Keybase)
return keybase.NewKeybase(flags.Cfg.Keybase)
}

func unableToConnectToRpc(err error) error {
Expand Down
1 change: 1 addition & 0 deletions charts/pocket/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ privateKeySecretKeyRef:
| config.fisherman.enabled | bool | `false` | |
| config.logger.format | string | `"json"` | |
| config.logger.level | string | `"debug"` | |
| config.p2p.enable_peer_discovery_debug_rpc | bool | `false` | |
| config.p2p.hostname | string | `""` | |
| config.p2p.is_empty_connection_type | bool | `false` | |
| config.p2p.max_mempool_count | int | `100000` | |
Expand Down
10 changes: 6 additions & 4 deletions charts/pocket/templates/statefulset.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -100,10 +100,12 @@ spec:
valueFrom:
fieldRef:
fieldPath: status.podIP
livenessProbe:
httpGet:
path: /v1/health
port: rpc
- name: POCKET_P2P_ENABLE_PEER_DISCOVERY_DEBUG_RPC
value: "true"
# livenessProbe:
# httpGet:
# path: /v1/health
# port: rpc
readinessProbe:
httpGet:
path: /v1/health
Expand Down
1 change: 1 addition & 0 deletions charts/pocket/values.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,7 @@ config:
is_empty_connection_type: false
private_key: "" # @ignored This value is needed but ignored - use privateKeySecretKeyRef instead
max_mempool_count: 100000
enable_peer_discovery_debug_rpc: false
telemetry:
enabled: true
address: 0.0.0.0:9000
Expand Down
Loading