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.

Sign up for GitHub

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

Jump to bottom

Use upstream libp2p #820

Merged
merged 2 commits into from
Oct 14, 2021
Merged

Use upstream libp2p #820

merged 2 commits into from
Oct 14, 2021

Conversation

rishflab
Copy link
Member

@rishflab rishflab commented Oct 12, 2021 •
edited
Loading

todo: remove ping

@rishflab rishflab changed the title List seller test timeout Use upstream libp2p Oct 12, 2021
@rishflab
Copy link
Member Author

rishflab commented Oct 13, 2021 •
edited
Loading

the rendezvous unit test hangs locally. There were no failures in CI and im pretty sure it would of run. Investigating... may just be a one off failure

thomaseizinger
thomaseizinger suggested changes Oct 13, 2021
View reviewed changes
Copy link
Contributor

@thomaseizinger thomaseizinger left a comment

Choose a reason for hiding this comment

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

Thanks!

Couple of comments inline, I wouldn't worry about the test locally if it passes CI.

swap/Cargo.toml Outdated Show resolved Hide resolved
swap/src/cli.rs Outdated Show resolved Hide resolved
swap/src/cli.rs Outdated Show resolved Hide resolved
swap/src/cli/list_sellers.rs Outdated Show resolved Hide resolved
swap/src/cli/list_sellers.rs Outdated Show resolved Hide resolved
swap/src/network/cbor_request_response.rs Outdated Show resolved Hide resolved
@thomaseizinger
Copy link
Contributor

It is not passing CI either but hanging forever, I will have a look!

thomaseizinger
thomaseizinger suggested changes Oct 13, 2021
View reviewed changes
swap/src/cli.rs Outdated Show resolved Hide resolved
swap/src/cli.rs Outdated Show resolved Hide resolved
swap/src/network/cbor_request_response.rs Outdated
@@ -59,7 +55,7 @@ where
where
T: AsyncRead + Unpin + Send,
{
let message = upgrade::read_one(io, BUF_SIZE)
let message = upgrade::read_length_prefixed(io, BUF_SIZE)
.await
.map_err(|e| io::Error::new(io::ErrorKind::InvalidData, e))?;
Copy link
Contributor

Choose a reason for hiding this comment

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

Also get rid of this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we not use ? here?

swap/src/network/json_pull_codec.rs
@@ -55,7 +55,7 @@ where
where
T: AsyncRead + Unpin + Send,
{
let message = upgrade::read_one(io, BUF_SIZE)
let message = upgrade::read_length_prefixed(io, BUF_SIZE)
.await
.map_err(|e| io::Error::new(io::ErrorKind::InvalidData, e))?;
Copy link
Contributor

Choose a reason for hiding this comment

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

And here.

@rishflab rishflab force-pushed the rendezvous-upstream branch 2 times, most recently from 14dcaae to 102e6b4 Compare October 13, 2021 23:48
thomaseizinger
thomaseizinger suggested changes Oct 14, 2021
View reviewed changes
Copy link
Contributor

@thomaseizinger thomaseizinger left a comment

Choose a reason for hiding this comment

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

Thanks! Couple of comments :)

Cargo.lock
Comment on lines +2478 to +2512
[[package]]
name = "open-metrics-client"
version = "0.12.0"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "7337d80c23c2d8b1349563981bc4fb531220733743ba8115454a67b181173f0d"
dependencies = [
"dtoa",
"itoa",
"open-metrics-client-derive-text-encode",
"owning_ref",
]

[[package]]
name = "open-metrics-client-derive-text-encode"
version = "0.1.1"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "a15c83b586f00268c619c1cb3340ec1a6f59dd9ba1d9833a273a68e6d5cd8ffc"
dependencies = [
"proc-macro2",
"quote",
"syn",
]
Copy link
Contributor

Choose a reason for hiding this comment

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

I've opened an issue about this: libp2p/rust-libp2p#2286

swap/src/asb/event_loop.rs
SwarmEvent::Behaviour(OutEvent::Rendezvous(libp2p::rendezvous::Event::RegisterFailed(error))) => {
tracing::error!("Registration with rendezvous node failed: {:#}", error);
SwarmEvent::Behaviour(OutEvent::Rendezvous(libp2p::rendezvous::client::Event::RegisterFailed(error))) => {
tracing::error!("Registration with rendezvous node failed: {:?}", error);
Copy link
Contributor

Choose a reason for hiding this comment

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

The error does not implement Display? Damn.

Copy link
Member Author

Choose a reason for hiding this comment

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

dont think so :(

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you open an upstream issue / PR please?

swap/src/cli.rs Outdated
Comment on lines 62 to 67
let mut rendezvous_node = new_swarm(|_, _| RendezvousPointBehaviour {
rendezvous: libp2p::rendezvous::server::Behaviour::new(
libp2p::rendezvous::server::Config::default(),
),
ping: Default::default(),
});
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we could implement Default here now and make this a bit more concise.

Copy link
Member Author

Choose a reason for hiding this comment

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

its only constructed once and in a test. not sure if worth it

swap/src/cli.rs Outdated
impl NetworkBehaviourEventProcess<libp2p::rendezvous::Event> for StaticQuoteAsbBehaviour {
fn inject_event(&mut self, event: libp2p::rendezvous::Event) {
if let libp2p::rendezvous::Event::Registered { .. } = event {
impl NetworkBehaviourEventProcess<libp2p::rendezvous::client::Event> for StaticQuoteAsbBehaviour {
Copy link
Contributor

Choose a reason for hiding this comment

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

Doing use libp2p::rendezvous; would go along way in this file as well.

swap/src/network/cbor_request_response.rs Outdated
@@ -59,7 +55,7 @@ where
where
T: AsyncRead + Unpin + Send,
{
let message = upgrade::read_one(io, BUF_SIZE)
let message = upgrade::read_length_prefixed(io, BUF_SIZE)
.await
.map_err(|e| io::Error::new(io::ErrorKind::InvalidData, e))?;
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we not use ? here?

@rishflab
Use upstream libp2p instead of our fork
a42484f 
The rendezvous protocol has been merged into upstream. We no longer need
 to depend on our fork. The ASB no longer works as a rendezvous point.
@rishflab
Implement default for rendezvous point behaviour
a0df789
thomaseizinger
thomaseizinger approved these changes Oct 14, 2021
View reviewed changes
Copy link
Contributor

@thomaseizinger thomaseizinger left a comment

Choose a reason for hiding this comment

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

Nice! Thank you!

Feel free to merge once green.

@rishflab
Copy link
Member Author

bors r+

@bors
Copy link
Contributor

bors bot commented Oct 14, 2021

Build succeeded:

@bors bors bot merged commit 97c11e4 into master Oct 14, 2021
@bors bors bot deleted the rendezvous-upstream branch October 14, 2021 01:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thomaseizinger thomaseizinger thomaseizinger approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@rishflab @thomaseizinger