Retrieval query ask v2 by hannahhoward · Pull Request #671 · filecoin-project/boost

hannahhoward · 2022-07-29T08:51:08Z

Goals

fix #666 #667 #668

Implementation

Generate query ask v2 types & ipld schema based on the one documented in QueryAsk v2 Protocol #666
Create a libp2p implementation to send and request v2 Query's & QueryResponses -- retrievalmarket/lp2pimpl, based on storagemarket/lp2pimpl
Build a retrieval provider which implements query handling. Most of the code here is copied from the go-fil-markets retrieval provider query handling, with additional logic to add http info when neccesary, and to handle piece only queries
I also removed the RetrievalProviderNode abstraction since the storagemarket side in boost removes this abstraction
Setup a test of the retrieval query system, based on tests in go-fil-markets retrievalmarket impl, plus additional tests for http logic
Add config for HTTPRetrievalURL to DealMaking config
Add builder logic to construct the retrieval provider for boost
Add a cli command to send a query and output a response, specifically for piece only retrievals for now (tailored for Evergreen usage)

For Discussion

to do: complete an integration test, manual test the retrieval command

rvagg · 2022-07-29T09:18:16Z

retrievalmarket/types/queryaskv2.go

+		{(*QueryProtocols)(nil), "QueryProtocols"},
+		{(*GraphsyncFilecoinV1Response)(nil), "GraphsyncFilecoinV1Response"},
+		{(*HTTPFilecoinV1Response)(nil), "HTTPFilecoinV1Response"},


I think these aren't used directly via the registry but they are wired up through QueryResponse so they shouldn't need to be registered here.

rvagg · 2022-07-29T09:18:31Z

retrievalmarket/types/queryaskv2.ipldsch

+# Query is a query to a given provider to determine information about a piece
+# they may have available for retrieval
+type Query struct {
+  # If kind is Payload, response will error if not included


there's some space vs tabs inconsistencies in this file

rvagg · 2022-07-29T09:25:05Z

cmd/boost/retrieve_piece_cmd.go

+		if resp.Error != "" {
+			return fmt.Errorf("query response error: %s", resp.Error)
+		}
+		fmt.Println(resp.Protocols.HTTPFilecoinV1.URL)


resp.Protocols.HTTPFilecoinV1 could be nil couldn't it? Maybe something like this:

Suggested change

fmt.Println(resp.Protocols.HTTPFilecoinV1.URL)

if resp.Protocols.HTTPFilecoinV1 != nil {

fmt.Println(resp.Protocols.HTTPFilecoinV1.URL)

} else {

return fmt.Errorf("peer does not support HTTP retrievals")

}

rvagg · 2022-07-29T09:29:16Z

retrievalmarket/lp2pimpl/net.go

+
+	// Write the retrieval query to the stream
+	// Write the re to the client
+	err = types.BindnodeRegistry.TypeToWriter(query, s, dagcbor.Encode)


maybe at this point it should interrogate query to see if it's valid, i.e. has a PieceCID or a PayloadCID, before proceeding to send it since it'll be rejected anyway

rvagg · 2022-07-29T09:32:21Z

retrievalmarket/types/queryaskv2.ipldsch

+
+	# QueryResponseError indicates something went wrong generating a query response
+	| QueryResponseError ("2")
+} representation int


string enums are a lot nicer to see when looking at message contents and they shouldn't cost much more if you choose short names; I think we have a couple of places of regret in the current filecoin stack where int enums exist and we could have gone with strings (IIRC there was a discussion in graphsync when we did the initial bindnode convert). Up to you, not a big deal but the message format here is already fairly verbose and would be pretty nice to look at if you're just decoding the raw messages because of all the maps, but then you get to these numbers and they're not self explanatory.

Agree, I'd favour strings 👍

create an initial retrieval provider that only does query ask v2

setup build code for retrieval provider

adds command to retrieve piece and if so, output URL

dirkmc · 2022-08-17T08:07:02Z

client/retrieval.go

