fix: Added multiselect type consistency in negotiate method #838

unniznd · 2025-08-12T16:21:53Z

What was wrong?

The multiselect type was inconsistent

How was it fixed?

Set the return type to None and handled the places where the negotiate function is used.

Reopening the PR #814 due to an error in the rebase.
Summary of approach.

To-Do

Clean up commit history
Add or update documentation related to these changes
Add entry to the release notes

Cute Animal Picture

…es to handle none. Added newsfragment.

acul71 · 2025-08-14T00:38:00Z

Hello @unniznd
I think the Run tox / tox (3.10, docs) fails because of file name:
837.fix.rst in check https://github.com/libp2p/py-libp2p/blob/main/newsfragments/validate_files.py

ALLOWED_EXTENSIONS = {
    ".breaking.rst",
    ".bugfix.rst",
    ".deprecation.rst",
    ".docs.rst",
    ".feature.rst",
    ".internal.rst",
    ".misc.rst",
    ".performance.rst",
    ".removal.rst",
}

unniznd · 2025-08-14T02:01:13Z

@acul71 I have fixed it.

acul71 · 2025-08-14T02:44:21Z

@acul71 I have fixed it.

Great.
If you run make pr and make docs # or make linux-docs everything pass ?

unniznd · 2025-08-14T02:56:21Z

Yes, everything passes.

seetadev · 2025-08-25T11:03:07Z

@unniznd : HI Aanand. Thank you for submitting the PR. Appreciate it.

Please resolve the merge conflicts. CCing @acul71 and @sumanjeet0012, who will help you arrive at a good conclusion on the issue.

…iate_type

unniznd · 2025-08-25T15:39:24Z

@seetadev I have resolved the conflicts.

seetadev · 2025-08-25T16:49:33Z

@unniznd : Sorry, 4 CI/CD issues still remains. No worries, take your time :) Appreciate the efforts.

unniznd · 2025-08-25T17:36:57Z

@seetadev Fixed the CI/CD issue. Can you review the PR and give some feedback?

acul71

Thanks @unniznd
Can you modify the text message something like as suggested ?
Can you add a test the validates the change (raise StreamFailure) ?
What happened to this:
https://github.com/libp2p/py-libp2p/pull/814/files/96ffba93719df1647c7c99f3cf6a6e17402f9e83#diff-123980bde25d7c36a2c507e8eda411bbff661cff3aa6c97382b465d632d888ee

https://github.com/libp2p/py-libp2p/pull/814/files/96ffba93719df1647c7c99f3cf6a6e17402f9e83#diff-76852da30f93a2a560d41f2969b847b0230b66db15f8e84aeb43e052555385df

acul71 · 2025-08-26T23:28:28Z

libp2p/host/basic_host.py

@@ -288,6 +288,9 @@ async def _swarm_stream_handler(self, net_stream: INetStream) -> None:
            protocol, handler = await self.multiselect.negotiate(
                MultiselectCommunicator(net_stream), self.negotiate_timeout
            )
+            if protocol is None:
+                await net_stream.reset()
+                raise StreamFailure("No protocol selected")


For error messages, can you be more verbose, something like:
raise StreamFailure("Failed to negotiate protocol: no protocol selected")

unniznd · 2025-08-27T16:38:46Z

@acul71 I have added a testcase for this. Can you review the code and give some feedback? Should I update all the error messages since other messages were done by someone else in a different PR? I closed the other PR since I unfortunately rebased the branch, and other commits made in the main branch were applied to it. So I thought to close and create a new PR that will be cleaner

acul71 · 2025-08-28T21:31:03Z

Can you review the code and give some feedback? Should I update all the error messages since other messages were done by someone else in a different PR

Yes please. I think to remember I've see another couple of error messages to fix.

acul71 · 2025-08-28T21:33:12Z

Can you review the code and give some feedback

Thanks @unniznd the PR is well done.
If you can fix the other error messages I'll approve it and ask for merging.

seetadev · 2025-08-28T21:56:59Z

@unniznd : Wonderful contribution. We are pretty near to having the PR fully ready for final review.

@acul71 : Thank you so much Luca. Appreciate your great support and guidance on this PR.

unniznd · 2025-08-29T06:02:02Z

@acul71 @seetadev I have improved the error messages. Appreciate the feedback.

acul71 · 2025-08-29T09:51:07Z

@seetadev @pacrob
Thanks, @unniznd , I've reviewed and is ready to be merged.

Quick Overview

Fixed type consistency in Multiselect.negotiate() method by updating return type from tuple[TProtocol, ...] to tuple[TProtocol | None, ...] to properly reflect that it can return None.

Key Changes

Type Annotation: -> tuple[TProtocol | None, StreamHandlerFn | None]
Error Handling: Added proper StreamFailure handling in basic_host.py
Tests: Added 5 new test cases covering None protocol scenarios

Impact

✅ No breaking changes
✅ All existing code continues to work
✅ Improved type safety
✅ Better error messages

Files Changed

libp2p/protocol_muxer/multiselect.py - Core type fix
libp2p/host/basic_host.py - Error handling
tests/core/protocol_muxer/test_protocol_muxer.py - 4 new tests
tests/core/host/test_basic_host.py - 1 new test

Recommendation

✅ APPROVE - Clean, safe, well-tested fix that improves type consistency.

pacrob · 2025-08-29T18:58:38Z

libp2p/security/security_multistream.py

@@ -118,6 +118,8 @@ async def select_transport(
            # Select protocol if non-initiator
            protocol, _ = await self.multiselect.negotiate(communicator)
        if protocol is None:
-            raise MultiselectError("fail to negotiate a security protocol")
+            raise MultiselectError(
+                "fail to negotiate a security protocol: no protocl selected"


Suggested change

"fail to negotiate a security protocol: no protocl selected"

"Failed to negotiate a security protocol: no protocol selected"

pacrob · 2025-08-29T18:58:54Z

libp2p/stream_muxer/muxer_multistream.py

@@ -85,7 +85,9 @@ async def select_transport(self, conn: IRawConnection) -> TMuxerClass:
        else:
            protocol, _ = await self.multiselect.negotiate(communicator)
        if protocol is None:
-            raise MultiselectError("fail to negotiate a stream muxer protocol")
+            raise MultiselectError(
+                "fail to negotiate a stream muxer protocol: no protocol selected"


Suggested change

"fail to negotiate a stream muxer protocol: no protocol selected"

"Failed to negotiate a stream muxer protocol: no protocol selected"

pacrob · 2025-08-29T19:00:42Z

Overall good, just a couple places to update to keep messaging consistent.

seetadev · 2025-08-30T11:45:59Z

@unniznd : Great work, Aanand. Wish if you could address the feedback points shared by @pacrob.

Overall the PR is in good shape. Kindly share a detailed blog (you can open up a discussion page for the same) on how you did this PR once you resolve the above issues.

Since this was your initial PRs in py-libp2p, which is getting merged, we would like you to share your experiences from a technical perspective in detail. This would also help the new contributors to take up projects in py-libp2p and arrive at a good conclusion on them. Keep up the wonderful progress.

unniznd · 2025-09-01T04:18:51Z

@pacrob, I have made the changes.

seetadev · 2025-09-01T19:40:41Z

@unniznd : Great work, Aanand. This PR is indeed ready for final review + merge.

Congratulations on the wonderful progress. Keep it up :) Looking forward to sharing more detailed projects with you this week.

@acul71 and @pacrob : Thank you so much for your great support and mentorship. Appreciate it.

seetadev · 2025-09-01T20:07:53Z

@acul71 : Looks like an unrelated test case ( FAILED tests/core/pubsub/test_gossipsub_px_and_backoff.py) is failing the CI/CD checks. We can fix this in another issue.

@unniznd can make the requisite changes.

added none type to return value of negotiate and changed caller handl…

b838a0e

…es to handle none. Added newsfragment.

fixed newsfragment filename issue.

1ecff54

Merge remote-tracking branch 'origin/main' into fix_multiselect_negot…

cda50e0

…iate_type

removed redundant imports

6214697

acul71 suggested changes Aug 26, 2025

View reviewed changes

unniznd added 2 commits August 27, 2025 21:54

improve error message in basic host

c08007f

added the testcase for StreamFailure

9f80dba

acul71 approved these changes Aug 28, 2025

View reviewed changes

Merge branch 'main' into fix_multiselect_negotiate_type

426aae7

improved the error message

3c52b85

acul71 approved these changes Aug 29, 2025

View reviewed changes

pacrob reviewed Aug 29, 2025

View reviewed changes

message inconsistency fixed

7d6eb28

Merge branch 'main' into fix_multiselect_negotiate_type

fc6b290

seetadev added 2 commits September 2, 2025 01:40

Merge branch 'main' into fix_multiselect_negotiate_type

934f49a

Merge branch 'main' into fix_multiselect_negotiate_type

9df542f

	"fail to negotiate a security protocol: no protocl selected"
	"Failed to negotiate a security protocol: no protocol selected"

	"fail to negotiate a stream muxer protocol: no protocol selected"
	"Failed to negotiate a stream muxer protocol: no protocol selected"

fix: Added multiselect type consistency in negotiate method #838

Are you sure you want to change the base?

fix: Added multiselect type consistency in negotiate method #838

Conversation

unniznd commented Aug 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What was wrong?

How was it fixed?

To-Do

Cute Animal Picture

Uh oh!

acul71 commented Aug 14, 2025

Uh oh!

unniznd commented Aug 14, 2025

Uh oh!

acul71 commented Aug 14, 2025

Uh oh!

unniznd commented Aug 14, 2025

Uh oh!

seetadev commented Aug 25, 2025

Uh oh!

unniznd commented Aug 25, 2025

Uh oh!

seetadev commented Aug 25, 2025

Uh oh!

unniznd commented Aug 25, 2025

Uh oh!

acul71 left a comment

Choose a reason for hiding this comment

Uh oh!

acul71 Aug 26, 2025

Choose a reason for hiding this comment

Uh oh!

unniznd commented Aug 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

acul71 commented Aug 28, 2025

Uh oh!

acul71 commented Aug 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

seetadev commented Aug 28, 2025

Uh oh!

unniznd commented Aug 29, 2025

Uh oh!

acul71 commented Aug 29, 2025

Quick Overview

Key Changes

Impact

Files Changed

Recommendation

Uh oh!

pacrob Aug 29, 2025

Choose a reason for hiding this comment

Uh oh!

pacrob Aug 29, 2025

Choose a reason for hiding this comment

Uh oh!

pacrob commented Aug 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

seetadev commented Aug 30, 2025

Uh oh!

unniznd commented Sep 1, 2025

Uh oh!

seetadev commented Sep 1, 2025

Uh oh!

seetadev commented Sep 1, 2025

Uh oh!

Uh oh!

unniznd commented Aug 12, 2025 •

edited

Loading

unniznd commented Aug 27, 2025 •

edited

Loading

acul71 commented Aug 28, 2025 •

edited

Loading

pacrob commented Aug 29, 2025 •

edited

Loading