From dd3b69326869a4bb563a19d0528e3aada9855c7d Mon Sep 17 00:00:00 2001 From: Svarog Date: Tue, 26 Mar 2019 16:50:43 +0200 Subject: [PATCH] [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 | 82 ++++++++++++++++---------- addrmgr/addrmanager_test.go | 112 +++++++++++++++++++++++++++++------- 2 files changed, 143 insertions(+), 51 deletions(-) diff --git a/addrmgr/addrmanager.go b/addrmgr/addrmanager.go index b41857249..22bb8f971 100644 --- a/addrmgr/addrmanager.go +++ b/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 - 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 + triedBucketIndex := a.getTriedBucket(ka.na) + + if ka.tried { + // If this address was already tried, and subnetworkID didn't change - don't do anything + if subnetworkID.IsEqual(oldSubnetworkID) { + return + } + + // 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 { - // What? wasn't in a bucket after all.... Panic? - 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' + 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 diff --git a/addrmgr/addrmanager_test.go b/addrmgr/addrmanager_test.go index bde49e5c8..4a9f90480 100644 --- a/addrmgr/addrmanager_test.go +++ b/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