oasis-core icon indicating copy to clipboard operation
oasis-core copied to clipboard

go/worker/common/p2p: Bootstrap libp2p peers

Open peternose opened this issue 3 years ago • 6 comments

Task

Add libp2p seed node which can bootstrap libp2p peers.

Notes:

  • Rate limiter was not added. Peer store is simple without peer scoring, advanced peer selection, etc. This can/should be added in separate PRs.
  • Borrowed code from our p2p rpc implementation as client is not generic. To use it the client should be refactored first.

Configuration

Libp2p seed node can be configured using the following settings:

# Host
worker.p2p.port

# Connection manager
worker.p2p.max_num_peers
worker.p2p.peer_grace_period
worker.p2p.persistent_peers

# Connection gater
worker.p2p.blocked_peers

# Discovery
worker.p2p.discovery.bootstrap.enable

Peers still configure seeds using the following setting:

p2p.seeds

As we currently run 2 seed nodes on one address but on different ports, 2 seeds need to be configured.

p2p.seeds:
 - pubkey@IP:consensus-port
 - pubkey@IP:woker-p2p-port

Test

Tested locally. If we disable registry discovery (e.g. hack connectWorker in PeerManager to not connect at all), the peer nodes still connect.

"runtimes": {
  "8000000000000000000000000000000000000000000000000000000000000000": {
    "committee": {
      "peers": [
          "/ip4/192.168.0.2/tcp/20017/p2p/12D3KooWP2DNWFz7ivdpENeQirCkPTwCJ762EKChdKHCNEuAxUVC",
          "/ip4/192.168.0.2/tcp/20014/p2p/12D3KooWGe86FFdDn3vZtEupwTM55LjsL2WVGfM1jtrhEaGdgh7X",
          "/ip4/192.168.0.2/tcp/20019/p2p/12D3KooWD8gjDP1pxqhpjy2ZkPguWVKSMBu26mJXwridnoAynhxM"
      ],
    },
  },
}

Seed node now shows addresses and peers from both seed nodes (Tendermint, libp2p).

➜  seed-0 oasis-node control status 
{
  "seed": {
    "addresses": [
      // Tendermint address.
      "[email protected]:20000",
      // Libp2p address.
      "{12D3KooWALMXUq9y2BwY6VgEdJyVDcmWrdQsJ6LJth7eoCcRhheN: [/ip4/192.168.64.107/tcp/20001 /ip4/127.0.0.1/tcp/20001]}"
    ],
    "node_peers": [
      // Tendermint peers.
      "[email protected]:20006",
      "[email protected]:20015",
      "[email protected]:20009",
      "[email protected]:20012",
      "[email protected]:20018",
      "[email protected]:20002",
      "[email protected]:20004",
      // Libp2p peers.
      "/ip4/127.0.0.1/tcp/20008/p2p/12D3KooWCv1cFd8k1MhxaKU2LKuCanPLx8sYym6bz2qQWoPkhVsW",
      "/ip4/127.0.0.1/tcp/20011/p2p/12D3KooWEDUkJKh4xRnjrfA6iNQZCKCDvnrLrb6tZfhV9Kkm9gG4",
      "/ip4/127.0.0.1/tcp/20014/p2p/12D3KooWGe86FFdDn3vZtEupwTM55LjsL2WVGfM1jtrhEaGdgh7X",
      "/ip4/127.0.0.1/tcp/20017/p2p/12D3KooWP2DNWFz7ivdpENeQirCkPTwCJ762EKChdKHCNEuAxUVC",
      "/ip4/127.0.0.1/tcp/20019/p2p/12D3KooWD8gjDP1pxqhpjy2ZkPguWVKSMBu26mJXwridnoAynhxM"
    ]
  }
}

peternose avatar Oct 13 '22 09:10 peternose

Codecov Report

Merging #4981 (7a39265) into master (fc1a00b) will increase coverage by 0.21%. The diff coverage is 79.81%.

:exclamation: Current head 7a39265 differs from pull request most recent head 7b9edd2. Consider uploading reports for the commit 7b9edd2 to get more accurate results

@@            Coverage Diff             @@
##           master    #4981      +/-   ##
==========================================
+ Coverage   66.90%   67.11%   +0.21%     
==========================================
  Files         490      501      +11     
  Lines       52402    53179     +777     
