p2p/netutil: fix PredictFullConeNAT

The discovery protocol sends pings to previously-unseen nodes after
receiving a ping from them. This means there will always be a contact
entry and PredictFullConeNAT will always return false. Fix it by
checking that the contact occurred after the IP statement.

p2p/netutil/iptrack.go

@@ -31,7 +31,6 @@ type IPTracker struct {
 	clock         mclock.Clock
 	statements    map[string]ipStatement
 	contact       map[string]mclock.AbsTime
-	// TODO add DistinctNetSet for additional protection
 }
 
 type ipStatement struct {
@@ -57,13 +56,15 @@ func NewIPTracker(window, contactWindow time.Duration, minStatements int) *IPTra
 	}
 }
 
-// PredictFullConeNAT checks whether the local host is behind full cone NAT.
+// PredictFullConeNAT checks whether the local host is behind full cone NAT. It predicts by
+// checking whether any statement has been received from a node we didn't contact before
+// the statement was made.
 func (it *IPTracker) PredictFullConeNAT() bool {
 	now := it.clock.Now()
 	it.gcContact(now)
 	it.gcStatements(now)
-	for host := range it.statements {
-		if _, ok := it.contact[host]; !ok {
+	for host, st := range it.statements {
+		if c, ok := it.contact[host]; !ok || c > st.time {
 			return true
 		}
 	}
@@ -92,7 +93,8 @@ func (it *IPTracker) AddStatement(host, endpoint string) {
 	it.statements[host] = ipStatement{endpoint, it.clock.Now()}
 }
 
-// AddContact records that a packet containing endpoint information has been sent to a certain host.
+// AddContact records that a packet containing our endpoint information has been sent to a
+// certain host.
 func (it *IPTracker) AddContact(host string) {
 	it.contact[host] = it.clock.Now()
 }

p2p/netutil/iptrack_test.go

@@ -17,6 +17,7 @@
 package netutil
 
 import (
+	"fmt"
 	"testing"
 	"time"
 
@@ -27,6 +28,7 @@ const (
 	opStatement = iota
 	opContact
 	opPredict
+	opCheckFullCone
 )
 
 type iptrackTestEvent struct {
@@ -55,8 +57,25 @@ func TestIPTracker(t *testing.T) {
 			{opStatement, 10100, "127.0.0.1", "127.0.0.2"},
 			{opPredict, 10200, "127.0.0.1", ""},
 		},
+		"fullcone": {
+			{opContact, 0, "", "127.0.0.2"},
+			{opStatement, 10, "127.0.0.1", "127.0.0.2"},
+			{opContact, 2000, "", "127.0.0.3"},
+			{opStatement, 2010, "127.0.0.1", "127.0.0.3"},
+			{opContact, 3000, "", "127.0.0.4"},
+			{opStatement, 3010, "127.0.0.1", "127.0.0.4"},
+			{opCheckFullCone, 3500, "false", ""},
+		},
+		"fullcone_2": {
+			{opContact, 0, "", "127.0.0.2"},
+			{opStatement, 10, "127.0.0.1", "127.0.0.2"},
+			{opContact, 2000, "", "127.0.0.3"},
+			{opStatement, 2010, "127.0.0.1", "127.0.0.3"},
+			{opStatement, 3000, "127.0.0.1", "127.0.0.4"},
+			{opContact, 3010, "", "127.0.0.4"},
+			{opCheckFullCone, 3500, "true", ""},
+		},
 	}
-
 	for name, test := range tests {
 		t.Run(name, func(t *testing.T) { runIPTrackerTest(t, test) })
 	}
@@ -80,6 +99,11 @@ func runIPTrackerTest(t *testing.T, evs []iptrackTestEvent) {
 			if pred := it.PredictEndpoint(); pred != ev.ip {
 				t.Errorf("op %d: wrong prediction %q, want %q", i, pred, ev.ip)
 			}
+		case opCheckFullCone:
+			pred := fmt.Sprintf("%t", it.PredictFullConeNAT())
+			if pred != ev.ip {
+				t.Errorf("op %d: wrong prediction %s, want %s", i, pred, ev.ip)
+			}
 		}
 	}
 }