feat: pubsub#5435
Conversation
| defer bc.mu.Unlock() | ||
|
|
||
| for _, sub := range bc.subscribers { | ||
| msg := bc.mode.FormatBroadcast(bc, sub, rawMsg) |
There was a problem hiding this comment.
I dont think this is needed inside the fanout loop. Why would you reserialise for every subscriber?
There was a problem hiding this comment.
the message serialization varies between subscribers e.g. one of them did not get the SOC ID and address for message verification, another subscriber already got it. Then, these metadata will be either attached or not in the message payload.
There was a problem hiding this comment.
why would it vary between subscribers? In fact it must be the same, otherwise same msg can be repeated.
7011490 to
8539f8c
Compare
…send during create
…and msgType refactor
c3e2d28 to
acbface
Compare
|
|
||
| s.Handler = web.ChainHandlers( | ||
| httpaccess.NewHTTPAccessLogHandler(s.logger, s.tracer, "api access"), | ||
| handlers.CompressHandler, |
There was a problem hiding this comment.
how is it that this is needed? we've had websockets running in the API for years now. by what you're saying, they couldn't have worked so far. it would be good to know why this is needed
| // ResponseWriter, causing "response.Write on hijacked connection" errors. | ||
| func (rr *responseRecorder) Hijack() (net.Conn, *bufio.ReadWriter, error) { | ||
| return rr.ResponseWriter.(http.Hijacker).Hijack() | ||
| h, ok := rr.ResponseWriter.(http.Hijacker) |
There was a problem hiding this comment.
not sure why this is needed. can you please explain why the existing websocket machinery that is used to stream chunks cannot be reused? also you are affecting that machinery with this change.
| go func() { | ||
| pubsub.ListeningWs(ctx, conn, pubsub.WsOptions{PingPeriod: pingPeriod, Cancel: cancel}, logger, mode, isPublisher) | ||
| cancel() | ||
| _ = conn.Close() |
There was a problem hiding this comment.
please use defer statements inside the closure before the ListeningWs call for both cleanup commands. when that call panics, the runtime will do a stack unwind and will execute the defer statements. in your version here, since the calls are made after the potential panic, they never get cleaned up
| } | ||
|
|
||
| var topicAddr [32]byte | ||
| if decoded, err := hex.DecodeString(paths.Topic); err == nil && len(decoded) == swarm.HashSize { |
There was a problem hiding this comment.
can you please add some comments on what topic could be? there's zero comments on the handler and zero inline comments, which is not so nice to read (i'd rather not have to reach out to the openapi spec or BOS to check what a parameter might mean). ideally you can add a comment in the anonymous type on that field, explaining what it is.
also just to be a little bit more specific. if a parameter is a hex address OR an arbitrary length string, then it is ONLY an arbitrary length string. whether someone, by convention, chooses to use one over the other, it is a bad practice to disambiguate them. i would suggest to have just one type and use it however using conventions. i.e. always hash the topic.
|
|
||
| peerMultiaddr := r.URL.Query().Get("peer") | ||
| if peerMultiaddr == "" { | ||
| jsonhttp.BadRequest(w, "missing peer query param") |
There was a problem hiding this comment.
query params are usually optional, and rest path parameters are mandatory. if the peer multiaddr is mandatory, it should be in the path params, and you avoid these sort of checks then
| // verifying them, and returning the payload to forward to the WebSocket. | ||
| // If the subscriber is a Publisher, it also reads from the WebSocket | ||
| // and writes raw messages to the p2p stream. | ||
| func ListeningWs(ctx context.Context, conn *websocket.Conn, options WsOptions, logger log.Logger, mode Mode, isPublisher bool) { |
There was a problem hiding this comment.
i can't see why anything apart from the api package should leak websocket types. this does not make much sense and i would request that the abstractions handle this requirement properly. just in the same way that we don't leak cobra/viper types outside of the cmd package, in the same way no HTTP types should leak out of the api package. thanks
| } | ||
|
|
||
| s.logger.Info("connecting to broker peer", "underlay", underlay) | ||
| bzzAddr, err := s.p2p.ConnectAllowLight(ctx, []ma.Multiaddr{underlay}) |
There was a problem hiding this comment.
let's please not mix these things. a protocol should not be able to dial to another peer. this is not good news. there is already existing functionality in the api package to dial to peers. any sort of connection orchestration should be done from there by a different client that would know to tell bee to connect to another bee. for me having a protocol that can coerce a node to create connections via other messages (be they websocket or p2p messages) is a serious attack surface. a protocol should be "read message, write message". look at pushsync/pullsync and their relevant orchestration layers pusher/puller respectively. we are doing things with this architecture for a good reason. if this deviation is required we should at least be shown some justification as to why.
| defer sc.subsMu.Unlock() | ||
| for _, ch := range sc.subs { | ||
| select { | ||
| case ch <- msg: |
There was a problem hiding this comment.
one reader stalling delays the rest. and you're holding the mutex all that time
| } | ||
|
|
||
| // writeRaw writes raw bytes to the stream. | ||
| func writeRaw(stream p2p.Stream, data []byte) error { |
There was a problem hiding this comment.
why can't you just Write once? not sure why the helper is needed. libp2p streams can handle enough data unless you can prove otherwise?
| defer sc.subsMu.Unlock() | ||
| id := sc.nextID | ||
| sc.nextID++ | ||
| ch := make(chan []byte, 16) |
There was a problem hiding this comment.
channel buffer size is either 0 or 1 please. this is a recurring pattern i see that in PRs this rule is starting to be broken. this is against go styleguides and against our adopted styleguide.
pubsub
A brokered publish/subscribe protocol over Bee's p2p layer, exposed to end-users via WebSocket.
Overview
One node acts as a broker: it accepts p2p streams, validates incoming messages, and fans them out to all connected receivers. Other nodes connect as either a publisher (publish + receive) or a subscriber (receive only).
Connection flow
The connecting node receives a
SubscriberConnwhich is bridged to the WebSocket: inbound WS frames are forwarded to the p2p stream (publishers only); p2p frames from the broker are decoded and forwarded to the WS client (all connections).Mode system
Protocol-specific behaviour is isolated behind a
Modeinterface. TheServiceis mode-agnostic: it delegates header construction, message reading/validation, and broadcast formatting to the active mode. New protocol variants can be added by implementingModeand registering a mode ID innewMode.GSOC Ephemeral mode (mode 1)
Messages are Single Owner Chunks (SOC) signed by the holder of the topic's private key. The topic address is derived from the SOC owner public key and an arbitrary topic ID, so only the key holder(s) can publish and the messages carry full SOC wrapping.
Publisher → Broker (p2p)
The broker verifies the ECDSA signature on every message before broadcasting.
Broker → Subscriber (p2p)
Every broker frame begins with a 1-byte message type.
0x01is reserved at the service level and is valid across all modes; all other type bytes are mode-specific and decoded by the active mode implementation.0x010x02[SOC ID: 32 B][owner: 20 B][sig: 65 B][span: 8 B][payload: N B]0x03[sig: 65 B][span: 8 B][payload: N B]The handshake frame (SOC identity metadata) is sent once per topic on the first broadcast. Subsequent messages are data-only. Ping frames are consumed by
readServiceMessagebefore mode dispatch and never surfaced to the WebSocket client.WebSocket (both directions)
Both the publisher (inbound) and subscriber (outbound) see the raw SOC payload:
The node handles all p2p framing and signature verification transparently.
Multi-WebSocket multiplexer
Multiple WebSocket sessions to the same topic on the same node share a single p2p stream to the broker via
SubscriberConn:CreateSubscriberConnincrements a ref count instead of opening a new stream when one already existsrunMuxgoroutine fans out incoming broker messages to all per-session channels; on stream error it immediately clears the shared conn so new sessions get a fresh streamRemoveSubscriberConndecrements refs;FullCloseis called exactly once when refs reach zeroAPI
GET(WS upgrade)/pubsub/{topic}GET/pubsub/Query params for
/pubsub/{topic}:peer(required): multiaddr of the broker peergsoc-eth-address+gsoc-topic(optional): upgrade to publisher roleHeader:
swarm-keep-alive(optional): WebSocket ping period in seconds (default: 60)Configuration
--pubsub-broker-modefalseSubscriber nodes use
ConnectAllowLightwhen dialing the broker, so broker nodes can run in light-node mode.Extensibility
Modefor a different message format and register a mode ID.validatePublishercan enforce allow-lists, or stake checks.handleSubscribercan replay on connect.broadcastcan forward to an event emitter, making the broker embeddable in a WASM-compiled client without a separate p2p layer.formatBroadcastreceives the individualbrokerSubscriber, enabling selective filtering or transformation.Tests
pkg/pubsub(unit)mode_1_test.go—GSOCEphemeralMode: message formatting (handshake vs data),ReadBrokerMessagefor ping/handshake/data/invalid-sig,setGsocParamsaddress mismatch and valid params,SubscriberConnfanout, unsubscribe, and full-channel overflow.pubsub_test.go— broker handler error paths (disabled, missing/unknown headers) andService.Topics()(empty list, broker-role registration via spinlock).pkg/api(integration)pubsub_test.go— nil service → 404, empty list → 200, missing peer/invalid multiaddr/bad GSOC params → 400, subscriber WS connect/teardown, and a two-topic scenario: opens two concurrent WS sessions, writes broker pings through the mock p2p streams, queriesGET /pubsub/asserting both topics are listed, then closes each connection sequentially verifying the topic count decrements to 0.Checklist
Description
Open API Spec Version Changes (if applicable)
Motivation and Context (Optional)
Related Issue (Optional)
SWIPs #93 — PubSub protocol — this PR implements Milestone 1 (Direct messaging) of that SWIP.
Screenshots (if appropriate):
AI Disclosure