==========================================
+ Hits        35059    35692     +633     
- Misses      13056    13153      +97     
- Partials     4287     4334      +47     
Impacted Files Coverage Δ
go/oasis-node/cmd/control/control.go 21.42% <0.00%> (+0.67%) :arrow_up:
go/p2p/backup/memory.go 0.00% <0.00%> (ø)
go/oasis-node/cmd/node/seed.go 43.75% <28.57%> (-4.59%) :arrow_down:
go/p2p/host.go 51.72% <51.72%> (ø)
go/p2p/p2p.go 71.96% <67.64%> (+7.04%) :arrow_up:
go/p2p/discovery/bootstrap/server.go 75.55% <75.55%> (ø)
go/p2p/seed.go 76.00% <76.00%> (ø)
go/p2p/peermgmt/backup.go 74.35% <77.41%> (+6.17%) :arrow_up:
go/p2p/rpc/rpc.go 80.00% <77.77%> (-20.00%) :arrow_down:
go/p2p/api/convert.go 73.33% <79.31%> (+10.83%) :arrow_up:
... and 60 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

codecov[bot] avatar Oct 13 '22 10:10 codecov[bot]

Seed node now shows addresses and peers from both seed nodes (Tendermint, libp2p).

Can we have a consistent peer output (e.g. reuse the multiaddr string format as is used for committee peers)?

Tested locally. If we disable registry discovery (e.g. hack connectWorker in PeerManager to not connect at all), the peer nodes still connect.

Can you also test how this works between consensus-only nodes (e.g., between validators that don't have any runtimes configured)?

kostko avatar Oct 13 '22 11:10 kostko

Can we have a consistent peer output (e.g. reuse the multiaddr string format as is used for committee peers)?

First answer would be no, as I don't see Tendermint exposing peer's public key. They use IDs to verify public keys gained with secretConn.RemotePubKey(), but I don't see this being exposed anywhere.

Can you also test how this works between consensus-only nodes (e.g., between validators that don't have any runtimes configured)?

Consensus nodes use only Tendermint's p2p network, our libp2p worker is not even started. As they do not participate, there should be nothing to test. I could only check how Tendermint reacts when he gets an invalid seed address (same ip but different port). Locally had no problems. I did check libp2p and that one has no problems as seed addresses are merged into one multiaddr and the library then uses the right port.

peternose avatar Oct 13 '22 12:10 peternose

First answer would be no, as I don't see Tendermint exposing peer's public key. They use IDs to verify public keys gained with secretConn.RemotePubKey(), but I don't see this being exposed anywhere.

No, I mean for the libp2p peers, Tendermint peers can stay as they are. Currently it seems like libp2p peers are represented as a dict in some places (seed peers) and a multiaddr string in others (committee peers).

kostko avatar Oct 13 '22 12:10 kostko

Consensus nodes use only Tendermint's p2p network, our libp2p worker is not even started. As they do not participate, there should be nothing to test.

Ok then we need to do this later (e.g. always start the libp2p worker). Because the idea is that we will start using our libp2p stack for some consensus services (e.g. light clients, state sync) and slowly get rid of Tendermint P2P.

kostko avatar Oct 13 '22 12:10 kostko

No, I mean for the libp2p peers, Tendermint peers can stay as they are. Currently it seems like libp2p peers are represented as a dict in some places (seed peers) and a multiaddr string in others (committee peers).

Ok, will change.

peternose avatar Oct 13 '22 12:10 peternose

Addressed the comments:

  • Made discovery async. As peer channel is returned before seed responds, I had to move logic for detecting malicious seeds to the client. Same goes for backoffs. Malicious seeds are now blocked for 1 day.
  • Added BinaryAddrInfo, but I still prefer using json marshaller as code is more clean.
  • Extracted backup logic. Libp2p and discovery peerstores now use backup and scheduler packages for backups and cleanups.
  • The code ignored peer feedbacks because p2p rpc client doesn't have any listeners. And I'm not sure if we should add one as latency here is not that important. We want to connect to all seeds and not only the fastest ones. Furthermore, before using feedbacks we should probably modify them and bind them to methods, as e.g. advertisement can have lower latency than discovery.
  • Changed peerstoreBackup to backup all peers and not only the connected ones.

peternose avatar Nov 18 '22 08:11 peternose