Certified addresses #98

yusefnapora · 2019-11-08T19:07:59Z

This adds new methods to add "certified" addresses to the peerstore, as described in this RFC: libp2p/specs#217.

It fits the API defined in this core PR: libp2p/go-libp2p-core#73.

There are a few missing pieces still:

it seems like adding a new certified RoutingState should clear out addresses that were added from a previous RoutingState. Or we could just leave the old addrs and let them expire...
the addresses from a RoutingState record will expire after a given TTL, but the record itself is still retrievable from the SignedRoutingState(peerId) method. Should it expire also?
I'm storing the signed routing record in the datastore entry as a bytes blob, since I couldn't figure out how to import the .proto file from the -core PR into the peerstore .proto file. It would be nice to have the datastore entry just contain a SignedEnvelope protobuf message instead of serializing the envelope and storing the bytes.

Also, @Stebalien, @raulk & I talked in the RFC comments about hoisting the seq counter from the routing record to the envelope. I didn't, but now I kind of wish I had, since it's awkward to have to deserialize the routing record payload before you can compare seq numbers. In this branch so far, I'm storing both the last received routing record (still in the envelope) and separately storing the seq number, so I can skip unwrapping the envelope just to compare seqs.

Stebalien · 2019-11-09T19:41:20Z

pb/pstore.proto

+
+	// A serialized SignedEnvelope containing the most recent RoutingState record.
+	// TODO: import the SignedEnvelope type instead
+	bytes signedRoutingRecord = 4;


So, instead of duplicating everything, we can just decode with two different protobufs.

// Used by the peerstore to find addresses for a given peer. message AddrEntry { message SignedRoutingRecord { PublicKey publicKey = 1; RoutingState contents = 3; } SignedRoutingRecord record = 1; repeated AddressEntry additionalAddresses = 2; } // Use to pick out the signed record (for the DHT). message OpaqueAddrEntry { bytes record = 1; }

That's an interesting technique.

raulk

Did an initial pass; thanks for this first iteration @yusefnapora!

In general, I think this changeset can be simplified. I'll reason through how these changes would be used in practice (or at least my understanding), and what I think should happen on each action.

When the DHT receives a routing record for a peer, it will hand it off to the peerstore, telling it: "memorize this".
- It's possible that the peer already had a routing record, in which case we'll replace it atomically if the incoming seq is higher.
- We add all addresses from the record into our peerstore.
When the DHT is responding to incoming FIND_PEER, FIND_PROVS, GET requests, it will need to ask the peerstore for the latest routing record of the closest peers.
- A variadic func SignedPeerRecords(ids ...peer.ID) ([]SignedPeerRecord, error) would be useful to batch the query.
- If we have no signed peer record for one or many peers, the DHT would call the existing Addrs() for those.
We should remember the latest signed peer routing all addresses expire. When that happens, the signed peer routing record would be scrapped with it.

I don't think we need to inline the address status (certified flag) with each address. Unless there's a data access pattern that would benefit from that data layout? It's also a bit funky because addresses that are certified with seq = 1, may disappear from seq = 2, so we'd need to reset the certified flag of all addrs with every update, at which point the value of the field starts being questionable.

I'd like to explore this design:

addr_book_record
  \_ peer_id: bytes
  \_ signed_addrs: []AddrEntry
  \_ unsigned_addrs: []AddrEntry
  \_ certified_record
      \_ seq: bytes
      \_ raw: bytes

WDYT?

yusefnapora · 2019-11-11T14:45:14Z

@raulk thanks for the feedback, that makes a lot of sense. I especially like splitting the addr_entries into two sets, rather than having the certified flag. The flag does seem likely to get out of sync. And having a separate list makes it easy to clear out older certified addrs.

We should remember the latest signed peer routing all addresses expire. When that happens, the signed peer routing record would be scrapped with it.

For this, do you mean that when we expire the last entry from the signed_addrs list, we also discard the certified_record? That sounds like a good plan to me.

raulk · 2019-11-11T14:52:06Z

For this, do you mean that when we expire the last entry from the signed_addrs list, we also discard the certified_record? That sounds like a good plan to me.

Yup.

yusefnapora · 2019-11-12T22:30:06Z

I realized yesterday that with the addrs split into two lists, it's possible to have duplicates. Also, updating the TTL using the SetAddr method was only working for unsigned addresses, since it was only looking at the list of unsigned addresses.

The last commit updates the Add/Set addr methods in both implementations to ensure that an addr only exists in either the unsigned or signed list, but not both. It also changes the SetAddr behavior to update the TTL of existing signed addrs, instead of making a duplicate unsigned address entry with the new TTL as it was doing before.

Also, I found a bug! The deleteAddrs method in the datastore impl will throw an index out of bounds error if you call it with a number of addrs >= the number that exist in the store, since the survivors var is incremented when iterating over the input & isn't bounds checked.

I fixed it here, but I can also open a new PR with just the fix if that seems better.

vyzo

For the memory peerstore, it makes more sense to have each segment hold both signed and unsigned addresses.
This will simplify the code, and also obviate the need to take two locks when retrieving addresses.

pstoremem/addr_book.go

- removes CertifiedAddrs method from addr book implementations - asserts AddrBooks are also CertifiedAddrBooks - ensures we either have certified addrs or uncertified, but not both - updates tests for CertifiedAddrs

raulk

@yusefnapora I stopped my review midway because I realised that this PR is assuming that a peer can have both signed and unsigned addresses, which we later decided against. Mind revisiting this logic and updating it to the current model/thinking?

raulk · 2020-01-31T20:14:32Z

pb/pstore.proto

@@ -14,6 +14,11 @@ message AddrBookRecord {
 	// The multiaddresses. This is a sorted list where element 0 expires the soonest.
 	repeated AddrEntry addrs = 2;

+	repeated AddrEntry signedAddrs = 3;


Snake case for protobufs, please 🙏 (here and below)

peerstore.go

raulk · 2020-01-31T20:19:33Z

test/addr_book_suite.go

+		id, _ := peer.IDFromPrivateKey(priv)
+		allAddrs := GenerateAddrs(10)
+		certifiedAddrs := allAddrs[:5]
+		uncertifiedAddrs := allAddrs[5:]
+		rec := peer.NewPeerRecord()
+		rec.PeerID = id
+		rec.Addrs = certifiedAddrs
+		signedRec, err := rec.Sign(priv)


That's a lot of assignments! Enclosing these in a var () block could help readability, due to alignment.

raulk · 2020-01-31T20:20:49Z

pstoremem/addr_book.go

 }