+	}
+	opts := []libp2p.Option{
+		libp2p.DefaultTransports,
+		libp2p.ListenAddrStrings("/ip4/127.0.0.1/tcp/0"),


Do we need the ListenAddrStrings option if we're also supplying NoListenAddr?

dirkmc · 2022-08-17T08:12:24Z

client/retrieval.go

+}
+
+// Query sends a retrieval query v2 to another peer
+func (c *RetrievalClient) Query(ctx context.Context, providerID peer.ID, query types.Query) (*types.QueryResponse, error) {


Given that the RetrievalClient only has one method, I wonder if we can consolidate everything into the QueryClient?

dirkmc · 2022-08-17T08:27:45Z

node/modules/storageminer.go

+			// Start the Boost RP
+			log.Info("starting boost retrieval provider")
+			lp2pnet.Start(ctx)
+			log.Info("boost storage provider started successfully")


Suggested change

log.Info("boost storage provider started successfully")

log.Info("boost retrieval provider started successfully")

dirkmc · 2022-08-17T08:31:33Z

node/modules/storageminer.go

+			// Wait for the legacy SP to fire the "ready" event before starting
+			// the boost SP.
+			// Boost overrides some listeners so it must start after the legacy SP.


Is this comment correct?

dirkmc · 2022-08-17T08:33:13Z

node/modules/storageminer.go

+	modules.HandleRetrieval(host, lc, lrp, j)
+	return nil
+}
+func HandleBoostRetrievals(lc fx.Lifecycle, h host.Host, prov *retrievalmarket.Provider, legacyRP lotus_retrievalmarket.RetrievalProvider) {


Should we rename to HandleBoostRetrievalQueryAsk?

dirkmc · 2022-08-17T08:54:44Z

retrievalmarket/provider.go

+		// add http payload url if we are supporting HTTP retrievals
+		if p.config.HTTPRetrievalURL != "" {
+			answer.Protocols.HTTPFilecoinV1 = &types.HTTPFilecoinV1Response{
+				URL:  p.config.HTTPRetrievalURL + "/payload/" + q.PayloadCID.String() + ".car",


I'd suggest we just respond with the base URL. That way we can add more options in future without having to change any code here.
The paths can be "well-known", eg /payload/<cid> vs /payload/<cid>.car

dirkmc · 2022-08-17T08:59:13Z

retrievalmarket/provider.go

+// Given the CID of a block, find a piece that contains that block.
+// If the client has specified which piece they want, return that piece.
+// Otherwise prefer pieces that are already unsealed.
+func (p *Provider) getPieceInfoFromCid(ctx context.Context, payloadCID, clientPieceCID cid.Cid) (piecestore.PieceInfo, bool, error) {


It looks like a lot of the code in this file is similar to the code in markets. Can we share it instead of copy and pasting?

dirkmc · 2022-08-17T09:02:44Z

retrievalmarket/types/queryaskv2.ipldsch

+
+	# QueryResponseError indicates something went wrong generating a query response
+	| QueryResponseError ("2")
+} representation int


Agree, I'd favour strings 👍

dirkmc · 2022-08-17T09:07:40Z

cmd/boost/retrieve_piece_cmd.go

+)
+
+var retrievePieceCmd = &cli.Command{
+	Name:  "retrieve-piece",


I'd suggest adding a retrieve category, and renaming this action to query-ask, so the CLI command would be
boost retrieve query-ask --provider=t01234 --piece-cid=1234

I also don't think we really need to provide the piece cid. The path to get things can be "well-known", eg /piece/<cid>.car

dirkmc · 2022-08-17T09:09:22Z

cmd/boost/retrieve_piece_cmd.go

+		if resp.Error != "" {
+			return fmt.Errorf("query response error: %s", resp.Error)
+		}
+		fmt.Println(resp.Protocols.HTTPFilecoinV1.URL)


I'd suggest outputting all protocols and let the user parse for the one they want to use.
Let's also implement output json if the --json flag is set, so that it's easy for people to write parsers (see boost deal for an example)

dirkmc · 2022-08-19T07:59:01Z

Current Design

Client sends query over libp2p with piece cid / payload cid
Server queries sealing subsystem to check if it's possible to retrieve the piece / block
Server responds with either
- url to get data http://1.2.3.4:1234/piece/abc123.car
- error message (eg no unsealed piece found)
Client makes an HTTP request to get data
Server queries sealing subsystem to check if it's possible to retrieve the piece / block
Server responds with either
- data
- error message (eg no unsealed piece found)

Alternative Design

Client sends query over libp2p to ask what protocols server supports
Server responds with supported protocols (graphsync, http at http://1.2.3.4:1234/)
Client makes an HTTP request to get data
Server queries sealing subsystem to check if it's possible to retrieve the piece / block
Server responds with either
- data
- error message (eg no unsealed piece found)

The reason I'm proposing the alternative design is because

It simplifies the libp2p part (no need to query sealing subsystem), just returns an IP & port (not the entire URL with the path to the specific car file)
Once the client knows the address of the miner's HTTP interface the client doesn't need to send 2 requests every time it asks for data (one request to libp2p and a second request to http). It can just directly query the http interface.

rvagg reviewed Jul 29, 2022

View reviewed changes

hannahhoward force-pushed the feat/retrieval-query-ask-v2 branch 2 times, most recently from 43b6abf to f4e0672 Compare July 31, 2022 09:06

hannahhoward requested a review from nonsense August 5, 2022 15:56

jacobheun requested a review from dirkmc August 5, 2022 16:35

hannahhoward mentioned this pull request Aug 10, 2022

Add optional signature on query-ask v2 #691

Closed

hannahhoward marked this pull request as ready for review August 10, 2022 03:38

jacobheun added this to the v1.4.0 milestone Aug 10, 2022

hannahhoward mentioned this pull request Aug 12, 2022

Improve http docs filecoin-project/boost-docs#26

Merged

hannahhoward added 6 commits August 12, 2022 16:30

feat(retrievalmarket): create query ask v2 impl

5a6179c

create an initial retrieval provider that only does query ask v2

feat(node): build retrieval provider

94a937f

setup build code for retrieval provider

feat(cmd/boost): add retrieve-piece cmd

3945668

adds command to retrieve piece and if so, output URL

style(lint): fix lint errors and run gen scripts

ccb317c

fix(builder): fix builder setup

3311419

fix(builder): fix builder setup

62e8a24

hannahhoward force-pushed the feat/retrieval-query-ask-v2 branch from eabe821 to 62e8a24 Compare August 12, 2022 23:32

fix(deps): update to markets master with fix

a3c0fe2

dirkmc reviewed Aug 17, 2022

View reviewed changes

rvagg mentioned this pull request Aug 30, 2022

Failure to retrieve when root payload CID is an identity CID #715

Merged

hannahhoward closed this Sep 2, 2022

nonsense deleted the feat/retrieval-query-ask-v2 branch January 17, 2023 15:28

-		fmt.Println(resp.Protocols.HTTPFilecoinV1.URL)
+		if resp.Protocols.HTTPFilecoinV1 != nil {
+			fmt.Println(resp.Protocols.HTTPFilecoinV1.URL)
+		} else {
+			return fmt.Errorf("peer does not support HTTP retrievals")
+		}

	log.Info("boost storage provider started successfully")
	log.Info("boost retrieval provider started successfully")

Conversation

hannahhoward commented Jul 29, 2022

Goals

Implementation

For Discussion

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dirkmc commented Aug 19, 2022

Current Design

Alternative Design

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants