From 971511e1c5a6e1cfbd5d526340d4ca18fbda33e7 Mon Sep 17 00:00:00 2001
From: David Wragg <david@wragg.org>
Date: Fri, 24 Jul 2015 13:57:03 +0100
Subject: [PATCH] Introduce PeerShortIDs

These are needed to support vxlan encapsulation.  In a vxlan packet,
the only place we can straightforwardly hold the source and
destination peer identifiers used by the weave router is in the
24-bit VNI (vxlan network identifier) field.  This gives us 12 bits to
indicate a peer. The resulting limit of 4096 peers on a weave network
does not seem like a problem in the near future.  But it does mean
that decentralized allocation makes collisions fairly likely even in
weave networks of sizes that are common today.  Fortunately, the
problem is much more tractable than IPAM because we can detect when
collisions occur and take action to resolve them.
---
 router/connection.go |  11 +-
 router/local_peer.go |  18 ++-
 router/overlay.go    |   6 +
 router/peer.go       |  16 ++-
 router/peers.go      | 239 ++++++++++++++++++++++++++++++++++++++--
 router/peers_test.go | 256 +++++++++++++++++++++++++++++++++++++++++++
 router/router.go     |  19 ++--
 router/sleeve.go     |   4 +
 router/utils.go      |  19 ++--
 9 files changed, 558 insertions(+), 30 deletions(-)

diff --git a/router/connection.go b/router/connection.go
index c5cae87a..99577e57 100644
--- a/router/connection.go
+++ b/router/connection.go
@@ -255,6 +255,7 @@ func (conn *LocalConnection) makeFeatures() map[string]string {
 		"PeerNameFlavour": PeerNameFlavour,
 		"Name":            conn.local.Name.String(),
 		"NickName":        conn.local.NickName,
+		"ShortID":         fmt.Sprint(conn.local.ShortID),
 		"UID":             fmt.Sprint(conn.local.UID),
 		"ConnID":          fmt.Sprint(conn.uid),
 	}
@@ -276,7 +277,7 @@ func (features features) Get(key string) string {
 }
 
 func (conn *LocalConnection) parseFeatures(features features) (*Peer, error) {
-	if err := features.MustHave([]string{"PeerNameFlavour", "Name", "NickName", "UID", "ConnID"}); err != nil {
+	if err := features.MustHave([]string{"PeerNameFlavour", "Name", "NickName", "ShortID", "UID", "ConnID"}); err != nil {
 		return nil, err
 	}
 
@@ -292,6 +293,12 @@ func (conn *LocalConnection) parseFeatures(features features) (*Peer, error) {
 
 	nickName := features.Get("NickName")
 
+	shortID, err := strconv.ParseUint(features.Get("ShortID"), 10,
+		PeerShortIDBits)
+	if err != nil {
+		return nil, err
+	}
+
 	uid, err := ParsePeerUID(features.Get("UID"))
 	if err != nil {
 		return nil, err
@@ -303,7 +310,7 @@ func (conn *LocalConnection) parseFeatures(features features) (*Peer, error) {
 	}
 
 	conn.uid ^= remoteConnID
-	return NewPeer(name, nickName, uid, 0), nil
+	return NewPeer(name, nickName, uid, 0, PeerShortID(shortID)), nil
 }
 
 func (conn *LocalConnection) registerRemote(remote *Peer, acceptNewPeer bool) error {
diff --git a/router/local_peer.go b/router/local_peer.go
index 15076963..1d159221 100644
--- a/router/local_peer.go
+++ b/router/local_peer.go
@@ -19,7 +19,8 @@ type LocalPeerAction func()
 func NewLocalPeer(name PeerName, nickName string, router *Router) *LocalPeer {
 	actionChan := make(chan LocalPeerAction, ChannelSize)
 	peer := &LocalPeer{
-		Peer:       NewPeer(name, nickName, randomPeerUID(), 0),
+		Peer: NewPeer(name, nickName, randomPeerUID(), 0,
+			randomPeerShortID()),
 		router:     router,
 		actionChan: actionChan,
 	}
@@ -205,7 +206,13 @@ func (peer *LocalPeer) handleDeleteConnection(conn Connection) {
 // helpers
 
 func (peer *LocalPeer) broadcastPeerUpdate(peers ...*Peer) {
-	peer.router.BroadcastTopologyUpdate(append(peers, peer.Peer))
+	// Some tests run without a router.  This should be fixed so
+	// that the relevant part of Router can be easily run in the
+	// context of a test, but that will involve significant
+	// reworking of tests.
+	if peer.router != nil {
+		peer.router.BroadcastTopologyUpdate(append(peers, peer.Peer))
+	}
 }
 
 func (peer *LocalPeer) checkConnectionLimit() error {
@@ -241,3 +248,10 @@ func (peer *LocalPeer) connectionCount() int {
 	defer peer.RUnlock()
 	return len(peer.connections)
 }
+
+func (peer *LocalPeer) setShortID(shortID PeerShortID) {
+	peer.Lock()
+	defer peer.Unlock()
+	peer.ShortID = shortID
+	peer.Version++
+}
diff --git a/router/overlay.go b/router/overlay.go
index b84db7d6..b552e179 100644
--- a/router/overlay.go
+++ b/router/overlay.go
@@ -15,6 +15,9 @@ type Overlay interface {
 	// The routes have changed, so any cached information should
 	// be discarded.
 	InvalidateRoutes()
+
+	// A mapping of a short id to a peer has changed
+	InvalidateShortIDs()
 }
 
 type ForwarderParams struct {
@@ -90,6 +93,9 @@ func (NullOverlay) MakeForwarder(ForwarderParams) (OverlayForwarder, error) {
 func (NullOverlay) InvalidateRoutes() {
 }
 
+func (NullOverlay) InvalidateShortIDs() {
+}
+
 func (NullOverlay) SetListener(OverlayForwarderListener) {
 }
 
diff --git a/router/peer.go b/router/peer.go
index 7dfd0857..82ae2201 100644
--- a/router/peer.go
+++ b/router/peer.go
@@ -17,11 +17,24 @@ func ParsePeerUID(s string) (PeerUID, error) {
 	return PeerUID(uid), err
 }
 
+// Short IDs exist for the sake of fast datapath.  They are 12 bits,
+// randomly assigned, but we detect and recover from collisions.  This
+// does limit us to 4096 peers, but that should be sufficient for a
+// while.
+type PeerShortID uint16
+
+const PeerShortIDBits = 12
+
+func randomPeerShortID() PeerShortID {
+	return PeerShortID(randUint16() & (1<<PeerShortIDBits - 1))
+}
+
 type PeerSummary struct {
 	NameByte []byte
 	NickName string
 	UID      PeerUID
 	Version  uint64
+	ShortID  PeerShortID
 }
 
 type Peer struct {
@@ -41,12 +54,13 @@ func NewPeerFromSummary(summary PeerSummary) *Peer {
 	}
 }
 
-func NewPeer(name PeerName, nickName string, uid PeerUID, version uint64) *Peer {
+func NewPeer(name PeerName, nickName string, uid PeerUID, version uint64, shortID PeerShortID) *Peer {
 	return NewPeerFromSummary(PeerSummary{
 		NameByte: name.Bin(),
 		NickName: nickName,
 		UID:      uid,
 		Version:  version,
+		ShortID:  shortID,
 	})
 }
 
diff --git a/router/peers.go b/router/peers.go
index 6287d49d..163f8c48 100644
--- a/router/peers.go
+++ b/router/peers.go
@@ -4,14 +4,29 @@ import (
 	"bytes"
 	"encoding/gob"
 	"io"
+	"math/rand"
 	"sync"
 )
 
 type Peers struct {
 	sync.RWMutex
-	ourself *LocalPeer
-	byName  map[PeerName]*Peer
-	onGC    []func(*Peer)
+	ourself   *LocalPeer
+	byName    map[PeerName]*Peer
+	byShortID map[PeerShortID]ShortIDPeers
+	onGC      []func(*Peer)
+
+	// Called when the mapping from short ids to peers changes
+	onInvalidateShortIDs []func()
+}
+
+type ShortIDPeers struct {
+	// If we know about a single peer with the short id, this is
+	// that peer.  If there is a collision, this is the peer with
+	// the lowest Name.
+	peer *Peer
+
+	// In case of a collision, this holds the other peers.
+	others []*Peer
 }
 
 type UnknownPeerError struct {
@@ -31,15 +46,25 @@ type ConnectionSummary struct {
 	Established   bool
 }
 
-// Pending notifications due to changes to Peers that need to be ent
+// Pending notifications due to changes to Peers that need to be sent
 // out once the Peers is unlocked.
 type PeersPendingNotifications struct {
 	// Peers that have been GCed
 	removed []*Peer
+
+	// The mapping from shorts ids to peers changed
+	invalidateShortIDs bool
+
+	// The local short ID needs reassigning due to a collision
+	reassignLocalShortID bool
 }
 
 func NewPeers(ourself *LocalPeer) *Peers {
-	peers := &Peers{ourself: ourself, byName: make(map[PeerName]*Peer)}
+	peers := &Peers{
+		ourself:   ourself,
+		byName:    make(map[PeerName]*Peer),
+		byShortID: make(map[PeerShortID]ShortIDPeers),
+	}
 	peers.FetchWithDefault(ourself.Peer)
 	return peers
 }
@@ -55,9 +80,20 @@ func (peers *Peers) OnGC(callback func(*Peer)) {
 	peers.onGC = append(peers.onGC, callback)
 }
 
+func (peers *Peers) OnInvalidateShortIDs(callback func()) {
+	peers.Lock()
+	defer peers.Unlock()
+
+	// Safe, as in OnGC
+	peers.onInvalidateShortIDs = append(peers.onInvalidateShortIDs, callback)
+}
+
 func (peers *Peers) unlockAndNotify(pending *PeersPendingNotifications) {
+	broadcastLocalPeer := pending.reassignLocalShortID && peers.reassignLocalShortID(pending)
 	onGC := peers.onGC
+	onInvalidateShortIDs := peers.onInvalidateShortIDs
 	peers.Unlock()
+
 	if pending.removed != nil {
 		for _, callback := range onGC {
 			for _, peer := range pending.removed {
@@ -65,16 +101,168 @@ func (peers *Peers) unlockAndNotify(pending *PeersPendingNotifications) {
 			}
 		}
 	}
+
+	if pending.invalidateShortIDs {
+		for _, callback := range onInvalidateShortIDs {
+			callback()
+		}
+	}
+
+	if broadcastLocalPeer {
+		peers.ourself.broadcastPeerUpdate()
+	}
+}
+
+func (peers *Peers) addByShortID(peer *Peer, pending *PeersPendingNotifications) {
+	entry, ok := peers.byShortID[peer.ShortID]
+	if !ok {
+		entry = ShortIDPeers{peer: peer}
+	} else if entry.peer == nil {
+		// This short ID is free, but was used in the past.
+		// Because we are reusing it, it's an invalidation
+		// event.
+		entry.peer = peer
+		pending.invalidateShortIDs = true
+	} else if peer.Name < entry.peer.Name {
+		// Short ID collision, this peer becomes the principal
+		// peer for the short ID, bumping the previous one
+		// into others.
+
+		if entry.peer == peers.ourself.Peer {
+			// The bumped peer is peers.ourself, so we
+			// need to look for a new short id
+			pending.reassignLocalShortID = true
+		}
+
+		entry.others = append(entry.others, entry.peer)
+		entry.peer = peer
+		pending.invalidateShortIDs = true
+	} else {
+		// Short ID collision, this peer is secondary
+		entry.others = append(entry.others, peer)
+	}
+
+	peers.byShortID[peer.ShortID] = entry
+}
+
+func (peers *Peers) deleteByShortID(peer *Peer, pending *PeersPendingNotifications) {
+	entry := peers.byShortID[peer.ShortID]
+	var otherIndex int
+
+	if peer != entry.peer {
+		// peer is secondary, find its index in others
+		otherIndex = -1
+
+		for i, other := range entry.others {
+			if peer == other {
+				otherIndex = i
+				break
+			}
+		}
+
+		if otherIndex < 0 {
+			return
+		}
+	} else if len(entry.others) != 0 {
+		// need to find the peer with the lowest name to
+		// become the new principal one
+		otherIndex = 0
+		minName := entry.others[0].Name
+
+		for i := 1; i < len(entry.others); i++ {
+			otherName := entry.others[i].Name
+			if otherName < minName {
+				minName = otherName
+				otherIndex = i
+			}
+		}
+
+		entry.peer = entry.others[otherIndex]
+		pending.invalidateShortIDs = true
+	} else {
+		// This is the last peer with the short id.  We clear
+		// the entry, don't delete it, in order to detect when
+		// it gets re-used.
+		peers.byShortID[peer.ShortID] = ShortIDPeers{}
+		return
+	}
+
+	entry.others[otherIndex] = entry.others[len(entry.others)-1]
+	entry.others = entry.others[:len(entry.others)-1]
+	peers.byShortID[peer.ShortID] = entry
+}
+
+func (peers *Peers) reassignLocalShortID(pending *PeersPendingNotifications) bool {
+	newShortID, ok := peers.chooseShortID()
+	if ok {
+		peers.setLocalShortID(newShortID, pending)
+		return true
+	}
+
+	// Otherwise we'll try again later on in garbageColleect
+	return false
+}
+
+func (peers *Peers) setLocalShortID(newShortID PeerShortID, pending *PeersPendingNotifications) {
+	peers.deleteByShortID(peers.ourself.Peer, pending)
+	peers.ourself.setShortID(newShortID)
+	peers.addByShortID(peers.ourself.Peer, pending)
+}
+
+// Choose an available short id at random
+func (peers *Peers) chooseShortID() (PeerShortID, bool) {
+	rng := rand.New(rand.NewSource(int64(randUint64())))
+
+	// First, just try picking some short ids at random, and
+	// seeing if they are available:
+	for i := 0; i < 10; i++ {
+		shortID := PeerShortID(rng.Intn(1 << PeerShortIDBits))
+		if peers.byShortID[shortID].peer == nil {
+			return shortID, true
+		}
+	}
+
+	// Looks like most short ids are used.  So count the number of
+	// unused ones, and pick one at random.
+	available := int(1 << PeerShortIDBits)
+	for _, entry := range peers.byShortID {
+		if entry.peer != nil {
+			available--
+		}
+	}
+
+	if available == 0 {
+		// All short ids are used.
+		return 0, false
+	}
+
+	n := rng.Intn(available)
+	var i PeerShortID
+	for {
+		if peers.byShortID[i].peer == nil {
+			if n == 0 {
+				return PeerShortID(i), true
+			}
+
+			n--
+		}
+
+		i++
+	}
 }
 
 func (peers *Peers) FetchWithDefault(peer *Peer) *Peer {
 	peers.Lock()
-	defer peers.Unlock()
+	var pending PeersPendingNotifications
+	defer peers.unlockAndNotify(&pending)
+
 	if existingPeer, found := peers.byName[peer.Name]; found {
 		existingPeer.localRefCount++
 		return existingPeer
 	}
+
 	peers.byName[peer.Name] = peer
+	peers.addByShortID(peer, &pending)
 	peer.localRefCount++
 	return peer
 }
@@ -95,6 +283,12 @@ func (peers *Peers) FetchAndAddRef(name PeerName) *Peer {
 	return peer
 }
 
+func (peers *Peers) FetchByShortID(shortID PeerShortID) *Peer {
+	peers.RLock()
+	defer peers.RUnlock()
+	return peers.byShortID[shortID].peer
+}
+
 func (peers *Peers) Dereference(peer *Peer) {
 	peers.Lock()
 	defer peers.Unlock()
@@ -125,15 +319,18 @@ func (peers *Peers) ApplyUpdate(update []byte) (PeerNameSet, PeerNameSet, error)
 		return nil, nil, err
 	}
 
+	ourVersion := peers.ourself.Version
+
 	// By this point, we know the update doesn't refer to any peers we
 	// have no knowledge of. We can now apply the update. Start by
-	// adding in any new peers into the cache.
+	// adding in any new peers.
 	for name, newPeer := range newPeers {
 		peers.byName[name] = newPeer
+		peers.addByShortID(newPeer, &pending)
 	}
 
 	// Now apply the updates
-	newUpdate := peers.applyUpdate(decodedUpdate, decodedConns)
+	newUpdate := peers.applyUpdate(decodedUpdate, decodedConns, &pending)
 	peers.garbageCollect(&pending)
 	for _, peerRemoved := range pending.removed {
 		delete(newUpdate, peerRemoved.Name)
@@ -144,6 +341,13 @@ func (peers *Peers) ApplyUpdate(update []byte) (PeerNameSet, PeerNameSet, error)
 		updateNames[peer.Name] = void
 	}
 
+	if peers.ourself.Version != ourVersion {
+		// Our short id changed, i.e. due to a short ID
+		// change.  So we need to include ourself in the
+		// update
+		newUpdate[peers.ourself.Name] = void
+	}
+
 	return updateNames, newUpdate, nil
 }
 
@@ -187,12 +391,21 @@ func (peers *Peers) garbageCollect(pending *PeersPendingNotifications) {
 	peers.ourself.RLock()
 	_, reached := peers.ourself.Routes(nil, false)
 	peers.ourself.RUnlock()
+
 	for name, peer := range peers.byName {
 		if _, found := reached[peer.Name]; !found && peer.localRefCount == 0 {
 			delete(peers.byName, name)
+			peers.deleteByShortID(peer, pending)
 			pending.removed = append(pending.removed, peer)
 		}
 	}
+
+	if peers.byShortID[peers.ourself.ShortID].peer != peers.ourself.Peer {
+		// The local peer doesn't own its short id.  Garbage
+		// collection might have freed some up, so try to
+		// reassign.
+		pending.reassignLocalShortID = true
+	}
 }
 
 func (peers *Peers) decodeUpdate(update []byte) (newPeers map[PeerName]*Peer, decodedUpdate []*Peer, decodedConns [][]ConnectionSummary, err error) {
@@ -240,7 +453,7 @@ func (peers *Peers) decodeUpdate(update []byte) (newPeers map[PeerName]*Peer, de
 	return
 }
 
-func (peers *Peers) applyUpdate(decodedUpdate []*Peer, decodedConns [][]ConnectionSummary) PeerNameSet {
+func (peers *Peers) applyUpdate(decodedUpdate []*Peer, decodedConns [][]ConnectionSummary, pending *PeersPendingNotifications) PeerNameSet {
 	newUpdate := make(PeerNameSet)
 	for idx, newPeer := range decodedUpdate {
 		connSummaries := decodedConns[idx]
@@ -266,8 +479,16 @@ func (peers *Peers) applyUpdate(decodedUpdate []*Peer, decodedConns [][]Connecti
 		// the router.Peers, so there can be no race here.
 		peer.Version = newPeer.Version
 		peer.connections = makeConnsMap(peer, connSummaries, peers.byName)
+
+		if newPeer.ShortID != peer.ShortID {
+			peers.deleteByShortID(peer, pending)
+			peer.ShortID = newPeer.ShortID
+			peers.addByShortID(peer, pending)
+		}
+
 		newUpdate[name] = void
 	}
+
 	return newUpdate
 }
 
diff --git a/router/peers_test.go b/router/peers_test.go
index d24aabb5..e5f328f9 100644
--- a/router/peers_test.go
+++ b/router/peers_test.go
@@ -4,6 +4,7 @@ import (
 	"fmt"
 	"math/rand"
 	"testing"
+	"time"
 
 	"github.com/stretchr/testify/require"
 )
@@ -118,3 +119,258 @@ func TestPeersGarbageCollection(t *testing.T) {
 	ps1.DeleteTestConnection(p3)
 	checkPeerArray(t, garbageCollect(ps1), p3)
 }
+
+func TestShortIDCollisions(t *testing.T) {
+	rng := rand.New(rand.NewSource(time.Now().UnixNano()))
+	_, peers := newNode(PeerName(1 << PeerShortIDBits))
+
+	// Make enough peers that short id collisions are
+	// overwhelmingly likely
+	ps := make([]*Peer, 1<<PeerShortIDBits)
+	for i := 0; i < 1<<PeerShortIDBits; i++ {
+		ps[i] = NewPeer(PeerName(i), "", PeerUID(i), 0,
+			PeerShortID(rng.Intn(1<<PeerShortIDBits)))
+	}
+
+	shuffle := func() {
+		for i := range ps {
+			j := rng.Intn(i + 1)
+			ps[i], ps[j] = ps[j], ps[i]
+		}
+	}
+
+	// Fill peers
+	shuffle()
+	var pending PeersPendingNotifications
+	for _, p := range ps {
+		peers.addByShortID(p, &pending)
+	}
+
+	// Check invariants
+	counts := make([]int, 1<<PeerShortIDBits)
+	saw := func(p *Peer) {
+		if p != peers.ourself.Peer {
+			counts[p.UID]++
+		}
+	}
+
+	for shortID, entry := range peers.byShortID {
+		if entry.peer == nil {
+			// no principal peer for this short id, so
+			// others must be empty
+			require.Empty(t, entry.others)
+			continue
+		}
+
+		require.Equal(t, shortID, entry.peer.ShortID)
+		saw(entry.peer)
+
+		for _, p := range entry.others {
+			saw(p)
+			require.Equal(t, shortID, p.ShortID)
+
+			// the principal peer should have the lowest name
+			require.True(t, p.Name > entry.peer.Name)
+		}
+	}
+
+	// Check that every peer was seen
+	for _, n := range counts {
+		require.Equal(t, 1, n)
+	}
+
+	// Delete all the peers
+	shuffle()
+	for _, p := range ps {
+		peers.deleteByShortID(p, &pending)
+	}
+
+	for _, entry := range peers.byShortID {
+		if entry.peer != peers.ourself.Peer {
+			require.Nil(t, entry.peer)
+		}
+
+		require.Empty(t, entry.others)
+	}
+}
+
+// Test the easy case of short id reassignment, when few short ids are taken
+func TestShortIDReassignmentEasy(t *testing.T) {
+	rng := rand.New(rand.NewSource(time.Now().UnixNano()))
+	_, peers := newNode(PeerName(0))
+
+	for i := 1; i <= 10; i++ {
+		peers.FetchWithDefault(NewPeer(PeerName(i), "", PeerUID(i), 0,
+			PeerShortID(rng.Intn(1<<PeerShortIDBits))))
+	}
+
+	checkShortIDReassignment(t, peers)
+}
+
+// Test the hard case of short id reassignment, when most short ids are taken
+func TestShortIDReassignmentHard(t *testing.T) {
+	rng := rand.New(rand.NewSource(time.Now().UnixNano()))
+	_, peers := newNode(PeerName(1 << PeerShortIDBits))
+
+	// Take all short ids
+	ps := make([]*Peer, 1<<PeerShortIDBits)
+	var pending PeersPendingNotifications
+	for i := 0; i < 1<<PeerShortIDBits; i++ {
+		ps[i] = NewPeer(PeerName(i), "", PeerUID(i), 0,
+			PeerShortID(i))
+		peers.addByShortID(ps[i], &pending)
+	}
+
+	// As all short ids are taken, an attempted reassigment won't
+	// do anything
+	oldShortID := peers.ourself.ShortID
+	require.False(t, peers.reassignLocalShortID(&pending))
+	require.Equal(t, oldShortID, peers.ourself.ShortID)
+
+	// Free up a few ids
+	for i := 0; i < 10; i++ {
+		x := rng.Intn(len(ps))
+		if ps[x] != nil {
+			peers.deleteByShortID(ps[x], &pending)
+			ps[x] = nil
+		}
+	}
+
+	checkShortIDReassignment(t, peers)
+}
+
+func checkShortIDReassignment(t *testing.T, peers *Peers) {
+	oldShortID := peers.ourself.ShortID
+	peers.reassignLocalShortID(&PeersPendingNotifications{})
+	require.NotEqual(t, oldShortID, peers.ourself.ShortID)
+	require.Equal(t, peers.ourself.Peer, peers.byShortID[peers.ourself.ShortID].peer)
+}
+
+func TestShortIDInvalidation(t *testing.T) {
+	_, peers := newNode(PeerName(1 << PeerShortIDBits))
+
+	// need to use a short id that is not the local peer's
+	shortID := peers.ourself.ShortID + 1
+
+	var pending PeersPendingNotifications
+
+	requireInvalidateShortIDs := func(expect bool) {
+		require.Equal(t, expect, pending.invalidateShortIDs)
+		pending.invalidateShortIDs = false
+	}
+
+	// The use of a fresh short id does not cause invalidation
+	a := NewPeer(PeerName(1), "", PeerUID(1), 0, shortID)
+	peers.addByShortID(a, &pending)
+	requireInvalidateShortIDs(false)
+
+	// An addition which does not change the mapping
+	b := NewPeer(PeerName(2), "", PeerUID(2), 0, shortID)
+	peers.addByShortID(b, &pending)
+	requireInvalidateShortIDs(false)
+
+	// An addition which does change the mapping
+	c := NewPeer(PeerName(0), "", PeerUID(0), 0, shortID)
+	peers.addByShortID(c, &pending)
+	requireInvalidateShortIDs(true)
+
+	// A deletion which does not change the mapping
+	peers.deleteByShortID(b, &pending)
+	requireInvalidateShortIDs(false)
+
+	// A deletion which does change the mapping
+	peers.deleteByShortID(c, &pending)
+	requireInvalidateShortIDs(true)
+
+	// Deleting the last peer with a short id does not cause invalidation
+	peers.deleteByShortID(a, &pending)
+	requireInvalidateShortIDs(false)
+
+	// .. but subsequent reuse of that short id does cause invalidation
+	peers.addByShortID(a, &pending)
+	requireInvalidateShortIDs(true)
+}
+
+func TestShortIDPropagation(t *testing.T) {
+	_, peers1 := newNode(PeerName(1))
+	_, peers2 := newNode(PeerName(2))
+
+	peers1.AddTestConnection(peers2.ourself.Peer)
+	peers1.ApplyUpdate(peers2.EncodePeers(peers2.Names()))
+	peers12 := peers1.Fetch(PeerName(2))
+	old := peers12.PeerSummary
+
+	require.True(t,
+		peers2.reassignLocalShortID(&PeersPendingNotifications{}))
+	peers1.ApplyUpdate(peers2.EncodePeers(peers2.Names()))
+	require.NotEqual(t, old.Version, peers12.Version)
+	require.NotEqual(t, old.ShortID, peers12.ShortID)
+}
+
+func TestShortIDCollision(t *testing.T) {
+	// Create 3 peers
+	_, peers1 := newNode(PeerName(1))
+	_, peers2 := newNode(PeerName(2))
+	_, peers3 := newNode(PeerName(3))
+
+	var pending PeersPendingNotifications
+	peers1.setLocalShortID(1, &pending)
+	peers2.setLocalShortID(2, &pending)
+	peers3.setLocalShortID(3, &pending)
+
+	peers2.AddTestConnection(peers1.ourself.Peer)
+	peers3.AddTestConnection(peers2.ourself.Peer)
+
+	// Propogate from 1 to 2 to 3
+	peers2.ApplyUpdate(peers1.EncodePeers(peers1.Names()))
+	peers3.ApplyUpdate(peers2.EncodePeers(peers2.Names()))
+
+	// Force the short id of peer 1 to collide with peer 2.  Peer
+	// 1 has the lowest name, so it gets to keep the short id
+	peers1.setLocalShortID(2, &pending)
+
+	oldShortID := peers2.ourself.ShortID
+	_, updated, _ := peers2.ApplyUpdate(peers1.EncodePeers(peers1.Names()))
+
+	// peer 2 should have noticed the collision and resolved it
+	require.NotEqual(t, oldShortID, peers2.ourself.ShortID)
+
+	// The Peers do not have a Router, so broadcastPeerUpdate does
+	// nothing in the context of this test.  So we fake what it
+	// would do.
+	updated[PeerName(2)] = void
+
+	// the update from peer 2 should include its short id change
+	peers3.ApplyUpdate(peers2.EncodePeers(updated))
+	require.Equal(t, peers2.ourself.ShortID,
+		peers3.Fetch(PeerName(2)).ShortID)
+}
+
+// Test the case where all short ids are taken, but then some peers go
+// away, so the local peer reassigns
+func TestDeferredShortIDReassignment(t *testing.T) {
+	rng := rand.New(rand.NewSource(time.Now().UnixNano()))
+	_, us := newNode(PeerName(1 << PeerShortIDBits))
+
+	// Connect us to other peers occupying all short ids
+	others := make([]*Peers, 1<<PeerShortIDBits)
+	var pending PeersPendingNotifications
+	for i := range others {
+		_, others[i] = newNode(PeerName(i))
+		others[i].setLocalShortID(PeerShortID(i), &pending)
+		us.AddTestConnection(others[i].ourself.Peer)
+	}
+
+	// Check that, as expected, the local peer does not own its
+	// short id
+	require.NotEqual(t, us.ourself.Peer,
+		us.byShortID[us.ourself.ShortID].peer)
+
+	// Disconnect one peer, and we should now be able to claim its
+	// short id
+	other := others[rng.Intn(1<<PeerShortIDBits)]
+	us.DeleteTestConnection(other.ourself.Peer)
+	us.GarbageCollect()
+
+	require.Equal(t, us.ourself.Peer, us.byShortID[us.ourself.ShortID].peer)
+}
diff --git a/router/router.go b/router/router.go
index 93dc923f..6c877c57 100644
--- a/router/router.go
+++ b/router/router.go
@@ -62,17 +62,18 @@ func NewRouter(config Config, name PeerName, nickName string) *Router {
 		router.Overlay = NullOverlay{}
 	}
 
-	onMacExpiry := func(mac net.HardwareAddr, peer *Peer) {
-		log.Println("Expired MAC", mac, "at", peer)
-	}
-	onPeerGC := func(peer *Peer) {
+	router.Ourself = NewLocalPeer(name, nickName, router)
+	router.Macs = NewMacCache(macMaxAge,
+		func(mac net.HardwareAddr, peer *Peer) {
+			log.Println("Expired MAC", mac, "at", peer)
+		})
+	router.Peers = NewPeers(router.Ourself)
+	router.Peers.OnGC(func(peer *Peer) {
 		router.Macs.Delete(peer)
 		log.Println("Removed unreachable peer", peer)
-	}
-	router.Ourself = NewLocalPeer(name, nickName, router)
-	router.Macs = NewMacCache(macMaxAge, onMacExpiry)
-	router.Peers = NewPeers(router.Ourself)
-	router.Peers.OnGC(onPeerGC)
+	})
+	router.Peers.OnInvalidateShortIDs(router.Overlay.InvalidateShortIDs)
+
 	router.Routes = NewRoutes(router.Ourself, router.Peers, router.Overlay.InvalidateRoutes)
 	router.ConnectionMaker = NewConnectionMaker(router.Ourself, router.Peers, router.Port, router.PeerDiscovery)
 	router.TopologyGossip = router.NewGossip("topology", router)
diff --git a/router/sleeve.go b/router/sleeve.go
index 4dd110b0..957ab9dd 100644
--- a/router/sleeve.go
+++ b/router/sleeve.go
@@ -129,6 +129,10 @@ func (*SleeveOverlay) InvalidateRoutes() {
 	// no cached information, so nothing to do
 }
 
+func (*SleeveOverlay) InvalidateShortIDs() {
+	// no cached information, so nothing to do
+}
+
 func (sleeve *SleeveOverlay) lookupForwarder(peer PeerName) *sleeveForwarder {
 	sleeve.lock.Lock()
 	defer sleeve.lock.Unlock()
diff --git a/router/utils.go b/router/utils.go
index 24ea5d76..3530abd2 100644
--- a/router/utils.go
+++ b/router/utils.go
@@ -3,6 +3,7 @@ package router
 import (
 	"bytes"
 	"crypto/rand"
+	"encoding/binary"
 	"encoding/gob"
 	"fmt"
 	"net"
@@ -62,15 +63,19 @@ func Concat(elems ...[]byte) []byte {
 	return res
 }
 
-func randUint64() (r uint64) {
-	buf := make([]byte, 8)
+func randBytes(n int) []byte {
+	buf := make([]byte, n)
 	_, err := rand.Read(buf)
 	checkFatal(err)
-	for _, v := range buf {
-		r <<= 8
-		r |= uint64(v)
-	}
-	return
+	return buf
+}
+
+func randUint64() (r uint64) {
+	return binary.LittleEndian.Uint64(randBytes(8))
+}
+
+func randUint16() (r uint16) {
+	return binary.LittleEndian.Uint16(randBytes(2))
 }
 
 func GobEncode(items ...interface{}) []byte {