update comments, mostly TODO/FIXME notes

src/yggdrasil/address.go

@@ -44,7 +44,7 @@ func address_addrForNodeID(nid *NodeID) *address {
 		}
 		if !done && bit == 0 {
 			done = true
-			continue // FIXME this assumes that ones <= 127
+			continue // FIXME? this assumes that ones <= 127, probably only worth changing by using a variable length uint64, but that would require changes to the addressing scheme, and I'm not sure ones > 127 is realistic
 		}
 		bits = (bits << 1) | bit
 		nBits++

src/yggdrasil/admin.go

@@ -6,9 +6,8 @@ import "bytes"
 import "fmt"
 import "sort"
 
-// TODO: Make all of this JSON
+// TODO? Make all of this JSON
 // TODO: Add authentication
-// TODO: Is any of this thread safe?
 
 type admin struct {
 	core       *Core
@@ -138,7 +137,7 @@ func (a *admin) handleRequest(conn net.Conn) {
 				if isIn {
 					continue
 				}
-				newInfo.name = "missing"
+				newInfo.name = "?"
 				newInfo.key = key
 				infos[key] = newInfo
 			}

src/yggdrasil/core.go

@@ -6,9 +6,6 @@ import "regexp"
 
 type Core struct {
 	// This is the main data structure that holds everything else for a node
-	// TODO? move keys out of core and into something more appropriate
-	//  e.g. box keys live in sessions
-	//  sig keys live in peers or sigs (or wherever signing/validating logic is)
 	boxPub      boxPubKey
 	boxPriv     boxPrivKey
 	sigPub      sigPubKey
@@ -42,7 +39,7 @@ func (c *Core) init(bpub *boxPubKey,
 	// TODO separate init and start functions
 	//  Init sets up structs
 	//  Start launches goroutines that depend on structs being set up
-	// This is pretty much required to avoid race conditions
+	// This is pretty much required to completely avoid race conditions
 	util_initByteStore()
 	c.log = log.New(ioutil.Discard, "", 0)
 	c.boxPub, c.boxPriv = *bpub, *bpriv

src/yggdrasil/dht.go

@@ -18,10 +18,6 @@ Slight changes *do* make it blackhole hard, bootstrapping isn't an easy problem
 
 */
 
-// TODO handle the case where we try to look ourself up
-//  Ends up at bucket index NodeIDLen
-//  That's 1 too many
-
 import "sort"
 import "time"
 
@@ -34,7 +30,6 @@ const dht_lookup_size = 2               // This should be at least 1, below 2 is
 const dht_bucket_number = 8 * NodeIDLen // This shouldn't be changed
 
 type dhtInfo struct {
-	// TODO save their nodeID so we don't need to rehash if we need it again
 	nodeID_hidden *NodeID
 	key           boxPubKey
 	coords        []byte
@@ -115,7 +110,7 @@ func (t *dht) handleRes(res *dhtRes) {
 	rinfo := dhtInfo{
 		key:    res.key,
 		coords: res.coords,
-		send:   time.Now(), // Technically wrong but should be OK... FIXME or not
+		send:   time.Now(), // Technically wrong but should be OK...
 		recv:   time.Now(),
 	}
 	// If they're already in the table, then keep the correct send time

src/yggdrasil/ndp.go

@@ -6,7 +6,7 @@ package yggdrasil
 
 import "golang.org/x/net/icmp"
 import "encoding/binary"
-import "unsafe"
+import "unsafe" // TODO investigate if this can be done without resorting to unsafe
 
 type macAddress [6]byte
 type ipv6Address [16]byte

src/yggdrasil/peer.go

@@ -1,11 +1,26 @@
 package yggdrasil
 
 // TODO cleanup, this file is kind of a mess
+//  Commented code should be removed
+//  Live code should be better commented
 
-// FIXME? this part may be at least sligtly vulnerable to replay attacks
+// FIXME (!) this part may be at least sligtly vulnerable to replay attacks
 //  The switch message part should catch / drop old tstamps
 //  So the damage is limited
 //  But you could still mess up msgAnc / msgHops and break some things there
+//  It needs to ignore messages with a lower seq
+//  Probably best to start setting seq to a timestamp in that case...
+
+// FIXME (!?) if it takes too long to communicate all the msgHops, then things hit a horizon
+//  That could happen with a peer over a high-latency link, with many msgHops
+//  Possible workarounds:
+//    1. Pre-emptively send all hops when one is requested, or after any change
+//      Maybe requires changing how the throttle works and msgHops are saved
+//      In case some arrive out of order or are dropped
+//      This is relatively easy to implement, but could be wasteful
+//    2. Save your old locator, sigs, etc, so you can respond to older ancs
+//      And finish requesting an old anc before updating to a new one
+//      But that may lead to other issues if not done carefully...
 
 import "time"
 import "sync"
@@ -166,7 +181,7 @@ func (p *peer) handleTraffic(packet []byte, pTypeLen int) {
 	toPort, newTTL := p.core.switchTable.lookup(coords, ttl)
 	if toPort == p.port {
 		return
-	} // FIXME? shouldn't happen, does it? would loop
+	}
 	to := p.core.peers.getPorts()[toPort]
 	if to == nil {
 		return
@@ -200,7 +215,6 @@ func (p *peer) sendLinkPacket(packet []byte) {
 
 func (p *peer) handleLinkTraffic(bs []byte) {
 	packet := wire_linkProtoTrafficPacket{}
-	// TODO throttle on returns?
 	if !packet.decode(bs) {
 		return
 	}

src/yggdrasil/router.go

@@ -2,7 +2,7 @@ package yggdrasil
 
 // This part does most of the work to handle packets to/from yourself
 // It also manages crypto and dht info
-// TODO? move dht stuff into another goroutine?
+// TODO clean up old/unused code, maybe improve comments on whatever is left
 
 // Send:
 //  Receive a packet from the tun
@@ -43,14 +43,19 @@ func (r *router) init(core *Core) {
 	r.addr = *address_addrForNodeID(&r.core.dht.nodeID)
 	in := make(chan []byte, 1024)                             // TODO something better than this...
 	p := r.core.peers.newPeer(&r.core.boxPub, &r.core.sigPub) //, out, in)
-	// TODO set in/out functions on the new peer...
+	p.out = func(packet []byte) {
-	p.out = func(packet []byte) { in <- packet } // FIXME in theory it blocks...
+		// This is to make very sure it never blocks
+		for {
+			select {
+			case in <- packet:
+				return
+			default:
+				util_putBytes(<-in)
+			}
+		}
+	}
 	r.in = in
-	// TODO? make caller responsible for go-ing if it needs to not block
+	r.out = func(packet []byte) { p.handlePacket(packet, nil) } // The caller is responsible for go-ing if it needs to not block
-	r.out = func(packet []byte) { p.handlePacket(packet, nil) }
-	// TODO attach these to the tun
-	//  Maybe that's the core's job...
-	//  It creates tun, creates the router, creates channels, sets them?
 	recv := make(chan []byte, 1024)
 	send := make(chan []byte, 1024)
 	r.recv = recv
@@ -154,7 +159,7 @@ func (r *router) sendPacket(bs []byte) {
 }
 
 func (r *router) recvPacket(bs []byte, theirAddr *address, theirSubnet *subnet) {
-	// TODO? move this into the session?
+	// Note: called directly by the session worker, not the router goroutine
 	//fmt.Println("Recv packet")
 	if len(bs) < 24 {
 		util_putBytes(bs)

src/yggdrasil/search.go

@@ -169,7 +169,7 @@ func (s *searches) handleSearchRes(res *searchRes) {
 			panic("This should never happen")
 		}
 	}
-	// FIXME replay attacks could mess with coords?
+	// FIXME (!) replay attacks could mess with coords? Give it a handle (tstamp)?
 	sinfo.coords = res.coords
 	sinfo.packet = info.packet
 	s.core.sessions.ping(sinfo)

src/yggdrasil/session.go

@@ -29,7 +29,6 @@ type sessionInfo struct {
 	tstamp       int64 // tstamp from their last session ping, replay attack mitigation
 }
 
-// FIXME replay attacks (include nonce or some sequence number)
 type sessionPing struct {
 	sendPermPub boxPubKey // Sender's permanent key
 	handle      handle    // Random number to ID session
@@ -42,15 +41,15 @@ type sessionPing struct {
 // Returns true if the session was updated, false otherwise
 func (s *sessionInfo) update(p *sessionPing) bool {
 	if !(p.tstamp > s.tstamp) {
+		// To protect against replay attacks
 		return false
 	}
 	if p.sendPermPub != s.theirPermPub {
+		// Should only happen if two sessions got the same handle
+		// That shouldn't be allowed anyway, but if it happens then let one time out
 		return false
-	} // Shouldn't happen
+	}
 	if p.sendSesPub != s.theirSesPub {
-		// FIXME need to protect against replay attacks
-		//  Put a sequence number or a timestamp or something in the pings?
-		// Or just return false, make the session time out?
 		s.theirSesPub = p.sendSesPub
 		s.theirHandle = p.handle
 		s.sharedSesKey = *getSharedKey(&s.mySesPriv, &s.theirSesPub)
@@ -144,7 +143,7 @@ func (ss *sessions) createSession(theirPermKey *boxPubKey) *sessionInfo {
 	pub, priv := newBoxKeys()
 	sinfo.mySesPub = *pub
 	sinfo.mySesPriv = *priv
-	sinfo.myNonce = *newBoxNonce() // TODO make sure nonceIsOK tolerates this
+	sinfo.myNonce = *newBoxNonce()
 	higher := false
 	for idx := range ss.core.boxPub {
 		if ss.core.boxPub[idx] > sinfo.theirPermPub[idx] {

src/yggdrasil/switch.go

@@ -11,6 +11,11 @@ package yggdrasil
 // TODO? use a pre-computed lookup table (python version had this)
 //  A little annoying to do with constant changes from bandwidth estimates
 
+// FIXME (!) throttle how often root updates are accepted
+//  If the root starts spaming with new timestamps, it should only affect their neighbors
+//  The rest of the network should see announcements at a somewhat reasonable rate
+//  Maybe no faster than 2x the usual update interval
+
 import "time"
 import "sync"
 import "sync/atomic"
@@ -250,7 +255,7 @@ func (t *switchTable) cleanPeers() {
 }
 
 func (t *switchTable) cleanDropped() {
-	// TODO only call this after root changes, not periodically
+	// TODO? only call this after root changes, not periodically
 	for root := range t.drop {
 		if !firstIsBetter(&root, &t.data.locator.root) {
 			delete(t.drop, root)

src/yggdrasil/udp.go

@@ -5,10 +5,13 @@ package yggdrasil
 // It's intended to use UDP, so debugging/optimzing this is a high priority
 // TODO? use golang.org/x/net/ipv6.PacketConn's ReadBatch and WriteBatch?
 //  To send all chunks of a message / recv all available chunks in one syscall
+//  That might be faster on supported platforms, but it needs investigation
 // Chunks are currently murged, but outgoing messages aren't chunked
 // This is just to support chunking in the future, if it's needed and debugged
 //  Basically, right now we might send UDP packets that are too large
 
+// TODO remove old/unused code and better document live code
+
 import "net"
 import "time"
 import "sync"
@@ -21,8 +24,6 @@ type udpInterface struct {
 	conns map[connAddr]*connInfo
 }
 
-//type connAddr string // TODO something more efficient, but still a valid map key
-
 type connAddr struct {
 	ip   [16]byte
 	port int
@@ -153,7 +154,7 @@ func (iface *udpInterface) handleKeys(msg []byte, addr connAddr) {
 	}
 	iface.mutex.RLock()
 	conn, isIn := iface.conns[addr]
-	iface.mutex.RUnlock() // TODO? keep the lock longer?...
+	iface.mutex.RUnlock()
 	if !isIn {
 		udpAddr := addr.toUDPAddr()
 		themNodeID := getNodeID(&ks.box)

src/yggdrasil/util.go

@@ -42,7 +42,7 @@ func util_unlockthread() {
 	runtime.UnlockOSThread()
 }
 
-/*
+/* Used previously, but removed because casting to an interface{} allocates...
 var byteStore sync.Pool = sync.Pool{
   New: func () interface{} { return []byte(nil) },
 }
@@ -52,7 +52,7 @@ func util_getBytes() []byte {
 }
 
 func util_putBytes(bs []byte) {
-  byteStore.Put(bs) // FIXME? The cast to interface{} allocates...
+  byteStore.Put(bs) // This is the part that allocates
 }
 */
 

src/yggdrasil/wire.go

@@ -3,6 +3,8 @@ package yggdrasil
 // Wire formatting tools
 // These are all ugly and probably not very secure
 
+// TODO clean up unused/commented code, and add better comments to whatever is left
+
 // Packet types, as an Encode_uint64 at the start of each packet
 // TODO? make things still work after reordering (after things stabilize more?)
 //  Type safety would also be nice, `type wire_type uint64`, rewrite as needed?
@@ -10,9 +12,9 @@ const (
 	wire_Traffic             = iota // data being routed somewhere, handle for crypto
 	wire_ProtocolTraffic            // protocol traffic, pub keys for crypto
 	wire_LinkProtocolTraffic        // link proto traffic, pub keys for crypto
-	wire_SwitchAnnounce             // TODO put inside protocol traffic header
+	wire_SwitchAnnounce             // inside protocol traffic header
-	wire_SwitchHopRequest           // TODO put inside protocol traffic header
+	wire_SwitchHopRequest           // inside protocol traffic header
-	wire_SwitchHop                  // TODO put inside protocol traffic header
+	wire_SwitchHop                  // inside protocol traffic header
 	wire_SessionPing                // inside protocol traffic header
 	wire_SessionPong                // inside protocol traffic header
 	wire_DHTLookupRequest           // inside protocol traffic header
@@ -119,7 +121,6 @@ func wire_decode_coords(packet []byte) ([]byte, int) {
 }
 
 ////////////////////////////////////////////////////////////////////////////////
-// TODO move this msg stuff somewhere else, use encode() and decode() methods
 
 // Announces that we can send parts of a Message with a particular seq
 type msgAnnounce struct {
@@ -295,7 +296,7 @@ func wire_chop_uint64(toUInt64 *uint64, fromSlice *[]byte) bool {
 // Wire traffic packets
 
 type wire_trafficPacket struct {
-	ttl     uint64 // TODO? hide this as a wire format detail, not set by user
+	ttl     uint64
 	coords  []byte
 	handle  handle
 	nonce   boxNonce
@@ -336,7 +337,7 @@ func (p *wire_trafficPacket) decode(bs []byte) bool {
 }
 
 type wire_protoTrafficPacket struct {
-	ttl     uint64 // TODO? hide this as a wire format detail, not set by user
+	ttl     uint64
 	coords  []byte
 	toKey   boxPubKey
 	fromKey boxPubKey