Dynamic Retrieval pricing #6175

aarshkshah1992 · 2021-05-03T13:39:33Z

Implement Dynamic Retrieval Pricing.

This PR supports two modes of dynamically pricing retrieval deals:

Lotus will ship with a "default" pricing policy that will:

Use the ask configured in the Ask Store(set via lotus retrieval-deals set-ask) to price deals.
However, it will NOT charge for Unsealing if we already have an Unsealed sector we can serve the retrieval from.
It will NOT charge for the data transfer if there exists a verified storage deal for the payload being retrieved. This behaviour can be turned off by setting the VerifiedDealsFreeTransfer flag to false in the config.

Users can configure an external shell script(similar to the deal filter mechanism) that takes a json marshalled PricingInput as the input and outputs a json marshalled Ask. One of the input parameters is the ask configured in the Ask Store via the retrieval set-ask CLI command. To use this mode instead of the default above, the UseDynamicRetrievalPricing flag needs to be set to true in the config.

Depends on filecoin-project/go-fil-markets#542.

Manual Testing

Mainnet

Default pricing: Transfer price is zero for retrieval of payload contained in a verified storage deal.
Default pricing: Transfer price is quoted from the ask store for retrieval of payload NOT contained in a verified storage deal.
Default pricing: Change miner's ask in the AskStore using set-ask and then the correct transfer price is quoted from the ask store for retrieval of payload NOT contained in a verified storage deal.
Default pricing: Change the default pricing config to charge data-transfer fee for Verified deals as well and then the transfer price is also quoted from the ask store for retrieval of a payload contained in a verified storage deal.
Default pricing: Do not charge for Unsealing if we have already have an Unsealed sector file containing the payload.
External pricing script: Quote prices as expected for verified and unverified deals.

Devnet

External pricing script: Quotes prices as expected based on an externally configured script using the ask configured in the ask-store and deal pricing params such as Sealed/Unsealed deal etc.
Default pricing: Charge for Unsealing if we do NOT have an Unsealed sector file for the payload.

TODO

Merge Dynamic Retrieval Pricing go-fil-markets#542 and update deps.

Note: This Lotus PR depends on go-fil-markets v1.5.0 which is the Markets version that contains the changes needed for Dynamic Retrieval Pricing.

raulk · 2021-05-05T11:15:53Z

cmd/lotus-storage-miner/retrieval-deals.go

-}
-
-var retrievalGetAskCmd = &cli.Command{
+// TODO FIX THIS


If this PR is incomplete, better mark it as draft ;-)

raulk · 2021-05-05T11:17:58Z

cmd/lotus-storage-miner/retrieval-deals.go

@@ -160,80 +155,8 @@ var retrievalDealsListCmd = &cli.Command{
 	},
 }

