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/comms behaviour protocol #210

Merged
merged 43 commits into from
Aug 26, 2021
Merged

Feat/comms behaviour protocol #210

merged 43 commits into from
Aug 26, 2021

Conversation

elenaf9
Copy link
Contributor

@elenaf9 elenaf9 commented May 7, 2021

Description of change

Implementation of a custom Network Behaviour protocol for stronghold-communication.

Replace the currently used Network Behaviour with a custom protocol. The new protocol follows the basic concepts of the libp2p-request-response protocol, but additionally implements a firewall and directly integrates and leverages libp2p-relay and libp2p-mdns.

Type of change

  • Enhancement (a non-breaking change which adds functionality)
  • Documentation Fix

Change checklist

  • I have followed the contribution guidelines for this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

Start implementation of a new Network Behaviour protocol based on the
currently used libp2p RequestResponse protocol.
Refactor the event types that are used in the protocol.
Add the MDNS and relay network behaviours to the new network behaviour
protocol analogous to how the NetworkBehaviour macro implements it.
Add PeerConnectionManager to manage for peers the known addresses and pending requests on the current connections.
Return the response for an outbound request via channel instead of
emitting a Behaviour Event for it.
Bubble incoming requests up and emit as BehaviourEvent, with a channel
to respond. Do not emit events for ResponseSent or InboundFailures,
since there is no way to re-send a response, and it's the responsibility
of the remote to retry by re-sending the request.
Rename:
- struct MessageProtocol -> CommunicationProtocol
- struct RequestResponse -> NetBehaviour
- trait MessageEvent -> RqRsMessage
- struct RequestResponseHandler -> ConnectionHandler
Add Firewall configuration, and a mpsc channel to explicitly ask for
peer rules and approvals if necessary. Verify for each inbound and
outbound request that they are approved before forwarding them.
Add RequestManager to cache requests that are waiting for approval or a
connection to the remove peer, integrate the PeerConnectionsManager into
the RequestManager. Track all pending events with their current status
in the RequestManager, manage FIFO queue for required Network Actions.
Maintain a list of relay peers that are tried if a peer can not be
connected directly. Manage the list of known addresses for each peer,
their currently used endpoints (if connected), and whether a relay is
used.
@elenaf9 elenaf9 linked an issue May 24, 2021 that may be closed by this pull request
If for one peer a specific relay is set, but no address for that relay
is known, it should not fallback on using other relays.
Instead, an DialError will be returned.
Clean exports, format imports.
Add `RequestMessage` type for inbound/outbound Request-Response `Query`s.
Add `ShCommunication` as interface for the library to abstract from the
libp2p logic. Use one mpsc channel to forward incoming request, and one
for queries to the firewall.
Add a first basic test for the interface.
Optionally publish relevant SwarmEvents and Inbound / Outbound-Failures
from the interface through a mpsc channel.
Change the firewall test to use the interface.
Fix errors when disabling the mdns feature.
Allow graceful shutdown of the initially spawned thread in the
interface.
Report also the NetworkEvents that are produced by proactive
method calls.
Make behaviour module private.
Temporarily change the Github workflows to test
/communication-refactored instead of /communication.
Will be reverted once communication-refactored replaces the old
communication.
@mxinden
Copy link

mxinden commented Jun 11, 2021

Great to see usage of rust-libp2p in general and libp2p-relay in specific! I would be really curious to hear any feedback on libp2p-relay (and of course on the greater rust-libp2p project). Also note the effort around circuit relay v2 implementation.

@mxinden
Copy link

mxinden commented Jun 11, 2021

In case you are interested in hole punching for stronghold.rs, the following resources might be interesting:

Let me know in case you need more info. Happy to schedule a call or continue in some async fashion.

Add comments to all relevant methods.
Emit `ProtocolSupport` to Handler from `NetBehaviour`, instead of sending
the firewall rules.
Add smaller bugfixes.
@elenaf9
Copy link
Contributor Author

