Skip to content
This repository has been archived by the owner on Jun 26, 2023. It is now read-only.

feat!: change connection encryption interface to uint8arraylist #278

Merged
merged 2 commits into from
Aug 5, 2022

Conversation

mpetrunic
Copy link
Member

@mpetrunic mpetrunic commented Aug 4, 2022

replacing #275

BREAKING CHANGE: connection encryption interface now accepts and yields uint8arraylist instead of uint8array

@wemeetagain wemeetagain merged commit 1fa580c into master Aug 5, 2022
@wemeetagain wemeetagain deleted the feat/conn-uint8arraylist branch August 5, 2022 14:35
@achingbrain
Copy link
Member

achingbrain commented Aug 6, 2022

What's the change to@chainsafe/libp2p-noise that meant MultiaddrConnections had to start sourcing/sinking Uint8ArrayLists? From what I can see it's still Uint8Arrays in and out because the data goes through encryption libraries which only ever accept/return Uint8Arrays.

@mpetrunic
Copy link
Member Author

What's the change to@chainsafe/libp2p-noise that meant MultiaddrConnections had to start sourcing/sinking Uint8ArrayLists? From what I can see it's still Uint8Arrays in and out because the data goes through encryption libraries which only ever accept/return Uint8Arrays.

Its not yet updated in noise and secio, will do next week

@achingbrain
Copy link
Member

secio is long deprecated & is unsupported so there's no need to spend time on that. The other implementation that'll need updating is PlainText but noise should be done first as it doesn't depend on libp2p.

Looking at noise, there's some concatenation being done during the handshake that's now unnecessary since you can write a Uint8ArrayList directly into writeLP, but it's probably not a bottleneck because it only happens once at the start of the connection.

This pipeline is the hot code path but from what I can see because decryptStream yields values from handshake.decrypt, which calls chaCha20Poly1305Decrypt it's only ever going to yield Uint8Arrays so there's nothing to gain from using Uint8ArrayList.

What am I missing?

github-actions bot pushed a commit that referenced this pull request Aug 7, 2022
## [@libp2p/interface-connection-encrypter-v2.0.0](https://github.com/libp2p/js-libp2p-interfaces/compare/@libp2p/interface-connection-encrypter-v1.0.3...@libp2p/interface-connection-encrypter-v2.0.0) (2022-08-07)

### ⚠ BREAKING CHANGES

* change connection encryption interface to uint8arraylist (#278)

### Features

* change connection encryption interface to uint8arraylist ([#278](#278)) ([1fa580c](1fa580c))

### Trivial Changes

* update project config ([#271](#271)) ([59c0bf5](59c0bf5))
@github-actions
Copy link

github-actions bot commented Aug 7, 2022

🎉 This PR is included in version @libp2p/interface-connection-encrypter-v2.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

github-actions bot pushed a commit that referenced this pull request Aug 7, 2022
## [@libp2p/interface-connection-v3.0.0](https://github.com/libp2p/js-libp2p-interfaces/compare/@libp2p/interface-connection-v2.1.1...@libp2p/interface-connection-v3.0.0) (2022-08-07)

### ⚠ BREAKING CHANGES

* change stream muxer interface (#279)
* change connection encryption interface to uint8arraylist (#278)

### Features

* change connection encryption interface to uint8arraylist ([#278](#278)) ([1fa580c](1fa580c))
* change stream muxer interface ([#279](#279)) ([1ebe269](1ebe269))

### Trivial Changes

* update project config ([#271](#271)) ([59c0bf5](59c0bf5))
@github-actions
Copy link

github-actions bot commented Aug 7, 2022

🎉 This PR is included in version @libp2p/interface-connection-v3.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

github-actions bot pushed a commit that referenced this pull request Aug 7, 2022
## [@libp2p/interface-connection-encrypter-compliance-tests-v2.0.0](https://github.com/libp2p/js-libp2p-interfaces/compare/@libp2p/interface-connection-encrypter-compliance-tests-v1.0.2...@libp2p/interface-connection-encrypter-compliance-tests-v2.0.0) (2022-08-07)

### ⚠ BREAKING CHANGES

* change connection encryption interface to uint8arraylist (#278)

### Features

* change connection encryption interface to uint8arraylist ([#278](#278)) ([1fa580c](1fa580c))

### Trivial Changes

* update project config ([#271](#271)) ([59c0bf5](59c0bf5))

### Dependencies

* update sibling dependencies ([f859920](f859920))
* update sibling dependencies ([93a89b1](93a89b1))
@github-actions
Copy link

github-actions bot commented Aug 7, 2022

🎉 This PR is included in version @libp2p/interface-connection-encrypter-compliance-tests-v2.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

github-actions bot pushed a commit that referenced this pull request Aug 7, 2022
## [@libp2p/interface-mocks-v4.0.0](https://github.com/libp2p/js-libp2p-interfaces/compare/@libp2p/interface-mocks-v3.0.3...@libp2p/interface-mocks-v4.0.0) (2022-08-07)

### ⚠ BREAKING CHANGES

* change stream muxer interface (#279)
* change connection encryption interface to uint8arraylist (#278)

### Features

* change connection encryption interface to uint8arraylist ([#278](#278)) ([1fa580c](1fa580c))
* change stream muxer interface ([#279](#279)) ([1ebe269](1ebe269))

### Dependencies

* update sibling dependencies ([f75e927](f75e927))
* update sibling dependencies ([d98a5ea](d98a5ea))
* update sibling dependencies ([f859920](f859920))
* update sibling dependencies ([93a89b1](93a89b1))
@github-actions
Copy link

github-actions bot commented Aug 7, 2022

🎉 This PR is included in version @libp2p/interface-mocks-v4.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@mpetrunic
Copy link
Member Author

This pipeline is the hot code path but from what I can see because decryptStream yields values from handshake.decrypt, which calls chaCha20Poly1305Decrypt it's only ever going to yield Uint8Arrays so there's nothing to gain from using Uint8ArrayList.

What am I missing?

I think you are right. I don't think noise cares if it yields or returns Uint8Array or Uint8ArrayList. But ain't we gonna have type issues if mplex or yamux is trying to pipe Uint8ArrayList into Secured connection that accepts Uint8Arrays only?

@achingbrain
Copy link
Member

No, because everything goes through it-length-prefixed which only emits Uint8Arrays when encoding.

@mpetrunic
Copy link
Member Author

No, because everything goes through it-length-prefixed which only emits Uint8Arrays when encoding.

ugh, should we revert this change and release then?

@achingbrain
Copy link
Member

Yes & I think the generics can be removed from MultiaddrConnection - e.g.

export interface MultiaddrConnection<T extends Uint8Array | Uint8ArrayList = Uint8Array> extends Duplex<T> {

can go back to

export interface MultiaddrConnection extends Duplex<Uint8Array> {

@mpetrunic mpetrunic restored the feat/conn-uint8arraylist branch August 9, 2022 09:54
@mpetrunic mpetrunic deleted the feat/conn-uint8arraylist branch August 9, 2022 09:54
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants