mdns: Refactor encoding/decoding #2162
Conversation
|
I reviewed the part where it randomizes the delay. Looks OK. The IPV6 stuff I am not sufficiently familiar with. |
|
The ipv6 stuff is handled in a different PR #2161 |
|
Is this related to the “devices not syncing” issue Oli showed you recently? (Will review tomorrow) |
|
No, this is related to https://github.com/Actyx/Cosmos/issues/6645 . I don't know why discovery is failing for oli. His problem was connecting mac and windows which was resolved after a restart. I told him how to debug it and posted it again in the banyan channel incase someone else has a similar issue. But as far as I know we don't support mac anyway? I have not been able to reproduce an issue with mdns on linux so far and I have tested it on windows in the past, but I did discover a major problem with our discovery protocol which I haven't yet decided how to fix. Not sure if it is related to oli's issue, probably not. |
For the benefit of the reviewers, it would be then preferable to take it out of this PR, or make this a PR against the other branch. |
dvc94ch
force-pushed
the
mdns-improvements
branch
3 times, most recently
from
6d235ed
to
7f13b2f
Compare
dvc94ch
force-pushed
the
mdns-improvements
branch
from
7f13b2f
to
4c293d4
Compare
|
@mxinden what do you think about this one? I think the first commit is uncontroversial, the second one removes the dependency dns-parser that has not been updated in 4y. I think the new implementation is easier to read, supports utf8 (dns-parser erroneously errors if strings are not ascii) and at least for me easier to maintain/bugfix. I tested it with the previous rust-libp2p, so it should be compatible and if that one was compatible with go/js then it should still be compatible too. |
Co-authored-by: Max Inden <mail@max-inden.de>
There was a problem hiding this comment.
I think the first commit is uncontroversial, the second one removes the dependency dns-parser that has not been updated in 4y.
I am in favor of the first change. I am at least hesitant about the second. I would need to do more research on whether a self-written DNS parser is the right way forward for rust-libp2p.
Would you mind splitting the second change out into its own pull request?
dvc94ch
changed the title
|
@mxinden I'll close this PR for now |
cc @rkuhn