elenaf9 commented Jul 16, 2021

Thanks @mxinden!

Allow configuration of a custom executor to use in libp2p::Swarm and
for spawning the `SwarmTask`.

Change `start_listening` to always require a Multiaddress, instead of
optionally using an OS-assigned one. This is necessary because a
transport may be used where `/ip4/0.0.0.0/tcp/0` is not be supported.

Adjust tests to the changes.
@mxinden
Copy link

mxinden commented Jul 23, 2021

One note on the usage of Cargo features to enable/disable mdns and relay: Features are additive, thus in case this crate is imported twice with different feature sets, the union of the two will be used.

@elenaf9 elenaf9 force-pushed the feat/comms-behaviour-protocol branch from 8b88ac1 to 9f87e8d Compare July 27, 2021 13:25
Rename:
- Package `stronghold-communication` -> `stronghold-p2p`
- Interface `ShCommunication` -> `StrongholdP2p`
- Actor `CommunicationActor` -> `NetworkActor`
- `CommunicationProtocol` -> `MessageProtocol`
@elenaf9 elenaf9 force-pushed the feat/comms-behaviour-protocol branch from 9f87e8d to dc18c19 Compare July 27, 2021 22:02
Add `EventChannel`, which wraps the `mpsc::Receiver` of the channels that
are used by the `SwarmTask` to forward inbound requests and network
events.
Support different `ChannelSinkConfig`s in case that the channel is full
when a new event occurs:
- block until there is capacity
- drop new event
- buffer event, drop older buffered events if favor of new ones.
@elenaf9 elenaf9 force-pushed the feat/comms-behaviour-protocol branch 2 times, most recently from 5fe45cd to e055f22 Compare August 11, 2021 09:21
client/Cargo.toml Outdated Show resolved Hide resolved
Copy link
Contributor

@felsweg-iota felsweg-iota left a comment

Choose a reason for hiding this comment

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

That was a big PR and a good one. Good work! From my side the code is good so far.

client/src/interface.rs Show resolved Hide resolved
client/src/interface.rs Show resolved Hide resolved
client/src/interface.rs Outdated Show resolved Hide resolved
client/src/interface.rs Outdated Show resolved Hide resolved
p2p/src/behaviour.rs Outdated Show resolved Hide resolved
p2p/src/behaviour.rs Outdated Show resolved Hide resolved
p2p/src/behaviour.rs Show resolved Hide resolved
p2p/src/behaviour/firewall/permissions.rs Show resolved Hide resolved
p2p/src/behaviour/request_manager.rs Show resolved Hide resolved
Move NetworkActor from stronghold-p2p crate into iota-stronghold crate.
Add missing functionality to Network-Actor.
Integrate NetworkActor into Stronghold interface, to allow read/ write/
executing procedure on a remote Stronghold.
@elenaf9 elenaf9 force-pushed the feat/comms-behaviour-protocol branch from 6877836 to ceeeee0 Compare August 25, 2021 10:56
Copy link
Contributor

@lucas-tortora lucas-tortora left a comment

Choose a reason for hiding this comment

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

LGTM!

Ok(Ok(Err(e))) => ProcResult::Error(e.to_string()),
Ok(Err(e)) => ProcResult::Error(e.to_string()),
Err(e) => ProcResult::Error(e.to_string()),
let receive_response = unwrap_or_err!(actor.send(send_request).await);
Copy link
Contributor

Choose a reason for hiding this comment

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

nice!

Copy link
Contributor

@felsweg-iota felsweg-iota left a comment

Choose a reason for hiding this comment

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

good one.

@elenaf9 elenaf9 merged commit b1cb7b5 into dev Aug 26, 2021
@elenaf9 elenaf9 deleted the feat/comms-behaviour-protocol branch August 26, 2021 09:20
@elenaf9 elenaf9 mentioned this pull request Oct 29, 2021
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment