Dynamic Retrieval Pricing #542

aarshkshah1992 · 2021-04-22T13:56:56Z

Markets PR for the Dynamic Retrieval Pricing feature. Please see the corresponding Lotus PR for the motivation behind this work.

Here is an explanation of the changes made in the Markets module to support Dynamic Retrieval Pricing:

Instead of using the price configured in the Ask Store to blanket price all Retrieval deals, we now call the Pricing function configured by Lotus with the following parameters and use the output of the function to price a retrieval deal:

type PricingInput struct {
	// PayloadCID is the cid of the payload to retrieve.
	PayloadCID cid.Cid
	// PieceCID is the cid of the Piece from which the Payload will be retrieved.
	PieceCID cid.Cid
	// PieceSize is the size of the Piece from which the payload will be retrieved.
	PieceSize abi.UnpaddedPieceSize
	// Client is the peerID of the retrieval client.
	Client peer.ID
	// VerifiedDeal is true if there exists a verified storage deal for the PayloadCID.
	VerifiedDeal bool
	// Unsealed is true if there exists an unsealed sector from which we can retrieve the given payload.
	Unsealed bool
	// CurrentAsk is the current configured ask in the ask-store by using the lotus `set-ask` retrieval 
	// CLI command.
	CurrentAsk Ask
}

Introduces/Exports a "default pricing function" that Lotus can configure Markets to use for retrieval pricing. This pricing function:
- Does NOT charge for Unsealing if we already have an Unsealed copy of a sector file containing the retrieval payload.
- Does NOT charge for data-transfer if there exists a Verified storage deal containing the retrieval payload AND the user hasn't turned off this particular functionality in the Lotus config.
If the Miner has multiple Sector files containing the required retrieval payload AND the client hasn't mandated that we retrieve the payload from a specific Piece, we will ALWAYS read the payload from an Unsealed Sector file and also price deals accordingly.
If the client wants to retrieve the payload from a specific Piece, the above logic still applies but we will only consider Sector files that have the Piece mandated by the user.

codecov-commenter · 2021-04-23T05:24:01Z

Codecov Report

Merging #542 (dc16894) into master (5b6d12f) will decrease coverage by 0.15%.
The diff coverage is 72.26%.

@@            Coverage Diff             @@
##           master     #542      +/-   ##
==========================================
- Coverage   65.77%   65.63%   -0.14%     
==========================================
  Files          56       56              
  Lines        3634     3750     +116     
==========================================
+ Hits         2390     2461      +71     
- Misses       1009     1039      +30     
- Partials      235      250      +15

Impacted Files	Coverage Δ
retrievalmarket/network/old_query_stream.go	`75.87% <0.00%> (-2.70%)`	⬇️
retrievalmarket/network/query_stream.go	`50.00% <0.00%> (-3.84%)`	⬇️
retrievalmarket/types.go	`57.15% <ø> (ø)`
retrievalmarket/impl/dtutils/dtutils.go	`79.44% <50.00%> (ø)`
storagemarket/impl/dtutils/dtutils.go	`75.00% <50.00%> (ø)`
retrievalmarket/impl/provider_environments.go	`59.26% <69.50%> (+3.31%)`	⬆️
...ievalmarket/impl/providerstates/provider_states.go	`88.00% <71.43%> (-2.69%)`	⬇️
retrievalmarket/impl/provider.go	`65.65% <73.81%> (-2.41%)`	⬇️
...market/impl/requestvalidation/requestvalidation.go	`87.66% <81.82%> (-1.81%)`	⬇️
retrievalmarket/impl/clientstates/client_states.go	`94.34% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 672687c...dc16894. Read the comment docs.

retrievalmarket/impl/provider.go

dirkmc

The test coverage is really nice 👍

retrievalmarket/impl/provider.go

retrievalmarket/impl/provider_environments.go

dirkmc · 2021-05-06T11:32:50Z

retrievalmarket/impl/provider_test.go

@@ -37,32 +37,522 @@ import (
 	tut "github.com/filecoin-project/go-fil-markets/shared_testutil"
 )

+func TestDynamicPricing(t *testing.T) {


I love how thorough this test is 👍 👍

dirkmc · 2021-05-06T11:33:21Z

retrievalmarket/impl/provider_test.go

+			expectedUnsealPrice:             expectedUnsealPrice,
+		},
+
+		{name: "When PieceCID is not provided, prefer a piece for which an unsealed sector already exists and price it accordingly",


retrievalmarket/impl/providerstates/provider_states.go

retrievalmarket/impl/requestvalidation/requestvalidation.go

Co-authored-by: raulk <[email protected]> Co-authored-by: dirkmc <[email protected]>

retrievalmarket/impl/provider.go

retrievalmarket/impl/provider_environments.go

Co-authored-by: dirkmc <[email protected]>

aarshkshah1992 · 2021-05-23T05:22:12Z

@dirkmc I've fixed a bug in commit 6c11597 wherein we were quoting the retrieval data transfer price based on the padded piece size instead of the unpadded piece size even though don't actually (obviously) transfer the padding across the wire.

raulk · 2021-06-02T11:44:11Z

@aarshkshah1992 Mind updating this PR's description with an explanation of what has been done concretely in this module (as part of the linked Lotus side PR) + what's left, so that the communityi can follow?

Do we need to add more test coverage here?

aarshkshah1992 · 2021-06-03T04:12:09Z

@raulk @dirkmc

I've updated the description of the PR to give more information about the changes introduced by this PR.

This PR is in pretty good shape and has solid tests. I just need @dirkmc to look at one last small commit that I recently introduced:
6c11597

If that looks good, we are good to go.

dirkmc · 2021-06-03T12:14:23Z

Looks good, thanks Aarsh 👍

Before we merge I'd like to do some real world testing, for correctness and also to make sure that it's performant.

ribasushi · 2021-07-18T15:24:02Z

retrievalmarket/types.go

+// PricingInput provides input parameters required to price a retrieval deal.
+type PricingInput struct {
+	// PayloadCID is the cid of the payload to retrieve.
+	PayloadCID cid.Cid
+	// PieceCID is the cid of the Piece from which the Payload will be retrieved.
+	PieceCID cid.Cid
+	// PieceSize is the size of the Piece from which the payload will be retrieved.
+	PieceSize abi.UnpaddedPieceSize
+	// Client is the peerID of the retrieval client.
+	Client peer.ID
+	// VerifiedDeal is true if there exists a verified storage deal for the PayloadCID.
+	VerifiedDeal bool
+	// Unsealed is true if there exists an unsealed sector from which we can retrieve the given payload.
+	Unsealed bool
+	// CurrentAsk is the current configured ask in the ask-store.
+	CurrentAsk Ask
+}


( I am well aware I am commenting on an old, merged PR )

@aarshkshah1992 @hannahhoward @willscott was it a deliberate decision to exclude the client-provided selector in this structure, or just an omission? Even if we do not have the tools for miners to interpret/decide on a selector-driven partial-retrieval, the structure is being exposed to 3rd party tools. It is probably prudent to provide all the information available from the get-go, instead of adding it later on.

An extra nit regarding the title-comment:

If the Miner has multiple Sector files containing the required retrieval payload AND the client hasn't mandated that we retrieve the payload from a specific Piece,

A specific deal PayloadCID (root-dag) + PieceSize will always invariably correspond to an identical PieceCID. That is - if a deal is stored in multiple sectors, some sealed and some unsealed, a PieceCID can not differentiate among them: it will be identical among them all.

Never too late to provide useful feedback! ;-)

Re: including the selector in the pricing function input: how useful would this be? The function can't evaluate the selector against the data, and it can't really derive anything useful from it intrinsically AFAIK.

If it's to symbolise that a partial retrieval is being performed, for pricing purposes, the price-per-byte formulation of the quote already decouples the final price from the actual data being selected. We could pass a "partial retrieval = true/false" flag, but it's not possible to calculate it correctly because the selector could be selecting the entire deal.

Re: extensibility/completeness, it's non-invasive to add a JSON field in the future if we do decide we want this for some reason. But until there's a standardised, canonical text representation for selectors, I don't think it's practical to even consider.

A specific deal PayloadCID (root-dag) + PieceSize will always invariably correspond to an identical PieceCID. That is - if a deal is stored in multiple sectors, some sealed and some unsealed, a PieceCID can not differentiate among them: it will be identical among them all.

If there's an unsealed copy, we always select the unsealed copy as input to the pricing function, and as the source of the retrieval.

@raulk right, I was specifically questioning this part of the sentence:

the client hasn't mandated that we retrieve the payload from a specific Piece

There is no way for a client to signal "I want to retrieve from this physical piece" via the struct in question: they all have the same PieceCid

Oh, what @aarshkshah1992 was trying to get to is that if "universal retrieval" is enabled (such that clients don't need to specify the PieceCID they want to retrieve a given PayloadCID from), and there are multiple PieceCIDs that contain that PayloadCID, we prefer the deal that's unsealed, should there be one. I don't think any of that implies that the client gets to choose the sector, or concrete physical piece within a sector.

But until there's a standardized, canonical text representation for selectors, I don't think it's practical to even consider.

The reason I brought this up is that the selector is already exposed in a standard-ish encoded-cbor representation via the RetrievalFilter codepath: https://github.com/filecoin-project/lotus/blob/v1.11.0-rc2/markets/dealfilter/cli.go#L28-L62

I do not have a preference either way. Rather am raising the inconsistency of having two sets of external tools, one being fed the structure in this pr, the other being fed this: https://github.com/Murmuration-Labs/bitscreen/blob/master/testdata/retrieval_proposal.json

Thanks for the pointer. Note that this isn't a filtering function, but rather a pricing function. It runs when the retrieval provider is asked for a quote. At that time, the selector is not available. So in terms of the data interface, the deal filter and the pricing function can't be symmetrical.

It runs when the retrieval provider is asked for a quote. At that time, the selector is not available.

@raulk indeed you are correct: it is all commented out :cryingbear:
https://github.com/filecoin-project/go-fil-markets/blob/v1.6.0-rc1/retrievalmarket/types.go#L158-L167

refactor for dynamic pricing

fb28e0a

aarshkshah1992 requested a review from dirkmc April 22, 2021 13:57

test differential pricing

86145f7

aarshkshah1992 added 2 commits May 3, 2021 19:05

dynamic pricing

a93936a

changes for dynamic pricing

8bd6d00

aarshkshah1992 marked this pull request as draft May 6, 2021 09:39

aarshkshah1992 force-pushed the feat/dynamic-retreival-pricing branch from 4a61fc5 to 8bd6d00 Compare May 6, 2021 10:05

aarshkshah1992 mentioned this pull request May 6, 2021

Dynamic Retrieval pricing filecoin-project/lotus#6175

Merged

9 tasks

raulk reviewed May 6, 2021

View reviewed changes

retrievalmarket/impl/provider.go Outdated Show resolved Hide resolved

dirkmc reviewed May 6, 2021

View reviewed changes

aarshkshah1992 and others added 2 commits May 7, 2021 12:58

Apply suggestions from code review

a18e80b

Co-authored-by: raulk <[email protected]> Co-authored-by: dirkmc <[email protected]>

changes as per review and test json

f842a9d

dirkmc reviewed May 7, 2021

View reviewed changes

aarshkshah1992 and others added 3 commits May 7, 2021 17:46

Apply suggestions from code review

169c536

Co-authored-by: dirkmc <[email protected]>

fix compilation

382aea4

changes as per review

d32334a

dirkmc approved these changes May 7, 2021

View reviewed changes

aarshkshah1992 added 5 commits May 11, 2021 18:20

default retrieval pricing function

3f47318

Merge branch 'master' into feat/dynamic-retreival-pricing

9ce880f

fix compilation

7d33a6e

test the default pricing function

6f4ff93

fix bug in quoting price

6c11597

aarshkshah1992 marked this pull request as ready for review June 3, 2021 04:13

aarshkshah1992 and others added 2 commits June 4, 2021 18:32

Merge branch 'master' into feat/dynamic-retreival-pricing

dc16894

fix: go mod tidy

bae766b

dirkmc merged commit 32e5cce into master Jun 4, 2021

dirkmc deleted the feat/dynamic-retreival-pricing branch June 4, 2021 13:23

dirkmc mentioned this pull request Jun 4, 2021

release: v1.5.0 #553

Merged

ribasushi reviewed Jul 18, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Dynamic Retrieval Pricing #542

Dynamic Retrieval Pricing #542

aarshkshah1992 commented Apr 22, 2021 •

edited

Loading

codecov-commenter commented Apr 23, 2021 •

edited

Loading

dirkmc left a comment

dirkmc May 6, 2021

dirkmc May 6, 2021

aarshkshah1992 commented May 23, 2021

raulk commented Jun 2, 2021

aarshkshah1992 commented Jun 3, 2021 •

edited

Loading

dirkmc commented Jun 3, 2021

ribasushi Jul 18, 2021

ribasushi Jul 18, 2021

raulk Jul 19, 2021 •

edited

Loading

raulk Jul 19, 2021

ribasushi Jul 19, 2021

raulk Jul 19, 2021 •

edited

Loading

ribasushi Jul 19, 2021

raulk Jul 19, 2021

ribasushi Jul 19, 2021

Dynamic Retrieval Pricing #542

Dynamic Retrieval Pricing #542

Conversation

aarshkshah1992 commented Apr 22, 2021 • edited Loading

codecov-commenter commented Apr 23, 2021 • edited Loading

Codecov Report

dirkmc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aarshkshah1992 commented May 23, 2021

raulk commented Jun 2, 2021

aarshkshah1992 commented Jun 3, 2021 • edited Loading

dirkmc commented Jun 3, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raulk Jul 19, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raulk Jul 19, 2021 • 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

aarshkshah1992 commented Apr 22, 2021 •

edited

Loading

codecov-commenter commented Apr 23, 2021 •

edited

Loading

aarshkshah1992 commented Jun 3, 2021 •

edited

Loading

raulk Jul 19, 2021 •

edited

Loading

raulk Jul 19, 2021 •

edited

Loading