fix(peerstore): replace stale addrs on newer signed peer record#3487
fix(peerstore): replace stale addrs on newer signed peer record#3487
Conversation
lidel
changed the title
Treat ConsumePeerRecord as replace-semantics rather than merge: when a peer publishes a newer signed peer record, addrs that were present in the previously stored signed record but omitted from the new one are evicted, so the peerstore reflects the peer's current self-advertised set instead of the accumulated union. Unsigned addrs (e.g. DHT gossip or identify exchanges without a signed record) are untouched, and addrs held by a live connection (TTL >= ConnectedAddrTTL) are kept so active sessions are not torn down. Both backends recover the prior addr set by decoding the stored envelope: pstoremem reuses the Envelope already kept on peerRecordState; pstoreds fetches it via GetPeerRecord and drops superseded addrs with deleteAddrs. Envelope.Record() caches on first access, so the diff is cheap. The shared CertifiedAddresses assertion is updated to match the new behavior.
lidel
force-pushed
the
feat/pstore-replace-signed-addrs
branch
from
7385b32 to
b8b5f55
Compare
| // Adding a new envelope should clear existing certified addresses. | ||
| // Only the newly-added ones should remain |
There was a problem hiding this comment.
ℹ️ intention was documented, but never implemented. this PR aims to implement it
this PR does not purge all addrs, only one matching previous signed record, but we could also pivot it to do full clean so only signed addrs remain
(not feeling strongly, whatever feels better)
There was a problem hiding this comment.
replacing it makes sense. The full purge is a bigger riskier change I think
| } | ||
| pr.RUnlock() | ||
|
|
||
| superseded := make([]ma.Multiaddr, 0, len(prevRec.Addrs)) |
There was a problem hiding this comment.
A very minor nit: if you return an iter.Seq you avoid this allocation.
There was a problem hiding this comment.
I'd lean toward keeping the slice here: caller gates on len(superseded) > 0 and deleteInPlace iterates the set per surviving entry, so iter.Seq would just push the alloc into deleteAddrs or force a signature change.. feels like not worth the noise.
2 participants
Note
This is another fix that aims to reduce stale addr accumulation (defensive, on Consumer side).
By looking at commented code, tt seems there was intention to do it in the past.
This PR
When we deal with signed peer records that have monotonic
Seq, treatConsumePeerRecordas replace-semantics rather than merge.When a peer publishes a newer signed peer record, addrs that were present in the previously stored signed record but omitted from the new one are evicted, so the peerstore reflects the peer's current self-advertised set instead of the accumulated union. Unsigned addrs (e.g. DHT gossip or identify exchanges without a signed record) are untouched, and addrs held by a live connection (TTL >= ConnectedAddrTTL) are kept so active sessions are not torn down.
Both backends recover the prior addr set by decoding the stored envelope:
pstorememreuses the Envelope already kept onpeerRecordState;pstoredsfetches it viaGetPeerRecordand drops superseded addrs withdeleteAddrs.Envelope.Record()caches on first access, so the diff is cheap. The sharedCertifiedAddressesassertion is updated to match the new behavior.