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

Kitsune2 Top-Level P2P Protocol #38

Merged
merged 13 commits into from
Dec 4, 2024
Merged

Kitsune2 Top-Level P2P Protocol #38

merged 13 commits into from
Dec 4, 2024

Conversation

neonphog
Copy link
Collaborator

@neonphog neonphog commented Dec 2, 2024

RESOLVES: #26

Consider before approving:

  • Are we cool with protobuf as a departure from msgpack? I like the self-spec-ing nature, but it means pulling in and building the protobuf-src crate having to deal with needing the protoc binary. If we're switching to this, we should probably use it for the module message encoding as well (e.g. gossip messages / fetch queue messages / etc).
    • sigh protobuf-src doesn't work on windows -- I can pretty trivially get protoc installed in windows ci and use that instead, but this is another downside, so let me know what you think before I move forward on that.
    • UPDATE: tried protoc-prebuilt package and it works locally, but the ci runner gets rate-limited from others downloading it... So I've made that an optional feature and am just running a gha to install protoc on ci
  • Do we want this in the api crate? We could instead split this out into a separate protocol crate that only the tx module (??) would use?

@neonphog neonphog requested a review from a team December 2, 2024 22:59
crates/api/src/wire.proto Outdated Show resolved Hide resolved
crates/api/src/wire.proto Outdated Show resolved Hide resolved
crates/api/src/wire.proto Outdated Show resolved Hide resolved
crates/api/build.rs Outdated Show resolved Hide resolved
crates/api/src/wire.proto Outdated Show resolved Hide resolved
crates/api/src/wire.proto Outdated Show resolved Hide resolved
@cdunster
Copy link
Contributor

cdunster commented Dec 3, 2024

@neonphog, have you ever worked with flatbuffers? I've never used them but I've heard that they're a better and more efficient version of protocol buffers.

Cargo.toml Outdated Show resolved Hide resolved
crates/api/src/wire.proto Outdated Show resolved Hide resolved
crates/api/src/wire.proto Outdated Show resolved Hide resolved
crates/api/src/wire.proto Outdated Show resolved Hide resolved
@ThetaSinner
Copy link
Member

Protobuf is good with me, I think it's easier to keep track of breaking changes because protobuf provide good guidance for how to think about that and this doesn't affect app developers anyway.

I can't think why somebody would want to integrate with Kitsune at the wire level but not want to use any of our types. I think that would only happen if they weren't using Rust and then what they'll actually want is the .proto files. I'm not sure how we best package those if somebody wants them but we can figure that out if somebody asks for it.
Then for a program that is embedding Kitsune, it's going to have to build the protos however we split the libraries. So I'm not seeing a strong case for separating those out up front.

@neonphog
Copy link
Collaborator Author

neonphog commented Dec 3, 2024

@cdunster

have you ever worked with flatbuffers? I've never used them but I've heard that they're a better and more efficient version of protocol buffers.

I have not, although I'd heard the same. My sense is that protobuf has the first mover advantage in popularity and tooling/rust integration. And I'm not too worried about the small amount of protocol overhead as the majority of the data we'll be transferring is the op content. But I'm totally up for experimenting with it if you think it is worth it!

@cdunster
Copy link
Contributor

cdunster commented Dec 3, 2024

@neonphog

I have not, although I'd heard the same. My sense is that protobuf has the first mover advantage in popularity and tooling/rust integration. And I'm not too worried about the small amount of protocol overhead as the majority of the data we'll be transferring is the op content. But I'm totally up for experimenting with it if you think it is worth it!

I don't know enough about flatbuffers to risk convincing you to use them 😉 all I know is that I trust the person who told me they are easier to work with and that it would avoid a lot of the boilerplate we added to support things like backwards compatibility.

Copy link
Contributor

@jost-s jost-s left a comment

Choose a reason for hiding this comment

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

Top-level protocol, does this mean that inside of this protobuf message encoding there'll be MessagePack encoded messages that are passed onto Holochain?

crates/api/Cargo.toml Outdated Show resolved Hide resolved
@neonphog
Copy link
Collaborator Author

neonphog commented Dec 3, 2024

@jost-s

Top-level protocol, does this mean that inside of this protobuf message encoding there'll be MessagePack encoded messages that are passed onto Holochain?

Setting asside holochain for the moment, we indeed can do anything for the sub-encoding. Messagepack, json, cbor, whatever. If we're picking protobuf for the top-level, though, it probably makes the most sense to use it also for our internal module protocols.

Now, on to holochain. The op encoding will be entirely defined by holochain. I don't think we want to change that yet, so that will remain messagepack. But that is opaque to kitsune and would be included in a sub-sub-encoding of the modules' protocols.

@jost-s
Copy link
Contributor

jost-s commented Dec 3, 2024

@neonphog

Setting asside holochain for the moment, we indeed can do anything for the sub-encoding. Messagepack, json, cbor, whatever. If we're picking protobuf for the top-level, though, it probably makes the most sense to use it also for our internal module protocols.

So there are 3 levels of encoding. Is the data (= payload/content) of a top-level message the 2nd layer?

@neonphog
Copy link
Collaborator Author

neonphog commented Dec 4, 2024

@jost-s

So there are 3 levels of encoding. Is the data (= payload/content) of a top-level message the 2nd layer?

Yep : )

Copy link
Contributor

@jost-s jost-s left a comment

Choose a reason for hiding this comment

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

Fine with it all =)

@neonphog neonphog merged commit 1c74bb5 into main Dec 4, 2024
4 checks passed
@neonphog neonphog deleted the proto branch December 4, 2024 16:47
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.

[k2] top-level wire protocol spec and encoding implementation
4 participants