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 39 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
20 changes: 7 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,15 @@ func init() {
if err := viper.BindPFlag("verbose", rootCmd.PersistentFlags().Lookup("verbose")); err != nil {
log.Fatalf(flagBindErrFormat, "verbose", err)
}

rootCmd.AddCommand(peer.PeerCmd)
}

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
101 changes: 101 additions & 0 deletions app/client/cli/peer/list.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,101 @@
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 init() {
PeerCmd.AddCommand(NewListCommand())
}

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
}
27 changes: 27 additions & 0 deletions app/client/cli/peer/peer.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
package peer

import (
"github.com/spf13/cobra"

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

var (
allFlag,
stakedFlag,
unstakedFlag,
localFlag bool

PeerCmd = &cobra.Command{
Use: "peer",
Short: "Manage peers",
PersistentPreRunE: helpers.P2PDependenciesPreRunE,
}
)

func init() {
PeerCmd.PersistentFlags().BoolVarP(&allFlag, "all", "a", false, "operations apply to both staked & unstaked router peerstores (default)")
PeerCmd.PersistentFlags().BoolVarP(&stakedFlag, "staked", "s", false, "operations only apply to staked router peerstore (i.e. raintree)")
PeerCmd.PersistentFlags().BoolVarP(&unstakedFlag, "unstaked", "u", false, "operations only apply to unstaked (including staked as a subset) router peerstore (i.e. gossipsub)")
PeerCmd.PersistentFlags().BoolVarP(&localFlag, "local", "l", false, "operations apply to the local (CLI binary's) P2P module instead of being broadcast")
}
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
2 changes: 2 additions & 0 deletions charts/pocket/templates/statefulset.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,8 @@ spec:
valueFrom:
fieldRef:
fieldPath: status.podIP
- name: POCKET_P2P_ENABLE_PEER_DISCOVERY_DEBUG_RPC
value: "true"
livenessProbe:
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
57 changes: 25 additions & 32 deletions p2p/background/router.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@ import (
"github.com/pokt-network/pocket/logger"
"github.com/pokt-network/pocket/p2p/config"
"github.com/pokt-network/pocket/p2p/protocol"
"github.com/pokt-network/pocket/p2p/providers"
"github.com/pokt-network/pocket/p2p/providers/peerstore_provider"
typesP2P "github.com/pokt-network/pocket/p2p/types"
"github.com/pokt-network/pocket/p2p/unicast"
Expand All @@ -36,8 +35,8 @@ var (
// TECHDEBT: Make these values configurable
// TECHDEBT: Consider using an exponential backoff instead
const (
connectMaxRetries = 5
connectRetryTimeout = time.Second * 2
connectMaxRetries = 7
connectRetryTimeout = time.Second * 3
)

type backgroundRouterFactory = modules.FactoryWithConfig[typesP2P.Router, *config.BackgroundConfig]
Expand Down Expand Up @@ -73,7 +72,7 @@ type backgroundRouter struct {
subscription *pubsub.Subscription
// kadDHT is a kademlia distributed hash table used for routing and peer discovery.
kadDHT *dht.IpfsDHT
// TECHDEBT: `pstore` will likely be removed in future refactoring / simplification
// TECHDEBT(#747, #749): `pstore` will likely be removed in future refactoring / simplification
// of the `Router` interface.
// pstore is the background router's peerstore. Assigned in `backgroundRouter#setupPeerstore()`.
pstore typesP2P.Peerstore
Expand Down Expand Up @@ -258,18 +257,11 @@ func (rtr *backgroundRouter) setupDependencies(ctx context.Context, _ *config.Ba
}

func (rtr *backgroundRouter) setupPeerstore(ctx context.Context) (err error) {
rtr.logger.Warn().Msg("setting up peerstore...")

// TECHDEBT(#810, #811): use `bus.GetPeerstoreProvider()` after peerstore provider
// TECHDEBT(#811): use `bus.GetPeerstoreProvider()` after peerstore provider
// is retrievable as a proper submodule
pstoreProviderModule, err := rtr.GetBus().GetModulesRegistry().
GetModule(peerstore_provider.PeerstoreProviderSubmoduleName)
pstoreProvider, err := peerstore_provider.GetPeerstoreProvider(rtr.GetBus())
if err != nil {
return fmt.Errorf("retrieving peerstore provider: %w", err)
}
pstoreProvider, ok := pstoreProviderModule.(providers.PeerstoreProvider)
if !ok {
return fmt.Errorf("unexpected peerstore provider type: %T", pstoreProviderModule)
return err
}

rtr.logger.Debug().Msg("setupCurrentHeightProvider")
Expand All @@ -284,10 +276,7 @@ func (rtr *backgroundRouter) setupPeerstore(ctx context.Context) (err error) {
}

// TECHDEBT(#859): integrate with `p2pModule#bootstrap()`.
if err := rtr.bootstrap(ctx); err != nil {
return fmt.Errorf("bootstrapping peerstore: %w", err)
}

rtr.bootstrap(ctx)
return nil
}

Expand Down Expand Up @@ -343,33 +332,38 @@ func (rtr *backgroundRouter) setupSubscription() (err error) {
}

// TECHDEBT(#859): integrate with `p2pModule#bootstrap()`.
Copy link
Member

Choose a reason for hiding this comment

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

What does integrate with imply longer term? Kind of like consolidation of the two?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#bootstrap() is currently being called in one of the router's setup functions and should be called as part of the P2P module's bootstrapping process.

func (rtr *backgroundRouter) bootstrap(ctx context.Context) error {
func (rtr *backgroundRouter) bootstrap(ctx context.Context) {
// CONSIDERATION: add `GetPeers` method, which returns a map,
// to the `PeerstoreProvider` interface to simplify this loop.
for _, peer := range rtr.pstore.GetPeerList() {
peerList := rtr.pstore.GetPeerList()
for _, peer := range peerList {
if err := utils.AddPeerToLibp2pHost(rtr.host, peer); err != nil {
return err
rtr.logger.Error().Err(err).Msg("error adding peer to libp2p host")
continue
}

libp2pAddrInfo, err := utils.Libp2pAddrInfoFromPeer(peer)
if err != nil {
return fmt.Errorf(
"converting peer info, pokt address: %s: %w",
peer.GetAddress(),
err,
)
rtr.logger.Error().Err(err).Msg("error converting peer info")
continue
}

// don't attempt to connect to self
if rtr.host.ID() == libp2pAddrInfo.ID {
return nil
rtr.logger.Debug().Msg("not bootstrapping against self")
continue
}

rtr.logger.Debug().Fields(map[string]any{
"peer_id": libp2pAddrInfo.ID.String(),
"peer_addr": libp2pAddrInfo.Addrs[0].String(),
h5law marked this conversation as resolved.
Show resolved Hide resolved
"num_peers": len(peerList) - 1, // -1 as includes self
}).Msg("connecting to peer")
if err := rtr.connectWithRetry(ctx, libp2pAddrInfo); err != nil {
return fmt.Errorf("connecting to peer: %w", err)
rtr.logger.Error().Err(err).Msg("error connecting to bootstrap peer")
continue
}
}
return nil
}

// connectWithRetry attempts to connect to the given peer, retrying up to connectMaxRetries times
Expand All @@ -382,11 +376,11 @@ func (rtr *backgroundRouter) connectWithRetry(ctx context.Context, libp2pAddrInf
return nil
}

fmt.Printf("Failed to connect (attempt %d), retrying in %v...\n", i+1, connectRetryTimeout)
rtr.logger.Error().Msgf("failed to connect (attempt %d), retrying in %v...", i+1, connectRetryTimeout)
time.Sleep(connectRetryTimeout)
}

return fmt.Errorf("failed to connect after %d attempts, last error: %w", 5, err)
return fmt.Errorf("failed to connect after %d attempts, last error: %w", connectMaxRetries, err)
}

// topicValidator is used in conjunction with libp2p-pubsub's notion of "topic
Expand Down Expand Up @@ -430,7 +424,6 @@ func (rtr *backgroundRouter) readSubscription(ctx context.Context) {
return
}
msg, err := rtr.subscription.Next(ctx)

if err != nil {
rtr.logger.Error().Err(err).
Msg("error reading from background topic subscription")
Expand Down
Loading
Loading