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

New core #568

Merged
merged 5 commits into from Oct 17, 2018
Merged

New core #568

merged 5 commits into from Oct 17, 2018

Conversation

tomaka
Copy link
Member

@tomaka tomaka commented Oct 16, 2018

  • Removes the MultiaddrFuture system from both the Transport and ConnectionUpgrade traits.
  • Removes the MuxedTransport trait.
  • Renames Swarm to RawSwarm. The name Swarm should be for something higher level.
  • The poll() methods that can't fail and can't end now return directly Async<Item> instead of Poll<Option<Item>, Error>.
  • The HandledNodesTasks and CollectionStream now return back the handler if they failed to reach a node. This makes it possible for the RawSwarm to reuse the same (yet unused) handler for multiple attempts.
  • Removes the HandlerFactory trait. When you use the RawSwarm you must now manually pass a handler when you dial and on incoming connection.
  • The public key mismatch situation is now handled by making the dialing future fail, instead of letting it succeed then closing the connection if necessary. This is because otherwise it would be possible to call peer(peer_id).or_connect(handler) and end up having handler handle a different peer ID, which is confusing and a possible source of bugs.
  • Adds a missing loop in the TCP transport.

@tomaka
Copy link
Member Author

tomaka commented Oct 16, 2018

One big issue is that this breaks floodsub, as it is no longer capable of knowing which address we're talking to.
I'm considering removing floodsub entirely and restoring it in a different PR.

Copy link
Contributor

@twittner twittner left a comment

Choose a reason for hiding this comment

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

Great simplification!
Some files have trailing whitespace.

Ok(Async::Ready((EitherOutput::Second(item), future::Either::B(addr))))
}
&mut EitherFuture::First(ref mut a) => a.poll().map(|v| v.map(EitherOutput::First)),
&mut EitherFuture::Second(ref mut a) => a.poll().map(|v| v.map(EitherOutput::Second)),
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we can remove ref mut and &mut now that rustc (>= 1.26) can handle this in match expressions?

type Listener: Stream<Item = Self::ListenerUpgrade, Error = IoError>;

/// Future that produces the multiaddress of the remote.
type MultiaddrFuture: Future<Item = Multiaddr, Error = IoError>;
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

Copy link
Contributor

Choose a reason for hiding this comment

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

What's the rationale for removing MultiaddrFuture, considering that doing so breaks floodsub?

Copy link
Contributor

Choose a reason for hiding this comment

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

Nevermind, I found #487.

}

// TODO: remove unneeded bounds
struct IdRetreiver<TMuxer>
Copy link
Contributor

Choose a reason for hiding this comment

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

Was id_transport.rs supposed to be part of this PR? Regardless you probably meant IdRetriever here.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll submit it separately.

// TODO: right now it is possible for a node handler to be built, then shut down right after if we
// realize we dialed the wrong peer for example ; this could be surprising and should either
// be documented or changed (favouring the "documented" right now)
pub trait NodeHandler<TSubstream> {
pub trait NodeHandler {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not keeping NodeHandler generic over the substream type?

Copy link
Member Author

Choose a reason for hiding this comment

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

If TSubstream is generic, that means you can implement the NodeHandler trait multiple times on the same struct with different types for TSubstream.

However the implementation of NodeHandler may need to store fields that depend on TSubstream and therefore it doesn't make sense to allow multiple trait implementations.

That's the theoretical explanation, in practice I've encountered several inference issues with TSubstream being generic.

Copy link
Contributor

@twittner twittner left a comment

Choose a reason for hiding this comment

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

I think there is still trailing whitespace in several files. Maybe it could be removed before merging.

@tomaka
Copy link
Member Author

tomaka commented Oct 16, 2018

I don't see what the problem is with trailing whitespaces. Also this PR doesn't seem to add trailing whitespaces, so it can be done separately if necessary.

@twittner
Copy link
Contributor

I don't see what the problem is with trailing whitespaces.

It gets in the way by showing up in commit diffs and by potentially disturbing editing (e.g. when jumping to the end of a line). In general it requires extra work to cope with it, diff tools which need to ignore whitespace changes, extra editor commands to jump to the last non-whitespace character of a line, etc. On the other hand, what do we gain by adding trailing whitespace? Nothing.

Also this PR doesn't seem to add trailing whitespaces

> git diff master --check
core/src/nodes/raw_swarm.rs:675: trailing whitespace.
+
core/src/nodes/raw_swarm.rs:1016: trailing whitespace.
+
core/src/transport/and_then.rs:82: trailing whitespace.
+

@tomaka
Copy link
Member Author

tomaka commented Oct 17, 2018

Ah you meant whitespaces after the last character of a line. Fixed.

@tomaka tomaka merged commit 5d1c54c into libp2p:master Oct 17, 2018
@tomaka tomaka deleted the new-core branch October 17, 2018 09:17
dvdplm added a commit to dvdplm/rust-libp2p that referenced this pull request Oct 17, 2018
@tomaka tomaka mentioned this pull request Oct 17, 2018
dvdplm added a commit to dvdplm/rust-libp2p that referenced this pull request Oct 17, 2018
…e-handled_node_tasks

* dp/chore/test-core-handled_node:
  Fix tests after recent changes on master
  New core (libp2p#568)
@jamesray1
Copy link
Contributor

jamesray1 commented Oct 19, 2018

One big issue is that this breaks floodsub, as it is no longer capable of knowing which address we're talking to. I'm considering removing floodsub entirely and restoring it in a different PR.

That's even more problematic when I'm extending floodsub with gossipsub in #521, and they both need to be in the same file.

@jamesray1
Copy link
Contributor

Now I'll need to go back to making floodsub compatible before I can continue working on gossipsub.

@jamesray1
Copy link
Contributor

Do you have any suggestions on how that may be done?

@tomaka
Copy link
Member Author

tomaka commented Oct 24, 2018

Do you have any suggestions on how that may be done?

That's unfortunately a hard question ; I'm currently waiting for a review on #573 then will probably update floodsub in a short time frame.

dvdplm added a commit to dvdplm/rust-libp2p that referenced this pull request Nov 1, 2018
…ref-debug-impl

* upstream/master:
  Use paritytech/rust-secp256k1 (libp2p#598)
  Use websocket 0.21.0 (libp2p#597)
  Reexport multihash from the facade (libp2p#587)
  Add substrate to the list of projects using libp2p (libp2p#595)
  Remove spaces before semicolons (libp2p#591)
  Add protocol to report external address view. (libp2p#566)
  Add a TransportExt trait (libp2p#533)
  libp2p#399 remove tokio_current_thread tests (libp2p#577)
  Remove even more unused files (libp2p#581)
  Fix the polling process in handled node (libp2p#582)
  Fix panicking when Kad responder is destroyed (libp2p#575)
  Remove other unused files (libp2p#570)
  Fix panic in raw swarm (libp2p#571)
  Remove two unused files (libp2p#567)
  New core (libp2p#568)
  Remove the old API (libp2p#565)
  Change some `nat_traversal`s to consider a prefix. (libp2p#550)
  Add some documentation to listeners stream (libp2p#547)
  Add shutdown functionality to `NodeStream`. (libp2p#560)
jxs added a commit to jxs/rust-libp2p that referenced this pull request Jan 30, 2024
* Fix panics?

* Update protocols/gossipsub/src/behaviour.rs

Co-authored-by: João Oliveira <hello@jxs.pt>

---------

Co-authored-by: João Oliveira <hello@jxs.pt>
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.

None yet

3 participants