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

[TRACKING] Active local discovery in Brave #21

Closed
parkan opened this issue Feb 5, 2020 · 14 comments
Closed

[TRACKING] Active local discovery in Brave #21

parkan opened this issue Feb 5, 2020 · 14 comments
Assignees
Labels
tracking issue Tracking issue for a grant proposal in review type:targeted-grant A grant made directly to a specific recipient

Comments

@parkan
Copy link
Contributor

parkan commented Feb 5, 2020

Tracking for https://github.com/ipfs/devgrants/blob/master/targeted-grants/active-local-discovery-in-brave.md

@parkan parkan added type:targeted-grant A grant made directly to a specific recipient tracking issue Tracking issue for a grant proposal in review labels Feb 5, 2020
@parkan parkan self-assigned this Feb 5, 2020
@lidel
Copy link
Member

lidel commented Apr 1, 2020

Late update: grant is approved and paperwork signed, @RangerMauve will be picking this up in the second half of April (finishing ongoing projects).

Note to self: sync before work starts, update on latest developments in js-ipfs/libp2p/Brave.

@RangerMauve
Copy link
Contributor

Getting started on this this week. 😁

@RangerMauve
Copy link
Contributor

Did some initial digging, traced it down to there being an error when binding that says the address is already in use.

There was a chromium patch three years ago that should fix this.

I think we need to try to get the patch into Brave.

Gonna clone brave and try to apply the patch to see if it fixes it

@RangerMauve
Copy link
Contributor

Getting errors trying to build Brave on Linux.

/usr/bin/ld: cannot find Scrt1.o: No such file or directory
          /usr/bin/ld: cannot find crti.o: No such file or directory
          collect2: error: ld returned 1 exit status

Will investigate tomorrow

@RangerMauve
Copy link
Contributor

The error magically changed overnight. Probably because I was trying to build some electron based app and had installed some packages for that.

Kinda blocked for now since I'm not comfortable with these build systems. Hoping to get some help from the brave folks.

@RangerMauve
Copy link
Contributor

RangerMauve commented May 4, 2020

Got brave compiling and running (WOW that took a while), looking at applying the patch to UDP socket API. First going to apply it naively, then follow the guidelines here (Thanks Lidel!)

@RangerMauve
Copy link
Contributor

Looks like the source changed a bunch since that patch was made so this might require me to make something new. 🙃

@RangerMauve
Copy link
Contributor

I think I've got a patch together. Will test it either later tonight or tomorrow.

@RangerMauve
Copy link
Contributor

Fixed up some syntax errors and stuff I missed in my C++ and it seems to work with my minimal test case! https://github.com/RangerMauve/chromium-mdns-test

I've got both the browser and node broadcasting / receiving MDNS queries, and the errors that showed up saying that the address was already bound in Brave are no longer there.

Gonna make a branch of chrome-dgram with my changes, then test it out with IPFS-companion proper.

I'm pretty excited since this would require no changes to the current MDNS spec in IPFS and would only need an update to chrome-dgram in the dependencies of IPFS-companion! 😁

@RangerMauve
Copy link
Contributor

It's working! I've got two brave instances running and they're finding each other over MDNS. 😁

My chrome-dgram changes are here: https://github.com/RangerMauve/chrome-dgram/tree/allow-address-reuse

Gonna start working on following the guide for making a proper brave patch.

@lidel
Copy link
Member

lidel commented Jul 6, 2020

The final part of the last milestone started taking much longer than expected due to external circumstances, so I believe it is useful to write down some context, as it makes interesting case study of a successful devgrant that got impacted by externals circumstances.

Shifting landscape

Brave reviewed fix with Chromium changes (brave/brave-browser#9630) and was open to accepting it, as long the patch works on macOS and Windows. Unfortunately it turned out to be incompatible with the way port sharing works in Linux, and additional work is required for macOS-specific version of the patch.

And here is where things get difficult: even if that work is successful, upstream Chromium patch is effectively dead on arrival because Google announced deprecation of Chrome Apps APIs by 2022, so submitting upstream fix is quite futile (by the time it gets reviewed and goes over Canary/Beta/Stable it will be even closer to EOL of used APIs anyway).

The patch could be maintained in userland (Brave), but that approach puts all the maintenance burden on the Brave team.
In the meantime, parallel work on bundling embedded go-ipfs with Brave started in brave/brave-browser#10220, changing the landscape of needs and priorities in Brave. Namely, switching to go-ipfs would fix local discovery too, making chrome.sockets.udp deprecation in Chromium no longer a problem.

TL;DR

Given those external circumstances, I believe the main purpose of devgrant got fulfilled: @RangerMauve confirmed local discovery with our DNS-SD/mdns spec works in Brave on Linux, and the problem turned out to be a bug in Chromium, which requires platform-specific fixes of already deprecated APIs.

@parkan I synced with @RangerMauve and @autonome, and we agreed to end devgrant and consider it fully completed – what would be the next step for @RangerMauve?

@autonome
Copy link
Contributor

autonome commented Jul 8, 2020

This was fantastic work - a super challenging spelunking mission, and we're chuffed at being able to work with you on it @RangerMauve! Looking forward to more of these 😄

@RangerMauve
Copy link
Contributor

Hey, just wanted to follow up on this. Would anyone mind sending me an email to let me know where to send the invoice? 😁

@autonome
Copy link
Contributor

Sending email!

@lidel lidel closed this as completed Oct 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tracking issue Tracking issue for a grant proposal in review type:targeted-grant A grant made directly to a specific recipient
Projects
None yet
Development

No branches or pull requests

4 participants