We recently added low-level rate-limiting in 0x Mesh. It works by using a BandwidthCounter to periodically check the incoming bandwidth for each peer and then banning peers who exceed the bandwidth limit. We are also piping logs to our Elasticsearch stack so we can monitor bandwidth usage.

The logs are showing occasional massive spikes in bandwidth usage, far higher than what is reasonable/possible. In one case BandwidthCounter reported an incoming bandwidth of 9 exabytes/second from a single peer, which unfortunately caused that peer to be temporarily banned 😦. There is no way we are actually using that much bandwidth on commodity cloud hosting services like AWS or DigitalOcean, even over a very short time interval. I think the only reasonable explanation is a bug in BandwidthCounter.

I'll attach some screenshots from our logging infrastructure. Let me know if any additional information could be helpful.

This is what normal bandwidth usage looks like (incoming bandwidth maxes out at around 1.5MB/s total across all peers):

Getting ready to start prototyping a multiselect implementation, how do we feel about it living here? It is core.

While working on the utp transport I was in trouble with the Mutliaddr() because I wasn't able to return multiple address if it was listening on 0.0.0.0. So after a bit of testing I discovered returning a ma with ip 0.0.0.0 some how get transformed to mutliple address with the right interface.
But I don't think that a good solution because that highly specialized, lets say I do the same thing for an ethernet transport that would not works, yes I can still contrib to the package but I see 2 big issue with this :

