Announce protocol using libp2p #2283

rishflab · 2020-03-19T06:19:45Z

We think we have the overall structure and event propagation sorted. I think we should add tests at the network behaviour level like the chat example in libp2p to confirm everything is working how we expect.

thomaseizinger

Really nice work @rishflab & @tcharding ! :)

50% review done!
One challenging question though:

What happens if Alice announces two swaps with the same digest at the same time to Bob? How is she going to tell the responses apart if the event only contains the digest?

I see three possible ways of going about this:

We ignore it, making it ambiguous for Alice, which of the announced swaps has which SwapId.
This could actually be feasible because if the swaps have the same digest, they are literally the same and it shouldn't matter if Bob is prepared for both or just one. There should be no problems downstream.
We return a unique "token" (struct with a random/monotonically-increasing u32 inside) from the start_announce_protocol function that we will carry along internally and also emit in the ConfirmationReceived event. That will allow the caller (Alice) to match, which of the calls she made was actually successful and which one failed.
We force the caller to pass some context information into the function and emit that one back in the event. Basically the same as (2) but we just don't have control over the data.

(1) sounds like a shortcut but it may actually be feasible.
(2) is the most "isolated" solution and does not assume anything about the rest of the program
(3) could be interesting because we are already going to have an identifier for this: the "local" swap id we use to index the created swap data in the database.

I think we could go for all 3 solutions (or are there even more?).

Opinions @rishflab @tcharding ?

cnd/src/network/protocols/announce/behaviour.rs

thomaseizinger · 2020-03-19T06:45:23Z

cnd/src/network/protocols/announce/behaviour.rs

+    }
+
+    fn inject_connected(&mut self, _peer_id: PeerId, _endpoint: ConnectedPoint) {
+        // No need to do anything, both this node and connected peer now have a


We will need to keep track of our connections here same as we do in our current NetworkBehaviour (called ComitNode I think).

creating a follow up pr

thomaseizinger · 2020-03-19T06:46:23Z

cnd/src/network/protocols/announce/behaviour.rs

+    },
+}
+
+impl NetworkBehaviourEventProcess<BehaviourEvent> for Announce {


We shouldn't need this at this level.

Will we need this trait implemented to combine this announce protocol with the other protocols? We weren't completely sure what this trait does it the chat example had it implemented. They were using it to print events so it was difficult to infer what its actual purpose was.

cnd/src/network/protocols/announce/handler.rs

thomaseizinger · 2020-03-19T06:48:21Z

cnd/src/network/protocols/announce/handler.rs

+        if !self.dial_queue.is_empty() {
+            let upgrade = self.dial_queue.remove(0).unwrap(); // Cannot fail, queue is not empty.


Change this to an if let Some(event) = self.dial_queue.pop()

error[E0599]: no method named `pop` found for struct `std::collections::VecDeque<network::protocols::announce::protocol::OutboundConfig>` in the current scope --> cnd/src/network/protocols/announce/handler.rs:125:46 | 125 | if let Some(event) = self.dial_queue.pop() { | ^^^ method not found in `std::collections::VecDeque<network::protocols::announce::protocol::OutboundConfig>`

Ah if it is a VecDeque the fn is called pop_front or something, look it up in the docs :)

yeh changed it to pop_front with if let Some(..)

Ok, this one was never resolved. I had a quick hack, and it looks like this is a bigger issue than just using the correct function. We (mis)use a Vec as a FILO queue for events and we (mis)use a VecDequeue as a FIFO queue for dial_queue. We should decide what behaviour we actually want here and also we should use the data structures correctly. Since bors is running I'll add a ticket for this.

Tracking issue: #2306

cnd/src/network/protocols/announce/mod.rs

cnd/src/network/protocols/announce/protocol.rs

tcharding

Excluding the connection stuff, LGTM.

Disable clippy type complexity for function defined by libp2p. For the futures we use, add a type that aliases all the pin/box/dyn stuff.

thomaseizinger

I assume connection handling will be followed up :)

thomaseizinger · 2020-03-24T04:54:14Z

@tcharding @rishflab

Have the two of you talked about the problem I pointed out in my previous review in regards to simultaneous swaps?

I don't see an response to that here :)

tcharding · 2020-03-24T05:02:12Z

We ignore it, making it ambiguous for Alice, which of the announced swaps has which SwapId.
This could actually be feasible because if the swaps have the same digest, they are literally the same and it shouldn't matter if Bob is prepared for both or just one. There should be no problems downstream.

I'm with 1 for now because I'm feeling lazy.

tcharding · 2020-03-24T05:03:54Z

I assume connection handling will be followed up :)

@Rishab can you please create a ticket, I don't see one there yet. Thanks.

rishflab

Really nice work @rishflab & @tcharding ! :)

50% review done!
One challenging question though:

What happens if Alice announces two swaps with the same digest at the same time to Bob? How is she going to tell the responses apart if the event only contains the digest?

I see three possible ways of going about this:

We ignore it, making it ambiguous for Alice, which of the announced swaps has which SwapId.
This could actually be feasible because if the swaps have the same digest, they are literally the same and it shouldn't matter if Bob is prepared for both or just one. There should be no problems downstream.

