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

*: Upstream parity-multiaddr changes #40

Merged
merged 88 commits into from
May 26, 2021

Conversation

mxinden
Copy link
Member

@mxinden mxinden commented May 19, 2021

parity-multiaddr is a fork of this repository with various changes and additions. As part of libp2p/rust-libp2p#758 this pull request does the following in order to merge the two while preserving the git history of both repositories:

Main merge happens via 935a966. As far as I can tell all features of multiaddr and parity-multiaddr are preserved. Naming closely follows parity-multiaddr, thus crates depending on multiaddr (as far as I can tell none) will face breaking changes. In case of conflicts parity-multiaddr is favored over multiaddr given that the former is more up-to-date and has a larger user base. Version is fast-forwarded to v0.12.0 to not conflict with existing parity-multiaddr versions.

gnunicorn and others added 30 commits May 19, 2021 18:41
* Remove unused circular-buffer crate
* Move transports into subdirectory
* Move misc into subdirectory
* Move stores into subdirectory
* Move multiplexers
* Move protocols
* Move libp2p top layer
* Fix Test: skip doctest if secio isn't enabled
* Use `unsigned-varint` crate.

* Implement `Display` for `Protocol`.

Gives `ToString` for free.

* Use `Cow` in `AddrComponent`.

* Add `AddrComponent::acquire`.

* Document `AddrComponent::acquire`.
Refactor multiaddr crate.

- Remove `AddrComponent`. Instead `Protocol` directly contains its
associated data.

- Various smaller changes around conversions to Multiaddr from other
types, e.g. socket addresses.

- Expand tests to include property tests which test encoding/decoding
identity.
* multiaddr: add support for onion protocol.

* Comment `read_onion`.

* Split onion into address and port parts.
multiaddr: explain the use of `&str` for `Unix`.

Closes #408.
Append to the existing vector instead of allocating a temporary one and
copying bytes over.
Change some `nat_traversal`s to consider a prefix.

Transports should consider only the relevant address prefix.
* Update many dependencies

* Upgrade some secio deps
* Rename multiaddr and mulithash to parity-*

* Fix doctests
* update secio dependencies: ed25519-dalek, sha2, hmac
* Update websocket dependencies
* Update multiaddr dependencies
* Decode multiaddresses from visit_seq

* Serialize as slices
* Fix the multiaddr reexport

* Minor fixes
* Rewrite the MemoryTransport to be similar to the TcpConfig

* Add small test

* Test and bug fixes
* Change `Multiaddr` representation to `Bytes`.

* Mark several `Multiaddr` methods as deprecated.
The functionality is available through `Multiaddr::replace`.
What we currently call "nat_traversal" is merley a replacement of an IP
address prefix in a `Multiaddr`, hence it can be done directly on
`Multiaddr` values instead of having to go through a `Transport`.

In addition this PR consolidates changes made to `Multiaddr` in
previous commits which resulted in lots of deprecations. It adds some
more (see below for the complete list of API changes) and removes all
deprecated functionality, requiring a minor version bump.

Here are the changes to `multiaddr` compared to the currently published
version:

1.  Removed `into_bytes` (use `to_vec` instead).
2.  Renamed `to_bytes` to `to_vec`.
3.  Removed `from_bytes` (use the `TryFrom` impl instead).
4.  Added `with_capacity`.
5.  Added `len`.
6.  Removed `as_slice` (use `AsRef` impl instead).
7.  Removed `encapsulate` (use `push` or `with` instead).
8.  Removed `decapsulate` (use `pop` instead).
9.  Renamed `append` to `push`.
10. Added `with`.
11. Added `replace`.
12. Removed `ToMultiaddr` trait (use `TryFrom` instead).
The current implementation does not properly reserve enough space for
`Write::write_all` to succeed. The property test has been improved to
trigger that issue.
* Bump to 0.7.0

* Update CHANGELOG.md

Co-Authored-By: tomaka <pierre.krieger1708@gmail.com>

* Update for #1078

* New version of multihash and multiaddr as well
* Add parity_multiaddr::from_websockets_url

* Make from_websockets_url more genreic

* Oops, forgot the file

* Use the URL crate

* Add dns_and_port test

* Address review

* Add support for unix
* Allow a path for WS multiaddresses

* Fix tests

* Finish

* Tests

* Don't accept any path other than /
* Publish v0.9.0

* Update CHANGELOG.md

Co-Authored-By: Roman Borschel <romanb@users.noreply.github.com>

* Published soketto
* core/src/translation: Add unit tests

* core/src/translation: Support dns4 and dns6

Add dns4 and dns6 as valid protocol replacements for the origin address
to construct external addresses of a given node.

* core/nodes/network: %s/nat_translation/address_translation/

When receiving an observed address on a tcp connection that we initiated, the
observed address contains our tcp dial port, not our tcp listen port. We know
which port we are listening on, thereby we can replace the port within the
observed address.

