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

fix: Revert "feat: shard aware peer management (#2151)" #2312

Merged
merged 2 commits into from
Dec 20, 2023

Conversation

Ivansete-status
Copy link
Collaborator

Description

This reverts commit dba9820.

We need to revert this commit because
the waku-simulator stopped working. i.e. the nodes couldn't establish connections among them: https://github.com/waku-org/waku-simulator/tree/054ba9e33f4fdcdb590bcfe760a5254069c5cb9f

Also, the following js-waku test fails due to this commit: "same cluster, different shard: nodes connect"

This approach is preferred because the #2151 PR was quite complex and I think is better to take a temporary
step back, re-think and re-test it until we make sure the waku-simulator and all js-waku tests work.

This reverts commit dba9820.

We need to revert this commit because
the waku-simulator stopped working. i.e. the nodes couldn't establish
connections among them: https://github.com/waku-org/waku-simulator/tree/054ba9e33f4fdcdb590bcfe760a5254069c5cb9f

Also, the following js-waku test fails due to this commit:
"same cluster, different shard: nodes connect"
Copy link

You can find the image built from this PR at

quay.io/wakuorg/nwaku-pr:2312

Built from fa7218c

Copy link
Contributor

@gabrielmer gabrielmer left a comment

Choose a reason for hiding this comment

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

LGTM, thanks so much!

Copy link
Contributor

@NagyZoltanPeter NagyZoltanPeter left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for going after steadily.

@Ivansete-status Ivansete-status merged commit 32668f4 into master Dec 20, 2023
10 checks passed
@Ivansete-status Ivansete-status deleted the revert-dba9820c1 branch December 20, 2023 14:23
@alrevuelta
Copy link
Contributor

alrevuelta commented Dec 20, 2023

Mind elaborating on why this is causing issues? I would like to fully understand where the problem is before reverting. Not sure "the waku-simulator stopped working. i.e. the nodes couldn't establish connections among them" is enough as a reason.

waku-simulator is not using cluster-id, perhaps that can be related. that may be impacting peer management strategy. But ofc, even without that, it should work.

created a tracker for that, slightly related.
waku-org/waku-simulator#16

@Ivansete-status
Copy link
Collaborator Author

Ivansete-status commented Dec 21, 2023

Mind elaborating on why this is causing issues? I would like to fully understand where the problem is before reverting. Not sure "the waku-simulator stopped working. i.e. the nodes couldn't establish connections among them" is enough as a reason.

waku-simulator is not using cluster-id, perhaps that can be related. that may be impacting peer management strategy. But ofc, even without that, it should work.

created a tracker for that, slightly related. waku-org/waku-simulator#16

Thanks for the comments @alrevuelta !

This is precisely the actual reason: we cannot deliver things that are not backward-compatible.

That change made the current waku-simulator setup fail and this shouldn't happen, at least in an abrupt way.

All the breaking changes should be properly documented and informed. If we make a big change we need to ensure a smooth transition to users in the same way we keep deprecated wakunode2 parameters for at least two consecutive releases.

I didn't test this issue in wakuv2.test but it will likely happen as well because the wakuv2.* fleets don't set any cluster-id either.

Another reason to justify that "revert" decision is the following issue from js-waku tests: waku-org/js-waku#1765
EDITED: that was the js-waku test that failed: "same cluster, different shard: nodes connect".

Let me know if need more details :)

Cheers

Ivansete-status added a commit that referenced this pull request Jan 9, 2024
This reverts commit dba9820.

We need to revert this commit because
the waku-simulator stopped working. i.e. the nodes couldn't establish
connections among them: https://github.com/waku-org/waku-simulator/tree/054ba9e33f4fdcdb590bcfe760a5254069c5cb9f

Also, the following js-waku test fails due to this commit:
"same cluster, different shard: nodes connect"

* waku_lightpush/protocol.nim: minor changes to make it compile after revert
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.

None yet

4 participants