Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: Adjusted peer-exchange to the latest changes made due to rate limit DOS protection #39

Merged
merged 7 commits into from
Sep 18, 2024

Conversation

NagyZoltanPeter
Copy link
Contributor

As a consequence of waku-org/nwaku#3035 and waku-org/nwaku#3028
Peer Exchange protocol specification must be changed.
In order to reflect possible error cases such as request is rejected due to rate limitation on responder side we must introduce response_status field which can hold detailed information of the processing outcome.

Status codes are also defined while response_status can contain descriptive information also.

Copy link

@chaitanyaprem chaitanyaprem left a comment

Choose a reason for hiding this comment

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

one more point to note is changes to this spec needs to be implemented in go-waku and hence need to go live together in status app and fleet nodes. Otherwise compatibility needs to be maintained so that even if we go live in fleet first, clients should not get affected.

Also note that peer-exchange service is being provided by status-desktop as well which means update of fleet and desktop instances should be planned together if possible.

So, not sure if we want to take this live as part of status fleets or only as general protoocl update. just wanted to higlight this point.

standards/core/peer-exchange.md Outdated Show resolved Hide resolved
standards/core/peer-exchange.md Show resolved Hide resolved
@NagyZoltanPeter
Copy link
Contributor Author

one more point to note is changes to this spec needs to be implemented in go-waku and hence need to go live together in status app and fleet nodes. Otherwise compatibility needs to be maintained so that even if we go live in fleet first, clients should not get affected.

Also note that peer-exchange service is being provided by status-desktop as well which means update of fleet and desktop instances should be planned together if possible.

So, not sure if we want to take this live as part of status fleets or only as general protoocl update. just wanted to higlight this point.

Yes, thank you for this point. I meant this PR also to talk about it openly.
My idea was not to break existing functionality while able to add DOS protection to PeerEx.
Also in nwaku implementation I found that there are error cases handled but no answer sent back at all to requester.
This is fixed also in PR: waku-org/nwaku#3035

But in normal - previous - usage these change shall not affect clients using the old protocol.
Feel free to review nwaku pr.

…rd compatibility with the former protocol format
@NagyZoltanPeter
Copy link
Contributor Author

one more point to note is changes to this spec needs to be implemented in go-waku and hence need to go live together in status app and fleet nodes. Otherwise compatibility needs to be maintained so that even if we go live in fleet first, clients should not get affected.
Also note that peer-exchange service is being provided by status-desktop as well which means update of fleet and desktop instances should be planned together if possible.
So, not sure if we want to take this live as part of status fleets or only as general protoocl update. just wanted to higlight this point.

Yes, thank you for this point. I meant this PR also to talk about it openly. My idea was not to break existing functionality while able to add DOS protection to PeerEx. Also in nwaku implementation I found that there are error cases handled but no answer sent back at all to requester. This is fixed also in PR: waku-org/nwaku#3035

But in normal - previous - usage these change shall not affect clients using the old protocol. Feel free to review nwaku pr.

@chaitanyaprem: I applied the changes, please have a look.

Copy link

@chaitanyaprem chaitanyaprem left a comment

Choose a reason for hiding this comment

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

LGTM, barring minor comment.

standards/core/peer-exchange.md Outdated Show resolved Hide resolved
Copy link
Contributor

@jm-clius jm-clius left a comment

Choose a reason for hiding this comment

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

Thanks! I have a couple of comments below, mostly minor and related to formulation. I think the most important to clarify is around field names. I think at some point the status and desc fields might have changed names from respond and response_status and this is not yet reflected in the description?

standards/core/peer-exchange.md Outdated Show resolved Hide resolved
standards/core/peer-exchange.md Outdated Show resolved Hide resolved
standards/core/peer-exchange.md Outdated Show resolved Hide resolved
standards/core/peer-exchange.md Outdated Show resolved Hide resolved
standards/core/peer-exchange.md Outdated Show resolved Hide resolved
standards/core/peer-exchange.md Outdated Show resolved Hide resolved
Copy link
Contributor

@jm-clius jm-clius left a comment

Choose a reason for hiding this comment

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

Very minor comments, but can be merged when addressed

standards/core/peer-exchange.md Outdated Show resolved Hide resolved
standards/core/peer-exchange.md Outdated Show resolved Hide resolved
standards/core/peer-exchange.md Outdated Show resolved Hide resolved
Copy link

@chaitanyaprem chaitanyaprem left a comment

Choose a reason for hiding this comment

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

few comments otherwise looks good.

standards/core/peer-exchange.md Outdated Show resolved Hide resolved
standards/core/peer-exchange.md Outdated Show resolved Hide resolved
NagyZoltanPeter and others added 2 commits September 18, 2024 14:38
Co-authored-by: Hanno Cornelius <68783915+jm-clius@users.noreply.github.com>
@NagyZoltanPeter NagyZoltanPeter merged commit af8c261 into master Sep 18, 2024
0 of 2 checks passed
@NagyZoltanPeter NagyZoltanPeter deleted the chore-enhance-peerexchange-status-response branch September 18, 2024 12:43
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