We return a unique "token" (struct with a random/monotonically-increasing u32 inside) from the start_announce_protocol function that we will carry along internally and also emit in the ConfirmationReceived event. That will allow the caller (Alice) to match, which of the calls she made was actually successful and which one failed.

We force the caller to pass some context information into the function and emit that one back in the event. Basically the same as (2) but we just don't have control over the data.

(1) sounds like a shortcut but it may actually be feasible.

(2) is the most "isolated" solution and does not assume anything about the rest of the program

(3) could be interesting because we are already going to have an identifier for this: the "local" swap id we use to index the created swap data in the database.

I think we could go for all 3 solutions (or are there even more?).

Opinions @rishflab @tcharding ?

Don't feel like I have the knowledge/foresight at this point to make a decision on this. I am thinking we should come back to it after adding integration tests.

rishflab · 2020-03-19T21:45:12Z

cnd/src/network/protocols/announce/behaviour.rs

+    },
+}
+
+impl NetworkBehaviourEventProcess<BehaviourEvent> for Announce {


Will we need this trait implemented to combine this announce protocol with the other protocols? We weren't completely sure what this trait does it the chat example had it implemented. They were using it to print events so it was difficult to infer what its actual purpose was.

tcharding · 2020-03-24T05:16:35Z

@tcharding @rishflab

Have the two of you talked about the problem I pointed out in my previous review in regards to simultaneous swaps?

I don't see an response to that here :)

I accept full responsibility here, process fail by me. I should have communicated more with you @rishflab and not made assumptions. Live and learn :)

mergify · 2020-03-24T05:23:23Z

bors r+

bors · 2020-03-24T06:01:56Z

Build succeeded

ci/circleci: debug-build-test

github-actions bot assigned rishflab Mar 19, 2020

rishflab force-pushed the announce-rishab branch from 34f85e9 to b53e0e1 Compare March 19, 2020 06:21

rishflab requested a review from thomaseizinger March 19, 2020 06:21

tcharding force-pushed the announce-rishab branch 2 times, most recently from 9930605 to 6f011d8 Compare March 19, 2020 06:35

thomaseizinger reviewed Mar 19, 2020

View reviewed changes

rishflab force-pushed the announce-rishab branch 4 times, most recently from 2768d98 to acf4f23 Compare March 23, 2020 06:18

Add announce protocol

9be18ab

rishflab force-pushed the announce-rishab branch from acf4f23 to 9be18ab Compare March 23, 2020 06:22

rishflab requested review from thomaseizinger and tcharding March 23, 2020 06:23

tcharding approved these changes Mar 23, 2020

View reviewed changes

tcharding self-requested a review March 23, 2020 06:29

rishflab marked this pull request as ready for review March 24, 2020 02:36

tcharding approved these changes Mar 24, 2020

View reviewed changes

Fix clippy type complexity warnings

35e835c

Disable clippy type complexity for function defined by libp2p. For the futures we use, add a type that aliases all the pin/box/dyn stuff.

thomaseizinger approved these changes Mar 24, 2020

View reviewed changes

rishflab commented Mar 24, 2020

View reviewed changes

tcharding mentioned this pull request Mar 24, 2020

Queues are mixed up #2306

Closed

bors bot merged commit c028f2f into dev Mar 24, 2020

thomaseizinger deleted the announce-rishab branch March 24, 2020 06:15

rishflab linked an issue Mar 25, 2020 that may be closed by this pull request

Implement the "announce" protocol #2115

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Announce protocol using libp2p #2283

Announce protocol using libp2p #2283

rishflab commented Mar 19, 2020 •

edited

thomaseizinger left a comment •

edited

thomaseizinger Mar 19, 2020

rishflab Mar 24, 2020

thomaseizinger Mar 19, 2020

rishflab Mar 19, 2020

thomaseizinger Mar 19, 2020

tcharding Mar 23, 2020

thomaseizinger Mar 23, 2020

rishflab Mar 24, 2020

tcharding Mar 24, 2020 •

edited

tcharding Mar 24, 2020

tcharding left a comment

thomaseizinger left a comment

thomaseizinger commented Mar 24, 2020

tcharding commented Mar 24, 2020

tcharding commented Mar 24, 2020

rishflab left a comment

rishflab Mar 19, 2020

tcharding commented Mar 24, 2020

mergify bot commented Mar 24, 2020

bors bot commented Mar 24, 2020

		if !self.dial_queue.is_empty() {
		let upgrade = self.dial_queue.remove(0).unwrap(); // Cannot fail, queue is not empty.

Announce protocol using libp2p #2283

Announce protocol using libp2p #2283

Conversation

rishflab commented Mar 19, 2020 • edited

thomaseizinger left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tcharding Mar 24, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tcharding left a comment

Choose a reason for hiding this comment

thomaseizinger left a comment

Choose a reason for hiding this comment

thomaseizinger commented Mar 24, 2020

tcharding commented Mar 24, 2020

tcharding commented Mar 24, 2020

rishflab left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tcharding commented Mar 24, 2020

mergify bot commented Mar 24, 2020

bors bot commented Mar 24, 2020

Build succeeded

rishflab commented Mar 19, 2020 •

edited

thomaseizinger left a comment •

edited

tcharding Mar 24, 2020 •

edited