feat(protobuf): added error wrappers for invalid length validation #1563
Conversation
LNSD
force-pushed
the
feat-common-protobuf
branch
from
17870ea
to
7c958a8
Compare
Honestly, I don't know if we should. IIRC Gossipsub RFC does not specify a maximum topic length; hence it is up to the implementations to set limits. I see here the same kind of situation. Wdyt? |
Works either way, maybe this is better noted in a "Recommendations" section |
There was a problem hiding this comment.
Maybe the RFC should include the 128 byte size constraint on content_topic?
I would say whatever can create an incompatibility issue should be added to the spec. I mean, if one implementation has that 128bytes limit and another doesn't, a message sent with a content_topic longer than 128byte, may not be routed by the second implementation (depending on how we do message validation).
regarding the PR, lgtm!
| @@ -0,0 +1,103 @@ | |||
|
|
|||
There was a problem hiding this comment.
nice. I actually was thinkinig about testing the encode, decode functions of the protos that were changed, like PagingInfoRPC.
A simple back and forth converstion like this to assert that encode()/decode() work as expected and protect the code from future modifications. Also, test this extra logic if topic.len > MaxPubsubTopicLen: trying to encode eg a FilterRequest with a longer topic, and asserting it fails.
But we can leave that for another PR.
There was a problem hiding this comment.
(ofc testing all encode/decode functions would be a long PR) beyond this, but perhaps we are lacking some testing in protos in general?
There was a problem hiding this comment.
(ofc testing all encode/decode functions would be a long PR) beyond this, but perhaps we are lacking some testing in protos in general?
Protocol-specific protobuf tests can be found in the different protocol folders (see the tests/v2/waku_store module). Additionally, the different protocols' integration/end-to-end tests implicitly test protocol RPC serialisation.
I don't see the need to add test suites for unit testing/covering specifically the RPC serialization of the different protocols. If we had to add tests to cover this area: Interoperability tests (a.k.a integration-tests) would be the ones providing additional value.
|
@jm-clius Looking forward to your feedback on the size limit of the topic and if we should specify it as part of the 14/WAKU2-MESSAGE RFC 😁 |
There was a problem hiding this comment.
Indeed! Since these can introduce incompatibilities/breaking change in how the protocol is used, it should be part of an RFC and agreed upon before merging, especially since we don't know how this will affect current applications. My suggestion is https://rfc.vac.dev/spec/23/ as recommendations (i.e. the pubsubTopic SHOULD be less than 128 characters. Individual Waku nodes MAY choose to discard messages that does not adhere to....). To get this PR merged, unstaging the changes specific to max topic length and leaving that for a next PR (after RFC agreement) will work. 👍
LNSD
force-pushed
the
feat-common-protobuf
branch
from
7c958a8
to
732ceb8
Compare
I have split the different codecs' topic length validation changes into a separate commit. Waiting for re-review/approval. cc @jm-clius @alrevuelta @rymnc I will open the "max topic length recommendation" RFC PR with the split-out changes after this PR gets merged. EDIT:
|
| const | ||
| MaxWakuMessageSize* = 1024 * 1024 # 1 Mibytes. Corresponds to PubSub default |
There was a problem hiding this comment.
| const | |
| MaxWakuMessageSize* = 1024 * 1024 # 1 Mibytes. Corresponds to PubSub default | |
| const MaxWakuMessageSize* = 1024 * 1024 # 1 Mibytes. Corresponds to PubSub default |
There was a problem hiding this comment.
As I plan to restore the max topic length constants, I opted not to restore the single line const :)
Context
At this moment, no protobuf max field length validation is performed. In 14/WAKU2-MESSAGE RFC, no maximum length for the
content_topicattribute is specified. This is potentially problematic. Additionally, this is not supported by libp2p's minprotobuf implementation.This requirement motivates the present PR to add support in the
common/protobufwrapper module for validating the protobuf field length.Acceptance criteria
content_topicattribute length: 128 bytes (128 ASCII chars).Other Considerations
Note that the validation is only executed during the deserialization phase. The content topic length is assumed to be valid in the serialisation phase.
Initially, the minprotobuf's
getRequirefFieldwas also considered. But as there isn't a max length validation equivalent, for homogeneity, I discarded its usage and added also the "required field missing" error variant.