Implementation of delegated routing based on the Edelweiss compiler #11

petar · 2022-03-08T16:39:27Z

Ref.

Resolves ipld/edelweiss#12

BigLep · 2022-03-08T20:56:55Z

@petar : probably my naivety not to know or see it, but can you provide:

command/process you ran to generate this code?
the schema that was used for the generation?

petar · 2022-03-08T21:06:54Z

It’s all in the repo in package gen.

…

On Tue, Mar 8, 2022 at 12:57 PM Steve Loeppky ***@***.***> wrote: @petar <https://github.com/petar> : probably my naivety not to know or see it, but can you provide: 1. command/process you ran to generate this code? 2. the schema that was used for the generation? — Reply to this email directly, view it on GitHub <#11 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AACFTS4XYUXX33IO6PYYHDLU665KLANCNFSM5QG33JQQ> . You are receiving this because you were mentioned.Message ID: ***@***.***>

gen/routing.go

BigLep · 2022-03-11T06:00:14Z

A couple of thoughts here: I'm sure there's legit and genuine curiosity here given that this is some of the first generated code we're seeing come out of Edelweiss (great). I know @petar needs something to target towards and is using the existing proposed schema. We ultimately though need to get the schema fully decided as part of #8.

Thanks @petar for doing some explanation. I know it's on the Edelweiss project roadmap to be working more on docs and communication this month. It looks like those will be welcome and appreciated! Good stuff!

BigLep · 2022-03-29T16:39:24Z

Moving back to in progress because this can't be in review until the spec ipfs/specs#272 is completed.

BigLep · 2022-04-04T18:10:43Z

@petar : thanks for owning this and make the updates for ipfs/specs#272. I'm assuming once the spec changes fully land, you'll keep updating this.

Also, given we don't want to spend people's time reviewing the generated code itself, it would be great if the unit test coverage on go-delegated-server was pretty high for client/server communication so that we can have confidence in the generated code as a result. (I'm saying that as a general statement. I don't know how high unit test coverage is right now, but if it should have more, let's please include it.)

petar · 2022-04-12T14:51:46Z

@aschmahmann First draft of delegated routing using the new Reframe API is ready. You can do a pass on this PR and then let's regroup to discuss next steps. At the moment, this PR implements the content routing interface which we already use to connect delegated routing into Hydra. As such, this PR is a drop-in replacement for the current delegated routing in master.

In particular, Get/PutIPNS are not implemented. Also, FindProviders extracts the AddrInfo of all returned provider nodes, regardless of which transport protocols they offer. Perhaps we need to filter out only the bitswap providers, or otherwise we need to extend the Go content routing interface to capture supported transfer protocols.

If I recall correctly, you also want to go a step further and implement a more general content routing interface so that it can be used both in Hydra and IPFS? Let's discuss what that would be.

petar · 2022-04-12T22:32:54Z

@aschmahmann I finished all remaining items we mentioned at standup today:

FindProviders filters Bitswap nodes
GetIPNS and PutIPNS included in the Go interface
Test covers all code-generated code (all method calls)

aschmahmann · 2022-04-13T15:25:01Z

@petar can you hook this up to unified CI? I'm seeing the tests fail and/or hang locally (Windows).

petar · 2022-04-13T16:23:13Z

Yep, I’ve already filed a PR with unified CI :)

…

On Wed, Apr 13, 2022 at 8:25 AM Adin Schmahmann ***@***.***> wrote: @petar <https://github.com/petar> can you hook this up to unified CI? I'm seeing the tests fail and/or hang locally. — Reply to this email directly, view it on GitHub <#11 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AACFTS44MNPADBYOAXU3MXLVE3RNRANCNFSM5QG33JQQ> . You are receiving this because you were mentioned.Message ID: ***@***.***>

… than take them as arguments)

petar · 2022-04-26T23:14:59Z

server/findproviders.go

+func buildPeerFromAddrInfo(addrInfo peer.AddrInfo) *proto.Peer {
+	pm := make([]values.Bytes, len(addrInfo.Addrs))
+	for i, addr := range addrInfo.Addrs {
+		// XXX: Adin, please, verify this is as intended.


@aschmahmann Please, verify this is as intended. PeerAddrs are created from whatever the server-implementing user passes. The code rewrites the multiaddresses to make sure they conform to the spec.

A peer.AddrInfo should not have the last component be a /p2p/peerID in any event. For example some functions that would stop working include https://github.com/libp2p/go-libp2p-core/blob/02cbdcc4191b0f512c6cf4f2b1a8eeaaf32f1061/peer/addrinfo.go#L88

We could clean the data here, but really if this is happening then something else is likely wrong with your application. To me this means either we should just pass it through without validation, or when we validate and there's a problem we should return an error to the application to let them know they passed in invalid input.

petar · 2022-04-26T23:15:54Z

client/findproviders.go

-			}
-			infos = append(infos, *ai)
+// ParseNodeAddresses parses peer node addresses from the protocol structure Peer.
+// XXX: Adin, please, verify this is as intended.


@aschmahmann Please, verify this is as intended.

Looks good 👍

petar · 2022-04-27T17:32:15Z

@aschmahmann I've addressed all the comments. Please take another look.

aschmahmann

Looks good! The main comments are around lack of validation of the responses we get as a client and missing chunks of the IPNS records.

For the IPNS records I'm fine if you'd prefer to split it into a different PR, but I don't think it'll be too much if you want to do it here (i.e. do validation and some basic testing).

aschmahmann · 2022-04-29T19:26:09Z

client/findproviders.go

-			}
-			infos = append(infos, *ai)
+// ParseNodeAddresses parses peer node addresses from the protocol structure Peer.
+// XXX: Adin, please, verify this is as intended.


Looks good 👍

aschmahmann · 2022-04-29T19:33:52Z

server/findproviders.go

+func buildPeerFromAddrInfo(addrInfo peer.AddrInfo) *proto.Peer {
+	pm := make([]values.Bytes, len(addrInfo.Addrs))
+	for i, addr := range addrInfo.Addrs {
+		// XXX: Adin, please, verify this is as intended.


A peer.AddrInfo should not have the last component be a /p2p/peerID in any event. For example some functions that would stop working include https://github.com/libp2p/go-libp2p-core/blob/02cbdcc4191b0f512c6cf4f2b1a8eeaaf32f1061/peer/addrinfo.go#L88

We could clean the data here, but really if this is happening then something else is likely wrong with your application. To me this means either we should just pass it through without validation, or when we validate and there's a problem we should return an error to the application to let them know they passed in invalid input.

client/contentrouting.go

client/findproviders.go

client/contentrouting_test.go

test/clientserver_test.go

client/findproviders.go

aschmahmann · 2022-04-29T20:33:20Z

client/contentrouting.go

+		close(addrInfoCh)
+		return addrInfoCh
+	}
+	go func() {


Thoughts (not blockers for merge):

Not sure if this is avoidable but wanted your thoughts on this. It looks like every abstraction layer involving a channel turns into another goroutine to pull x from the channel and return f(x) into a new channel.

Maybe once generics become more widely supported in our codebase (once Go 1.19 is released) this will become easier by letting us return generic channels and pass in functions to convert between types so a goroutine isn't required.

To be fair short lived goroutines aren't necessarily a big issue unless we're doing them on mass so we can cross this bridge when it becomes a problem.

Yes. Without generics, we would need to resort to empty interfaces, which I think makes the code very unmanageable. I lean towards code readability unless there is a clear performance problem. And indeed it seems like Go generics can fix this issue.

client/contentrouting.go

test/clientserver_test.go

go.mod

test/clientserver_test.go

client/getipns.go

petar · 2022-05-03T16:49:11Z

@aschmahmann It's worth another look. I think I've addressed all comments in this round.

aschmahmann

Looks good! There may be some more things we learn about here and need to improve the wrapper APIs down the road, but this is enough for us to make some good progress trying to integrate.

Thanks for all the hard work here 🙏

petar · 2022-05-04T17:00:26Z

@aschmahmann Here's what I am thinking should be next steps:

I'll make a release on Edelweiss at its current commit
Bump the dependencies here on the Edelweiss release
Merge this PR
Then:
Start a Hydra PR that utilizes this PR
Then we need to discuss re-deployment of Hydra
Sound good?

aschmahmann · 2022-05-04T20:47:54Z

@petar sounds like a plan to me 😄. In order for the hydras to properly utilize it there needs to be some server responding to Reframe requests.

However, you might save yourself some pain by working on getting the indexers to serve the data using Reframe earlier. Otherwise you'll have to get the Hydras to support both the Indexer and Reframe endpoints.

Your call though, if you think it'll be easier to start off working in the Hydras then start there.

petar · 2022-05-05T02:02:18Z

@petar sounds like a plan to me 😄. In order for the hydras to properly utilize it there needs to be some server responding to Reframe requests.

However, you might save yourself some pain by working on getting the indexers to serve the data using Reframe earlier. Otherwise you'll have to get the Hydras to support both the Indexer and Reframe endpoints.

Your call though, if you think it'll be easier to start off working in the Hydras then start there.

Sounds good! Thanks for reviewing this monster PR :)

petar requested a review from aschmahmann March 8, 2022 16:39

BigLep assigned petar Mar 8, 2022

BigLep added this to the go-ipfs 0.13 milestone Mar 8, 2022

This was referenced Mar 9, 2022

delegated-routing support v1: use configurable delegated-routing endpoints for content-routing ipfs/kubo#8775

Closed

Update to use delegated-routing for querying storetheindex libp2p/hydra-booster#162

Closed

petar mentioned this pull request Mar 9, 2022

Move definition of API into IPLD schema #6

Closed

3 tasks

willscott reviewed Mar 9, 2022

View reviewed changes

gen/routing.go Outdated Show resolved Hide resolved

petar mentioned this pull request Mar 22, 2022

load test auto-generated server/clients as part of CI ipld/edelweiss#12

Closed

BigLep marked this pull request as draft March 29, 2022 16:39

petar marked this pull request as ready for review April 12, 2022 14:45

petar added 11 commits April 14, 2022 15:39

add candidate schema

12cdbc4

split provider record into node and protocol parts

7da4fec

client uses IPLD generically

2f97ac8

adapt FindProviders to new RPC stubs

d9d6e60

first draft of server rewrite

b322e03

client/server pass end-to-end test

3bab749

renames

26b5803

add comments

3ea0a92

add edelweiss defs

27a5cad

integrate delegated client with edelweiss

d84340d

integrate server with edelweiss

6aaa99c

update the service interface to return channels for responses (rather…

b90cf5d

… than take them as arguments)

petar mentioned this pull request Apr 26, 2022

Add Reframe protocol server ipni/storetheindex#251

Closed

petar added 3 commits April 26, 2022 14:57

update edelweiss dep; client now caches unsupported server methods

c3789ea

drop multiaddrs that end in /p2p/peerID

281706c

fix tests and peeraddr encoding/decoding to conform to spec

b0fb24c

petar commented Apr 26, 2022

View reviewed changes

add test for synchronous and asynchronous error propagation

8969196

aschmahmann suggested changes Apr 29, 2022

View reviewed changes

aschmahmann reviewed Apr 29, 2022

View reviewed changes

client/contentrouting.go Show resolved Hide resolved

petar added 4 commits May 2, 2022 08:20

clarifications

7e2df90

update tests

a35c144

add validator to client

1c0cbf6

update test to generate valid IPNS key and record

2bbc866

petar commented May 2, 2022

View reviewed changes

test/clientserver_test.go Show resolved Hide resolved

aschmahmann reviewed May 3, 2022

View reviewed changes

go.mod Outdated Show resolved Hide resolved

aschmahmann reviewed May 3, 2022

View reviewed changes

go.mod Outdated Show resolved Hide resolved

aschmahmann reviewed May 3, 2022

View reviewed changes

go.mod Outdated Show resolved Hide resolved

aschmahmann suggested changes May 3, 2022

View reviewed changes

petar added 3 commits May 3, 2022 06:36

update deps

f907a44

adjustments to ipns handling

dcb5639

reduce test iterations

2617e0c

aschmahmann approved these changes May 4, 2022

View reviewed changes

update deps

3325fa2

petar merged commit 2bc3457 into main May 4, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implementation of delegated routing based on the Edelweiss compiler #11

Implementation of delegated routing based on the Edelweiss compiler #11

petar commented Mar 8, 2022 •

edited

Loading

BigLep commented Mar 8, 2022

petar commented Mar 8, 2022 via email

BigLep commented Mar 11, 2022

BigLep commented Mar 29, 2022

BigLep commented Apr 4, 2022

petar commented Apr 12, 2022

petar commented Apr 12, 2022

aschmahmann commented Apr 13, 2022 •

edited

Loading

petar commented Apr 13, 2022 via email

petar Apr 26, 2022

aschmahmann Apr 29, 2022

petar Apr 26, 2022

aschmahmann Apr 29, 2022

petar commented Apr 27, 2022

aschmahmann left a comment •

edited

Loading

aschmahmann Apr 29, 2022

aschmahmann Apr 29, 2022

aschmahmann Apr 29, 2022

petar May 3, 2022

petar commented May 3, 2022

aschmahmann left a comment

petar commented May 4, 2022

aschmahmann commented May 4, 2022

petar commented May 5, 2022

Implementation of delegated routing based on the Edelweiss compiler #11

Implementation of delegated routing based on the Edelweiss compiler #11

Conversation

petar commented Mar 8, 2022 • edited Loading

BigLep commented Mar 8, 2022

petar commented Mar 8, 2022 via email

BigLep commented Mar 11, 2022

BigLep commented Mar 29, 2022

BigLep commented Apr 4, 2022

petar commented Apr 12, 2022

petar commented Apr 12, 2022

aschmahmann commented Apr 13, 2022 • edited Loading

petar commented Apr 13, 2022 via email

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

petar commented Apr 27, 2022

aschmahmann left a comment • edited Loading

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

Choose a reason for hiding this comment

petar commented May 3, 2022

aschmahmann left a comment

Choose a reason for hiding this comment

petar commented May 4, 2022

aschmahmann commented May 4, 2022

petar commented May 5, 2022

petar commented Mar 8, 2022 •

edited

Loading

aschmahmann commented Apr 13, 2022 •

edited

Loading

aschmahmann left a comment •

edited

Loading