Merge pull request #155 from libp2p/fix/peer-record-bugs

fix two bugs in signed address handling

pstoreds/addr_book.go

@@ -278,8 +278,9 @@ func (ab *dsAddrBook) ConsumePeerRecord(recordEnvelope *record.Envelope, ttl tim
 		return false, fmt.Errorf("signing key does not match PeerID in PeerRecord")
 	}
 
-	// ensure that the seq number from envelope is > any previously received seq no
-	if ab.latestPeerRecordSeq(rec.PeerID) >= rec.Seq {
+	// ensure that the seq number from envelope is >= any previously received seq no
+	// update when equal to extend the ttls
+	if ab.latestPeerRecordSeq(rec.PeerID) > rec.Seq {
 		return false, nil
 	}
 

pstoremem/addr_book.go

@@ -159,7 +159,7 @@ func (mab *memoryAddrBook) AddAddrs(p peer.ID, addrs []ma.Multiaddr, ttl time.Du
 	// if peerRec != nil {
 	// 	return
 	// }
-	mab.addAddrs(p, addrs, ttl, false)
+	mab.addAddrs(p, addrs, ttl)
 }
 
 // ConsumePeerRecord adds addresses from a signed peer.PeerRecord (contained in
@@ -178,38 +178,41 @@ func (mab *memoryAddrBook) ConsumePeerRecord(recordEnvelope *record.Envelope, tt
 		return false, fmt.Errorf("signing key does not match PeerID in PeerRecord")
 	}
 
-	// ensure seq is greater than last received
+	// ensure seq is greater than, or equal to, the last received
 	s := mab.segments.get(rec.PeerID)
 	s.Lock()
+	defer s.Unlock()
 	lastState, found := s.signedPeerRecords[rec.PeerID]
-	if found && lastState.Seq >= rec.Seq {
-		s.Unlock()
+	if found && lastState.Seq > rec.Seq {
 		return false, nil
 	}
 	s.signedPeerRecords[rec.PeerID] = &peerRecordState{
 		Envelope: recordEnvelope,
 		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)
+	mab.addAddrsUnlocked(s, rec.PeerID, rec.Addrs, ttl, true)
 	return true, nil
 }
 
-func (mab *memoryAddrBook) addAddrs(p peer.ID, addrs []ma.Multiaddr, ttl time.Duration, signed bool) {
+func (mab *memoryAddrBook) addAddrs(p peer.ID, addrs []ma.Multiaddr, ttl time.Duration) {
 	if err := p.Validate(); err != nil {
 		log.Warningf("tried to set addrs for invalid peer ID %s: %s", p, err)
 		return
 	}
 
+	s := mab.segments.get(p)
+	s.Lock()
+	defer s.Unlock()
+
+	mab.addAddrsUnlocked(s, p, addrs, ttl, false)
+}
+
+func (mab *memoryAddrBook) addAddrsUnlocked(s *addrSegment, p peer.ID, addrs []ma.Multiaddr, ttl time.Duration, signed bool) {
 	// if ttl is zero, exit. nothing to do.
 	if ttl <= 0 {
 		return
 	}
 
-	s := mab.segments.get(p)
-	s.Lock()
-	defer s.Unlock()
-
 	amap, ok := s.addrs[p]
 	if !ok {
 		amap = make(map[string]*expiringAddr)

test/addr_book_suite.go

@@ -375,10 +375,18 @@ func testCertifiedAddresses(m pstore.AddrBook) func(*testing.T) {
 		allAddrs := GenerateAddrs(10)
 		certifiedAddrs := allAddrs[:5]
 		uncertifiedAddrs := allAddrs[5:]
-		rec := peer.NewPeerRecord()
-		rec.PeerID = id
-		rec.Addrs = certifiedAddrs
-		signedRec, err := record.Seal(rec, priv)
+		rec1 := peer.NewPeerRecord()
+		rec1.PeerID = id
+		rec1.Addrs = certifiedAddrs
+		signedRec1, err := record.Seal(rec1, priv)
+		if err != nil {
+			t.Errorf("error creating signed routing record: %v", err)
+		}
+
+		rec2 := peer.NewPeerRecord()
+		rec2.PeerID = id
+		rec2.Addrs = certifiedAddrs
+		signedRec2, err := record.Seal(rec2, priv)
 		if err != nil {
 			t.Errorf("error creating signed routing record: %v", err)
 		}
@@ -390,10 +398,13 @@ func testCertifiedAddresses(m pstore.AddrBook) func(*testing.T) {
 		AssertAddressesEqual(t, uncertifiedAddrs, m.Addrs(id))
 
 		// add the signed record to addr book
-		_, err = cab.ConsumePeerRecord(signedRec, time.Hour)
+		accepted, err := cab.ConsumePeerRecord(signedRec2, time.Hour)
 		if err != nil {
 			t.Errorf("error adding signed routing record to addrbook: %v", err)
 		}
+		if !accepted {
+			t.Errorf("should have accepted signed peer record")
+		}
 
 		// the non-certified addrs should be gone & we should get only certified addrs back from Addrs
 		// AssertAddressesEqual(t, certifiedAddrs, m.Addrs(id))
@@ -404,36 +415,37 @@ func testCertifiedAddresses(m pstore.AddrBook) func(*testing.T) {
 			t.Errorf("expected PeersWithAddrs to return 1, got %d", len(m.PeersWithAddrs()))
 		}
 
-		// adding the same peer record again should result in the record being ignored
-		accepted, err := cab.ConsumePeerRecord(signedRec, time.Hour)
+		// Adding an old record should fail
+		accepted, err = cab.ConsumePeerRecord(signedRec1, time.Hour)
 		if accepted {
-			t.Error("Expected record with duplicate sequence number to be ignored")
+			t.Error("We should have failed to accept a record with an old sequence number")
 		}
 		if err != nil {
-			t.Errorf("Expected record with duplicate sequence number to be ignored without error, got err: %s", err)
+			t.Errorf("expected no error, got: %s", err)
 		}
 
 		// once certified addrs exist, trying to add non-certified addrs should have no effect
 		// m.AddAddrs(id, uncertifiedAddrs, time.Hour)
 		// AssertAddressesEqual(t, certifiedAddrs, m.Addrs(id))
+		// XXX: Disabled until signed records are required
 		m.AddAddrs(id, uncertifiedAddrs, time.Hour)
 		AssertAddressesEqual(t, allAddrs, m.Addrs(id))
 
 		// we should be able to retrieve the signed peer record
-		rec2 := cab.GetPeerRecord(id)
-		if rec2 == nil || !signedRec.Equal(rec2) {
+		rec3 := cab.GetPeerRecord(id)
+		if rec3 == nil || !signedRec2.Equal(rec3) {
 			t.Error("unable to retrieve signed routing record from addrbook")
 		}
 
 		// Adding a new envelope should clear existing certified addresses.
 		// Only the newly-added ones should remain
 		certifiedAddrs = certifiedAddrs[:3]
-		rec = peer.NewPeerRecord()
-		rec.PeerID = id
-		rec.Addrs = certifiedAddrs
-		signedRec, err = record.Seal(rec, priv)
+		rec4 := peer.NewPeerRecord()
+		rec4.PeerID = id
+		rec4.Addrs = certifiedAddrs
+		signedRec4, err := record.Seal(rec4, priv)
 		test.AssertNilError(t, err)
-		_, err = cab.ConsumePeerRecord(signedRec, time.Hour)
+		_, err = cab.ConsumePeerRecord(signedRec4, time.Hour)
 		test.AssertNilError(t, err)
 		// AssertAddressesEqual(t, certifiedAddrs, m.Addrs(id))
 		AssertAddressesEqual(t, allAddrs, m.Addrs(id))
@@ -441,6 +453,7 @@ func testCertifiedAddresses(m pstore.AddrBook) func(*testing.T) {
 		// update TTL on signed addrs to -1 to remove them.
 		// the signed routing record should be deleted
 		// m.SetAddrs(id, certifiedAddrs, -1)
+		// XXX: Disabled until signed records are required
 		m.SetAddrs(id, allAddrs, -1)
 		if len(m.Addrs(id)) != 0 {
 			t.Error("expected zero certified addrs after setting TTL to -1")
@@ -450,7 +463,7 @@ func testCertifiedAddresses(m pstore.AddrBook) func(*testing.T) {
 		}
 
 		// Test that natural TTL expiration clears signed peer records
-		_, err = cab.ConsumePeerRecord(signedRec, time.Second)
+		_, err = cab.ConsumePeerRecord(signedRec4, time.Second)
 		test.AssertNilError(t, err)
 		AssertAddressesEqual(t, certifiedAddrs, m.Addrs(id))
 
@@ -462,7 +475,7 @@ func testCertifiedAddresses(m pstore.AddrBook) func(*testing.T) {
 		// adding a peer record that's signed with the wrong key should fail
 		priv2, _, err := test.RandTestKeyPair(crypto.Ed25519, 256)
 		test.AssertNilError(t, err)
-		env, err := record.Seal(rec, priv2)
+		env, err := record.Seal(rec4, priv2)
 		test.AssertNilError(t, err)
 
 		accepted, err = cab.ConsumePeerRecord(env, time.Second)