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

feat: connection gater #1142

Merged
merged 15 commits into from
Jan 25, 2022
Merged

feat: connection gater #1142

merged 15 commits into from
Jan 25, 2022

Conversation

achingbrain
Copy link
Member

Port of https://github.com/libp2p/go-libp2p-core/blob/master/connmgr/gater.go

Fixes: #175
Refs: #744
Refs: #769

Follows on from #987 - that PR does not allow maintainer commits so I've created a new one.

@achingbrain
Copy link
Member Author

I know this is a port of the related go-libp2p functionality and that's where the nomenclature comes from, but I would much rather test against positive action than negative action or ambiguous sentiment.

interceptPeerDial to me does not say "return false to allow dialling a peer", it says 'do something with the peer dial'. Instead I feel something like allowPeerDial which returns true to allow dialling a peer would be much simpler to reason about.

So, all of these would return true to allow the operation to continue:

interceptPeerDial -> allowDialPeer
interceptAddrDial -> allowDialMultiaddr
interceptAccept -> acceptConnection
interceptSecured -> acceptSecuredConnection
interceptUpgraded -> acceptUpgradedConnection

I also see that we call interceptAddrDial before adding a multiaddr for a peer to the addressbook - this is not doing what the method says it is doing. Suggest adding a allowStoreMultiaddrForPeer method or similar.

What do you think @wemeetagain @greenSnot ?

@wemeetagain
Copy link
Member

@achingbrain

I agree with your renaming thoughts.

Same with allowStoreMultiaddrForPeer. Although, this one feels strange, like the current implementation is incomplete or should be rethought. The flag only currently has effect in a single place in the dialer, and the multiaddr can still be added via peerStore.addressBook.add(peer, multiaddr), and this occurs in several other places.

@achingbrain
Copy link
Member Author

Good point, that filtering might be better done in the address book itself rather than just in the dialler.

Copy link
Member

@wemeetagain wemeetagain left a comment

Choose a reason for hiding this comment

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

lgtm 👍

@achingbrain achingbrain merged commit ff32eba into master Jan 25, 2022
@achingbrain achingbrain deleted the feat/connection-gater branch January 25, 2022 16:27
achingbrain pushed a commit that referenced this pull request Jan 25, 2022
## [0.36.0](https://www.github.com/libp2p/js-libp2p/compare/v0.35.8...v0.36.0) (2022-01-25)

### ⚠ BREAKING CHANGES

* abort-controller dep is gone from dependency tree
* `libp2p.handle`, `libp2p.registrar.register` and the peerstore methods have become async

### Features

* add fetch protocol ([#1036](https://www.github.com/libp2p/js-libp2p/issues/1036)) ([d8ceb0b](https://www.github.com/libp2p/js-libp2p/commit/d8ceb0bc66fe225d1335d3f05b9a3a30983c2a57))
* async peerstore backed by datastores ([#1058](https://www.github.com/libp2p/js-libp2p/issues/1058)) ([978eb36](https://www.github.com/libp2p/js-libp2p/commit/978eb3676fad5d5d50ddb28d1a7868f448cbb20b))
* connection gater ([#1142](https://www.github.com/libp2p/js-libp2p/issues/1142)) ([ff32eba](https://www.github.com/libp2p/js-libp2p/commit/ff32eba6a0fa222af1a7a46775d5e0346ad6ebdf))


### Bug Fixes

* cache build artefacts ([#1091](https://www.github.com/libp2p/js-libp2p/issues/1091)) ([5043cd5](https://www.github.com/libp2p/js-libp2p/commit/5043cd56435a264e83db4fb8388d33e9a0442fff))
* catch errors during identify ([#1138](https://www.github.com/libp2p/js-libp2p/issues/1138)) ([12f1bb0](https://www.github.com/libp2p/js-libp2p/commit/12f1bb0aeec4b639bd2af05807215f3b4284e379))
* import uint8arrays package in example ([#1083](https://www.github.com/libp2p/js-libp2p/issues/1083)) ([c3700f5](https://www.github.com/libp2p/js-libp2p/commit/c3700f55d5a0b62182d683ca37258887b24065b9))
* make tests more reliable ([#1139](https://www.github.com/libp2p/js-libp2p/issues/1139)) ([b7e8706](https://www.github.com/libp2p/js-libp2p/commit/b7e87066a69970f1adca4ba552c7fdf624916a7e))
* prevent auto-dialer from dialing self ([#1104](https://www.github.com/libp2p/js-libp2p/issues/1104)) ([9b22c6e](https://www.github.com/libp2p/js-libp2p/commit/9b22c6e2f987a20c6639cd07f31fe9c824e24923))
* remove abort-controller dep ([#1095](https://www.github.com/libp2p/js-libp2p/issues/1095)) ([0a4dc54](https://www.github.com/libp2p/js-libp2p/commit/0a4dc54d084c901df47cce1788bd5922090ee037))
* try all peer addresses when dialing a relay ([#1140](https://www.github.com/libp2p/js-libp2p/issues/1140)) ([63aa480](https://www.github.com/libp2p/js-libp2p/commit/63aa480800974515f44d3b7e013da9c8ccaae8ad))
* update any-signal and timeout-abort-controller ([#1128](https://www.github.com/libp2p/js-libp2p/issues/1128)) ([e0354b4](https://www.github.com/libp2p/js-libp2p/commit/e0354b4c6b95bb90656b868849182eb3efddf096))
* update multistream select ([#1136](https://www.github.com/libp2p/js-libp2p/issues/1136)) ([00e4959](https://www.github.com/libp2p/js-libp2p/commit/00e49592a356e39b20c889d5f40b9bb37d4bf293))
* update node-forge ([#1133](https://www.github.com/libp2p/js-libp2p/issues/1133)) ([a4bba35](https://www.github.com/libp2p/js-libp2p/commit/a4bba35948e1cd8dbe5147f2c8d6385b1fbb6fae))
---

This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@greenSnot
Copy link
Contributor

lgtm, hooray~

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.

[feature request]: Low-Level Connection Management
3 participants