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.

Sign up for GitHub

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

Jump to bottom

multistream: Dialer deny multiple /multistream/1.0.0 headers #61

Merged
merged 12 commits into from
Apr 24, 2024
Merged

multistream: Dialer deny multiple /multistream/1.0.0 headers #61

merged 12 commits into from
Apr 24, 2024

Conversation

lexnv
Copy link
Collaborator

@lexnv lexnv commented Mar 18, 2024

This PR ensures that the /multistream/1.0.0 header can only be received once during a protocol negotiation.
This is fixed for both the V1 and V1Lazy variations.

While at it, have added a few tests to check:

Closes: #59

cc @paritytech/networking

@lexnv
multistream/dialer: Await header line only once
a61e374 
Signed-off-by: Alexandru Vasile <[email protected]>
@lexnv
multistream/tests: Check protocol negotiation
a4f50d4 
Signed-off-by: Alexandru Vasile <[email protected]>
@lexnv
multistream/tests: Check negotiation fails
32b7284 
Signed-off-by: Alexandru Vasile <[email protected]>
@lexnv
multistream/tests: Check V1 lazy negotiation awaits completion
2d5a381 
Signed-off-by: Alexandru Vasile <[email protected]>
@lexnv
multistream/tests: Low level protocol negotiation
a1083f1 
Signed-off-by: Alexandru Vasile <[email protected]>
@lexnv
multistream/negotiated: Ensure lazy stream checks for duplicate headers
4436a69 
Signed-off-by: Alexandru Vasile <[email protected]>
@lexnv
multistream/tests: Fail on multiple headers
aed31d8 
Signed-off-by: Alexandru Vasile <[email protected]>
@lexnv
mutlistream/tests: Ensure panics are propagated for spawned tasks
c422f58 
Signed-off-by: Alexandru Vasile <[email protected]>
@lexnv lexnv self-assigned this Mar 18, 2024
@dmitry-markin
Copy link
Collaborator

@lexnv We have a pending PR not yet merged that fixes WebRTC: #51.

I'd double check that we don't have conflicting changes, probably merging that one first.

@lexnv
Copy link
Collaborator Author

lexnv commented Mar 18, 2024

That makes sense! Thanks for the info 🙏

I think would proly be good to have the CI up and running before we merge any other PRs (I've also put #57 on hold until we get more confidence in our testing)

After having a quick look over the WebRTC PR, it seems that the changes will not conflict:

  • as the main change is done to DialerState and adding tests for DialerState (prob need to double check names)
  • and it does not touch the negotiating part of the code

Yep, I think would be good to get that PR in first to ensure we don't break anything with these changes

dmitry-markin
dmitry-markin approved these changes Mar 19, 2024
View reviewed changes
Copy link
Collaborator

@dmitry-markin dmitry-markin left a comment

Choose a reason for hiding this comment

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

LGTM! Nice use of futures_ringbuf in tests, didn't know about it.

src/multistream_select/dialer_select.rs Show resolved Hide resolved
src/multistream_select/dialer_select.rs Show resolved Hide resolved
@lexnv lexnv mentioned this pull request Apr 22, 2024
Merged
1 task
@lexnv lexnv merged commit 2f5cfbc into master Apr 24, 2024
@lexnv lexnv deleted the lexnv/awaitheader-once branch April 24, 2024 12:40
@lexnv lexnv mentioned this pull request May 24, 2024
Merged
lexnv added a commit that referenced this pull request May 24, 2024
@lexnv
chore: Release v0.5.0 (#118)
0364bf5 
## [0.5.0] - 2023-05-24

This is a small patch release that makes the `FindNode` command a bit
more robst:

- The `FindNode` command now retains the K (replication factor) best
results.
- The `FindNode` command has been updated to handle errors and
unexpected states without panicking.

### Changed

- kad: Refactor FindNode query, keep K best results and add tests
([#114](#114))

## [0.4.0] - 2023-05-23

This release introduces breaking changes to the litep2p crate, primarily
affecting the `kad` module. Key updates include:

- The `GetRecord` command now exposes all peer records, not just the
latest one.
- A new `RecordType` has been introduced to clearly distinguish between
locally stored records and those discovered from the network.

Significant refactoring has been done to enhance the efficiency and
accuracy of the `kad` module. The updates are as follows:

- The `GetRecord` command now exposes all peer records.
- The `GetRecord` command has been updated to handle errors and
unexpected states without panicking.

Additionally, we've improved code coverage in the `kad` module by adding
more tests.

### Added

- Add release checklist
([#115](#115))
- Re-export `multihash` & `multiaddr` types
([#79](#79))
- kad: Expose all peer records of `GET_VALUE` query
([#96](#96))

### Changed

- multistream_select: Remove unneeded changelog.md
([#116](#116))
- kad: Refactor `GetRecord` query and add tests
([#97](#97))
- kad/store: Set memory-store on an incoming record for PutRecordTo
([#88](#88))
- multistream: Dialer deny multiple /multistream/1.0.0 headers
([#61](#61))
- kad: Limit MemoryStore entries
([#78](#78))
- Refactor WebRTC code
([#51](#51))
- Revert "Bring `rustfmt.toml` in sync with polkadot-sdk (#71)"
([#74](#74))
- cargo: Update str0m from 0.4.1 to 0.5.1
([#95](#95))

### Fixed

- Fix clippy  ([#83](#83))
- crypto: Don't panic on unsupported key types
([#84](#84))

---------

Signed-off-by: Alexandru Vasile <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dmitry-markin dmitry-markin dmitry-markin approved these changes

Assignees

@lexnv lexnv

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

multistream: Dialer accepts multiple /multistream/1.0.0 headers
2 participants
@lexnv @dmitry-markin