When receiving an observed address on a tcp connection that we did **not**
initiated, the observed address should contain our listening port. In case it
differs from our listening port there might be a NAT along the path.

With the above in mind, the function name `nat_translation` is misleading.
Roman S. Borschel and others added 10 commits May 19, 2021 18:44
`futures-codec` has not been updated in the recent months. It still
depends on `bytes` `v0.5` preventing all downstream dependencies to
upgrade to `bytes` `v1.0`.

This commit replaces `futures_codec` in favor of `asynchronous-codec`
The latter is a fully upgraded fork of the former.

In addition this commit upgrades:

- bytes to v1
- unsigned-varint to v0.6.0
- prost to v0.7
Co-authored-by: Roman Borschel <romanb@users.noreply.github.com>
* Update unsigned-varint requirement from 0.6 to 0.7

Updates the requirements on [unsigned-varint](https://github.com/paritytech/unsigned-varint) to permit the latest version.
- [Release notes](https://github.com/paritytech/unsigned-varint/releases)
- [Changelog](https://github.com/paritytech/unsigned-varint/blob/master/CHANGELOG.md)
- [Commits](paritytech/unsigned-varint@v0.6.0...v0.7.0)

Signed-off-by: dependabot[bot] <support@github.com>

* *: Update to asynchronous-codec v0.6

* transports/plaintext: Use Framed::into_parts

* *: Update cargo tomls and changelogs

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Max Inden <mail@max-inden.de>
* Implement `/dnsaddr` support on `libp2p-dns`.

To that end, since resolving `/dnsaddr` addresses needs
"fully qualified" multiaddresses when dialing, i.e. those
that end with the `/p2p/...` protocol, we make sure that
dialing always uses such fully qualified addresses by
appending the `/p2p` protocol as necessary. As a side-effect,
this adds support for dialing peers via "fully qualified"
addresses, as an alternative to using a `PeerId` together
with a `Multiaddr` with or without the `/p2p` protocol.

* Adapt libp2p-relay.

* Update versions, changelogs and small cleanups.
@mxinden mxinden marked this pull request as ready for review May 20, 2021 15:41
@mxinden
Copy link
Member Author

mxinden commented May 20, 2021

@vmx @Stebalien does one of you have some time to give this pull request a review? I am sorry for the rather large change-set. Let me know in case you see a way to simplify the review work.

@Stebalien
Copy link
Member

In a few weeks.

Copy link
Member

@vmx vmx left a comment

Choose a reason for hiding this comment

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

As the parity-multiaddr changes were already reviewed, I only concentrated on differences and potential incompatibilities.

There are obvious many breaking changes coming from rust-multiaddr, but that's find and documenting them in depth might not be worth it as current users (if there are any) would need to change a lot. Time might be better spend helping people migrate if they run into issues.

One thing that might be worth mentioning in the changelog/commit message is that you cannot any longer convert from an integer to a Protocol and back, though you have constants to get the integer values of protocols.

Other that that I didn't find anything that hasn't a replacement/workaround in the updated version.

const UNIX: u32 = 400;
const UTP: u32 = 302;
const WS: u32 = 477;
const WS_WITH_PATH: u32 = 4770; // Note: not standard
Copy link
Member

Choose a reason for hiding this comment

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

(general note, not related to this PR) Would adding those "not standard" ones to the https://github.com/multiformats/multicodec/ make sense?

Copy link
Member Author

Choose a reason for hiding this comment

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

From what I know today, yes. Though I still need to dig deeper into the websocket addressing schema to know for sure.

/// platform-specific. This means that the actual validation of paths needs to
/// happen separately.
#[derive(PartialEq, Eq, Clone, Debug)]
pub enum Protocol<'a> {
Copy link
Member

Choose a reason for hiding this comment

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

It is not Copy any more, it might be worth mentioning in the changelog/commit message.

@mxinden
Copy link
Member Author

mxinden commented May 21, 2021

One thing that might be worth mentioning in the changelog/commit message is that you cannot any longer convert from an integer to a Protocol and back, though you have constants to get the integer values of protocols.

Addressed in 11d6fcf.

It is not Copy any more, it might be worth mentioning in the changelog/commit message.

Addressed in 11d6fcf.


@vmx thank you for the review. Would you mind taking another look?

Copy link
Member

@vmx vmx left a comment

Choose a reason for hiding this comment

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

Yay! So good to see changes being upstreamed into a single source of truth.

@mxinden
Copy link
Member Author

mxinden commented May 21, 2021

In a few weeks.

@Stebalien I am interpreting this as an offer, not as a request to pause this pull request till then. Let me know if you would prefer to pause this pull request until you find time to review, after all this effort is in no rush.


Unless anything comes up, I will merge this sometime next week and cut the v0.12.0 release. Thanks @vmx for the help.

@Stebalien
Copy link
Member

I am interpreting this as an offer, not as a request to pause this pull request till then.

Ah, no. It was an offer to ping me in a few weeks if this got stuck. Thanks @vmx!

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.