Skip to content

P2P bootstrapping - #14

Merged
tzdybal merged 9 commits into
mainfrom
tzdybal/p2p/bootstrapping
Mar 9, 2021
Merged

P2P bootstrapping#14
tzdybal merged 9 commits into
mainfrom
tzdybal/p2p/bootstrapping

Conversation

@tzdybal

@tzdybaltzdybal commented Feb 27, 2021

Copy link
Copy Markdown
Contributor

First step in implementation of P2P layer will be connection bootstrapping.

  • Node private key will be generated and provided by cosmos-sdk.

This work is related to #2.

@tzdybal
tzdybal requested a review from liamsiFebruary 27, 2021 12:09
@tzdybal
tzdybal changed the base branch from main to tzdybal/abci_methodsFebruary 27, 2021 12:09
@codecov-io

codecov-io commented Feb 27, 2021

Copy link
Copy Markdown

Codecov Report

Merging #14 (aa83491) into main (363cec3) will increase coverage by 72.09%.
The diff coverage is 72.09%.

Impacted file tree graph

@@ Coverage Diff @@## main #14 +/- ##
=========================================
+ Coverage 0 72.09% +72.09% 
=========================================
Files 0 3 +3 Lines 0 43 +43 =========================================
+ Hits 0 31 +31 - Misses 0 6 +6 - Partials 0 6 +6 
Impacted FilesCoverage Δ
p2p/client.go62.96% <62.96%> (ø)
conv/crypto.go80.00% <80.00%> (ø)
conv/config.go100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 363cec3...aa83491. Read the comment docs.

Comment threadnode/node.go Outdated
func (n *Node) loadPrivateKey(nodeKey p2p.NodeKey) error {
switch nodeKey.PrivKey.Type() {
case "ed25519":
privKey, err := crypto.UnmarshalEd25519PrivateKey(nodeKey.PrivKey.Bytes())

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We should at least consider if doesn't make more sense, to directly generate a libp2p/crypto key in startInProcess and pass it into optimint directly instead. It seems like we have to modify that file (server/start.go) in the sdk anyway. For now that shouldn't matter to much until we have a clearer picture of the other changes in the sdk (if any).

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Very good comment. I was focusing on minimizing changes in cosmos-sdk, but probably I went to far.
Created tracking issue for this: #15.

@tzdybal
tzdybal changed the base branch from tzdybal/abci_methods to mainMarch 1, 2021 09:05
Libp2p private key is added to Node struct. Constructor accepts
NodeKey (from Tendermint) because it is used by cosmos-sdk.
It was a placeholder from initial commit.
@tzdybal
tzdybalforce-pushed the tzdybal/p2p/bootstrapping branch from faf5e50 to 504f90fCompareMarch 1, 2021 09:07
Type conversion from Tendermint/cosmos-sdk to libp2p/Optimint types is
extracted to `conv` package. Conversion functions can be used directly
in SDK, so Optimint is called with proper types, API is not cluttered
with different types.
This is just a beginning, more fields will be added as needed.
@tzdybal
tzdybal marked this pull request as ready for review March 3, 2021 10:31
@liamsi

Copy link
Copy Markdown
Contributor

So nice to see this coming together! Can you briefly explain what the current scope of this PR is? As far as I understand, it connects to a bunch of seed (aka bootstrap) nodes?

Comment threadp2p/client.go
var err error
var p2pId multiaddr.Multiaddr
if at := strings.IndexRune(addr, '@'); at != -1 {
p2pId, err = multiaddr.NewMultiaddr("/p2p/" + addr[:at])

@liamsiliamsiMar 4, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

/p2p/Is that really a valid MultiAddr? NVM, it's valid. I need to go and read more about multiaddrs myslef. preferred over /ipfs is not a really helpful description of the protocol here: https://github.com/multiformats/multiaddr/blob/master/protocols.csv

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

/p2p is something we can use without "registering" protocol, and it supports IDs - it's basically what we need, without reinventing the wheel.

Comment threadp2p/client_test.go
{"ip only", "127.0.0.1", nil, ErrInvalidAddress.Error()},
{"with invalid id", "deadbeef@127.0.0.1:1234", nil, "failed to parse multiaddr"},
{"valid", "127.0.0.1:1234", valid, ""},
{"valid with id", "k2k4r8oqamigqdo6o7hsbfwd45y70oyynp98usk7zmyfrzpqxh1pohl7@127.0.0.1:1234", withId, ""},

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thinking about this further: While it is right that SDK users will be used to the "@-format", we will likely use the public libp2p bootstrap nodes for some first actual experiments (let's say you and me want to run a node and see if they find each other). Then, we would go find the libp2p bootstrap nodes, translate those addrs to the @-format to use them in the local config, only for them to be translated back to the original format. So maybe we should actually just use the libp2p-style addrs directly 🤔

@evan-forbes

Copy link
Copy Markdown
Contributor

Just to make sure I'm understanding everything, the libp2p host.Host handles the "service" in the background for us and we just have to connect nodes via bootstrap, correct?

@tzdybal

Copy link
Copy Markdown
ContributorAuthor

@evan-forbes yes, host does all the heavy lifting. We still need static list of nodes, so we can begin peer discovery (I'm working on/debugging it in #17).

Comment threadp2p/client_test.go
t.Parallel()

valid := mustGetMultiaddr(t, "/ip4/127.0.0.1/tcp/1234")
withId := mustGetMultiaddr(t, "/ip4/127.0.0.1/tcp/1234/p2p/k2k4r8oqamigqdo6o7hsbfwd45y70oyynp98usk7zmyfrzpqxh1pohl7")

@evan-forbesevan-forbesMar 6, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

What is this address? It's in the multiaddr repo too.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

I just used some example to from multiaddr repo to ensure that this address is valid.

@evan-forbesevan-forbes left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I appreciate how strictly contained this PR is, and learned a thing or two about libp2p. LGTM 👍

@tzdybaltzdybal mentioned this pull request Mar 9, 2021

@liamsiliamsi left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks like a solid start! We might want to reconsider "translating" between tendermint / SDK node.IDs and libp2p IDs. This will be discussed in #16 .

@tzdybal
tzdybalforce-pushed the tzdybal/p2p/bootstrapping branch from 63118f1 to bb1da59CompareMarch 9, 2021 13:28
@tzdybal
tzdybal merged commit f7a551f into mainMar 9, 2021
@liamsi
liamsi deleted the tzdybal/p2p/bootstrapping branch March 9, 2021 13:31
This was referenced Nov 19, 2025
@claudeclaudeBot mentioned this pull request Jan 5, 2026
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@tzdybal@codecov-io@liamsi@evan-forbes