Skip to content

protocols/gossipsub: remove RPC message size check#1910

Closed
xu-cheng wants to merge 1 commit into
libp2p:masterfrom
xu-cheng:gossipsub
Closed

protocols/gossipsub: remove RPC message size check#1910
xu-cheng wants to merge 1 commit into
libp2p:masterfrom
xu-cheng:gossipsub

Conversation

@xu-cheng

@xu-cheng xu-cheng commented Jan 9, 2021

Copy link
Copy Markdown
Contributor

Since df7e73e, a too large sized RPC message will be fragmented. Therefore, the old message size check should be removed.

Since df7e73e, a too large sized
RPC message will be fragmented. Therefore, the old message size
check should be removed.

@mxinden mxinden left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If I recall correctly this length check is still needed, given that one can not fragment a single message, but only a set of messages wrapped in a single rpc type.

This might be related to https://github.com/sigp/rust-libp2p/pull/93/files/73b4d9f21209214b0a93fff6f5c7b4b8295f9716#r528471904

@AgeManning would you mind taking a look?

@AgeManning

Copy link
Copy Markdown
Contributor

Yep @mxinden is correct.

A gossipsub RPC can contain many messages, control messages and subscriptions. If the total message is larger than the max transmit size the fragmentation splits these into to separate gossipsub RPCs.

If the encoded message to be sent is larger than the maximum transmit size, then we cannot send that message and we can't fragment it. The check here is still relevant to catch these.

@mxinden - Sorry even though you pinged me in that chain in the PR I seem to have missed it. The comments that say it is within 10% of the true value are old. They were related to a custom encoded_len() function I built to prevent doing the entire protobuf encoding in the behaviour. This has been changed and the comments should get removed in the next update.

@xu-cheng

Copy link
Copy Markdown
Contributor Author

Thanks for the explanation.

@xu-cheng xu-cheng closed this Jan 10, 2021
@xu-cheng xu-cheng deleted the gossipsub branch January 10, 2021 23:46
@xu-cheng

Copy link
Copy Markdown
Contributor Author

BTW, any chance to publish a new release of libp2p for gossipsub 1.1?

@mxinden

mxinden commented Jan 12, 2021

Copy link
Copy Markdown
Member

Sure. I will cut a new release of unsigned-varint (paritytech/unsigned-varint#49), get that into rust-libp2p along with an upgrade to bytes and tokio and then prepare libp2p v0.34.0. In case it is merged in time, #1887 would be included in the release as well. Sounds good @xu-cheng?

@mxinden

mxinden commented Jan 12, 2021

Copy link
Copy Markdown
Member

@xu-cheng I did a full release, including libp2p-gossipsub. See v0.34.0. Let us know in case anything else comes up.

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.

3 participants