Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

identify: fix observed address handling #2825

Merged
merged 1 commit into from
Jun 8, 2024

Conversation

ivan4th
Copy link
Contributor

@ivan4th ivan4th commented Jun 7, 2024

If a connection was closed before receiving an identify response, the corresponding observed address could be wrongly removed as the connection counter for the local address was being decremented on each disconnect even if no messages were consumed by IDService previously.

The issues began after #2793

Patch by @sukunrt

Copy link
Member

@sukunrt sukunrt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops! sorry about that!

I believe this smaller patch will fix the issue.

diff --git a/p2p/protocol/identify/obsaddr.go b/p2p/protocol/identify/obsaddr.go
index 4437c4b0..fc1c100c 100644
--- a/p2p/protocol/identify/obsaddr.go
+++ b/p2p/protocol/identify/obsaddr.go
@@ -452,6 +452,12 @@ func (o *ObservedAddrManager) removeConn(conn connMultiaddrs) {
 	o.mu.Lock()
 	defer o.mu.Unlock()
 
+	observedTWAddr, ok := o.connObservedTWAddrs[conn]
+	if !ok {
+		return
+	}
+	delete(o.connObservedTWAddrs, conn)
+
 	// normalize before obtaining the thinWaist so that we are always dealing
 	// with the normalized form of the address
 	localTW, err := thinWaistForm(o.normalize(conn.LocalMultiaddr()))
@@ -467,11 +473,6 @@ func (o *ObservedAddrManager) removeConn(conn connMultiaddrs) {
 		delete(o.localAddrs, string(localTW.Addr.Bytes()))
 	}
 
-	observedTWAddr, ok := o.connObservedTWAddrs[conn]
-	if !ok {
-		return
-	}
-	delete(o.connObservedTWAddrs, conn)
 	observer, err := getObserver(conn.RemoteMultiaddr())
 	if err != nil {
 		return

If a connection was closed before receiving an identify response, the
corresponding observed address could be wrongly removed.

Patch by @sukunrt
@ivan4th
Copy link
Contributor Author

ivan4th commented Jun 7, 2024

@sukunrt yes, that also fixes the problem, thanks!

Updated the PR

Copy link
Member

@sukunrt sukunrt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @ivan4th

@MarcoPolo
Copy link
Collaborator

@ivan4th would a patch release containing this fix help you?

@ivan4th
Copy link
Contributor Author

ivan4th commented Jun 7, 2024

@MarcoPolo yes, that would help indeed, thank you!

@sukunrt sukunrt merged commit 172e1d6 into libp2p:master Jun 8, 2024
9 of 11 checks passed
ivan4th added a commit to ivan4th/go-libp2p that referenced this pull request Jun 8, 2024
@ivan4th
Copy link
Contributor Author

ivan4th commented Jun 8, 2024

Oops, just noticed that when replacing my patch, I also reverted the testing code. Added it back in #2828

ivan4th added a commit to ivan4th/go-libp2p that referenced this pull request Jun 8, 2024
This modifies TestObservedAddrManager to verify the fix in libp2p#2825
sukunrt pushed a commit that referenced this pull request Jun 9, 2024
This modifies TestObservedAddrManager to verify the fix in #2825
sukunrt pushed a commit that referenced this pull request Jun 12, 2024
This modifies TestObservedAddrManager to verify the fix in #2825
sukunrt pushed a commit that referenced this pull request Jun 12, 2024
This modifies TestObservedAddrManager to verify the fix in #2825
MarcoPolo added a commit that referenced this pull request Jun 12, 2024
* identify: Don't filter addr if remote is neither public nor private (#2820)

Updates the filterAddrs logic to no-op if the address is neither public nor private.

This fixes an issue in mocknet that assigns each node an address in the IPv6 discard prefix space. That doesn't interact well with this logic in identify.

The issue mocknet hits is that it filters out all received listen addresses and then doesn't remember any address for the peer.

* identify: fix bug in observed address handling (#2825)

* identify: add test for observed address handling (#2828)

This modifies TestObservedAddrManager to verify the fix in #2825

* libp2phttp: workaround for ResponseWriter's CloseNotifier (#2821)

* libp2phttp: workaround for CloseNotifier

* Add lintignore

* circuitv2: improve voucher validation (#2826)

* webrtc: fix ufrag prefix for dialing (#2832)

* webrtc: add a test for establishing many connections (#2801)

Update pion/ice to include the fix for out of order 
ConnectionState update callbacks

* release v0.35.1

---------

Co-authored-by: Marco Munizaga <git@marcopolo.io>
Co-authored-by: Ivan Shvedunov <ivan4th@users.noreply.github.com>
MarcoPolo pushed a commit that referenced this pull request Jun 13, 2024
This modifies TestObservedAddrManager to verify the fix in #2825
MarcoPolo pushed a commit that referenced this pull request Jun 13, 2024
This modifies TestObservedAddrManager to verify the fix in #2825
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants