Minows #22

ineiti · 2024-07-02T14:37:22Z

This is the final PR for the 2024 Spring semester project from Zhe Xion working with the DEDIS lab at EPFL.
It adds a websocket implementation for mino using libp2p, which allows a simple change from websockets to other communication protocols.
The code has been benchmarked against the old minogrpc and tested in d-voting tests.

pierluca

Reviewed half-way, in particular low-hanging fruits.
More reviewing to follow... time permitting.

core/ordering/cosipbft/blockstore/disk.go

mino/minogrpc/session/mod.go

mino/minows/key/mod.go

pierluca · 2024-07-03T20:20:44Z

mino/minows/rpc.go

+
+import (
+	"context"
+	"encoding/gob"


why are we using gob encoding everywhere instead of serde ? 🤔

IIRC, the serde implementation used json, and as the first benchmarks were slow, it has been changed to a binary format. But in the end it turned out that the slowdown was due to another part of the implementation. So I think we can revert that, if you prefer to have serde everywhere.

In fact he gob encodes the following structure:

type Packet struct { Source []byte Payload []byte }

And then uses serde to encode the payload.

Is it worth creating a serde abstraction for the Packet?

To keep everything uniform, I'd say yes.

OK, I looked at it, and I propose to keep the Packet as a gob-encoded structure, with the following reasoning:

to me it corresponds to the grpc-encoding found here, which is encoded using protobuf (if I got that right): overlayClient.Call

as it needs to interface with the Stream interface, I'll have to add a packet-size manually, so that the receiver knows how many bytes to receive

So, I would be implementing part of the gob interface myself. Which is why I propose to accept this as-is.

@pierluca what do you think?

mino/minows/mod.go

test/cosidela_test.go

mino/minows/key/mod.go

PascalinDe

reviewed 9/21 files, coming back later to it

jbsv · 2024-07-05T06:46:46Z

dela is now based on go 1.21. Is it possible to upgrade this PR to align it to 1.21 as well ?

ineiti · 2024-07-05T07:33:49Z

dela is now based on go 1.21. Is it possible to upgrade this PR to align it to 1.21 as well ?

Good point. I'll try to rebase our dela fork on the dedis dela, and see if things break too hard :)

Co-authored-by: Zach Xion XioZ@users.noreply.github.com

Co-authored-by: Zach Xion XioZ@users.noreply.github.com This commit implements a new Mino handler using libp2p with websockets. Contrary to MinoGRPC it doesn't use any locks and uses a 1-level tree to handle messages. It is at least as performant as the MinoGRPC implementation, and works with d-voting.

Co-authored-by: Zach Xion XioZ@users.noreply.github.com

ineiti · 2024-07-05T09:37:03Z

dela is now based on go 1.21. Is it possible to upgrade this PR to align it to 1.21 as well ?

Good point. I'll try to rebase our dela fork on the dedis dela, and see if things break too hard :)

Great - 1.21 or the updates of the libp2p library triggers a Heisenbug. I hate it. And it doesn't even tell me where the bug is. Just something breaks.

pierluca

Finally finished the review.
I think some parts will be massively simplified by adopting serde.

@ineiti do you have any spare cycles for this or should DEDIS look into it ?

mino/minogrpc/controller/mod_test.go

mino/minows/address.go

mino/minows/rpc.go

pierluca · 2024-07-15T07:41:04Z

mino/minows/session.go

here as well I'd try to get rid of gob and use serde, which can be easily injected wherever needed.

pierluca · 2024-07-15T07:42:27Z

mino/minows/session.go

+	src, err := p.myAddr.MarshalText()
+	if err != nil {
+		return xerrors.Errorf("could not marshal address: %v", err)
+	}
+	payload, err := msg.Serialize(p.rpc.context)
+	if err != nil {
+		return xerrors.Errorf("could not serialize message: %v", err)
+	}
+	dest, err := addr.MarshalText()
+	if err != nil {
+		return xerrors.Errorf("could not marshal address: %v", err)
+	}
+
+	// Send to orchestrator to relay to the destination participant
+	forward := Forward{
+		Packet:      Packet{Source: src, Payload: payload},
+		Destination: dest,
+	}
+	err = p.out.Encode(&forward)
+	if err != nil {
+		return xerrors.Errorf("could not encode packet: %v", err)
+	}


this would be so much simpler with serde, and not mixed with the session code.

mino/minows/session.go

pierluca · 2024-07-15T07:45:48Z

mino/minows/session.go

+	encoder, ok := o.outs[unwrapped.identity]
+	if !ok {
+		return xerrors.Errorf("%v: %v", ErrNotPlayer, addr)
+	}
+	src, err := o.myAddr.MarshalText()
+	if err != nil {
+		return xerrors.Errorf("could not marshal address: %v", err)
+	}
+	payload, err := msg.Serialize(o.rpc.context)
+	if err != nil {
+		return xerrors.Errorf("could not serialize message: %v", err)
+	}
+
+	err = encoder.Encode(&Packet{Source: src, Payload: payload})
+	if err != nil {
+		return xerrors.Errorf("could not encode packet: %v", err)
+	}


this would be so much simpler with serde, and not mixed with the session code.

mino/minows/session.go

ineiti · 2024-08-12T14:37:32Z

Great - 1.21 or the updates of the libp2p library triggers a Heisenbug. I hate it. And it doesn't even tell me where the bug is. Just something breaks.

OK, all should be good now, libp2p had a bug - will test this:

libp2p/go-libp2p#2867

ineiti · 2024-08-14T09:30:57Z

Great - 1.21 or the updates of the libp2p library triggers a Heisenbug. I hate it. And it doesn't even tell me where the bug is. Just something breaks.

OK, all should be good now, libp2p had a bug - will test this:

libp2p/go-libp2p#2867

Auto-congratulation: this actually did the trick...

ineiti · 2024-08-15T07:56:11Z

OK, this is my proposal for answering (most) of @pierluca 's comments:

small things implemented (don't export packet, avoid extensive indentation)
made the session.go simpler by merging orchestrator and participant into messageHandler
still not using serde :) But:
- I'll need one gob anyway, to correctly pass the packet from serde.
- messageHandler reduces further the encode/decode blocks
- forward doesn't exist anymore, it's only packet now

The current PR has been added with some commits for the different things I did. For the final PR I'll merge them all into @Pierluca's comments.

pierluca

Looks much better to me.
I'm not 100% sure I follow why gob is necessary, but at this point it's highly maintainable and that's what matters ! 👍

ineiti · 2024-08-15T11:45:09Z

Looks much better to me. I'm not 100% sure I follow why gob is necessary, but at this point it's highly maintainable and that's what matters ! 👍

At one point you need to encode the size of the message, so you know at the other end how many packets you need to read. In the minogrpc code, this is done by Google's grpc module. So the best one could do is to reduce the packet structure to Message []byte. Or you do it manually like it has been done in onet by first sending a 64-bit size field, followed by the raw bytes.

Here I cheat a bit by adding two other fields (one optional), so I don't have to wrap my head around how to implement the json-encoder/decoder.

- Adding new version of libp2p - Re-enabling failing test and remove goverall - Use peer.ID.Validate - Don't export Packet and Forward - Merging participant and orchestrator

ineiti force-pushed the minows branch 2 times, most recently from e977b67 to 2885f13 Compare July 2, 2024 14:43

ineiti self-assigned this Jul 2, 2024

ineiti requested review from PascalinDe and pierluca July 2, 2024 14:50

pierluca reviewed Jul 3, 2024

View reviewed changes

PascalinDe reviewed Jul 4, 2024

View reviewed changes

ineiti force-pushed the minows branch from 2885f13 to 6c566ca Compare July 4, 2024 08:17

ineiti force-pushed the main branch from 1db49b3 to 946c6e1 Compare July 5, 2024 07:36

ineiti force-pushed the minows branch from 6c566ca to 8e1bb51 Compare July 5, 2024 07:42

ineiti force-pushed the main branch from 720fe79 to 20fbfbd Compare July 5, 2024 08:01

ineiti force-pushed the minows branch from 8e1bb51 to 205851c Compare July 5, 2024 08:02

ineiti added 3 commits July 5, 2024 10:07

Code cleanup

4d51a74

Co-authored-by: Zach Xion XioZ@users.noreply.github.com

Integration tests for MinoWS

0d1740a

Co-authored-by: Zach Xion XioZ@users.noreply.github.com

ineiti force-pushed the minows branch 13 times, most recently from 2e19446 to eb6b928 Compare July 5, 2024 08:50

ineiti force-pushed the minows branch 3 times, most recently from 057263b to 85617df Compare July 5, 2024 09:36

ineiti force-pushed the minows branch 9 times, most recently from 3eaef28 to 6653054 Compare July 5, 2024 13:21

pierluca requested changes Jul 15, 2024

View reviewed changes

ineiti force-pushed the minows branch 7 times, most recently from 8054787 to 16eadf9 Compare August 14, 2024 13:03

pierluca approved these changes Aug 15, 2024

View reviewed changes

@pierluca's comments

684c54a

- Adding new version of libp2p - Re-enabling failing test and remove goverall - Use peer.ID.Validate - Don't export Packet and Forward - Merging participant and orchestrator

ineiti force-pushed the minows branch from 897e37a to 684c54a Compare August 15, 2024 11:50

ineiti merged commit 8b3e267 into main Aug 19, 2024
5 checks passed

ineiti deleted the minows branch August 19, 2024 06:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Minows #22

Minows #22

ineiti commented Jul 2, 2024

pierluca left a comment

pierluca Jul 3, 2024

ineiti Jul 4, 2024

ineiti Jul 4, 2024

pierluca Jul 15, 2024

ineiti Aug 14, 2024

PascalinDe left a comment

jbsv commented Jul 5, 2024

ineiti commented Jul 5, 2024

ineiti commented Jul 5, 2024

pierluca left a comment

pierluca Jul 15, 2024

pierluca Jul 15, 2024

pierluca Jul 15, 2024

ineiti commented Aug 12, 2024

ineiti commented Aug 14, 2024

ineiti commented Aug 15, 2024

pierluca left a comment

ineiti commented Aug 15, 2024

Minows #22

Minows #22

Conversation

ineiti commented Jul 2, 2024

pierluca left a 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

PascalinDe left a comment

Choose a reason for hiding this comment

jbsv commented Jul 5, 2024

ineiti commented Jul 5, 2024

ineiti commented Jul 5, 2024

pierluca left a 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

ineiti commented Aug 12, 2024

ineiti commented Aug 14, 2024

ineiti commented Aug 15, 2024

pierluca left a comment

Choose a reason for hiding this comment

ineiti commented Aug 15, 2024