feat: proxy all types of delegated routing #23

hacdias · 2023-11-20T13:11:20Z

This is my initial version of the "everything bagel" for someguy: proxy everything we get from other sources and spit it back when asked. I did not use that much code from go-libp2p-routing-helpers however. Our Delegated Routing stuff uses iterators, while libp2p relies on channels. Makes it very hard to re-use most of the code.

To Do

~~Local storage for local caching purposes~~ (Extracted to Local storage for local caching purposes #25)
Figure out why the network on my laptop stops working properly if I have this turned on for a few minutes.
- Only affects laptop, other devices connected to same network are fine.
- Doesn't seem to happen in all networks.
- Conclusion: maybe home router has some per-device limit? Likely. Disabling accelerated DHT works: feat: proxy all types of delegated routing #23 (review)

cc @aschmahmann

aschmahmann

Thanks for starting this 🙏.

Figure out why the network on my laptop stops working properly if I have this turned on for a few minutes.

My guess is running the accelerated dht client isn't making your router happy, but it has some QoS protections (like per-device limits) to protect your devices from each other. You can use a cli flag to enable/disable the accelerated DHT client and you should see if that makes the difference for you.

server_routers.go

aschmahmann · 2023-11-29T18:01:12Z

server_routers.go

+type parallelRouter struct {
+	routers []server.ContentRouter
+}


This code seems complicated enough that we should have some tests. The code looks similar to the go-libp2p-routing-helpers code so might be you can crib tests from there.

I still have to add more tests. It's not as easy to port the tests from libp2p as I thought

server_routers.go

hacdias · 2023-11-30T12:23:43Z

My guess is running the accelerated dht client isn't making your router happy, but it has some QoS protections (like per-device limits) to protect your devices from each other. You can use a cli flag to enable/disable the accelerated DHT client and you should see if that makes the difference for you.

~~I will check this once at home. I haven't had this issue on the coworking space.~~

Edit: seems to be the case.

aschmahmann · 2023-11-30T16:09:37Z

server_routers.go

+		it, itErr := call(ri)
+
+		if itErr != nil {
+			err = errors.Join(err, itErr)


how are you thinking we're going to detect errors (e.g. with cid.contact) in prod? e.g. should we be logging from here or within one of the other packages? Does sending an error back along with the results make sense or will that cause problems?

Personally, I would always prefer that the caller handles the errors. Unfortunately, Boxo's server implementation ignores closing errors:

https://github.com/ipfs/boxo/blob/08959f281f866bd4197b40bc90cf75c2828c07f0/routing/http/server/server.go#L351

https://github.com/ipfs/boxo/blob/08959f281f866bd4197b40bc90cf75c2828c07f0/routing/http/server/server.go#L495

So we'd probably have to add it here if we want them.

Fair, does that mean we should be doing some logging in the meanwhile as a sanity check around cid.contact errors?

server_routers.go

aschmahmann · 2023-11-30T20:42:05Z

server_routers.go

+	return mi.its[mi.next].Val()
+}
+
+func (mi *manyIter[T]) Close() error {


May not really matter here (and might not be required if the semantics of iter.Close are intentionally loose), but is calling Next() after Close() supposed to work or not?

Doesn't seem like you need to worry about concurrent reads+closes (https://github.com/ipfs/boxo/blob/08959f281f866bd4197b40bc90cf75c2828c07f0/routing/http/types/iter/iter.go#L6), but not sure if all implementations are supposed to follow a similar pattern to https://github.com/ipfs/boxo/blob/08959f281f866bd4197b40bc90cf75c2828c07f0/routing/http/types/iter/json.go#L55

May not really matter here (and might not be required if the semantics of iter.Close are intentionally loose), but is calling Next() after Close() supposed to work or not?

As far as I understand, yes. The documentation states Next sets the iterator to the next value, returning true if an attempt was made to get the next value., which for me reads that: after Close, Next will always return false as it does not make an attempt to get the next value.

codecov-commenter · 2023-12-04T09:15:10Z

Codecov Report

Attention: 156 lines in your changes are missing coverage. Please review.

Comparison is base (0d72338) 0.00% compared to head (cb65e75) 23.67%.

Files	Patch %	Lines
server_routers.go	57.26%	98 Missing and 5 partials ⚠️
server_dht.go	0.00%	41 Missing ⚠️
server.go	0.00%	12 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff            @@
##           main      #23       +/-   ##
=========================================
+ Coverage      0   23.67%   +23.67%     
=========================================
  Files         0        5        +5     
  Lines         0      583      +583     
=========================================
+ Hits          0      138      +138     
- Misses        0      440      +440     
- Partials      0        5        +5

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

- use WaitGroup and select for ctx.Done() - call .Val right after .Next inside Goroutine

hacdias · 2023-12-05T13:20:05Z

@aschmahmann I've added some tests. They're a bit more basic than the ones we have in https://github.com/libp2p/go-libp2p-routing-helpers, but they should cover enough for now. I think I addressed all the feedback.

I extracted the local storage to a separate issue to tackle in a different PR.

hacdias · 2024-01-02T12:23:05Z

Ping @aschmahmann

hacdias · 2024-01-12T13:28:13Z

new ping for @aschmahmann ;)

aschmahmann

Apologies for the delay here @hacdias and thanks for the pings + callouts. Made a few relatively small comments but should be good to go otherwise.

If after the changes you're blocked for me on review just ask @Jorropo to review. I'd expect the changes here should be pretty small an easy for me to review though so he doesn't have to load context into the whole PR.

aschmahmann · 2024-01-15T12:33:46Z

server_routers.go

+		it, itErr := call(ri)
+
+		if itErr != nil {
+			err = errors.Join(err, itErr)


Fair, does that mean we should be doing some logging in the meanwhile as a sanity check around cid.contact errors?

aschmahmann · 2024-01-15T12:48:57Z

server_routers.go

+	mi.done = true
+	mi.wg.Wait()


It seems like there might be a race here. For example: assume the context passed to newManyIter never gets cancelled, but:

it.Next returns true in the newManyIter goroutine but the select statement hasn't finished yet

The user calls it.Cancel

The it.Cancel call is blocked on wg.Wait because it'll never be possible to read it.val into mi.ch

One way to fix this is to add a cancel func to the struct that's tied to the context (via context.WithCancel()) and call that before triggering the Wait.

Jorropo · 2024-01-15T15:44:53Z

I'm missing something but it seems this code is duplicated from go-libp2p-routing-helpers, this is tricky code (I added a fair amount of deadlocks in go-libp2p-routing-helpers) I would rather squash bugs once there.

hacdias · 2024-01-15T15:58:16Z

I'm missing something but it seems this code is duplicated

It isn't. It can't be used directly. I had to make many changes. It simpler in many ways. We've discussed this before - perhaps only in calls. It wasn't possible to reuse the libp2p router helpers.

I'm merging this as per @aschmahmann approval. We can iterate later.

hacdias requested a review from aschmahmann November 20, 2023 13:11

hacdias force-pushed the parallel branch from abb0aad to de535a2 Compare November 27, 2023 10:11

hacdias changed the title ~~feat: proxy everything~~ feat: proxy all types of delegated routing Nov 27, 2023

hacdias force-pushed the parallel branch from de535a2 to 692345c Compare November 27, 2023 10:16

feat: proxy all delegated routing

3a9d78c

hacdias force-pushed the parallel branch from 8172c4d to 3a9d78c Compare November 27, 2023 11:22

aschmahmann requested changes Nov 29, 2023

View reviewed changes

hacdias added 4 commits November 30, 2023 10:59

refactor: use generics for FindProviders and FindPeers

72cc034

refactor: manyIter now channels

27a543b

refactor: cleanup names and interfaces

6b5640d

feat: add some tests

aff7675

hacdias force-pushed the parallel branch from e72bff8 to aff7675 Compare November 30, 2023 12:23

hacdias self-assigned this Nov 30, 2023

aschmahmann requested changes Nov 30, 2023

View reviewed changes

hacdias force-pushed the parallel branch 2 times, most recently from 8cde154 to 1975da4 Compare December 4, 2023 11:47

hacdias added 2 commits December 4, 2023 12:48

refactor: ensure goroutine-safety of manyIter

12b42a9

- use WaitGroup and select for ctx.Done() - call .Val right after .Next inside Goroutine

test: add some tests for manyIter

a71edeb

hacdias force-pushed the parallel branch from 1975da4 to a71edeb Compare December 4, 2023 11:48

add some basic tests

ad1b8e2

hacdias force-pushed the parallel branch from 884e2d9 to ad1b8e2 Compare December 5, 2023 13:18

hacdias requested a review from aschmahmann December 5, 2023 13:18

fmt

fabe115

2color mentioned this pull request Jan 8, 2024

Expose trustless gateway and delegated routing under new domains with certain policies ipshipyard/waterworks-community#1

Closed

19 tasks

ns4plabs mentioned this pull request Jan 11, 2024

add http handlers for CORS, metrics and compression #27

Closed

aschmahmann requested changes Jan 15, 2024

View reviewed changes

address feedback

cb65e75

hacdias requested a review from aschmahmann January 15, 2024 14:50

aschmahmann approved these changes Jan 15, 2024

View reviewed changes

hacdias merged commit df191a3 into main Jan 15, 2024
4 checks passed

hacdias deleted the parallel branch January 15, 2024 15:59

lidel mentioned this pull request Mar 13, 2024

delegated-ipfs.dev returning fewer results than local kubo ipshipyard/waterworks-community#2

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: proxy all types of delegated routing #23

feat: proxy all types of delegated routing #23

hacdias commented Nov 20, 2023 •

edited

aschmahmann left a comment •

edited

aschmahmann Nov 29, 2023

hacdias Nov 30, 2023

hacdias commented Nov 30, 2023 •

edited

aschmahmann Nov 30, 2023

hacdias Dec 4, 2023

aschmahmann Jan 15, 2024

aschmahmann Nov 30, 2023

hacdias Dec 4, 2023 •

edited

codecov-commenter commented Dec 4, 2023 •

edited

hacdias commented Dec 5, 2023

hacdias commented Jan 2, 2024

hacdias commented Jan 12, 2024

aschmahmann left a comment

aschmahmann Jan 15, 2024

aschmahmann Jan 15, 2024

Jorropo commented Jan 15, 2024

hacdias commented Jan 15, 2024 •

edited

feat: proxy all types of delegated routing #23

feat: proxy all types of delegated routing #23

Conversation

hacdias commented Nov 20, 2023 • edited

To Do

aschmahmann left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hacdias commented Nov 30, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hacdias Dec 4, 2023 • edited

Choose a reason for hiding this comment

codecov-commenter commented Dec 4, 2023 • edited

Codecov Report

hacdias commented Dec 5, 2023

hacdias commented Jan 2, 2024

hacdias commented Jan 12, 2024

aschmahmann left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jorropo commented Jan 15, 2024

hacdias commented Jan 15, 2024 • edited

hacdias commented Nov 20, 2023 •

edited

aschmahmann left a comment •

edited

hacdias commented Nov 30, 2023 •

edited

hacdias Dec 4, 2023 •

edited

codecov-commenter commented Dec 4, 2023 •

edited

hacdias commented Jan 15, 2024 •

edited