feat: support multiple shared caps #5363
Conversation
| @@ -539,7 +539,7 @@ where | |||
|
|
|||
| // all messages sent in this stream are subprotocol messages, so we need to switch the | |||
| // message id based on the offset | |||
| compressed[0] = item[0] + this.shared_capability.offset(); | |||
| compressed[0] = item[0] + MAX_RESERVED_MESSAGE_ID + 1; | |||
There was a problem hiding this comment.
is this right? it's not clear to me why we can use a constant here / stop using the offset
There was a problem hiding this comment.
this is correct because the offset is reserved + 1
see how the offsets are determined:
reth/crates/net/eth-wire/src/p2pstream.rs
Lines 630 to 633 in e056bfb
so what this does here it strips away the reserved offset so that the ID aligns with the first shared capability
There was a problem hiding this comment.
I'm adding some docs and maybe change the behaviour here so that it perhaps always normalizes messages or also returns the matching capability
There was a problem hiding this comment.
thx for the docs, yeah, this makes sense. for eth plus another capability we'd need to add the offset for the second capability somewhere else right?
There was a problem hiding this comment.
the offsets are computed after the initial handshake, but atm the caller of the p2p is responsible for matching the message id to a shared cap.
for example, shared caps:: [("eth", 10), ("other", 5)] the p2p stream would yield messages with first byte [0..14] (total number of messages), the reserved offset is already subtracted here.
for a single cap the message can be deserialized directly, but with multiple, we'd need to check, for example first byte 12 would actually belong to "other" and we'd need to subtract the number of messages of "eth" before
I'll make this more ergonomic in followups
| @@ -460,7 +460,7 @@ where | |||
| // * `eth/67` is reserved message IDs 0x10 - 0x19. | |||
| // * `qrs/65` is reserved message IDs 0x1a - 0x21. | |||
| // | |||
| decompress_buf[0] = bytes[0] - this.shared_capability.offset(); | |||
| decompress_buf[0] = bytes[0] - MAX_RESERVED_MESSAGE_ID - 1; | |||
| @@ -539,7 +539,7 @@ where | |||
|
|
|||
| // all messages sent in this stream are subprotocol messages, so we need to switch the | |||
| // message id based on the offset | |||
| compressed[0] = item[0] + this.shared_capability.offset(); | |||
| compressed[0] = item[0] + MAX_RESERVED_MESSAGE_ID + 1; | |||
There was a problem hiding this comment.
thx for the docs, yeah, this makes sense. for eth plus another capability we'd need to add the offset for the second capability somewhere else right?
Extracted parts from #2981 that are required for #5360
The underlying P2P stream is now no longer limited to eth and can handle any shared caps