From 420131a84520034d3e5452bd50728f9ad329016b Mon Sep 17 00:00:00 2001 From: Steven Allen Date: Thu, 4 Jan 2018 12:59:43 -0800 Subject: [PATCH 1/7] mark test helpers --- addr_manager_test.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/addr_manager_test.go b/addr_manager_test.go index b9cbfd9..e1ae0fd 100644 --- a/addr_manager_test.go +++ b/addr_manager_test.go @@ -9,6 +9,7 @@ import ( ) func IDS(t *testing.T, ids string) peer.ID { + t.Helper() id, err := peer.IDB58Decode(ids) if err != nil { t.Fatalf("id %q is bad: %s", ids, err) @@ -17,6 +18,7 @@ func IDS(t *testing.T, ids string) peer.ID { } func MA(t *testing.T, m string) ma.Multiaddr { + t.Helper() maddr, err := ma.NewMultiaddr(m) if err != nil { t.Fatal(err) @@ -25,6 +27,7 @@ func MA(t *testing.T, m string) ma.Multiaddr { } func testHas(t *testing.T, exp, act []ma.Multiaddr) { + t.Helper() if len(exp) != len(act) { t.Fatal("lengths not the same") } From f6bb5a31c193ed6265f3b782a09ab4b27be20fc0 Mon Sep 17 00:00:00 2001 From: Steven Allen Date: Thu, 4 Jan 2018 13:00:37 -0800 Subject: [PATCH 2/7] add method to atomically update TTLs This allows one to find all addresses with a given TTL and update them to have a new TTL. --- addr_manager.go | 38 ++++++++++++++++++++++++++++++++------ peerstore.go | 4 ++++ 2 files changed, 36 insertions(+), 6 deletions(-) diff --git a/addr_manager.go b/addr_manager.go index b94b37b..c7b5b35 100644 --- a/addr_manager.go +++ b/addr_manager.go @@ -40,12 +40,13 @@ const ( ) type expiringAddr struct { - Addr ma.Multiaddr - TTL time.Time + Addr ma.Multiaddr + TTL time.Duration + Expires time.Time } func (e *expiringAddr) ExpiredBy(t time.Time) bool { - return t.After(e.TTL) + return t.After(e.Expires) } type addrSet map[string]expiringAddr @@ -122,8 +123,8 @@ func (mgr *AddrManager) AddAddrs(p peer.ID, addrs []ma.Multiaddr, ttl time.Durat addrstr := string(addr.Bytes()) a, found := amap[addrstr] - if !found || exp.After(a.TTL) { - amap[addrstr] = expiringAddr{Addr: addr, TTL: exp} + if !found || exp.After(a.Expires) { + amap[addrstr] = expiringAddr{Addr: addr, Expires: exp, TTL: ttl} for _, sub := range subs { sub.pubAddr(addr) @@ -164,7 +165,7 @@ func (mgr *AddrManager) SetAddrs(p peer.ID, addrs []ma.Multiaddr, ttl time.Durat addrs := string(addr.Bytes()) if ttl > 0 { - amap[addrs] = expiringAddr{Addr: addr, TTL: exp} + amap[addrs] = expiringAddr{Addr: addr, Expires: exp, TTL: ttl} for _, sub := range subs { sub.pubAddr(addr) @@ -175,6 +176,31 @@ func (mgr *AddrManager) SetAddrs(p peer.ID, addrs []ma.Multiaddr, ttl time.Durat } } +// UpdateAddrs updates the addresses associated with the given peer that have +// the given oldTTL to have the given newTTL. +func (mgr *AddrManager) UpdateAddrs(p peer.ID, oldTTL time.Duration, newTTL time.Duration) { + mgr.addrmu.Lock() + defer mgr.addrmu.Unlock() + + if mgr.addrs == nil { + return + } + + amap, found := mgr.addrs[p] + if !found { + return + } + + exp := time.Now().Add(newTTL) + for addrstr, aexp := range amap { + if oldTTL == aexp.TTL { + aexp.TTL = newTTL + aexp.Expires = exp + amap[addrstr] = aexp + } + } +} + // Addresses returns all known (and valid) addresses for a given func (mgr *AddrManager) Addrs(p peer.ID) []ma.Multiaddr { mgr.addrmu.Lock() diff --git a/peerstore.go b/peerstore.go index 9747984..d0fe38c 100644 --- a/peerstore.go +++ b/peerstore.go @@ -69,6 +69,10 @@ type AddrBook interface { // This is used when we receive the best estimate of the validity of an address. SetAddrs(p peer.ID, addrs []ma.Multiaddr, ttl time.Duration) + // UpdateAddrs updates the addresses associated with the given peer that have + // the given oldTTL to have the given newTTL. + UpdateAddrs(p peer.ID, oldTTL time.Duration, newTTL time.Duration) + // Addresses returns all known (and valid) addresses for a given peer Addrs(p peer.ID) []ma.Multiaddr From 1a0684bd677cbbbd5cfae03ae42606fe86530604 Mon Sep 17 00:00:00 2001 From: Steven Allen Date: Thu, 4 Jan 2018 13:01:24 -0800 Subject: [PATCH 3/7] add a test for UpdateAddrs --- addr_manager_test.go | 38 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 38 insertions(+) diff --git a/addr_manager_test.go b/addr_manager_test.go index e1ae0fd..06ff66c 100644 --- a/addr_manager_test.go +++ b/addr_manager_test.go @@ -205,6 +205,44 @@ func TestSetNegativeTTLClears(t *testing.T) { testHas(t, nil, m.Addrs(id1)) } +func TestUpdateTTLs(t *testing.T) { + id1 := IDS(t, "QmcNstKuwBBoVTpSCSDrwzjgrRcaYXK833Psuz2EMHwyQN") + id2 := IDS(t, "QmcNstKuwBBoVTpSCSDrwzjgrRcaYXK833Psuz2EMHwyQM") + ma11 := MA(t, "/ip4/1.2.3.1/tcp/1111") + ma12 := MA(t, "/ip4/1.2.3.1/tcp/1112") + ma21 := MA(t, "/ip4/1.2.3.1/tcp/1121") + ma22 := MA(t, "/ip4/1.2.3.1/tcp/1122") + + m := AddrManager{} + m.SetAddr(id1, ma11, time.Hour) + m.SetAddr(id1, ma12, time.Minute) + m.SetAddr(id2, ma21, time.Hour) + m.SetAddr(id2, ma22, time.Minute) + + testHas(t, []ma.Multiaddr{ma11, ma12}, m.Addrs(id1)) + testHas(t, []ma.Multiaddr{ma21, ma22}, m.Addrs(id2)) + + m.UpdateAddrs(id1, time.Hour, time.Millisecond) + + testHas(t, []ma.Multiaddr{ma11, ma12}, m.Addrs(id1)) + testHas(t, []ma.Multiaddr{ma21, ma22}, m.Addrs(id2)) + + time.Sleep(time.Millisecond) + + testHas(t, []ma.Multiaddr{ma12}, m.Addrs(id1)) + testHas(t, []ma.Multiaddr{ma21, ma22}, m.Addrs(id2)) + + m.UpdateAddrs(id2, time.Hour, time.Millisecond) + + testHas(t, []ma.Multiaddr{ma12}, m.Addrs(id1)) + testHas(t, []ma.Multiaddr{ma21, ma22}, m.Addrs(id2)) + + time.Sleep(time.Millisecond) + + testHas(t, []ma.Multiaddr{ma12}, m.Addrs(id1)) + testHas(t, []ma.Multiaddr{ma22}, m.Addrs(id2)) +} + func TestNilAddrsDontBreak(t *testing.T) { id1 := IDS(t, "QmcNstKuwBBoVTpSCSDrwzjgrRcaYXK833Psuz2EMHwyQN") m := AddrManager{} From b53dd2e95e7b370f5e54af0c250474f2e1af1cc7 Mon Sep 17 00:00:00 2001 From: Steven Allen Date: Thu, 4 Jan 2018 14:51:36 -0800 Subject: [PATCH 4/7] make all TTLs distinct (also, make permanent TTLs actually permanent) --- addr_manager.go | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/addr_manager.go b/addr_manager.go index c7b5b35..0ffa3cf 100644 --- a/addr_manager.go +++ b/addr_manager.go @@ -2,6 +2,7 @@ package peerstore import ( "context" + "math" "sort" "sync" "time" @@ -27,16 +28,19 @@ const ( // OwnObservedAddrTTL is used for our own external addresses observed by peers. OwnObservedAddrTTL = time.Minute * 10 +) +// Perminent TTLs (distinct so we can distinguish between them) +const ( // PermanentAddrTTL is the ttl for a "permanent address" (e.g. bootstrap nodes) // if we haven't shipped you an update to ipfs in 356 days // we probably arent running the same bootstrap nodes... - PermanentAddrTTL = time.Hour * 24 * 356 + PermanentAddrTTL = math.MaxInt64 - iota // ConnectedAddrTTL is the ttl used for the addresses of a peer to whom // we're connected directly. This is basically permanent, as we will // clear them + re-add under a TempAddrTTL after disconnecting. - ConnectedAddrTTL = PermanentAddrTTL + ConnectedAddrTTL ) type expiringAddr struct { From 07712d63e3d41c907fd87f382ab6183a1c9069b8 Mon Sep 17 00:00:00 2001 From: Steven Allen Date: Thu, 4 Jan 2018 16:32:58 -0800 Subject: [PATCH 5/7] fix PerminentAddrTTL comment --- addr_manager.go | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/addr_manager.go b/addr_manager.go index 0ffa3cf..a12af8d 100644 --- a/addr_manager.go +++ b/addr_manager.go @@ -32,9 +32,7 @@ const ( // Perminent TTLs (distinct so we can distinguish between them) const ( - // PermanentAddrTTL is the ttl for a "permanent address" (e.g. bootstrap nodes) - // if we haven't shipped you an update to ipfs in 356 days - // we probably arent running the same bootstrap nodes... + // PermanentAddrTTL is the ttl for a "permanent address" (e.g. bootstrap nodes). PermanentAddrTTL = math.MaxInt64 - iota // ConnectedAddrTTL is the ttl used for the addresses of a peer to whom From 62cbfac7c6ac98698bb4ab61cdb1756118edc7a7 Mon Sep 17 00:00:00 2001 From: Steven Allen Date: Thu, 4 Jan 2018 18:15:01 -0800 Subject: [PATCH 6/7] spelling --- addr_manager.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/addr_manager.go b/addr_manager.go index a12af8d..1dfe9dc 100644 --- a/addr_manager.go +++ b/addr_manager.go @@ -30,7 +30,7 @@ const ( OwnObservedAddrTTL = time.Minute * 10 ) -// Perminent TTLs (distinct so we can distinguish between them) +// Permanent TTLs (distinct so we can distinguish between them) const ( // PermanentAddrTTL is the ttl for a "permanent address" (e.g. bootstrap nodes). PermanentAddrTTL = math.MaxInt64 - iota From 9cb6ba9eb7748e1e2aad4331c461ff77aaf75109 Mon Sep 17 00:00:00 2001 From: Steven Allen Date: Thu, 4 Jan 2018 18:23:02 -0800 Subject: [PATCH 7/7] improve UpdateTTLs test coverage --- addr_manager_test.go | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/addr_manager_test.go b/addr_manager_test.go index 06ff66c..9ade2c5 100644 --- a/addr_manager_test.go +++ b/addr_manager_test.go @@ -214,8 +214,16 @@ func TestUpdateTTLs(t *testing.T) { ma22 := MA(t, "/ip4/1.2.3.1/tcp/1122") m := AddrManager{} + + // Shouldn't panic. + m.UpdateAddrs(id1, time.Hour, time.Minute) + m.SetAddr(id1, ma11, time.Hour) m.SetAddr(id1, ma12, time.Minute) + + // Shouldn't panic. + m.UpdateAddrs(id2, time.Hour, time.Minute) + m.SetAddr(id2, ma21, time.Hour) m.SetAddr(id2, ma22, time.Minute)