[P2P] [Tooling] Peer discovery peer list subcommand

[bryanchriswhite]

@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 subcommand to the CLI:

Print addresses and service URLs of known peers                                                                                                                                                    │1
                                                                                                                                                                                                   │ 
Usage:                                                                                                                                                                                             │*
  p1 peer list [flags]                                                                                                                                                                             │1
                                                                                                                                                                                                   │4
Flags:                                                                                                                                                                                             │8
  -h, --help   help for list                                                                                                                                                                       │ 
                                                                                                                                                                                                   │!
Global Flags:                                                                                                                                                                                      │2
  -a, --all                     operations apply to both staked & unstaked router peerstores (default true)                                                                              │ 
      --config string           Path to config                                                                                                                                                     │?
      --data_dir string         Path to store pocket related data (keybase etc.) (default "/root/.pocket")                                                                                         │1
  -l, --local                   operations apply to the local (CLI binary's) P2P module rather than being sent to the --remote_cli_url                                                             │4
      --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

Related:

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

Dependencies:

• [P2P] refactor: peerstore provider (part 1) #804
• [CLI] Consistent config/flag parsing & common helpers #891

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 enable_peer_discovery_debug_rpc to P2P config
• added P2P debug message handling support
• added p1 peer subcommand
• added p1 peer list subcommand
• refactored PeerstoreProvider#GetUnstakedPeerstore() implementations
• added PeerstoreProvider#GetStakedPeerstoreAtCurrentHeight()
• refactored bootstrapping as necessary (pre-[P2P] Router bootstrapping #859)

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)

[h5law]

@Olshansk Am I missing something here but what does what about unstaked peers actors mean?

When using the staked router type we are only looking for staked actors? If we want both we would use AllRouterTypes

[bryanchriswhite]

This TODO goes along with the commented code line below. It's referring to the current actor (this local P2P module instance ). My thinking at the time was something along the lines of: if the current actor is unstaked, it may not make sense to ask the peerstore provider for the staked actor set. Perhaps it should indeed work as written and it would be the responsibility of the P2P module to ensure the appropriate peerstore provider implementation is registered and then that of the node setup to ensure a compatible persistence module implementation is also registered (which may not yet exist), for example in the case of light client actors.

[h5law]

@bryanchriswhite I appreciate your response during your time away so no rush on this reply. But AFAIK if we are checking whether the node is staked we need to retrieve the staked peerstore anyway? the p2pModule.isStakedActor() function is as follows:

func (m *p2pModule) getStakedPeerstore() (typesP2P.Peerstore, error) {
	pstoreProvider, err := peerstore_provider.GetPeerstoreProvider(m.GetBus())
	if err != nil {
		return nil, err
	}

	return pstoreProvider.GetStakedPeerstoreAtHeight(
		m.GetBus().GetCurrentHeightProvider().CurrentHeight(),
	)
}

// isStakedActor returns whether the current node is a staked actor at the current height.
// Return an error if a peerstore can't be provided.
func (m *p2pModule) isStakedActor() (bool, error) {
	pstore, err := m.getStakedPeerstore()
	if err != nil {
		return false, fmt.Errorf("getting staked peerstore: %w", err)
	}

	// Ensure self address is present in current height's staked actor set.
	if self := pstore.GetPeer(m.address); self != nil {
		return true, nil
	}
	return false, nil
}

So if we check the addr from bus.GetP2PModule().GetAddress() against this staked list and its not there we return the unstaked peerstore? I feel like this defeats the purpose of us passing in the router type in the first place if we are going to ignore it? Or am I misunderstanding?

[h5law]

Again here I am confused, are we not already getting both staked and unstaked?

[h5law]

To give an update on the changes I made:

• Merged [CLI] Consistent config/flag parsing & common helpers #891 and then merged main into this branch
• Updated errors to be clear they were errors
• Refactored CLI to follow similar pattern to rest (creator functions)
• Consolidated peerstore provider retrieval into a single functon prior to [P2P] Move Peerstore interface to a module-shared pkg #811 merger
• Fixed the fix of the flaky P2P test

There is 2 bugs that are being encountered in this branch currently:

1. on k8s localnet the nodes are unable to bootstrap

• this is unique to k8s localnet and the feat/peer-discovery-list branch
• this appears to be due to the peerstore using the wrong ips potentially?

2. the p1 peers list command does not work

• this is only on localnet but presumed also on k8s however issue issue 1 blocks this from being tested
• error message below

{"level":"debug","time":"2023-07-27T10:29:40+01:00","message":"Creating P2P module"}
{"level":"debug","module":"p2p","time":"2023-07-27T10:29:40+01:00","message":"setupPeerstoreProvider"}
{"level":"debug","module":"p2p","time":"2023-07-27T10:29:40+01:00","message":"loaded peerstore provider..."}
{"level":"fatal","error":"parsing multiaddr from config: resolving peer IP for hostname: validator1: resolving peer IP for hostname: validator1, lookup validator1: no such host","time":"2023-07-27T10:29:40+01:00","message":"Failed to create p2p module"}

When ./bin/p1 peer list calls P2PDependenciesPreRunE prior to running
in app/client/cli/helpers/setup.go

func setupAndStartP2PModule(rm runtime.Manager) {
	bus := rm.GetBus()
	mod, err := p2p.Create(bus)
	if err != nil {
		logger.Global.Fatal().Err(err).Msg("Failed to create p2p module")
	}

	var ok bool
	P2PMod, ok := mod.(modules.P2PModule)
	if !ok {
		logger.Global.Fatal().Msgf("unexpected P2P module type: %T", mod)
	}

	if err := P2PMod.Start(); err != nil {
		logger.Global.Fatal().Err(err).Msg("Failed to start p2p module")
	}
}

in p2p/module.go

	switch m.cfg.ConnectionType {
	case types.ConnectionType_TCPConnection:
		addr, err := m.getMultiaddr()
		if err != nil {
			return nil, fmt.Errorf("parsing multiaddr from config: %w", err)
		}
		m.listenAddrs = libp2p.ListenAddrs(addr)
	case types.ConnectionType_EmptyConnection:
		m.listenAddrs = libp2p.NoListenAddrs
	default:
		return nil, fmt.Errorf(
			// TECHDEBT: rename to "transport protocol" instead.
			"unsupported connection type: %s: %w",
			m.cfg.ConnectionType,
			err,
		)
	}

Specifically in them.getMultiaddr() call is what is failing when it in turn calls Libp2pMultiaddrFromServiceURL which is found in p2p/utils/url_conversion.go

CC: @Olshansk @bryanchriswhite

[Olshansk]

@h5law Appreciate for all the work, improvements, hours spent debugging as well as a concise update summary. I think this is in a good place to pause things for now and have @bryanchriswhite pick it up when he's back.

[h5law]

Appreciate for all the work, improvements, hours spent debugging as well as a concise update summary. I think this is in a good place to pause things for now and have @bryanchriswhite pick it up when he's back.

@Olshansk To give an update I pushed a commit to comment out some buggy lines in the charts yaml files. These were pointed out by @okdas as a sort of chicken-and-egg problem. This stops the bug which breaks bootstrapping in k8s localnet. This enabled me to check the p1 peer list command which works as expected in k8s localnet. This does not affect the bug in docker localnet with the same command