• That makes contributing to libp2p way harder than it should, the multiple repo system is quite confusing at first glance but it have some advantage such as repo are independent and you don't need to know how the whole project works to do your own muxer, transport or dht, you just have to expect them to works (that the real advantage of OOP). But if to make my transport working as expected I have to change other object this advantage its lost and there is no point to have an API system that complex, just one package libp2p with evrythings in it will do the work simply. (I know that a bad solution, this is just an example to show how libp2p does not agree with its principles on that, I don't expect somethings else to interfere with my transport (except maybe for nat traversal but in this case this is maybe better to have a layer below transport dedicated to that maybe with an ListenAndForward(ma multiaddr.MultiAddr) (net.Listener, error) // only accept thinwaist maddr function)).
• That not plug and play, I don't think all transport in the future are gonna be official libp2p transport, if someone makes an morse code transport he should be limited just because that not very usefull and his transport should integrate as well as official transport (just using libp2p.Transport(fancyButQuiteUselessTransport.New)) and not using a fork of libp2p made to be compatible with this transport.

I personaly think here straigh forward is better here, just change Multiaddr() ma.Multiaddr to Multiaddr() []ma.Multiaddr.
But that not retroincompatible ? No, we can easly create 2 wrapper, one for the Transport object (with direct call to the underlying function of the real transport except for Listen wich just return an wrapped Listener object) and one for Listener (with again direct call to all the underlying function of the real listener function except for Multiaddr wich return the original multi addr in a list (and do the transformation of 0.0.0.0 to the real address if needed)).
And wrapping older transport in libp2p.Transport to the newer one.

Obviously that just a discussion but I really think we need to be able to some how return multiple maddr from one listener, how to do it isn't very import (we can also maybe call listener.Multiaddr() multiple times, returning a new maddr each time, cycling beetween all possible and stopping when 2 duplicates are found (2 duplicates just indicate we have looped over the list and for older transport with just 1 maddr they will allways return the same wich is like having an list with a len of 1)).

You can see here that the docs are not being displayed on the new docs site. It looks like having two licenses is not well supported. I'm sending an email to [email protected] about it with a link to this issue and hopefully they'll resolve it. Their site says here that they're using licensecheck but I think it's the code that's using lisensecheck that's at fault, not licensecheck itself.

Where is the implementation of the provide method defined by the Contentrouting interface in this file? Thank you for your answer

There exist tooling that will detect compatibility breaking changes. It would alert us in cases like recently when a new version of insecure transport or addition of event bus broke the compatibility.
One of the tools is https://github.com/smola/gocompat
The idea would be to run it in CI for every PR and compare the compatibility with the latest release/master.

Regarding compatibility: https://blog.merovius.de/2015/07/29/backwards-compatibility-in-go.html is worth a read.

Would help in cases like #23 #37 #38 libp2p/go-libp2p#678

The code base looks very large. How do you achieve zero vulnerability state(its important for P2Pnetwork).

transport.CapableConn has IsClosed method, but it is only called by Conn and it is not public.

Is it possible to expose this interface to network.Conn,
or is there another way to check whether this connection is closed?

Hey there!

I'm using go-libp2p-core v0.9.0 (as an IPFS plugin). I am subscribed to network notifications via the network.Notifiee mechanism:

ipfsInstance.PeerHost.Network().Notify(wiretap)

I don't use the *Stream or Listen* methods, and this is what I currently use to track connected peers on my end:

// WireTap implements the `WireTap` interface to do stuff.
type WireTap struct {
	api         *core.IpfsNode
	connManager connectionManager
	/* ... */
}

type connectionManager struct {
	lock           sync.Mutex
	allConnections map[string]struct{}
	peers          map[peer.ID]peerConnection
}

type peerConnection struct {
	connections   map[string]network.Conn
}

// Connected is called when a connection is opened.
func (wt *WireTap) Connected(_ network.Network, conn network.Conn) {
	remotePeer := conn.RemotePeer()
	log.Debugf("Connection event for PID: %s, Addr: %s\n", remotePeer, conn.RemoteMultiaddr())

	wt.connManager.lock.Lock()
	defer wt.connManager.lock.Unlock()
	if _, ok := wt.connManager.allConnections[conn.ID()]; ok {
		log.Warnf("Duplicate connection ID %s (Duplicate Connection Event)", conn.ID())
		// What now?
	}
	wt.connManager.allConnections[conn.ID()] = struct{}{}

	peerConn, ok := wt.connManager.peers[remotePeer]
	if !ok {
		peerConn = peerConnection{
			connections:   map[string]network.Conn{conn.ID(): conn},
		}
	} else {
		if _, ok := peerConn.connections[conn.ID()]; ok {
			log.Warnf("Duplicate connection ID %s to peer %s", conn.ID(), remotePeer)
			// What now?
		}
		peerConn.connections[conn.ID()] = conn
	}
	wt.connManager.peers[remotePeer] = peerConn
}

// Disconnected is called when a connection is closed.
func (wt *WireTap) Disconnected(_ network.Network, conn network.Conn) {
	remotePeer := conn.RemotePeer()
	log.Debugf("Disconnection event for PID: %s, Addr: %s\n", remotePeer, conn.RemoteMultiaddr())

	wt.connManager.lock.Lock()
	defer wt.connManager.lock.Unlock()
	if _, ok := wt.connManager.allConnections[conn.ID()]; !ok {
		log.Warnf("Missing connection ID %s (Duplicate Disconnection Event or Missing Connection Event)", conn.ID())
		// What now?
	}
	delete(wt.connManager.allConnections, conn.ID())

	peerConn, ok := wt.connManager.peers[remotePeer]
	if !ok {
		log.Warnf("Missing entry for disconnected peer %s", remotePeer)
		// What now?
	} else {
		if _, ok := peerConn.connections[conn.ID()]; !ok {
			log.Warnf("Missing connection entry with ID %s for p %s", conn.ID(), remotePeer)
			// What now?
		}
		delete(peerConn.connections, conn.ID())
		
		if len(peerConn.connections) == 0 {
			delete(wt.connManager.peers, remotePeer)
		}
	}
}

Additionally, I have a goroutine in the background to occasionally print some stats about that:

go func() {
	for {
		peers := wt.api.PeerHost.Peerstore().Peers()
		numConns := 0
		for _, p := range peers {
			conns := wt.api.PeerHost.Network().ConnsToPeer(p)
			numConns += len(conns)
		}

		wt.connManager.lock.Lock()
		numOurPeers := len(wt.connManager.peers)
		numOurConns := len(wt.connManager.allConnections)
		wt.connManager.lock.Unlock()

		log.Infof("We have %d peers. IPFS node has %d connections, we have %d",  numOurPeers, numConns, numOurConns)

		time.Sleep(time.Duration(10) * time.Second)
	}
}()

This is not a 100% accurate comparison, but should be a nice guide.

This all works okay-ish. I occasionally see:

Missing connection ID QmNnooDu7b-3 (Duplicate Disconnection Event or Missing Connection Event)
Missing entry for disconnected peer QmNnooDu7b<redacted>

i.e., we either got a duplicate Disconnected call or didn't get a Connected call.

Additionally, I have noticed that our connection tracking lags behind IPFS', both for connection as well as disconnection events.

The documentation doesn't say much about correctness guarantees, consistency, ... -- What are some properties I can expect?

This is what prevents us from moving to the Unified CI setup (#189). This test is quite annoying, as it relies on time.Sleep invocations, and therefore takes 2s.

go test -run TestResetBandwidthCounter -count 100 -v ./metrics

Unfortunately, our flow metrics depend on time.Now(), there's no way to inject a clock interface, which we could mock in tests.

List of doc improvements mentioned in #1 that will potentially (linking so they don't get lost) get incorporated into the next iteration:

• https://github.com/libp2p/go-libp2p-core/pull/1/files#r276048081
• https://github.com/libp2p/go-libp2p-core/pull/1/files#r276049519
• https://github.com/libp2p/go-libp2p-core/pull/1/files#r276052657
• https://github.com/libp2p/go-libp2p-core/pull/1/files#r276055055
• https://github.com/libp2p/go-libp2p-core/pull/1/files#r276055848
• #1 (review)
• https://github.com/libp2p/go-libp2p-core/pull/1/files#r276497226

These tests are currently flaky, and we won't be able to proceed with repo consolidation (libp2p/go-libp2p#1556) until they're fixed. Injecting a mock clock (similar to what we did in https://github.com/libp2p/go-libp2p/pull/1516/files) should be a straightforward way to test this reliably, and dramatically speed up the test at the same time.

Can anyone help me with the sample snippet to convert libp2p PrivKey to https://pkg.go.dev/crypto/ecdsa PrivateKey?

We should set a minimum key size for RSA, and force the user to set a debug flag to lower it. Unfortunately, this may end up breaking a ton of tests so we'll have to do this carefully.

However, this is really quite critical. It's unfortunately easy to accidentally create small keys.

Using this library within a project that performs checks similar to FOSSA will trigger an error as this library is currently unlicensed.

I'm in the midst of writing my own libp2p transport. However, it would be very helpful to have interface tests like this js transport interface so that at the very least we, as transport writers, can ensure that we have the basic functionality to ensure that our transports work with libp2p.

A transport implementation should be able to signal the capabilities it has by default. Some transports might already have some of the layers we currently add on top, such as QUIC already having build in multiplexing, and cjdns already having transport level encryption.

This can even be extended to help us move to supporting unreliable transports.

I think some of the general traits we want to have are (non-exhaustively):

• HasEncryption
• HasMultiplexing
• IsReliable

I don't understand this bit of the API. When we are handling Closer interfaces, we close them when we are done using the resource. However, this Closer doesn't actually close the stream. It seems we have to call Reset which actually sends an error to the peer.

We have had some pain with this when trying the following logic:

• Send a message to the peer
• Close the connection because we don't care about the response or if they received it.

For the second step, if we call Reset then the peer will receive the error only and does not process the message. However, if we call Close and the peer doesn't then we have this dangling connection that we don't care about.

We have now merged the spec for the insecure channel and we need to implement it to (a) fix downstream bugs, (b) enable interoperability testing with py-libp2p and jvm-libp2p.

This is where it's at: https://github.com/libp2p/go-libp2p-core/blob/master/sec/insecure/insecure.go

CC @mhchia @Nashatyrev.

RandPeerID (and therefore RandPeerIDFatal functions can return the same peerID on systems that do not fully support time's UnixNano() (e.g. time.Now().UnixNano()). See golang/go#27301 for details.

There are any number of ways to solve this including:

1. Replace time.Now().UnixNano() with math/rand's rand.Int63()
2. Use a workaround increment counter as in:

   go-libp2p-core/test/crypto.go

   Lines 11 to 20 in 79bf30a

   	var generatedPairs int64 = 0
   	func RandTestKeyPair(typ, bits int) (ci.PrivKey, ci.PubKey, error) {
   	seed := time.Now().UnixNano()
   	// workaround for low time resolution
   	seed += atomic.AddInt64(&generatedPairs, 1) << 32
   	return SeededTestKeyPair(typ, bits, time.Now().UnixNano())
   	}

3. Use a global locked random reader as math/rand does internally https://github.com/golang/go/blob/f518a96e009157c51f8ecf619032bbb7873228f9/src/math/rand/rand.go#L288
4. Stop using non-seeded randomness in our tests so that they're actually reproducible (e.g. use random seeds controlled by the tests instead of by time)

My personal favorites are 1 and 4. However, whatever gets this issue fixed ASAP works for me. I'm also happy to implement the changes as long as I get pre-approval on which method (1,2,3,4, or your favorite method here) to use.

@raulk @Stebalien

Since peer.IDs defines themselves as str, defining a map[peer.ID]<type> and marshaling that to JSON does not make use of the MarshalText() facility to encode the map keys. Somehow default encoding/decoding when doing it doesn't work (it decodes peers with different IDs but doesn't complain, which is worse).

See golang/go#28827

Probably doing the same thing as cids and switching to type ID struct { str string } would fix this.

However, this might be very breaking because people rely on doing pid1 == pid2 (and I think that would stop working, at least from outside this package? it is not clear from the Go documentation).

Thoughts?

Unfortunately, this means that anything wanting to use the libp2p interfaces (even light-clients that leave all the heavy lifting up to a daemon) need to import opencensus.

From the looks of it, we aren't actually using any of the code in metrics/register.go. We might want to consider removing this code unless we think we're actually going to use it.

Note: this pulls in stuff like grpc, google's cloud API libraries, etc.

We just released v0.16.0, which include the btcec update (#245).

go mod tidy now produces the following error (see libp2p/go-libp2p#1521):

❯ go mod tidy
github.com/libp2p/go-libp2p imports
        github.com/libp2p/go-libp2p-core/crypto imports
        github.com/btcsuite/btcd/btcec/v2/ecdsa tested by
        github.com/btcsuite/btcd/btcec/v2/ecdsa.test imports
        github.com/btcsuite/btcd/chaincfg/chainhash loaded from github.com/btcsuite/[email protected],
        but go 1.16 would fail to locate it:
        ambiguous import: found package github.com/btcsuite/btcd/chaincfg/chainhash in multiple modules:
        github.com/btcsuite/btcd v0.22.0-beta (/Users/marten/src/go/pkg/mod/github.com/btcsuite/[email protected]/chaincfg/chainhash)
        github.com/btcsuite/btcd/chaincfg/chainhash v1.0.0 (/Users/marten/src/go/pkg/mod/github.com/btcsuite/btcd/chaincfg/[email protected])

To proceed despite packages unresolved in go 1.16:
        go mod tidy -e
If reproducibility with go 1.16 is not needed:
        go mod tidy -compat=1.17
For other options, see:
        https://golang.org/doc/modules/pruning

Updating btced to v2.2.0 (from v2.1.3) doesn't solve the problem. I also haven't succeeded in manually adding github.com/btcsuite/btcd/chaincfg/chainhash to go-libp2p's go.mod file.
People had a similar issue in btcsuite/btcd#1839, but I couldn't make it work in go-libp2p either.

This is now blocking the go-libp2p v0.20.0 release. I'm planning to revert #245, unless we can get this solved within the next 24 hours or so.

cc @brianathere

i read the code and find that there is no method to set Stat for Stream. right?

Signature index here is set to 5, https://github.com/libp2p/go-libp2p-core/blob/v0.6.0/record/pb/envelope.proto#L29.

The spec is 4, which is should be. https://github.com/libp2p/specs/pull/217/files#diff-1d3e44bb1e34e0581019f35b96151a0dR70

By adding EventBus() method to Host interface and releasing 0.0.4 a lot of projects will stop to build after dependency update.

I would suggest considering an act of adding a method to an interface a breaking change and:

1. be careful about it (first update and release the implementation)
2. increase version number accordingly. As we are still in v0, this would mean incrementing minor version number.

We currently collect per-peer bandwidth metrics and never clear them out. These structs are really tiny but they do add up over time so we should clear them out occasionally.

(Same for read/write variants.)

The current PrivKey is a Key that can also sign stuff, so it actually is a SigKey. You could also let MACs implement this interface.

• Signatures (SigKey and VerifyKey)
• Key Exchange (KXPrivKey.Exchange(KXPubKey)
• Symmetric Secrets implementing SignKey and VerifyKey as well as EncryptKey and DecryptKey and AuthEncKey and AuthDecKey (as in golang.org/x/crypto/nacl/secretbox).
• Asymmetric Authenticated Encryption, providing defaults by combining Key Exchange + symmetric AE (or just use golang.org/x/crypto/nacl/box).

I'll keep on thinking about this and drop some notes over the day.

See #1 (comment).

I spent all day in vain to find a small example

I wrote the classic listening echo server

Is it possible to receive an event on the server when a client connects?

I would like to know PeerID and its IP address

I tried writing this piece of code:

sub, err := cfg.Host.EventBus().Subscribe(&event.EvtPeerConnectednessChanged{}) if err != nil { panic(err) } defer sub.Close()

but then I wasn't able to bind it to a function to get the data on the client

I am getting

argument 2 has an unexpected type ConnectionGater

when am trying to create a libp2p host as follows
libp2p.Transport(libp2pquic.NewTransport),
I am using the github.com/libp2p/go-libp2p-quic-transport v0.4.0 and github.com/libp2p/go-libp2p-core v0.5.6.

Can you please help me resolve this issue?

EDIT: I am able to resolve this error by downgrading go-libp2p-quic-transport v0.3.7, I think go-libp2p-core v0.5.6. doesn't yet support the Conngating that is included in go-libp2p-quic-transport v0.4.0

Consider the following test case:

package main

import (
	"context"
	"fmt"
	"log"
	"time"

	"github.com/libp2p/go-libp2p"
	"github.com/libp2p/go-libp2p-core/event"
	"github.com/libp2p/go-libp2p-core/peer"
)

func main() {
	h, err := libp2p.New(context.Background())
	if err != nil {
		log.Fatal(err)
	}

	sub, err := h.EventBus().Subscribe(new(event.EvtPeerConnectednessChanged))
	if err != nil {
		log.Fatal(err)
	}
	defer sub.Close()

	go func() {
		for evt := range sub.Out() {
			fmt.Println(evt)
		}
	}()

	h2, err := libp2p.New(context.Background())
	if err != nil {
		log.Fatal(err)
	}

	if err = h2.Connect(context.Background(), peer.AddrInfo{
		ID:    h.ID(),
		Addrs: h.Addrs(),
	}); err != nil {
		log.Fatal(err)
	}

	fmt.Printf("have %d conn(s)\n", h.Network().Connectedness(h2.ID()))
	time.Sleep(time.Second)
}

Output:

have 1 conn(s)

Based on the docstring for EvtPeerConnectednessChanged, I would have expected the goroutine to print once.

Is this a bug?

This repository might've been useful 5 years ago when it was first created, but context has gotten really good first-class support in Golang, and its entirely possible to do exactly what goprocess does, except better, and more efficient.

Generally I see goprocess being used in a lot of places where goroutines are created. This makes creating a goroutine a very non-lightweight operation which is the exact opposite. Creating a goroutine should be lightweight!

Consider what happens everytime the Go function is called from goprocess, and this is done a lot!!! (https://github.com/jbenet/goprocess/blob/7f9d9ed286badffcf2122cfeb383ec37daf92508/impl-mutex.go#L26).

	return &process{
		teardown: tf,
		closed:   make(chan struct{}),
		closing:  make(chan struct{}),
		waitfors: make(map[*processLink]struct{}),
		children: make(map[*processLink]struct{}),
	}

That means every single goroutine we spin up creates a fuck ton of allocation. And its done twice every time Go is called!

// This is because having the process you
func Go(f ProcessFunc) Process {
	// return GoChild(Background(), f)

	// we use two processes, one for communication, and
	// one for ensuring we wait on the function (unclosable from the outside).
	p := newProcess(nil)
	waitFor := newProcess(nil)
	p.WaitFor(waitFor) // prevent p from closing
	go func() {
		f(p)
		waitFor.Close() // allow p to close.
		p.Close()       // ensure p closes.
	}()
	return p
}

There's so many simpler and more idiomatic approaches like:

go func() {
select {
case <-ctx.Done():
default:
}
  // do stuff
}()

In some of the forks I have for libp2p, and ipfs libraries the profiling I do makes jbenet/goprocess
show up as a noticeable consumer of resources, and I encourage this to be done by libp2p developers because it's pretty clear goprocess needs to go.

We currently depend on that for the infrequently used secp256k1 key type. Unfortunately, we do have users using this, at least for IPNS. We may be able to deal with this by supporting out-of-tree plugins for new key types.

We could also try extracting the package we need from upstream.

BandwidthCounter::Clear would be very useful to drop all previous data and start from the beginning. Restarting ipfs daemon is not a solution for all cases.

@Stebalien I can do that but I need help. Would you please guide me how to do this?

• Add methods to clear stats libp2p/go-flow-metrics#10
• Add API to reset bandwidth counters #71
• Expose reset as part of the interface #72
• #72 (comment)
• Release go-libp2p-core
• Release go-libp2p
• Release go-ipfs

Hey there,

I followed along the multipro example in the go-libp2p-examples repository.

When I'm writing to the stream in the following way:

func (t *TransferProtocol) Transfer(ctx context.Context, peerId peer.ID, payload io.Reader) (error) {

	s, err := t.node.NewStream(ctx, peerId, transfer)
	if err != nil {
		return err
	}

	_, err = io.Copy(s, payload)
	if err != nil {
		return err
	}
        
	err = s.Close()
	if err != nil {
		return err
	}
}

The recipient doesn't receive all of the data and errors out saying that the stream was closed (or something similar along the lines). Now I saw on the MuxedStream interface:

// Stream is a bidirectional io pipe within a connection.
type MuxedStream interface {
	...
	// Close closes the stream.
	//
	// * Any buffered data for writing will be flushed.
	// * Future reads will fail.
	// * Any in-progress reads/writes will be interrupted.
	//
	// Close may be asynchronous and _does not_ guarantee receipt of the
	// data.
	io.Closer

	// CloseWrite closes the stream for writing but leaves it open for
	// reading.
	//
	// CloseWrite does not free the stream, users must still call Close or
	// Reset.
	CloseWrite() error

	// CloseRead closes the stream for writing but leaves it open for
	// reading.
	//
	// CloseRead does not free the stream, users must still call Close or
	// Reset.
	CloseRead() error

	// Reset closes both ends of the stream. Use this to tell the remote
	// side to hang up and go away.
	Reset() error

	...
}

Close may be asynchronous and does not guarantee receipt of the data.

Now I'm wondering what is the correct way to send the data and free its associated resources.

Thanks in advance

I need to unicast messages to other peers on my network. I want to store the AddrInfo struct on my message so I have the nodes addr who unicast me so I can send a response back.

func init() {
    gob.Register(&Message{})
}

// Message stores a type of message and a body.
// The message is the data that gets passed between nodes when they communicate.
type Message struct {
    ID         string           `json:"id"`
    Body       []byte           `json:"body"`        // the actual payload.
    OriginNode peer.AddrInfo    `json:"origin_node"` // the node that sent the message
}

// NewMessage builds up a new message.
func NewMessage(b []byte, o peer.AddrInfo) Message {
    return Message{
        ID:        uuid.New().String(),
        Body:      b,
        OriginNode: o,
    }
}

// Marshal takes a node message and marshals it into an array of bytes.
func (m Message) Marshal() []byte {
    var buf bytes.Buffer

    enc := gob.NewEncoder(&buf)

    if err := enc.Encode(m); err != nil {
        zap.S().Fatal(err)
    }

    return buf.Bytes()
}

// UnmarshalMessage takes a slice of bytes and a returns a message struct.
func UnmarshalMessage(input []byte) (Message, error) {
    msg := Message{}
    buf := bytes.NewBuffer(input)
    dec := gob.NewDecoder(buf)

    if err := dec.Decode(&msg); err != nil {
        return Message{}, err
    }

    return msg, nil
}

When I marshal my message struct and then attempt to unmarshal I receive the following error:

panic: interface conversion: interface is nil, not encoding.BinaryUnmarshaler [recovered]
panic: interface conversion: interface is nil, not encoding.BinaryUnmarshaler
UnmarshalMessage(0xc0001c7200, 0x454, 0x480, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, .

Is there something I need to do to be able to marshal and unmarshal my struct that contains the peer.AddrInfo ?

This could block on some systems that restrict the number of open streams.

Currently peer.Set allows only to add peers but it would be nice to have a remove functionality.

I've been trying to figure out how to integrate the signed routing records (or maybe signed address records? names are hard) into the DHT and other peer routing systems in go-libp2p. This issue is just a place to think out loud about it and get feedback from other people before I start diving in too deep.

In my opinion so far, it looks like the simplest way to go would be to add another field (containing the signed record) to the peer.AddrInfo struct, since it's used in all the places where we'd like to add signed addresses - mostly the PeerRouting and ContentRouting interfaces, but also in the kad-dht internals.

Extending the AddrInfo struct with a new field would mean that we wouldn't need to define new SignedPeerRouting and SignedContentRouting interfaces; we could just use the existing interfaces and have the implementations include the signed address records if they were able to find them, and leave the field nil if they can't. Callers who care about signed addresses could use the new field,
and callers who don't could ignore it.

This seems like it would make updating the DHT much simpler than defining new interfaces that are essentially copies of PeerRouting and ContentRouting but with a different return type. For example, this gnarly findProvidersAsync implementation could just be tweaked to include signed records in the results, rather than having to duplicate the whole method to return signed records instead of AddrInfos.

However, it turns out that adding a field to a struct is a breaking change, since people might be embedding your struct in their own types and you can't guarantee that the new field won't collide with one of theirs. Since we're making other breaking changes to the Peerstore interface (in #73) to add signed records in the first place, maybe this isn't a huge deal if we communicate it clearly.

The bigger question is whether we really do want separate interfaces for finding signed peer addresses for design / architecture reasons. Maybe conflating the two is a bad idea, and they are different enough to warrant separate interfaces. In my mind they seem really related - when I call FindPeer, I just want to know the best way to connect to the target peer, and it seems like that should include the signed addresses if possible.

One argument for making them separate is that I might only want signed addresses, and I don't want calls to FindProvidersAsync to give me a bunch of useless unsigned results.

Another reason is that peer.AddrInfo seems to be used in a lot of places, and shoving a field in there because it's convenient for this one use case might be rude 😄

So, if we're not extending peer.AddrInfo, it seems like we want to define something like this:

// SignedPeerRouting is for finding SignedRoutingState records for remote
// peers. SignedRoutingState records contain address information that has been
// issued by the peer itself and signed to authenticate and prevent tampering.
// SignedPeerRouting may be used instead of or in addition to PeerRouting,
// depending on the security requirements of the system.
type SignedPeerRouting interface {
	// FindPeerRoutingState searches for a peer with the given ID and
	// returns a SignedRoutingState record produced and signed by that peer.
	FindPeerRoutingState(context.Context, peer.ID) (*SignedRoutingState, error)
}

// SignedContentRouting is similar to ContentRouting, in that it finds information
// about who has what content. However, addresses for content providers are returned
// in the form of SignedRoutingState records, which are issued and signed by the
// providing peer themselves.
type SignedContentRouting interface {
	// Search for peers who are able to provide a given key. A SignedRoutingState
	// record will be sent over the returned channel for each discovered content provider.
	FindSignedProvidersAsync(context.Context, cid.Cid, int) <-chan SignedRoutingState
}

Where SignedRoutingState is the address record defined in #73, which contains the original signed envelope along with an "unpacked" address list.

Anyway, that's where my head is at. What do people think?

Currently, there's no conflict management.

Hello,

I've just upgraded to github.com/libp2p/[email protected], which removes helpers.FullClose. As of this upgrade, the following packages are broken:

• github.com/libp2p/[email protected]
• github.com/libp2p/[email protected]
• github.com/libp2p/[email protected]

Here's the error message that appears:

# github.com/libp2p/go-libp2p/p2p/protocol/identify
../../../../pkg/mod/github.com/libp2p/[email protected]/p2p/protocol/identify/id.go:380:3: undefined: helpers.FullClose
../../../../pkg/mod/github.com/libp2p/[email protected]/p2p/protocol/identify/id.go:424:20: undefined: helpers.FullClose
../../../../pkg/mod/github.com/libp2p/[email protected]/p2p/protocol/identify/id_delta.go:29:8: undefined: helpers.FullClose
../../../../pkg/mod/github.com/libp2p/[email protected]/p2p/protocol/identify/peer_loop.go:132:8: undefined: helpers.FullClose
../../../../pkg/mod/github.com/libp2p/[email protected]/p2p/protocol/identify/peer_loop.go:153:8: undefined: helpers.FullClose
# github.com/libp2p/go-libp2p-pubsub
../../../../pkg/mod/github.com/libp2p/[email protected]/comm.go:128:8: undefined: helpers.FullClose
../../../../pkg/mod/github.com/libp2p/[email protected]/tracer.go:263:5: undefined: helpers.FullClose
# github.com/libp2p/go-libp2p-kad-dht
../../../../pkg/mod/github.com/libp2p/[email protected]/dht_net.go:396:7: undefined: helpers.FullClose
../../../../pkg/mod/github.com/libp2p/[email protected]/dht_net.go:446:7: undefined: helpers.FullClose