refactor(handshake): keep only the latest signed address in the cache

Author: gacevicljubisa

pkg/p2p/libp2p/internal/handshake/addresscache.go

@@ -5,55 +5,37 @@
 package handshake
 
 import (
-	"container/list"
 	"sync"
 
 	"github.com/ethersphere/bee/v2/pkg/bzz"
 )
 
-// addressCache stores the session-stable signed BzzAddress per cache key,
-// evicting the least recently used entry when capacity is exceeded.
+// addressCache stores the most recently minted session-stable signed
+// BzzAddress and the key (chequebook + underlay set) it was minted for. A key
+// change makes the previous record obsolete, so only the latest one is kept.
 //
-// The cache owns the mutex and runs mint inside it, which guarantees two
-// invariants no per-method locking could:
+// Minting runs inside the mutex, which guarantees:
 //
-//   - single flight: concurrent lookups for the same key produce exactly
-//     one signed record;
-//   - monotonic timestamps: every minted record carries a strictly greater
-//     timestamp than the previous one, across all keys, even when the wall
-//     clock repeats a second or steps backwards.
+//   - single flight: concurrent lookups for the same key mint exactly once;
+//   - monotonic timestamps: each mint is strictly newer than the last, even
+//     when the wall clock repeats a second or steps back. Peers reject
+//     records older than the one they hold (see bzz.CheckTimestamp).
 type addressCache struct {
-	mu      sync.Mutex
-	cap     int
-	entries map[string]*list.Element
-	lru     *list.List // *cacheEntry elements, most recently used in front
-	lastTS  int64      // timestamp of the most recently minted address
+	mu     sync.Mutex
+	key    string
+	addr   *bzz.Address
+	lastTS int64 // timestamp of the most recently minted address
 }
 
-// cacheEntry pairs the cache key with the signed address minted for it.
-type cacheEntry struct {
-	key  string
-	addr *bzz.Address
-}
-
-func newAddressCache(capacity int) *addressCache {
-	return &addressCache{
-		cap:     capacity,
-		entries: make(map[string]*list.Element),
-		lru:     list.New(),
-	}
-}
-
-// getOrMint returns the cached address for key, or calls mint exactly once
-// with the next monotonic timestamp, max(now, last+1), and caches the result.
-// A failed mint does not consume the timestamp.
+// getOrMint returns the cached address on a key match, otherwise mints with
+// the next monotonic timestamp, max(now, last+1), and caches the result. A
+// failed mint does not consume the timestamp.
 func (c *addressCache) getOrMint(key string, now int64, mint func(timestamp int64) (*bzz.Address, error)) (*bzz.Address, error) {
 	c.mu.Lock()
 	defer c.mu.Unlock()
 
-	if el, ok := c.entries[key]; ok {
-		c.lru.MoveToFront(el)
-		return el.Value.(*cacheEntry).addr, nil
+	if c.addr != nil && c.key == key {
+		return c.addr, nil
 	}
 
 	timestamp := max(now, c.lastTS+1)
@@ -63,30 +45,15 @@ func (c *addressCache) getOrMint(key string, now int64, mint func(timestamp int6
 		return nil, err
 	}
 	c.lastTS = timestamp
-
-	c.entries[key] = c.lru.PushFront(&cacheEntry{key: key, addr: addr})
-	if c.lru.Len() > c.cap {
-		oldest := c.lru.Back()
-		c.lru.Remove(oldest)
-		delete(c.entries, oldest.Value.(*cacheEntry).key)
-	}
+	c.key, c.addr = key, addr
 
 	return addr, nil
 }
 
-// purge drops all cached addresses; the next getOrMint per key re-signs.
+// purge drops the cached address; the next getOrMint re-signs.
 func (c *addressCache) purge() {
 	c.mu.Lock()
 	defer c.mu.Unlock()
 
-	clear(c.entries)
-	c.lru.Init()
-}
-
-// size returns the number of cached addresses.
-func (c *addressCache) size() int {
-	c.mu.Lock()
-	defer c.mu.Unlock()
-
-	return c.lru.Len()
+	c.key, c.addr = "", nil
 }

pkg/p2p/libp2p/internal/handshake/export_test.go

@@ -13,8 +13,6 @@ import (
 	ma "github.com/multiformats/go-multiaddr"
 )
 
-const MaxCachedAddresses = maxCachedAddresses
-
 func (s *Service) SetTime(f func() time.Time) {
 	s.now = f
 }
@@ -26,7 +24,3 @@ func (s *Service) ParseCheckAck(ctx context.Context, ack *pb.Ack) (*bzz.Address,
 func (s *Service) SignedAddress(underlays []ma.Multiaddr) (*bzz.Address, error) {
 	return s.signedAddress(underlays)
 }
-
-func (s *Service) AddressCacheLen() int {
-	return s.addrCache.size()
-}

pkg/p2p/libp2p/internal/handshake/handshake.go

@@ -41,10 +41,6 @@ const (
 	// MaxWelcomeMessageLength is maximum number of characters allowed in the welcome message.
 	MaxWelcomeMessageLength = 140
 	handshakeTimeout        = 15 * time.Second
-	// maxCachedAddresses bounds the signed-address cache so that peers
-	// reporting fabricated observed underlays cannot force unbounded growth
-	// or evict the legitimate entries fast enough to defeat record reuse.
-	maxCachedAddresses = 16
 )
 
 var (
@@ -107,7 +103,7 @@ type Service struct {
 	hostAddresser         Addresser
 	now                   func() time.Time
 
-	addrCache *addressCache // session-stable signed addresses, keyed by chequebook + underlays
+	addrCache addressCache // session-stable signed address, keyed by chequebook + underlays
 }
 
 // Info contains the information received from the handshake.
@@ -146,19 +142,17 @@ func New(signer crypto.Signer, advertisableAddresser AdvertisableAddressResolver
 		addressbook:           addrbook,
 		chequebookVerifier:    chequebookVerifier,
 		now:                   time.Now,
-		addrCache:             newAddressCache(maxCachedAddresses),
 	}
 	svc.welcomeMessage.Store(welcomeMessage)
 
 	return svc, nil
 }
 
 // SetChequebookAddress sets the local chequebook address included in
-// subsequent signed BzzAddress payloads. The zero value clears it. Cached
-// signed addresses are invalidated so the next handshake re-signs with the
-// new chequebook. An entry minted by a handshake in flight during this call
-// may survive the purge, but is keyed under the old chequebook and can no
-// longer be looked up; it ages out of the cache unused.
+// subsequent signed BzzAddress payloads; the zero value clears it. The cached
+// signed address is purged so the next handshake re-signs. A mint in flight
+// during this call may land after the purge, but is keyed under the old
+// chequebook and simply gets overwritten by the next handshake.
 func (s *Service) SetChequebookAddress(addr common.Address) {
 	if (addr == common.Address{}) {
 		s.chequebookAddr.Store(nil)
@@ -178,11 +172,10 @@ func (s *Service) chequebookAddress() common.Address {
 }
 
 // signedAddress returns the session-stable signed BzzAddress advertising the
-// given canonical underlay set, minting and caching one on first use. Reusing
-// the timestamp and signature keeps the record byte-stable across handshakes,
-// so receiving peers see it as unchanged and skip redundant addressbook
-// writes and gossip updates. A new address is minted only when the underlay
-// set or the local chequebook changes.
+// given canonical underlay set. The record is minted on first use and reused
+// byte-stable (same timestamp and signature) across handshakes, so receiving
+// peers see it as unchanged and skip redundant addressbook writes and gossip
+// updates. It is re-minted when the underlay set or the chequebook changes.
 func (s *Service) signedAddress(underlays []ma.Multiaddr) (*bzz.Address, error) {
 	underlaysBinary, err := bzz.SerializeUnderlays(underlays)
 	if err != nil {

pkg/p2p/libp2p/internal/handshake/handshake_test.go

@@ -1055,8 +1055,8 @@ func testUnderlays(t *testing.T, port int) []ma.Multiaddr {
 }
 
 // TestSignedAddress_StableAcrossCalls verifies that the signed address is
-// minted once per underlay set and reused verbatim on subsequent calls, even
-// when the clock advances (issue #23).
+// minted once and reused verbatim on subsequent calls, even when the clock
+// advances.
 func TestSignedAddress_StableAcrossCalls(t *testing.T) {
 	t.Parallel()
 
@@ -1152,57 +1152,39 @@ func TestSignedAddress_ClockRegression(t *testing.T) {
 	}
 }
 
-// TestSignedAddress_CacheEviction verifies the LRU semantics of the signed
-// address cache: recently used entries survive an overflow, the least
-// recently used entry is evicted and re-minted on next use.
-func TestSignedAddress_CacheEviction(t *testing.T) {
+// TestSignedAddress_LatestEntryOnly verifies the cache keeps only the most
+// recently minted address: returning to a previously used underlay set
+// re-mints with a strictly newer timestamp instead of reviving the old record.
+func TestSignedAddress_LatestEntryOnly(t *testing.T) {
 	t.Parallel()
 
 	networkID := uint64(3)
 	now := time.Unix(1700000000, 0)
 	svc := newTimestampTestService(t, networkID, now)
 
-	sets := make([][]ma.Multiaddr, handshake.MaxCachedAddresses+1)
-	timestamps := make([]int64, len(sets))
-	for i := range sets {
-		sets[i] = testUnderlays(t, 2000+i)
-	}
-
-	for i := range handshake.MaxCachedAddresses {
-		addr, err := svc.SignedAddress(sets[i])
-		if err != nil {
-			t.Fatal(err)
-		}
-		timestamps[i] = addr.Timestamp
-	}
-
-	// Touch the oldest entry so the upcoming eviction targets sets[1].
-	if _, err := svc.SignedAddress(sets[0]); err != nil {
+	first, err := svc.SignedAddress(testUnderlays(t, 2000))
+	if err != nil {
 		t.Fatal(err)
 	}
 
-	if _, err := svc.SignedAddress(sets[handshake.MaxCachedAddresses]); err != nil {
+	if _, err := svc.SignedAddress(testUnderlays(t, 2001)); err != nil {
 		t.Fatal(err)
 	}
 
-	if got := svc.AddressCacheLen(); got != handshake.MaxCachedAddresses {
-		t.Fatalf("cache length: got %d, want %d", got, handshake.MaxCachedAddresses)
-	}
-
-	survived, err := svc.SignedAddress(sets[0])
+	reminted, err := svc.SignedAddress(testUnderlays(t, 2000))
 	if err != nil {
 		t.Fatal(err)
 	}
-	if survived.Timestamp != timestamps[0] {
-		t.Fatalf("touched entry re-minted: got %d, want cached %d", survived.Timestamp, timestamps[0])
+	if reminted.Timestamp != first.Timestamp+2 {
+		t.Fatalf("expected re-mint with newer timestamp: got %d, want %d", reminted.Timestamp, first.Timestamp+2)
 	}
 
-	evicted, err := svc.SignedAddress(sets[1])
+	cached, err := svc.SignedAddress(testUnderlays(t, 2000))
 	if err != nil {
 		t.Fatal(err)
 	}
-	if evicted.Timestamp <= timestamps[1] {
-		t.Fatalf("evicted entry not re-minted: got %d, cached was %d", evicted.Timestamp, timestamps[1])
+	if cached.Timestamp != reminted.Timestamp || !bytes.Equal(cached.Signature, reminted.Signature) {
+		t.Fatal("latest entry not reused verbatim")
 	}
 }
 

pkg/p2p/libp2p/internal/handshake/metrics.go

@@ -81,7 +81,7 @@ func newMetrics() metrics {
 			Namespace: m.Namespace,
 			Subsystem: subsystem,
 			Name:      "address_minted_total",
-			Help:      "Number of session-stable signed addresses minted (signed-address cache misses). Should plateau at a small number per session; linear growth with handshakes indicates advertised-underlay churn.",
+			Help:      "Number of session-stable signed addresses minted. Plateaus per session; linear growth indicates advertised-underlay churn.",
 		}),
 		TimestampRejected: prometheus.NewCounterVec(
 			prometheus.CounterOpts{

pkg/p2p/libp2p/libp2p.go

@@ -773,11 +773,10 @@ func (s *Service) handleIncoming(stream network.Stream) {
 	s.logger.Debug("stream handler: successfully connected to peer (inbound)", "address", i.BzzAddress.Overlay, "light", i.LightString(), "user_agent", peerUserAgent)
 }
 
-// putHandshakeAddress persists a peer's BzzAddress from the handshake.
-// Timestamp and chequebook validation have already run in the handshake; here
-// we only need to perform the atomic registry-plus-addressbook write. The
-// addressbook write is skipped when the stored record is identical and
-// already verified, so steady-state reconnects cost no disk write.
+// putHandshakeAddress persists a peer's BzzAddress, already validated by the
+// handshake, as an atomic registry-plus-addressbook write. The addressbook
+// write is skipped when the stored record is identical and verified, so
+// steady-state reconnects cost no disk write.
 func (s *Service) putHandshakeAddress(addr *bzz.Address) error {
 	existing, verified, err := s.addressbook.Get(addr.Overlay)
 	if err != nil && !errors.Is(err, addressbook.ErrNotFound) {