-var retrievalSetAskCmd = &cli.Command{


Why are we removing this command? Shouldn't this retrieval ask be the fallback price? It could be sent to the pricing strategy, or the pricing strategy could return a boolean saying whether it has calculated a price, or if it wants to fall back to the fixed price.

raulk · 2021-05-05T11:20:58Z

markets/pricing/cli.go

+}
+
+func runPricingFunc(_ context.Context, cmd string, params interface{}) (retrievalmarket.Ask, error) {
+	j, err := json.MarshalIndent(params, "", "  ")


Why MarshalIndent? A single-line JSON would be easier to process and debug.

raulk · 2021-05-05T11:23:15Z

markets/retrievaladapter/provider.go

@@ -38,6 +38,50 @@ func NewRetrievalProviderNode(maddr dtypes.MinerAddress, secb sectorblocks.Secto
 	return &retrievalProviderNode{address.Address(maddr), secb, pp, full}
 }

+func (rpn *retrievalProviderNode) GetDealPricingParams(ctx context.Context, storageDeals []abi.DealID) (retrievalmarket.DealPricingParams, error) {
+	resp := retrievalmarket.DealPricingParams{}


Can we pack as much data as possible as input to the retrieval pricing function? PieceCID, PieceSize, at least.

raulk · 2021-05-05T11:23:45Z

markets/retrievaladapter/provider.go

@@ -38,6 +38,50 @@ func NewRetrievalProviderNode(maddr dtypes.MinerAddress, secb sectorblocks.Secto
 	return &retrievalProviderNode{address.Address(maddr), secb, pp, full}
 }

+func (rpn *retrievalProviderNode) GetDealPricingParams(ctx context.Context, storageDeals []abi.DealID) (retrievalmarket.DealPricingParams, error) {


Suggested change

func (rpn *retrievalProviderNode) GetDealPricingParams(ctx context.Context, storageDeals []abi.DealID) (retrievalmarket.DealPricingParams, error) {

func (rpn *retrievalProviderNode) GetRetrievalPricingInput(ctx context.Context, storageDeals []abi.DealID) (retrievalmarket.RetrievalPricingInput, error) {

raulk · 2021-05-05T11:27:23Z

node/config/def.go

+type DefaultRetrievalPricingParams struct {
+	PricePerByte            types.FIL
+	UnsealPrice             types.FIL
+	PaymentInterval         uint64
+	PaymentIntervalIncrease uint64


Let's keep the set retrieval ask command and remove this from configuration. We can't break that interface.

That allows for setting a "fixed" ask for when:

clients are querying the retrieval ask (get retrieval ask)

a dynamic pricing function (if enabled) refuses to calculate a dynamic price, or it errors

And miners will want to adjust the fixed price at runtime, so having these in configuration removes flexibility.

node/config/def.go

raulk

Follow-up comment to clarify what I think the UX should be here.

I think we need to support three policies for dynamic retrieval pricing.

none => no dynamic pricer is configured, all retrievals use the fixed ask, configured via set-ask.
default => (default we ship with) makes unsealed pieces (potentially) free to retrieve. We need to provide configuration values here because miners should be able to charge for data transfer if they want to (they just would rank lower in the reputation system)
external => an external script.

What the retrieval protocol would do is:

If none is configured, no changes to current behaviour.
If default is configured, call the default pricing function (which has been instantiated with some configuration values, maybe). The function should return a tuple (price: float, dynamic: bool).
- if dynamic==true, just return the price.
- if dynamic==false, the pricing policy has refused to calculate a price (e.g. the piece is sealed), and we fall back to the configured fixed ask.
- if it errors, we fall back to the fixed ask.
If external is configured, call the script and use the same logic above.

raulk · 2021-05-05T12:46:24Z

After chatting with @aarshkshah1992, we can simplify this.

retrieval pricing function is always a pure function that receives the relevant storage deal properties, (including whether it was a verified deal), sector status (sealed/unsealed), and the current configured ask, as input.
there are two retrieval pricing function types:
- default
- external (script)
the default/built-in retrieval pricing function sets the unseal price to 0 if the sector is unsealed (this is actually a bugfix).
- it takes a configuration value verified_deals_free_transfer=true/false to decide whether it provides free retrieval for verified deals or not.
  - if true and the deal was verified => price per byte == 0.
  - if false => price per byte is taken from the ask.

cmd/lotus-storage-miner/retrieval-deals.go

markets/pricing/cli.go

markets/retrievaladapter/provider.go

node/config/def.go

node/modules/storageminer.go

nonsense · 2021-05-11T13:21:00Z

extern/sector-storage/stores/remote.go

+	}
+
+	path := storiface.PathByType(paths, ft)
+	if path == "" {


When is path == "" ? This function always returns and string based on the ft, no?

dirkmc

I left a couple of suggestions.

When we test this out on a real miner we should measure the performance of checking if a piece in unsealed - it will be called as part of an externally facing API call so we need to make sure it's not too expensive.

extern/sector-storage/stores/remote.go

markets/pricing/cli.go

node/builder.go

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

aarshkshah1992 · 2021-06-08T04:40:04Z

@magik6k Have rebased & addressed your review. Please merge if happy.

raulk · 2021-06-11T09:56:01Z

@aarshkshah1992 do you really mean to update the FFI here?

extern/sector-storage/stores/remote.go

raulk · 2021-06-11T09:58:28Z

extern/sector-storage/stores/remote_test.go

+	tcs := map[string]struct {
+		storeFnc func(s *mocks.MockStore)
+		pfFunc   func(s *mocks.MockpartialFileHandler)
+		indexFnc func(s *mocks.MockSectorIndex, serverURL string)
+
+		needHttpServer bool
+
+		getAllocatedReturnCode int
+
+		serverUrl string
+
+		// expectation
+		errStr            string
+		expectedIsUnealed bool
+	}{


I defer to @magik6k to review the unsealing logic and the test coverage. I'm not so familiar with this part of the codebase.

go.sum

itests/kit/deals.go

markets/pricing/cli.go

markets/retrievaladapter/provider.go

node/builder.go

node/config/def.go

raulk · 2021-06-11T10:20:48Z

node/config/def.go

+				External: &RetrievalPricingExternal{
+					Path: "",
+				},


I think we should omit this altogether. Is it not possible?

@raulk Some tests complain that this is needed because of how the toml gets serialized/de-serialised. No harm in keeping it around unless you want me to dig deeper into why those tests complain.

raulk · 2021-06-11T10:25:29Z

@aarshkshah1992 we need to write docs for this. Can you draft a section for the "Mine >> Managing retrievals" subpage? https://docs.filecoin.io/mine/lotus/manage-retrieval-deals/ A markdown gist is fine. Possible structure for this section:

## Retrieval pricing

Lotus allows you to set different policies to calculate the quoted price of a retrieval deal:

* default: <explain>
* external: <explain>

### Using the default retrieval pricing policy

<explain>

### Using the external retrieval pricing policy

<explain>

@atopal pinging you here as your team will likely own the integration of the content ;-)

raulk

Actually, am I right that we're missing converage for the interface with the external pricing function? I'd expect these test cases:

OK: verify that we generate the right input, and that we can process well generated output.
ERR: path doesn't exist.
ERR: path is not executable.
ERR: we can handle badly structured output, e.g. not JSON.
ERR: we can handle bad output content, e.g. output is JSON, but all fields are nil values, and/or numeric fields overflow.

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

aarshkshah1992 · 2021-06-14T05:28:02Z

Retrieval pricing

Lotus allows you to set different policies to calculate the quoted price of a retrieval deal:

default: Lotus is shipped with an in-built default pricing policy that offers free retrievals for Verified Unsealed deals. Miners can still charge clients for data-transfer if they wish to by turning off free data-transfer for verified deals in the Miner config. Miners can also charge clients for Unsealing if they don't have an Unsealed sector file to retrieve the deal payload from.
external: Miners can configure an external pricing script that will be used instead of the default pricing policy to price retrieval deals. The script will be called with the relevant deal parameters and the output quote will be used by Lotus to price deals.

Using the default retrieval pricing policy

The default pricing policy uses the price configured in the Ask Store (set via the lotus retrieval-deals set-ask CLI command) to price all retrieval deals.
However, it will not charge for data-transfer if there exists a verified storage deal for the payload being retrieved. This behaviour can be turned off by setting the VerifiedDealsFreeTransfer flag to false in the DealMaking.RetrievalPricing section of the config as shown below.

 [Dealmaking.RetrievalPricing]
    Strategy = "default"
    [Dealmaking.RetrievalPricing.Default]
      VerifiedDealsFreeTransfer = false

It will also not charge for Unsealing if we already have an Unsealed sector file containing the retrieval payload.

Using the external retrieval pricing policy

Users can configure an external pricing script(similar to the deal filter mechanism) that takes a json marshalled PricingInput as the input and outputs a json marshalled Ask i.e. the quote. The PricingInput struct is defined as follows:

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 ask configured in the ask-store via the `lotus retrieval-deals set-ask` CLI command.
	CurrentAsk Ask
}

The output Ask is defined as:

type Ask struct {
   PricePerByte            abi.TokenAmount
   UnsealPrice             abi.TokenAmount
   PaymentInterval         uint64
   PaymentIntervalIncrease uint64
}

To use this mode instead of the default above, the DealMaking.RetrievalPricing section of the config needs to be configured to use the "external" pricing policy and needs to be given the absolute path of the pricing script as shown below:

[Dealmaking.RetrievalPricing]
    Strategy = "external"
    [Dealmaking.RetrievalPricing.External]
      Path = "/var/script"

aarshkshah1992 · 2021-06-14T05:35:06Z

@raulk I'd love to write tests for the external pricing script but it's not clear to me how instantiate the nodes with a desired config. For now, all nodes are instantiated with the default config. Does the testkit package have anything for this ?

aarshkshah1992 · 2021-06-14T05:35:23Z

@magik6k Please can you approve the PR if happy ?

jennijuju · 2021-06-14T16:35:52Z

se can you approve the PR if happy ?

@raulk @dirkmc do you guys ✅ this pr before Magik and aayush should take a look?

magik6k

Just one nitpick, looks good otherwise

magik6k · 2021-06-16T10:57:02Z

extern/sector-storage/stores/remote.go

+		log.Debugf("checked if local partial file has the piece %s (+%d,%d), returning answer=%t", path, offset, size, has)
+		return has, nil


Sector files can technically not have a piece unsealed locally, but have it unsealed in remote storage, so we probably want to return only if has is true

(Right now this usually won't be the case, but it may be more common if we get a PoRep with multi-TB sectors (numbers like 32tb sectors were floating in the research space))

(we may have the same bug below in Reader)

@magik6k Address this with some tests. Please take a look.

raulk · 2021-06-17T08:46:38Z

@aarshkshah1992 could you link the issue for the extra test we wanted to add (external pricing function)?

aarshkshah1992 · 2021-06-17T08:50:45Z

Filed an issue to write missing e2e tests for the external pricing script feature at #6508.

aarshkshah1992 requested review from arajasek and magik6k as code owners May 3, 2021 13:39

aarshkshah1992 changed the base branch from master to feat/direct-piecereader May 3, 2021 13:40

aarshkshah1992 changed the title ~~[WIP Draft] Dynamic Retrieval pricing~~ Dynamic Retrieval pricing May 5, 2021

aarshkshah1992 requested a review from dirkmc May 5, 2021 10:44

raulk requested changes May 5, 2021

View reviewed changes

dirkmc reviewed May 5, 2021

View reviewed changes

node/config/def.go Outdated Show resolved Hide resolved

aarshkshah1992 changed the title ~~Dynamic Retrieval pricing~~ [Draft] Dynamic Retrieval pricing May 5, 2021

aarshkshah1992 marked this pull request as draft May 5, 2021 11:34

raulk reviewed May 5, 2021

View reviewed changes

raulk reviewed May 6, 2021

View reviewed changes

aarshkshah1992 mentioned this pull request May 10, 2021

Dynamic Retrieval Pricing filecoin-project/go-fil-markets#542

Merged

nonsense reviewed May 11, 2021

View reviewed changes

dirkmc reviewed May 12, 2021

View reviewed changes

extern/sector-storage/stores/remote.go Outdated Show resolved Hide resolved

markets/pricing/cli.go Outdated Show resolved Hide resolved

markets/pricing/cli.go Outdated Show resolved Hide resolved

node/builder.go Outdated Show resolved Hide resolved

aarshkshah1992 and others added 15 commits May 20, 2021 15:25

bypass task scheduler for reading unsealed pieces

2a40c80

docs, logs and green ci

73613ee

Apply suggestions from code review

d33d426

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

address review comments

c58048d

test http handler

dd9c9fd

fix linting problems

8c4c26c

feat: TestPieceProviderReadPiece

ec85a97

use an actual worker in the integration tests

31a5f68

use mockgen

9b34494

unit tests for the remote store Reader

bd99590

fix go mod

50d7acf

remove read task type and run gen and docsgen

c17300d

finish integration tests

78a0458

more logging

85f2ac8

better logging

40642b2

changes as per review

4e9bb16

merge master

fed5afa

raulk reviewed Jun 11, 2021

View reviewed changes

aarshkshah1992 and others added 4 commits June 11, 2021 16:22

undo ffi change

ea9bed2

changes as per review

3d086df

Update node/config/def.go

bed2bc7

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

fix compilation

4419205

jennijuju added this to the v1.11.x milestone Jun 14, 2021

jennijuju added the P1 P1: Must be resolved label Jun 14, 2021

dirkmc approved these changes Jun 15, 2021

View reviewed changes

magik6k approved these changes Jun 16, 2021

View reviewed changes

local unsealed file wont have the unsealed piece

9002fcb

magik6k approved these changes Jun 17, 2021

View reviewed changes

magik6k merged commit 44de67c into master Jun 17, 2021

magik6k deleted the feat/dynamic-retreival-pricing branch June 17, 2021 08:25

aarshkshah1992 mentioned this pull request Jun 17, 2021

[Dynamic Retrieval Pricing]: Write e2e retrieval deal tests that use an external pricing script #6508

Closed

atopal mentioned this pull request Jun 17, 2021

Review the "dynamic retrieval pricing" branch. filecoin-project/filecoin-docs#895

Closed

stuberman mentioned this pull request Jul 18, 2021

[Tutorial] Dynamic Retrieval Pricing For Storage Provider #6781

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Dynamic Retrieval pricing #6175

Dynamic Retrieval pricing #6175

aarshkshah1992 commented May 3, 2021 •

edited

Loading

raulk May 5, 2021

raulk May 5, 2021

raulk May 5, 2021

raulk May 5, 2021

raulk May 5, 2021

raulk May 5, 2021

raulk left a comment

raulk commented May 5, 2021

nonsense May 11, 2021

dirkmc left a comment

aarshkshah1992 commented Jun 8, 2021

raulk commented Jun 11, 2021

raulk Jun 11, 2021

raulk Jun 11, 2021

raulk Jun 11, 2021

aarshkshah1992 Jun 14, 2021

raulk commented Jun 11, 2021

raulk left a comment

aarshkshah1992 commented Jun 14, 2021 •

edited by raulk

Loading

aarshkshah1992 commented Jun 14, 2021

aarshkshah1992 commented Jun 14, 2021

jennijuju commented Jun 14, 2021

magik6k left a comment

magik6k Jun 16, 2021

magik6k Jun 16, 2021

aarshkshah1992 Jun 17, 2021

raulk commented Jun 17, 2021

aarshkshah1992 commented Jun 17, 2021

	func (rpn *retrievalProviderNode) GetDealPricingParams(ctx context.Context, storageDeals []abi.DealID) (retrievalmarket.DealPricingParams, error) {
	func (rpn *retrievalProviderNode) GetRetrievalPricingInput(ctx context.Context, storageDeals []abi.DealID) (retrievalmarket.RetrievalPricingInput, error) {

		log.Debugf("checked if local partial file has the piece %s (+%d,%d), returning answer=%t", path, offset, size, has)
		return has, nil

Dynamic Retrieval pricing #6175

Dynamic Retrieval pricing #6175

Conversation

aarshkshah1992 commented May 3, 2021 • edited Loading

Manual Testing

Mainnet

Devnet

TODO

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

Choose a reason for hiding this comment

raulk left a comment

Choose a reason for hiding this comment

raulk commented May 5, 2021

Choose a reason for hiding this comment

dirkmc left a comment

Choose a reason for hiding this comment

aarshkshah1992 commented Jun 8, 2021

raulk commented Jun 11, 2021

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

raulk commented Jun 11, 2021

raulk left a comment

Choose a reason for hiding this comment

aarshkshah1992 commented Jun 14, 2021 • edited by raulk Loading

Retrieval pricing

Using the default retrieval pricing policy

Using the external retrieval pricing policy

aarshkshah1992 commented Jun 14, 2021

aarshkshah1992 commented Jun 14, 2021

jennijuju commented Jun 14, 2021

magik6k left a 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

raulk commented Jun 17, 2021

aarshkshah1992 commented Jun 17, 2021

aarshkshah1992 commented May 3, 2021 •

edited

Loading

aarshkshah1992 commented Jun 14, 2021 •

edited by raulk

Loading