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

[multistream-select] Listener conformity for failed negotiations. #1871

Merged
merged 4 commits into from
Dec 2, 2020

Conversation

romanb
Copy link
Contributor

@romanb romanb commented Dec 2, 2020

Context: paritytech/substrate#7478 (comment)

When V1Lazy is used and the listener does not support the optimistic (and singular) proposal of the dialer, it currently happens that dialer and listener get a different outcome of the negotiation. The dialer eventually detects the failed negotiation as soon as it tries to read from the stream, but the listener either encounters an invalid message or unexpected premature EOF, depending on the payload that the dialer sent prematurely after its protocol proposal. In these cases the listener must be lenient and fail the negotiation "normally", i.e. not with a protocol violation or an I/O error. The tests have been expanded to make sure both versions behave uniformly in this manner w.r.t. failed negotiations. Nevertheless it would be good to get final confirmation by testing paritytech/substrate#7478 with this branch. This qualifies for a patch release of multistream-select.

When `V1Lazy` is used and the listener does not support the
optimistic (and singular) proposal of the dialer, it currently
happens that dialer and listener get a different outcome of
the negotiation. The dialer eventually detects the failed
negotiation as soon as it tries to read from the stream, but
the listener either encounters an invalid message or unexpected
premature EOF, depending on the payload that the dialer sent
prematurely after its protocol proposal. In these cases the
listener must be lenient and fail the negotiation "normally",
i.e. not with a protocol violation or an I/O error.
@romanb romanb requested a review from mxinden December 2, 2020 12:03
Copy link
Member

@mxinden mxinden left a comment

Choose a reason for hiding this comment

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

Thanks for the patch!

Nevertheless it would be good to get final confirmation by testing paritytech/substrate#7478 with this branch.

Working on this right now.

misc/multistream-select/src/tests.rs Outdated Show resolved Hide resolved
misc/multistream-select/src/listener_select.rs Outdated Show resolved Hide resolved
@mxinden
Copy link
Member

mxinden commented Dec 2, 2020

Nevertheless it would be good to get final confirmation by testing paritytech/substrate#7478 with this branch.

The issue described in paritytech/substrate#7478 (comment) is resolved when using this branch.

romanb and others added 2 commits December 2, 2020 15:12
Only be lenient with garbage or sudden EOF when reading
just after having sent a protocol rejection.
misc/multistream-select/src/listener_select.rs Outdated Show resolved Hide resolved
@romanb
Copy link
Contributor Author

romanb commented Dec 2, 2020

I will publish this as multistream-select-0.9.1 with changelog updates upon merging.

@romanb romanb merged commit 8640231 into libp2p:master Dec 2, 2020
@romanb romanb deleted the multistream-listen-lazy-failed branch December 2, 2020 15:41
@romanb
Copy link
Contributor Author

romanb commented Dec 2, 2020

Published, also via libp2p-0.31.2 for convenience.

@mxinden
Copy link
Member

mxinden commented Dec 2, 2020

Published, also via libp2p-0.31.2 for convenience.

Thanks. I can do the update on Substrate later today. Will do this as a separate pull request given that paritytech/substrate#7478 might not be merged any time soon and given that others might run into this issue as well.

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.

2 participants