fix(handshake): reuse session-stable signed BzzAddress - #5493
Merged
Conversation
gacevicljubisa
requested review from
acud,
akrem-chabchoub,
janos,
martinconic and
sbackend123
and removed request for
janos
June 5, 2026 12:43
gacevicljubisa
force-pushed
the
fix/session-stable-bzzaddress
branch
from
June 5, 2026 13:10
de0507d to
3ac8098
Compare
Member
Author
janos
approved these changes
Jun 5, 2026
acud
approved these changes
Jun 5, 2026
| // misses the cache and re-signs with the new chequebook. | ||
| func (s *Service) SetChequebookAddress(addr common.Address) { | ||
| if (addr == common.Address{}) { | ||
| s.chequebookAddr.Store(nil) |
Contributor
There was a problem hiding this comment.
i wonder why we can't just save &addr here... why not save the default value?
mfw78
added a commit
to nxm-rs/vertex
that referenced
this pull request
Jun 8, 2026
`prepare_local_peer` signed our self `SwarmPeer` record with `Timestamp::now()` on every handshake (one ECDSA sign per connection), so an unchanged advertised address set produced a different signed record each time. Receivers store and re-gossip that record, so an advancing timestamp on otherwise-identical content churns the network for no semantic gain. This is the producer-side complement to #161 (which only rejects such churn on the receive side), and the root-cause version of the fix discussed in ethersphere/bee#5493. ## Changes - `HandshakeBehaviour` caches the signed self-record keyed by a fingerprint of the (deterministically ordered) advertised address set. While the set is unchanged it reuses a byte-identical record (same timestamp, same signature) across handshakes, so we re-sign only when the set changes. Computed at connection establishment, threaded to the handler/protocol. `LocalAddressManager` and the `AddressProvider` trait are untouched. - The timestamp now means "when the advertised set last changed or was last refreshed", not "now at each handshake". A `SELF_RECORD_REFRESH_INTERVAL` (1h) re-signs a stable node periodically so its record never ages out of a receiver's clock-skew window (`SwarmPeer::parse` rejects timestamps outside ~6h as `TimestampOutsideSkewWindow`); 1h is well under that window and above #161's 300s `too_soon` floor, so a refresh reads as a normal update. - The observed-address append is now last-resort only (empty set) for both inbound and outbound, so the common-case record is peer-independent and cacheable. The NAT-discovery role the inbound observed-append served is covered by AutoNAT v2 (#156) and advertising confirmed-external addresses (#160). ## Composition with #161 An unchanged record presents an equal timestamp, which #161 treats as `Stale` (a silent no-op), not `too_soon`. So this is what actually eliminates the churn #161 would otherwise just reject; the hourly refresh advances the timestamp by more than the 300s interval and is accepted as a normal update. ## Compatibility No wire-format change and no conformance vectors changed: the peer sign/parse interop vectors pass unchanged (the signed bytes are identical for fixed inputs aside from the now-stable timestamp). The residual hourly refresh is inherent to wall-clock-plus-skew-window records; eliminating it entirely (seq-style ordering without a freshness window) is a future SWIP/hardfork, not this PR. ## Testing Pure `needs_resign` unit tests (no-cache, same-fingerprint-within-interval, fingerprint-change, past-interval) and fingerprint stability/order-sensitivity; a behaviour-level test asserting two consecutive cached signs of an unchanged set return a byte-identical record (same timestamp and signature); `select_local_addrs` tests updated for last-resort-only semantics. handshake (28), peer + interop, and topology suites pass; clippy/fmt clean.
akrem-chabchoub
approved these changes
Jun 8, 2026
This was referenced Jun 11, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Checklist
Description
Every handshake created and signed a new BzzAddress with a fresh timestamp, so each record the node advertised differed from the previous one even when nothing changed.
Receiving nodes therefore treated every record as an update: each reconnect caused an addressbook write on the peer, and re-gossiped records with advancing timestamps caused periodic addressbook writes across the whole network.
bzz.CheckTimestamp), only the latest record is ever valid to present, so a single entry is sufficient — and bounded by design, regardless of what observed underlays peers report.putHandshakeAddressnow skips the addressbook write when the stored record is identical and already verified, so a reconnecting peer costs one cached read and zero disk writes; the chequebook registry mapping is still re-registered and unverified records are still upgraded.Open API Spec Version Changes (if applicable)
Motivation and Context (Optional)
Related Issue (Optional)
Closes https://github.com/ethersphere/bee-private/issues/23
Screenshots (if appropriate):
AI Disclosure