Protocols in providers #465

hannahhoward · 2024-01-25T02:55:47Z

Goals

Allow specifying providers with specific protocols
Cleanup the FixedPeers on request concept that was previously inerted a bit haphazard
Update to new boxo fork (on 0.15.0)

Implementation

Removed retriever.CandidateFinder -- this was a confusing concept, since there is already a types.CandidateFinder that is different. I instead changed this to types.CandidateSource.
retriever.CandidateFinder had two methods --> FindCandidates and FindCandidatesAsync. However, we only used FindCandidatesAsync. Moreover, this is a bit of a weird name, since while the method takes a callback, it is not actually an Async method in the sense that it is intended to be a blocking call. So on types.CandidateSource, we now have only one method FindCandidates, and it has the signature of the old FindCandidatesAsync. (note to keep in line with the interface name, I'm also find to rename this SourceCandidates if it feels important)
created a new Provider type -- this is similar to a types.RetrievalCandidate, but without the cid (I considered going back and removing this rootCID entirely but I think maybe we use it in several places? Also does it always make sense for it to be the root cid?)
modified the DirectCandidateFinder so it can be used with a fixed set of Providers with their own protocols, and you now specify an option if you want to add the protocol discovery features (or just return candidates immediately)
added a new syntax for specifying a peer+protocol combo -- essentially at the end of your multiaddr string, you add a +. This does remove + as an option to use in your multiaddr, but I'm not immediately aware of any cases where this is an allowed character in any of the allowed libraries (other than maybe "httppath" which isn't merged? Not even sure usually + is used in query strings).
updated ParseProviderStrings & GetDescriptorString to handle the parsing
update feat/lassie-fork over in boxo to be rebased on v0.15.0 and update dependency here
this is also supported at the CLI level

For Discussion

I originally tried to put CandidateSource directly on types.RetrievalRequest but then that gets confusing with GetDescriptorString (we'd have to make candidateSource itself have a descriptorString)
I then tried making CandidateSource a method on types.RetrievalRequest but that led to a lot of import cycling and I didn't get to the end of that :)
not sure the best naming here open to input.

instead of retriever.CandidateFinder, use types.CandidateSource, converging on single method FindCandidates

instead of using peer.AddrInfo, protocols now have providers

codecov-commenter · 2024-01-25T03:10:45Z

Codecov Report

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

Comparison is base (f2e96a7) 75.63% compared to head (ab1e2b5) 75.79%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #465      +/-   ##
==========================================
+ Coverage   75.63%   75.79%   +0.16%     
==========================================
  Files          87       87              
  Lines        6657     6656       -1     
==========================================
+ Hits         5035     5045      +10     
+ Misses       1341     1335       -6     
+ Partials      281      276       -5

Files	Coverage Δ
cmd/lassie/flags.go	`51.02% <100.00%> (ø)`
cmd/lassie/main.go	`47.66% <100.00%> (ø)`
pkg/indexerlookup/candidatesource.go	`66.29% <100.00%> (ø)`
pkg/indexerlookup/options.go	`31.66% <ø> (ø)`
pkg/internal/itest/mocknet/mocknet.go	`76.51% <100.00%> (ø)`
pkg/internal/itest/testpeer/generator.go	`75.00% <100.00%> (ø)`
pkg/internal/testutil/gen.go	`63.28% <100.00%> (+0.58%)`	⬆️
pkg/internal/testutil/mockcandidatefinder.go	`83.33% <100.00%> (ø)`
pkg/internal/testutil/verifier.go	`96.49% <100.00%> (ø)`
pkg/retriever/assignablecandidatefinder.go	`100.00% <100.00%> (ø)`
... and 7 more

... and 4 files with indirect coverage changes

pkg/retriever/directcandidatesource.go

rvagg · 2024-01-25T03:26:21Z

pkg/types/request.go

@@ -258,8 +259,24 @@ func ParseProviderStrings(v string) ([]peer.AddrInfo, error) {
 				return nil, err
 			}


would this make sense now? seems a reasonable assumption if they're specifying the address in http:// form

Suggested change

}

}

protocols = append(protocols, metadata.IpfsGatewayHttp{})

I actually had this but then that means GetDescriptorString produces a +http which is a bit odd :P

rvagg · 2024-01-25T03:27:39Z

pkg/types/request_test.go

@@ -200,6 +201,21 @@ func TestRequestStringRepresentations(t *testing.T) {
 		_, err := ParseProviderStrings("http://127.0.0.1:5000/nope")
 		require.ErrorContains(t, err, "paths not supported")
 	})
+
+	t.Run("fixed peer, protocol included", func(t *testing.T) {
+		pps, err := ParseProviderStrings("/ip4/127.0.0.1/tcp/5000/p2p/12D3KooWBSTEYMLSu5FnQjshEVah9LFGEZoQt26eacCEVYfedWA4+bitswap")


some multiaddr aficionado is going to find this one day and get all twitchy

some day, but not this day :P

rvagg · 2024-01-25T03:30:33Z

pkg/types/request.go

@@ -258,8 +259,24 @@ func ParseProviderStrings(v string) ([]peer.AddrInfo, error) {
 				return nil, err
 			}
 		} else {
+			parts := strings.Split(v, "+")


It's likely possible to break multiaddr with the + assumption, you could probably base64 encode the peer ID and end up with + in the /p2p/ component. Not sure how much it's worth worrying about since this is pretty niche use, although this can come in through the CLI and I don't think we really have a handle on how that might be used.

So, I wonder if it shouldn't be fatal below - if we hit the default, print a warning and fall-back to addrString = v and protocols = []?

think I got a decent solution to this -- check the change

rvagg

lgtm, I'll let you decide how to deal with my feedback, I'm fine either way

rvagg · 2024-01-25T03:31:49Z

TestAssignableCandidateFinder race test failures in CI probably worth investigating!

fix race in DirectCandidateSource when not using libp2p

Co-authored-by: Rod Vagg <rod@vagg.org>

bajtos · 2024-01-29T10:14:16Z

Is there any documentation on the new provider format? (We are using the HTTP interface provided by the Lassie Daemon.)
The change looks fully backwards compatible, could you please confirm?

rvagg · 2024-01-29T23:22:19Z

Not documented publicly (yet) but it impacts the "providers" query string and CLI argument, it's backward compatible and this just adds the ability to tack on protocols after the end of the provider ID string, just add a +http, +bitswap, +graphsync to limit the protocol(s) that will be attempted with the provider. They are additive, so:

provider=/dns/beep.boop.com/tcp/3747/p2p/12D3KooWDXAVxjSTKbHKpNk8mFVQzHdBDvR4kybu582Xd4Zrvagg will do what it does now, query the peer and see what it supports and go from there
provider=/dns/beep.boop.com/tcp/3747/p2p/12D3KooWDXAVxjSTKbHKpNk8mFVQzHdBDvR4kybu582Xd4Zrvagg+bitswap will skip the query and go straight to attempting an http retrieval to the address
provider=/dns/beep.boop.com/tcp/3747/p2p/12D3KooWDXAVxjSTKbHKpNk8mFVQzHdBDvR4kybu582Xd4Zrvagg+bitswap+graphsync` will queue up the provider on both bitswap and graphsync protocols and end up attempting them both.

hannahhoward added 4 commits January 24, 2024 08:26

refactor(types): CandidateFinder -> CandidateSource

7b40fb5

instead of retriever.CandidateFinder, use types.CandidateSource, converging on single method FindCandidates

feat(request): allow protocols in providers

f719654

instead of using peer.AddrInfo, protocols now have providers

fix(deps): use updated boxo fork

55ec482

test(retriever): add direct candidate source test

471422a

hannahhoward requested a review from rvagg January 25, 2024 02:56

hannahhoward added 2 commits January 24, 2024 18:59

style(lint): remove unused functions

0997fb8

style(mod): mod tidy

9e72582

hannahhoward requested a review from AmeanAsad January 25, 2024 03:03

rvagg reviewed Jan 25, 2024

View reviewed changes

pkg/retriever/directcandidatesource.go Show resolved Hide resolved

rvagg reviewed Jan 25, 2024

View reviewed changes

rvagg approved these changes Jan 25, 2024

View reviewed changes

hannahhoward and others added 3 commits January 25, 2024 18:27

fix(retriever): fix race

81cf0ff

fix race in DirectCandidateSource when not using libp2p

fix(types): relax typing on provider parsing

e4dba26

Update pkg/retriever/directcandidatesource.go

ab1e2b5

Co-authored-by: Rod Vagg <rod@vagg.org>

hannahhoward merged commit bf83e4b into main Jan 27, 2024
9 checks passed

hannahhoward deleted the feat/protocols-in-providers branch January 27, 2024 01:37

rvagg mentioned this pull request Feb 29, 2024

Update dependencies and golang version #466

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Protocols in providers #465

Protocols in providers #465

hannahhoward commented Jan 25, 2024 •

edited

Loading

codecov-commenter commented Jan 25, 2024 •

edited

Loading

rvagg Jan 25, 2024

hannahhoward Jan 25, 2024

rvagg Jan 25, 2024

hannahhoward Jan 26, 2024

rvagg Jan 25, 2024

hannahhoward Jan 26, 2024

rvagg left a comment

rvagg commented Jan 25, 2024

bajtos commented Jan 29, 2024

rvagg commented Jan 29, 2024

		@@ -258,8 +259,24 @@ func ParseProviderStrings(v string) ([]peer.AddrInfo, error) {
		return nil, err
		}

Protocols in providers #465

Protocols in providers #465

Conversation

hannahhoward commented Jan 25, 2024 • edited Loading

Goals

Implementation

For Discussion

codecov-commenter commented Jan 25, 2024 • edited Loading

Codecov Report

rvagg Jan 25, 2024

Choose a reason for hiding this comment

hannahhoward Jan 25, 2024

Choose a reason for hiding this comment

rvagg Jan 25, 2024

Choose a reason for hiding this comment

hannahhoward Jan 26, 2024

Choose a reason for hiding this comment

rvagg Jan 25, 2024

Choose a reason for hiding this comment

hannahhoward Jan 26, 2024

Choose a reason for hiding this comment

rvagg left a comment

Choose a reason for hiding this comment

rvagg commented Jan 25, 2024

bajtos commented Jan 29, 2024

rvagg commented Jan 29, 2024

hannahhoward commented Jan 25, 2024 •

edited

Loading

codecov-commenter commented Jan 25, 2024 •

edited

Loading