From a488ca7693995f1c6b5aa226a0a9faa6c2a29e11 Mon Sep 17 00:00:00 2001 From: NagyZoltanPeter <113987313+NagyZoltanPeter@users.noreply.github.com> Date: Fri, 13 Sep 2024 10:57:07 +0200 Subject: [PATCH 1/7] Adjusted peer-exchange to the latest changes made due to rate limit DOS protection applied on protocol. Extended with responseStatus. --- standards/core/peer-exchange.md | 28 ++++++++++++++++++++++++---- 1 file changed, 24 insertions(+), 4 deletions(-) diff --git a/standards/core/peer-exchange.md b/standards/core/peer-exchange.md index 3eccd45..2eed6ac 100644 --- a/standards/core/peer-exchange.md +++ b/standards/core/peer-exchange.md @@ -66,7 +66,7 @@ message PeerInfo { bytes enr = 1; } -message PeerExchangeQuery { +message PeerExchangeRequest { uint64 num_peers = 1; } @@ -74,18 +74,37 @@ message PeerExchangeResponse { repeated PeerInfo peer_infos = 1; } +message PeerExchangeResponseStatus { + uint32 status = 1; + optional string desc = 2; +} + message PeerExchangeRPC { - PeerExchangeQuery query = 1; - PeerExchangeResponse response = 2; + optional PeerExchangeRequest request = 1; + optional PeerExchangeResponse response = 2; + optional PeerExchangeResponseStatus status = 10; } ``` The `enr` field contains a Waku ENR as specified in [WAKU2-ENR](./enr.md). -Requesters send a `PeerExchangeQuery` to a peer. +Requesters send a `PeerExchangeRequest` to a peer. +Responders SHOULD not fill `request` field. Responders SHOULD include a maximum of `num_peers` `PeerInfo` instances into a response. Responders send a `PeerExchangeResponse` to requesters containing a list of `PeerInfo` instances, which in turn hold an ENR. +Responders, MUST include a `PeerExchangeResponseStatus` in the response in any case. If the request was not successful `PeerExchangeResponse` can be omitted. + +### Possible status codes + +| Result | Code | Note | +|--------|------|------| +| SUCCESS | 200 | Successfull request-respond. In response the answer must contain `PeerExchangeResponse` | +| BAD_REQUEST | 400 | Wrong request payload. It must only contain `reques` field. | +| BAD_RESPOND | 401 | Wrong respond payload. If success it must contain `respond` and `responseStatus` fields. If failure, only `responseStatus` is set. | +| TOO_MANY_REQUESTS | 429 | DOS protection prevented this request as the current request exceeds the configured request rate. | +| SERVICE_UNAVAILABLE | 503 | Request cannot be served, either issue on Responder side or having no suitable peer to issue request. | +| DIAL_FAILRE | 599 | Requester side problem calling PeerExchange peer. | ## Implementation Suggestions @@ -143,6 +162,7 @@ Requesters send a simple message request which causes responders to engage in am As a mitigation, responders MAY feature a `seen cache` for requests and only answer once per time interval. The exchange-peer cache discussed in [Theory and Protocol Semantics Section](#theory-and-protocol-semantics) also provides mitigation. Still, frequent queries can tigger the refresh cycle more often. The `seen cache` MAY be used in conjunction to provide additional mitigation. +Responders MAY apply request limits and thus can reject answering requests within a certain time window. Requestes must be prepared to handle this case. ### Further Considerations From 4c7515d632f19f7a604e05c46431608bc81eb6b0 Mon Sep 17 00:00:00 2001 From: NagyZoltanPeter <113987313+NagyZoltanPeter@users.noreply.github.com> Date: Fri, 13 Sep 2024 11:07:36 +0200 Subject: [PATCH 2/7] Fix field naming --- standards/core/peer-exchange.md | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/standards/core/peer-exchange.md b/standards/core/peer-exchange.md index 2eed6ac..b104a22 100644 --- a/standards/core/peer-exchange.md +++ b/standards/core/peer-exchange.md @@ -82,7 +82,7 @@ message PeerExchangeResponseStatus { message PeerExchangeRPC { optional PeerExchangeRequest request = 1; optional PeerExchangeResponse response = 2; - optional PeerExchangeResponseStatus status = 10; + optional PeerExchangeResponseStatus response_status = 10; } ``` @@ -93,7 +93,7 @@ Requesters send a `PeerExchangeRequest` to a peer. Responders SHOULD not fill `request` field. Responders SHOULD include a maximum of `num_peers` `PeerInfo` instances into a response. Responders send a `PeerExchangeResponse` to requesters containing a list of `PeerInfo` instances, which in turn hold an ENR. -Responders, MUST include a `PeerExchangeResponseStatus` in the response in any case. If the request was not successful `PeerExchangeResponse` can be omitted. +Responders, MUST include a `PeerExchangeResponseStatus` in the response in any case. If the request was not successful `response` field can be omitted. ### Possible status codes @@ -101,7 +101,7 @@ Responders, MUST include a `PeerExchangeResponseStatus` in the response in any c |--------|------|------| | SUCCESS | 200 | Successfull request-respond. In response the answer must contain `PeerExchangeResponse` | | BAD_REQUEST | 400 | Wrong request payload. It must only contain `reques` field. | -| BAD_RESPOND | 401 | Wrong respond payload. If success it must contain `respond` and `responseStatus` fields. If failure, only `responseStatus` is set. | +| BAD_RESPOND | 401 | Wrong respond payload. If success it must contain `respond` and `response_status` fields. If failure, only `response_status` is set. | | TOO_MANY_REQUESTS | 429 | DOS protection prevented this request as the current request exceeds the configured request rate. | | SERVICE_UNAVAILABLE | 503 | Request cannot be served, either issue on Responder side or having no suitable peer to issue request. | | DIAL_FAILRE | 599 | Requester side problem calling PeerExchange peer. | From 90efdfa6af2c6fdde699ff736b454f312617b116 Mon Sep 17 00:00:00 2001 From: NagyZoltanPeter <113987313+NagyZoltanPeter@users.noreply.github.com> Date: Mon, 16 Sep 2024 11:59:27 +0200 Subject: [PATCH 3/7] Fixed message structure upon review agreement in order to keep backward compatibility with the former protocol format --- standards/core/peer-exchange.md | 15 +++++---------- 1 file changed, 5 insertions(+), 10 deletions(-) diff --git a/standards/core/peer-exchange.md b/standards/core/peer-exchange.md index b104a22..de321cb 100644 --- a/standards/core/peer-exchange.md +++ b/standards/core/peer-exchange.md @@ -72,17 +72,13 @@ message PeerExchangeRequest { message PeerExchangeResponse { repeated PeerInfo peer_infos = 1; -} - -message PeerExchangeResponseStatus { - uint32 status = 1; - optional string desc = 2; + uint32 status = 10; + optional string desc = 11; } message PeerExchangeRPC { - optional PeerExchangeRequest request = 1; - optional PeerExchangeResponse response = 2; - optional PeerExchangeResponseStatus response_status = 10; + PeerExchangeRequest request = 1; + PeerExchangeResponse response = 2; } ``` @@ -90,10 +86,9 @@ message PeerExchangeRPC { The `enr` field contains a Waku ENR as specified in [WAKU2-ENR](./enr.md). Requesters send a `PeerExchangeRequest` to a peer. -Responders SHOULD not fill `request` field. Responders SHOULD include a maximum of `num_peers` `PeerInfo` instances into a response. Responders send a `PeerExchangeResponse` to requesters containing a list of `PeerInfo` instances, which in turn hold an ENR. -Responders, MUST include a `PeerExchangeResponseStatus` in the response in any case. If the request was not successful `response` field can be omitted. +Responders, MUST include a `status` in the response in any case and possible descriptive form of the problem if any. ### Possible status codes From 6a0041544babca5afe53ecc200c04934666bf5d0 Mon Sep 17 00:00:00 2001 From: NagyZoltanPeter <113987313+NagyZoltanPeter@users.noreply.github.com> Date: Tue, 17 Sep 2024 11:18:51 +0200 Subject: [PATCH 4/7] Fixing typos and namings on review findings --- standards/core/peer-exchange.md | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/standards/core/peer-exchange.md b/standards/core/peer-exchange.md index de321cb..ed1901b 100644 --- a/standards/core/peer-exchange.md +++ b/standards/core/peer-exchange.md @@ -94,12 +94,12 @@ Responders, MUST include a `status` in the response in any case and possible des | Result | Code | Note | |--------|------|------| -| SUCCESS | 200 | Successfull request-respond. In response the answer must contain `PeerExchangeResponse` | -| BAD_REQUEST | 400 | Wrong request payload. It must only contain `reques` field. | -| BAD_RESPOND | 401 | Wrong respond payload. If success it must contain `respond` and `response_status` fields. If failure, only `response_status` is set. | +| SUCCESS | 200 | Successfull request-respond. | +| BAD_REQUEST | 400 | Wrong request payload. It must only contain `request` field. | +| BAD_RESPONSE | 401 | Wrong response payload. If success it must contain `response` and `status` fields. If failure, only `status` is set. | | TOO_MANY_REQUESTS | 429 | DOS protection prevented this request as the current request exceeds the configured request rate. | | SERVICE_UNAVAILABLE | 503 | Request cannot be served, either issue on Responder side or having no suitable peer to issue request. | -| DIAL_FAILRE | 599 | Requester side problem calling PeerExchange peer. | +| DIAL_FAILURE | 599 | Requester side problem calling PeerExchange peer. | ## Implementation Suggestions From df559456295d52cd4ab31fa63eae3c1ddb27477c Mon Sep 17 00:00:00 2001 From: NagyZoltanPeter <113987313+NagyZoltanPeter@users.noreply.github.com> Date: Tue, 17 Sep 2024 12:20:18 +0200 Subject: [PATCH 5/7] changing status and desc to status_code and status_desc --- standards/core/peer-exchange.md | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/standards/core/peer-exchange.md b/standards/core/peer-exchange.md index ed1901b..081b18a 100644 --- a/standards/core/peer-exchange.md +++ b/standards/core/peer-exchange.md @@ -72,8 +72,8 @@ message PeerExchangeRequest { message PeerExchangeResponse { repeated PeerInfo peer_infos = 1; - uint32 status = 10; - optional string desc = 11; + uint32 status_code = 10; + optional string status_desc = 11; } message PeerExchangeRPC { @@ -88,7 +88,7 @@ The `enr` field contains a Waku ENR as specified in [WAKU2-ENR](./enr.md). Requesters send a `PeerExchangeRequest` to a peer. Responders SHOULD include a maximum of `num_peers` `PeerInfo` instances into a response. Responders send a `PeerExchangeResponse` to requesters containing a list of `PeerInfo` instances, which in turn hold an ENR. -Responders, MUST include a `status` in the response in any case and possible descriptive form of the problem if any. +Responders, MUST include a `status_code` in the response in any case and possible descriptive form of the problem if any. ### Possible status codes @@ -96,7 +96,7 @@ Responders, MUST include a `status` in the response in any case and possible des |--------|------|------| | SUCCESS | 200 | Successfull request-respond. | | BAD_REQUEST | 400 | Wrong request payload. It must only contain `request` field. | -| BAD_RESPONSE | 401 | Wrong response payload. If success it must contain `response` and `status` fields. If failure, only `status` is set. | +| BAD_RESPONSE | 401 | Wrong response payload. If success it must contain `response` and `status_code` fields. If failure, only `status_code` is set. | | TOO_MANY_REQUESTS | 429 | DOS protection prevented this request as the current request exceeds the configured request rate. | | SERVICE_UNAVAILABLE | 503 | Request cannot be served, either issue on Responder side or having no suitable peer to issue request. | | DIAL_FAILURE | 599 | Requester side problem calling PeerExchange peer. | From dede8e757f6231be8e771b15a8f28c7cf395e63c Mon Sep 17 00:00:00 2001 From: NagyZoltanPeter <113987313+NagyZoltanPeter@users.noreply.github.com> Date: Wed, 18 Sep 2024 14:38:55 +0200 Subject: [PATCH 6/7] Apply suggestions from code review Co-authored-by: Hanno Cornelius <68783915+jm-clius@users.noreply.github.com> --- standards/core/peer-exchange.md | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/standards/core/peer-exchange.md b/standards/core/peer-exchange.md index 081b18a..a88c9cb 100644 --- a/standards/core/peer-exchange.md +++ b/standards/core/peer-exchange.md @@ -88,13 +88,14 @@ The `enr` field contains a Waku ENR as specified in [WAKU2-ENR](./enr.md). Requesters send a `PeerExchangeRequest` to a peer. Responders SHOULD include a maximum of `num_peers` `PeerInfo` instances into a response. Responders send a `PeerExchangeResponse` to requesters containing a list of `PeerInfo` instances, which in turn hold an ENR. -Responders, MUST include a `status_code` in the response in any case and possible descriptive form of the problem if any. +Responders, MUST include a `status_code` in the response. +They MAY include a descriptive status in the `status_desc` field if available. ### Possible status codes | Result | Code | Note | |--------|------|------| -| SUCCESS | 200 | Successfull request-respond. | +| SUCCESS | 200 | Successful request-response. | | BAD_REQUEST | 400 | Wrong request payload. It must only contain `request` field. | | BAD_RESPONSE | 401 | Wrong response payload. If success it must contain `response` and `status_code` fields. If failure, only `status_code` is set. | | TOO_MANY_REQUESTS | 429 | DOS protection prevented this request as the current request exceeds the configured request rate. | @@ -157,7 +158,8 @@ Requesters send a simple message request which causes responders to engage in am As a mitigation, responders MAY feature a `seen cache` for requests and only answer once per time interval. The exchange-peer cache discussed in [Theory and Protocol Semantics Section](#theory-and-protocol-semantics) also provides mitigation. Still, frequent queries can tigger the refresh cycle more often. The `seen cache` MAY be used in conjunction to provide additional mitigation. -Responders MAY apply request limits and thus can reject answering requests within a certain time window. Requestes must be prepared to handle this case. +Responders MAY apply request limits and thus can reject answering requests within a certain time window. +Requesters must be prepared to handle this case. ### Further Considerations From eb1711de4a38cd74dd366867803cced8685419ba Mon Sep 17 00:00:00 2001 From: NagyZoltanPeter <113987313+NagyZoltanPeter@users.noreply.github.com> Date: Wed, 18 Sep 2024 14:42:02 +0200 Subject: [PATCH 7/7] Describe status code means better. --- standards/core/peer-exchange.md | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/standards/core/peer-exchange.md b/standards/core/peer-exchange.md index a88c9cb..2f23954 100644 --- a/standards/core/peer-exchange.md +++ b/standards/core/peer-exchange.md @@ -97,10 +97,10 @@ They MAY include a descriptive status in the `status_desc` field if available. |--------|------|------| | SUCCESS | 200 | Successful request-response. | | BAD_REQUEST | 400 | Wrong request payload. It must only contain `request` field. | -| BAD_RESPONSE | 401 | Wrong response payload. If success it must contain `response` and `status_code` fields. If failure, only `status_code` is set. | +| BAD_RESPONSE | 401 | Malformed or illegal response payload returned. If success it must contain `response` and `status_code` fields. If failure, only `status_code` is set. | | TOO_MANY_REQUESTS | 429 | DOS protection prevented this request as the current request exceeds the configured request rate. | -| SERVICE_UNAVAILABLE | 503 | Request cannot be served, either issue on Responder side or having no suitable peer to issue request. | -| DIAL_FAILURE | 599 | Requester side problem calling PeerExchange peer. | +| SERVICE_UNAVAILABLE | 503 | Request cannot be performed on requester (client) side, either issue on Responder side or having no suitable peer to issue request. | +| DIAL_FAILURE | 599 | Requester (client) side problem calling PeerExchange peer. | ## Implementation Suggestions