-func (s *addrSegments) get(p peer.ID) *addrSegment {
-	return s[byte(p[len(p)-1])]
+func (segments *addrSegments) get(p peer.ID) *addrSegment {


Receiver names are initials in go. Try ss (plural of s -- segment).

pstoremem/addr_book.go

since signed and unsigned addrs are now mutually exclusive, there's no benefit to keeping two lists of addresses.

raulk

A few important changes needed here.

raulk · 2020-02-10T20:24:34Z

pstoreds/addr_book.go

+	// this has to be done after we add the addresses, since if
+	// we try to flush a datastore record with no addresses,
+	// it will just get deleted
+	pr, err := ab.loadRecord(p, true, false)


Need to lock the record here.

raulk · 2020-02-10T20:25:08Z

pstoreds/addr_book.go

+		log.Errorf("unable to load record for peer %s: %v", p.Pretty(), err)
+		return nil
+	}
+	if pr.CertifiedRecord == nil || len(pr.CertifiedRecord.Raw) == 0 || len(pr.Addrs) == 0 {


Need to lock the record for read here.

raulk · 2020-02-10T20:26:45Z

pstoreds/addr_book.go

+// GetPeerRecord returns a record.Envelope containing a peer.PeerRecord for the
+// given peer id, if one exists.
+// Returns nil if no signed PeerRecord exists for the peer.
+func (ab *dsAddrBook) GetPeerRecord(p peer.ID) *record.Envelope {


Hmmmm... I think this this method should return (*record.Envelope, ok bool, err error). Rationale:

the ok bool is idiomatic in go.

we are currently swallowing errors.

raulk · 2020-02-10T20:28:32Z

pstoreds/addr_book.go

+	addrs := make([]ma.Multiaddr, len(pr.Addrs))
+	for i, a := range pr.Addrs {
+		addrs[i] = a.Addr


Any reason for this change?

raulk · 2020-02-10T20:33:37Z

pstoremem/addr_book.go

@@ -98,6 +110,7 @@ func (mab *memoryAddrBook) gc() {
 	now := time.Now()
 	for _, s := range mab.segments {
 		s.Lock()
+		var collectedPeers []peer.ID


Could use a single slice for all segments, resetting to [:0] on each iteration. That would produce a smaller memory footprint.

raulk · 2020-02-10T20:36:05Z

pstoremem/addr_book.go

+	pidSet := peer.NewSet()
 	for _, s := range mab.segments {
 		s.RLock()
-		for pid, _ := range s.addrs {
-			pids = append(pids, pid)
+		for pid, amap := range s.addrs {
+			if amap != nil && len(amap) > 0 {
+				pidSet.Add(pid)
+			}
 		}
 		s.RUnlock()
 	}
-	return pids
+	return pidSet.Peers()


Why this change? peer.Set is synchronized. Aren't peer IDs unique anyway?

raulk · 2020-02-10T20:37:52Z

pstoremem/addr_book.go

-		amap = make(map[string]*expiringAddr, len(addrs))
+	amap, ok := s.addrs[p]
+	if !ok {
+		amap = make(map[string]*expiringAddr)


Why remove the initial length?

raulk · 2020-02-10T20:40:44Z

pstoremem/addr_book.go

@@ -171,6 +240,21 @@ func (mab *memoryAddrBook) AddAddrs(p peer.ID, addrs []ma.Multiaddr, ttl time.Du
 			}
 		}
 	}
+
+	// when adding signed addrs, make sure only the addrs from the input list remain.
+	if signed {


You should do the removal before. This can remove valid signed addresses if we're transitioning from unsigned to signed.

raulk · 2020-02-10T20:47:45Z

pstoremem/addr_book.go

+		Seq:      rec.Seq,
+	}
+	s.Unlock() // need to release the lock, since addAddrs will try to take it
+	mab.addAddrs(rec.PeerID, rec.Addrs, ttl, true)


Will this behave well in the transition from unsigned to signed? Or when updating the record? Shouldn't we clear the address set first? This is not a purely additive operation, it's a replacement operation.

Stebalien · 2020-02-15T00:37:23Z

pstoremem/addr_book.go

+	// if we have a valid peer record, ignore unsigned addrs
+	peerRec := mab.GetPeerRecord(p)
+	if peerRec != nil {
+		return


This is a bit of a tricky case. We don't want to add random addresses from the network, but we might want to add our own observations (not to advertise, just for local use).

Concrete problem: We have an old record for a peer and now ask IPFS to dial the peer at a new address. IPFS will try to add the new address to the peerstore and fail.

Yeah. We might have to bite the bullet and extend the write methods (Add, Set) to accept option flags, or provenances. This really steps on the toes of the proposal for peerstore v2, though.

Ideally, I'd like to have different "levels" of addresses and, mark the address "level" when returning it (locally set > signed > hearsay). However, that might be a bit tricky to work into the API.

Temporarily store all addresses in the addrbook without classifying their provenance/authoritativeness; even in the presence of a signed peer record.

raulk · 2020-03-06T11:46:20Z

We want to merge this changeset as soon as possible, but in its original form (certified addresses erase uncertified ones, and block any uncertified addresses added thereafter), it would cause regressive behaviour for a few use cases:

ipfs swarm connect command, where the user manually specifies addresses to dial.
Users/apps having their own authenticated records (e.g. Textile threads).

The final solution will revolve around address labels, which are being incubated here: libp2p/go-libp2p-core#123

In the meantime, @Stebalien and I have agreed to merge this PR lifting the restrictions on addresses. Basically, the peerstore will swallow all addresses.

This means that dials can be insecure (i.e. hitting uncertified addresses when a certified record exists), but it will allow us to integrate certified records downstream, while we work on tagging addresses properly so they can be subsequently filtered.

raulk · 2020-03-06T11:46:38Z

There's a huge refactor of the peerstore coming anyway.

yusefnapora added 5 commits November 8, 2019 09:32

add support for certified address records

263c5fe

store original signed routing records & add retreival method

58b7936

go fmt

ad09bee

add tests for certified addrs

5ba6ece

fix bugs adding certified addrs

a05a1e1

yusefnapora requested review from bigs, Stebalien and raulk November 8, 2019 19:08

Stebalien reviewed Nov 9, 2019

View reviewed changes

raulk reviewed Nov 11, 2019

View reviewed changes

yusefnapora added 4 commits November 11, 2019 15:33

use separate lists of signed & unsigned addrs

731dde0

remove signed record when addrs expire

1c5732a

add test for setting all TTLs to -1

8df072f

dedup addrs between signed & unsigned sets

6c26bc5

yusefnapora added 10 commits November 13, 2019 12:15

better bounds checking when deleting addrs

9d4e39c

slight optimization

c446558

accept & return serialized envelopes

c821f29

import PR branch of go-libp2p-core

924831e

go fmt

714dc3d

clear out old signed addrs when adding new routing state

e5cfa72

rm dead code

0c1916a

doc comments

5b63f9f

track API changes in -core

f368134

bump -core dep to new PR commit

50b0a8d

vyzo self-requested a review November 21, 2019 18:03

yusefnapora mentioned this pull request Nov 21, 2019

exchange signed routing records in identify libp2p/go-libp2p#747

Merged

2 tasks

vyzo reviewed Nov 21, 2019

View reviewed changes

pstoremem/addr_book.go Outdated Show resolved Hide resolved

yusefnapora and others added 12 commits January 15, 2020 17:51

adopt CertifiedAddrBook interface

02e9e96

- removes CertifiedAddrs method from addr book implementations - asserts AddrBooks are also CertifiedAddrBooks - ensures we either have certified addrs or uncertified, but not both - updates tests for CertifiedAddrs

test that peerstore is a CertifiedAddrBook

1c176d5

return concrete types from constructor funcs

db7fc65

embed concrete types into peerstore structs

55e6abc

update to reflect name changes in -core

8c8e310

bump -core dep to new PR commit

57a7872

use go 1.13.x on travis

32e09db

add a TODO.

084bc23

pre-allocate addr slice

2e7d7b9

track changes in -core PR

1f4670b

fix slice allocation

7a4e883

Merge master into certified-addrs

d32f2a2

yusefnapora removed the request for review from bigs January 22, 2020 19:53

yusefnapora mentioned this pull request Jan 23, 2020

Signed Peer Records libp2p/go-libp2p#776

Closed

10 tasks

raulk suggested changes Jan 31, 2020

View reviewed changes

yusefnapora and others added 6 commits February 4, 2020 14:52

simplify addr book implementation

1bda3ba

since signed and unsigned addrs are now mutually exclusive, there's no benefit to keeping two lists of addresses.

track API changes from -core

1ef508a

validate that PeerRecord was signed by correct key

681e2e5

fix doc comment

fc8dd0b

go mod tidy.

07a4aea

fix some nits.

ffe5f0c

raulk suggested changes Feb 10, 2020

View reviewed changes

Stebalien reviewed Feb 15, 2020

View reviewed changes

temp: store all addresses in the addrbook.

18b766a

Temporarily store all addresses in the addrbook without classifying their provenance/authoritativeness; even in the presence of a signed peer record.

raulk approved these changes Mar 6, 2020

View reviewed changes

Merge branch 'master' into certified-addrs

88e44fe

raulk merged commit 59abcf5 into master Mar 6, 2020

raulk deleted the certified-addrs branch March 6, 2020 12:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Certified addresses #98

Certified addresses #98

yusefnapora commented Nov 8, 2019

Stebalien Nov 9, 2019

raulk Mar 6, 2020

raulk left a comment

yusefnapora commented Nov 11, 2019

raulk commented Nov 11, 2019

yusefnapora commented Nov 12, 2019

vyzo left a comment

raulk left a comment •

edited

raulk Jan 31, 2020

raulk Jan 31, 2020

raulk Jan 31, 2020

raulk left a comment

raulk Feb 10, 2020

raulk Feb 10, 2020

raulk Feb 10, 2020

raulk Feb 10, 2020

raulk Feb 10, 2020

raulk Feb 10, 2020

raulk Feb 10, 2020

raulk Feb 10, 2020

raulk Feb 10, 2020

Stebalien Feb 15, 2020

Stebalien Feb 15, 2020

raulk Feb 15, 2020

Stebalien Feb 15, 2020

raulk commented Mar 6, 2020

raulk commented Mar 6, 2020

Certified addresses #98

Certified addresses #98

Conversation

yusefnapora commented Nov 8, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raulk left a comment

Choose a reason for hiding this comment

yusefnapora commented Nov 11, 2019

raulk commented Nov 11, 2019

yusefnapora commented Nov 12, 2019

vyzo left a comment

Choose a reason for hiding this comment

raulk left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raulk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raulk commented Mar 6, 2020

raulk commented Mar 6, 2020

raulk left a comment •

edited