[NOD-77] Fix addrmanager behaviour when address's subnetworkID is in conflict with what is known (#230)

* [NOD-77] AddrManager.Good now updates subnetwork in case it was modified

* [NOD-77] Do not update subnetworkID in updateAddr if address already known

* [NOD-77] Restructure case where ka.tried = true for more readable code

* [NOD-77] Some corrections to comments

* [NOD-77] Fixd typo

addrmgr/addrmanager.go

@@ -193,7 +193,6 @@ func (a *AddrManager) updateAddress(netAddr, srcAddr *wire.NetAddress, subnetwor
 			naCopy.Timestamp = netAddr.Timestamp
 			naCopy.AddService(netAddr.Services)
 			ka.na = &naCopy
-			ka.subnetworkID = subnetworkID
 		}
 
 		// If already in tried, we have nothing to do here.
@@ -953,46 +952,60 @@ func (a *AddrManager) Good(addr *wire.NetAddress, subnetworkID *subnetworkid.Sub
 	ka.attempts = 0
 	ka.subnetworkID = subnetworkID
 
-	// move to tried set, optionally evicting other addresses if neeed.
-	if ka.tried {
-		return
-	}
-
-	// ok, need to move it to tried.
-
-	// remove from all new buckets.
-	// record one of the buckets in question and call it the `first'
 	addrKey := NetAddressKey(addr)
-	oldBucket := -1
+	triedBucketIndex := a.getTriedBucket(ka.na)
-	for i := range a.addrNew[*oldSubnetworkID] {
+
-		// we check for existence so we can record the first one
+	if ka.tried {
-		if _, ok := a.addrNew[*oldSubnetworkID][i][addrKey]; ok {
+		// If this address was already tried, and subnetworkID didn't change - don't do anything
-			delete(a.addrNew[*oldSubnetworkID][i], addrKey)
+		if subnetworkID.IsEqual(oldSubnetworkID) {
-			ka.refs--
+			return
-			if oldBucket == -1 {
+		}
-				oldBucket = i
+
+		// If this address was already tried, but subnetworkID was changed -
+		// update subnetworkID, than continue as though this is a new address
+		bucketList := a.addrTried[*oldSubnetworkID][triedBucketIndex]
+		for e := bucketList.Front(); e != nil; e = e.Next() {
+			if NetAddressKey(e.Value.(*KnownAddress).NetAddress()) == addrKey {
+				bucketList.Remove(e)
+				break
 			}
 		}
 	}
-	a.nNew[*oldSubnetworkID]--
 
-	if oldBucket == -1 {
+	// Ok, need to move it to tried.
-		// What? wasn't in a bucket after all.... Panic?
+
-		return
+	// Remove from all new buckets.
+	// Record one of the buckets in question and call it the `first'
+	oldBucket := -1
+	if !ka.tried {
+		for i := range a.addrNew[*oldSubnetworkID] {
+			// we check for existence so we can record the first one
+			if _, ok := a.addrNew[*oldSubnetworkID][i][addrKey]; ok {
+				delete(a.addrNew[*oldSubnetworkID][i], addrKey)
+				ka.refs--
+				if oldBucket == -1 {
+					oldBucket = i
+				}
+			}
+		}
+		a.nNew[*oldSubnetworkID]--
+
+		if oldBucket == -1 {
+			// What? wasn't in a bucket after all.... Panic?
+			return
+		}
 	}
 
-	bucket := a.getTriedBucket(ka.na)
-
 	// Room in this tried bucket?
-	if a.nTried[*ka.subnetworkID] == 0 || a.addrTried[*ka.subnetworkID][bucket].Len() < triedBucketSize {
+	if a.nTried[*ka.subnetworkID] == 0 || a.addrTried[*ka.subnetworkID][triedBucketIndex].Len() < triedBucketSize {
 		ka.tried = true
-		a.updateAddrTried(bucket, ka)
+		a.updateAddrTried(triedBucketIndex, ka)
 		a.nTried[*ka.subnetworkID]++
 		return
 	}
 
 	// No room, we have to evict something else.
-	entry := a.pickTried(ka.subnetworkID, bucket)
+	entry := a.pickTried(ka.subnetworkID, triedBucketIndex)
 	rmka := entry.Value.(*KnownAddress)
 
 	// First bucket it would have been put in.
@@ -1001,10 +1014,21 @@ func (a *AddrManager) Good(addr *wire.NetAddress, subnetworkID *subnetworkid.Sub
 	// If no room in the original bucket, we put it in a bucket we just
 	// freed up a space in.
 	if len(a.addrNew[*ka.subnetworkID][newBucket]) >= newBucketSize {
-		newBucket = oldBucket
+		if oldBucket == -1 {
+			// If addr was a tried bucket with updated subnetworkID - oldBucket will be equal to -1.
+			// In that case - find some non-full bucket.
+			// If no such bucket exists - throw rmka away
+			for newBucket := range a.addrNew[*ka.subnetworkID] {
+				if len(a.addrNew[*ka.subnetworkID][newBucket]) < newBucketSize {
+					break
+				}
+			}
+		} else {
+			newBucket = oldBucket
+		}
 	}
 
-	// replace with ka in list.
+	// Replace with ka in list.
 	ka.tried = true
 	entry.Value = ka
 

addrmgr/addrmanager_test.go

@@ -2,7 +2,7 @@
 // Use of this source code is governed by an ISC
 // license that can be found in the LICENSE file.
 
-package addrmgr_test
+package addrmgr
 
 import (
 	"errors"
@@ -14,7 +14,6 @@ import (
 
 	"github.com/daglabs/btcd/util/subnetworkid"
 
-	"github.com/daglabs/btcd/addrmgr"
 	"github.com/daglabs/btcd/wire"
 )
 
@@ -105,7 +104,7 @@ func lookupFunc(host string) ([]net.IP, error) {
 }
 
 func TestStartStop(t *testing.T) {
-	n := addrmgr.New("teststartstop", lookupFunc, subnetworkid.SubnetworkIDSupportsAll)
+	n := New("teststartstop", lookupFunc, subnetworkid.SubnetworkIDSupportsAll)
 	n.Start()
 	err := n.Stop()
 	if err != nil {
@@ -138,7 +137,7 @@ func TestAddAddressByIP(t *testing.T) {
 		},
 	}
 
-	amgr := addrmgr.New("testaddressbyip", nil, subnetworkid.SubnetworkIDSupportsAll)
+	amgr := New("testaddressbyip", nil, subnetworkid.SubnetworkIDSupportsAll)
 	for i, test := range tests {
 		err := amgr.AddAddressByIP(test.addrIP, subnetworkid.SubnetworkIDSupportsAll)
 		if test.err != nil && err == nil {
@@ -160,41 +159,41 @@ func TestAddAddressByIP(t *testing.T) {
 func TestAddLocalAddress(t *testing.T) {
 	var tests = []struct {
 		address  wire.NetAddress
-		priority addrmgr.AddressPriority
+		priority AddressPriority
 		valid    bool
 	}{
 		{
 			wire.NetAddress{IP: net.ParseIP("192.168.0.100")},
-			addrmgr.InterfacePrio,
+			InterfacePrio,
 			false,
 		},
 		{
 			wire.NetAddress{IP: net.ParseIP("204.124.1.1")},
-			addrmgr.InterfacePrio,
+			InterfacePrio,
 			true,
 		},
 		{
 			wire.NetAddress{IP: net.ParseIP("204.124.1.1")},
-			addrmgr.BoundPrio,
+			BoundPrio,
 			true,
 		},
 		{
 			wire.NetAddress{IP: net.ParseIP("::1")},
-			addrmgr.InterfacePrio,
+			InterfacePrio,
 			false,
 		},
 		{
 			wire.NetAddress{IP: net.ParseIP("fe80::1")},
-			addrmgr.InterfacePrio,
+			InterfacePrio,
 			false,
 		},
 		{
 			wire.NetAddress{IP: net.ParseIP("2620:100::1")},
-			addrmgr.InterfacePrio,
+			InterfacePrio,
 			true,
 		},
 	}
-	amgr := addrmgr.New("testaddlocaladdress", nil, subnetworkid.SubnetworkIDSupportsAll)
+	amgr := New("testaddlocaladdress", nil, subnetworkid.SubnetworkIDSupportsAll)
 	for x, test := range tests {
 		result := amgr.AddLocalAddress(&test.address, test.priority)
 		if result == nil && !test.valid {
@@ -211,7 +210,7 @@ func TestAddLocalAddress(t *testing.T) {
 }
 
 func TestAttempt(t *testing.T) {
-	n := addrmgr.New("testattempt", lookupFunc, subnetworkid.SubnetworkIDSupportsAll)
+	n := New("testattempt", lookupFunc, subnetworkid.SubnetworkIDSupportsAll)
 
 	// Add a new address and get it
 	err := n.AddAddressByIP(someIP+":8333", subnetworkid.SubnetworkIDSupportsAll)
@@ -233,7 +232,7 @@ func TestAttempt(t *testing.T) {
 }
 
 func TestConnected(t *testing.T) {
-	n := addrmgr.New("testconnected", lookupFunc, subnetworkid.SubnetworkIDSupportsAll)
+	n := New("testconnected", lookupFunc, subnetworkid.SubnetworkIDSupportsAll)
 
 	// Add a new address and get it
 	err := n.AddAddressByIP(someIP+":8333", subnetworkid.SubnetworkIDSupportsAll)
@@ -253,7 +252,7 @@ func TestConnected(t *testing.T) {
 }
 
 func TestNeedMoreAddresses(t *testing.T) {
-	n := addrmgr.New("testneedmoreaddresses", lookupFunc, subnetworkid.SubnetworkIDSupportsAll)
+	n := New("testneedmoreaddresses", lookupFunc, subnetworkid.SubnetworkIDSupportsAll)
 	addrsToAdd := 1500
 	b := n.NeedMoreAddresses()
 	if !b {
@@ -285,7 +284,7 @@ func TestNeedMoreAddresses(t *testing.T) {
 }
 
 func TestGood(t *testing.T) {
-	n := addrmgr.New("testgood", lookupFunc, subnetworkid.SubnetworkIDSupportsAll)
+	n := New("testgood", lookupFunc, subnetworkid.SubnetworkIDSupportsAll)
 	addrsToAdd := 64 * 64
 	addrs := make([]*wire.NetAddress, addrsToAdd)
 	subnetworkCount := 32
@@ -331,9 +330,78 @@ func TestGood(t *testing.T) {
 	}
 }
 
+func TestGoodChangeSubnetworkID(t *testing.T) {
+	n := New("test_good_change_subnetwork_id", lookupFunc, subnetworkid.SubnetworkIDSupportsAll)
+	addr := wire.NewNetAddressIPPort(net.IPv4(173, 144, 173, 111), 8333, 0)
+	addrKey := NetAddressKey(addr)
+	srcAddr := wire.NewNetAddressIPPort(net.IPv4(173, 144, 173, 111), 8333, 0)
+
+	oldSubnetwork := subnetworkid.SubnetworkIDNative
+	n.AddAddress(addr, srcAddr, oldSubnetwork)
+	n.Good(addr, oldSubnetwork)
+
+	// make sure address was saved to addrIndex under oldSubnetwork
+	ka := n.find(addr)
+	if ka == nil {
+		t.Fatalf("Address was not found after first time .Good called")
+	}
+	if !ka.SubnetworkID().IsEqual(oldSubnetwork) {
+		t.Fatalf("Address index did not point to oldSubnetwork")
+	}
+
+	// make sure address was added to correct bucket under oldSubnetwork
+	bucket := n.addrTried[*oldSubnetwork][n.getTriedBucket(addr)]
+	wasFound := false
+	for e := bucket.Front(); e != nil; e = e.Next() {
+		if NetAddressKey(e.Value.(*KnownAddress).NetAddress()) == addrKey {
+			wasFound = true
+		}
+	}
+	if !wasFound {
+		t.Fatalf("Address was not found in the correct bucket in oldSubnetwork")
+	}
+
+	// now call .Good again with a different subnetwork
+	newSubnetwork := subnetworkid.SubnetworkIDRegistry
+	n.Good(addr, newSubnetwork)
+
+	// make sure address was updated in addrIndex under newSubnetwork
+	ka = n.find(addr)
+	if ka == nil {
+		t.Fatalf("Address was not found after second time .Good called")
+	}
+	if !ka.SubnetworkID().IsEqual(newSubnetwork) {
+		t.Fatalf("Address index did not point to newSubnetwork")
+	}
+
+	// make sure address was removed from bucket under oldSubnetwork
+	bucket = n.addrTried[*oldSubnetwork][n.getTriedBucket(addr)]
+	wasFound = false
+	for e := bucket.Front(); e != nil; e = e.Next() {
+		if NetAddressKey(e.Value.(*KnownAddress).NetAddress()) == addrKey {
+			wasFound = true
+		}
+	}
+	if wasFound {
+		t.Fatalf("Address was not removed from bucket in oldSubnetwork")
+	}
+
+	// make sure address was added to correct bucket under newSubnetwork
+	bucket = n.addrTried[*newSubnetwork][n.getTriedBucket(addr)]
+	wasFound = false
+	for e := bucket.Front(); e != nil; e = e.Next() {
+		if NetAddressKey(e.Value.(*KnownAddress).NetAddress()) == addrKey {
+			wasFound = true
+		}
+	}
+	if !wasFound {
+		t.Fatalf("Address was not found in the correct bucket in newSubnetwork")
+	}
+}
+
 func TestGetAddress(t *testing.T) {
 	localSubnetworkID := &subnetworkid.SubnetworkID{0xff}
-	n := addrmgr.New("testgetaddress", lookupFunc, localSubnetworkID)
+	n := New("testgetaddress", lookupFunc, localSubnetworkID)
 
 	// Get an address from an empty set (should error)
 	if rv := n.GetAddress(); rv != nil {
@@ -451,7 +519,7 @@ func TestGetBestLocalAddress(t *testing.T) {
 		*/
 	}
 
-	amgr := addrmgr.New("testgetbestlocaladdress", nil, subnetworkid.SubnetworkIDSupportsAll)
+	amgr := New("testgetbestlocaladdress", nil, subnetworkid.SubnetworkIDSupportsAll)
 
 	// Test against default when there's no address
 	for x, test := range tests {
@@ -464,7 +532,7 @@ func TestGetBestLocalAddress(t *testing.T) {
 	}
 
 	for _, localAddr := range localAddrs {
-		amgr.AddLocalAddress(&localAddr, addrmgr.InterfacePrio)
+		amgr.AddLocalAddress(&localAddr, InterfacePrio)
 	}
 
 	// Test against want1
@@ -479,7 +547,7 @@ func TestGetBestLocalAddress(t *testing.T) {
 
 	// Add a public IP to the list of local addresses.
 	localAddr := wire.NetAddress{IP: net.ParseIP("204.124.8.100")}
-	amgr.AddLocalAddress(&localAddr, addrmgr.InterfacePrio)
+	amgr.AddLocalAddress(&localAddr, InterfacePrio)
 
 	// Test against want2
 	for x, test := range tests {
@@ -493,7 +561,7 @@ func TestGetBestLocalAddress(t *testing.T) {
 	/*
 		// Add a Tor generated IP address
 		localAddr = wire.NetAddress{IP: net.ParseIP("fd87:d87e:eb43:25::1")}
-		amgr.AddLocalAddress(&localAddr, addrmgr.ManualPrio)
+		amgr.AddLocalAddress(&localAddr, ManualPrio)
 
 		// Test against want3
 		for x, test := range tests {
@@ -512,7 +580,7 @@ func TestNetAddressKey(t *testing.T) {
 
 	t.Logf("Running %d tests", len(naTests))
 	for i, test := range naTests {
-		key := addrmgr.NetAddressKey(&test.in)
+		key := NetAddressKey(&test.in)
 		if key != test.want {
 			t.Errorf("NetAddressKey #%d\n got: %s want: %s", i, key, test.want)
 			continue