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

mdns: don't discover ourselves #1661

Merged
merged 5 commits into from
Aug 2, 2022
Merged

Conversation

MGMCN
Copy link
Contributor

@MGMCN MGMCN commented Jul 26, 2022

No description provided.

@marten-seemann marten-seemann changed the title bug fixed in mdns.go mdns: don't discover ourselves Jul 26, 2022
@marten-seemann marten-seemann self-requested a review July 26, 2022 12:11
@marten-seemann
Copy link
Contributor

Looks like we had a test case TestSelfDiscovery asserting that we discover ourselves. I'm not why that would be useful. @Stebalien, wdyt?

@MGMCN
Copy link
Contributor Author

MGMCN commented Jul 26, 2022

Thank you for your reply.
Yes, I saw the test.
Maybe I overlooked some details?
But I don't know the meaning of finding ourselves.

@MGMCN
Copy link
Contributor Author

MGMCN commented Jul 26, 2022

image

Because after this code modification, both "TestSelfDiscovery" and "TestOtherDiscovery" failed. I have a bold guess.

Before we modify the code, whether these two tests passed because peer found itself.

But this is just my assumption.

Looking forward to your reply.

@Stebalien
Copy link
Member

Looks like we had a test case TestSelfDiscovery asserting that we discover ourselves. I'm not why that would be useful. @Stebalien, wdyt?

I have no idea...

@marten-seemann
Copy link
Contributor

Then let's remove it. I don't see a use case for self-discovery. @MGMCN, do you want to modify the test cases?

@MGMCN
Copy link
Contributor Author

MGMCN commented Jul 27, 2022

Thank you for your reply.

I have removed this test function(TestSelfDiscovery).

@marten-seemann
Copy link
Contributor

Looks like there's another test that's failing as well.

@MGMCN
Copy link
Contributor Author

MGMCN commented Jul 27, 2022

Thank you for your reply.

Yes, I've seen it.

I'll check it tomorrow.

@MGMCN
Copy link
Contributor Author

MGMCN commented Jul 28, 2022

Hello, I think I probably know why our TestOtherDiscovery test failed.

for _, id := range hostIDs { ... )

Because we traverse all hostids in line 82 of the mdns_test.go, but because we have modified the mDNS code, we make the current host unable to find itself. In other words, when we test the file, if we find that the ith host is checking whether we have found peers other than ourselves, we should skip checking whether this host is in our discovery list. But if we follow the original logic, we will also find out whether ith host are in this ith host's discovery list.

After I modify the logic of the test code, the test case passes.

@marten-seemann marten-seemann self-requested a review July 28, 2022 14:18
@MGMCN
Copy link
Contributor Author

MGMCN commented Jul 31, 2022

Sorry, is there anything else I need to correct about this PR?

Copy link
Contributor

@marten-seemann marten-seemann left a comment

Choose a reason for hiding this comment

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

Please don't remove the range loops in the test. Passing around array indices is very difficult to reason about.

ids := make([]peer.ID, 0, len(infos))
for _, info := range infos {
ids = append(ids, info.ID)
}
if !containsAllHostIDs(ids) {
if !containsAllHostIDs(ids,i) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not just pass the peer ID into this function?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for your suggestion.
I've revised it.
Do you think it's ok?
Please contact me if there is anything else that needs to be modified.

Copy link
Contributor

@marten-seemann marten-seemann left a comment

Choose a reason for hiding this comment

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

The code lgtm, but gofmt is unhappy.

@MGMCN
Copy link
Contributor Author

MGMCN commented Aug 2, 2022

Hahaha.
Thank you for your reply.
Maybe I need to execute "gofmt -s -w" on the mdns_test.go?

@marten-seemann
Copy link
Contributor

Thank you @MGMCN!

@marten-seemann marten-seemann merged commit 52e5a35 into libp2p:master Aug 2, 2022
@MGMCN MGMCN deleted the BugFixInMdns branch August 3, 2022 01:52
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