pocket icon indicating copy to clipboard operation
pocket copied to clipboard

[P2P] [Tooling] Peer discovery `peer list` subcommand

Open bryanchriswhite opened this issue 1 year ago • 4 comments

@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 :police_officer::rotating_light:, 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:

  • #730

Dependencies:

  • #804
  • #891

Type of change

Please mark the relevant option(s):

  • [x] 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-#859)

Testing

  • [x] 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
  • [x] k8s LocalNet; if any infrastructure or configuration changes were made

Required Checklist

  • [x] I have performed a self-review of my own code
  • [x] I have commented my code, particularly in hard-to-understand areas
  • [x] I have added, or updated, godoc format comments on touched members (see: tip.golang.org/doc/comment)
  • [x] 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 avatar Jul 07 '23 11:07 bryanchriswhite

:rotating_light::rotating_light: BEFORE MERGING: double check penultimate commit's CI checks! :rotating_light::rotating_light: (see #900)

bryanchriswhite avatar Jul 13 '23 13:07 bryanchriswhite

To give an update on the changes I made:

  • Merged #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 #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?
  1. 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

h5law avatar Jul 28 '23 17:07 h5law

@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 avatar Jul 28 '23 20:07 Olshansk

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

h5law avatar Jul 31 '23 13:07 h5law