Miner payment management #1845

lkowalick · 2019-02-08T15:35:19Z

We coalesce the the storage of storage deals for both storage miners and storage clients into a single datastore with a single namespace. This allows easy querying of storage deals, regardless of whether one is a miner or client. This also easily allows a miner to obtain vouchers for storage deals that have been successfully executed, thus enabling miner payments with minimal hoop-jumping.

Resolves #1433

phritz · 2019-02-08T17:56:05Z

node/node.go

@@ -409,6 +410,7 @@ func (nc *Config) Build(ctx context.Context) (*Node, error) {
 		MsgQueryer: msg.NewQueryer(nc.Repo, fcWallet, chainReader, &cstOffline, bs),
 		MsgSender:  msg.NewSender(nc.Repo, fcWallet, chainReader, msgPool, fsub.Publish),
 		MsgWaiter:  msg.NewWaiter(chainReader, bs, &cstOffline),
+		Deals:      dls.New(nc.Repo.DealsDatastore()),


ok this i is interesting. right now plumbing only accesses information that is widely available in the node. it doesnt access information owned by a particular protocol. i think access to information owned by a protocol should probably be accessed through that protocol's api. we dont have a mechanism for that yet. it seems really natural though to have an api for each protocol that has things like start and stop and to take actions on it. it would not be part of porcelain. we need to talk about this on our end. for now keep this but file an issue in the porcelain epic and referenece here about surfacing protocol private data in our apis how to do that. and @rosalinekarr and i will talk about it and come make it do the right thing.

actually @ZenGround0 is going to look at this, @ZenGround0 please ack

I think this is a good idea and opens the door to some potentially useful features in the future. Having this datastore exposed without the protocol could allow users to list their active deals, view how much money each deal has made, export deals to a backup and import deals on another client with access to the same sector base.

phritz · 2019-02-08T17:56:48Z

plumbing/dls/dls.go

+}
+
+// Query returns a channel of deals matching the given query.
+func (querier *Querier) Query(qry query.Query) (<-chan *deal.Deal, <-chan error) {


why return a channel? why not just iterate them and return a slice?

Mainly because @whyrusleeping made this comment on the previous persisting deals PR:

#1526 (comment)

hooray for premature optimization at the cost of code clarity

Yeah, because technical debt is awesome and everyone loves dealing with it.

porcelain/api.go

whyrusleeping · 2019-02-12T03:02:20Z

plumbing/dls/dls.go

+}
+
+// Ls returns a channel of deals matching the given query.
+func (lser *Lser) Ls() (<-chan *deal.Deal, <-chan error) {


returning two channels like this is really hard to use as a consumer without race conditions, instead create an option type (would be great if go had these natively) and return a channel of MaybeDeal (or something with a better name).

phritz · 2019-02-12T05:36:40Z

@rkowalick this is still labeled wip so i'm not sure if i should review. please add me back as a reviewer when it is ready.

porcelain/deals.go

api/impl/client.go

commands/client.go

rosalinekarr

Other than @phritz's suggestions, this LGTM

rosalinekarr · 2019-03-05T16:00:15Z

node/block_propagate_test.go

@@ -143,7 +143,7 @@ func (r *zeroRewarder) GasReward(ctx context.Context, st state.Tree, minerAddr a
 }

 // makeNodes makes at least two nodes, a miner and a client; numNodes is the total wanted
-func makeNodes(ctx context.Context, t *testing.T, assertions *assert.Assertions, numNodes int) (address.Address, []*Node) {


🎉 for removing unnecessary contexts!

rosalinekarr · 2019-03-05T16:03:43Z

plumbing/api.go

+// DealPut puts a given deal in the datastore
+func (api *API) DealPut(storageDeal *storagedeal.Deal) error {
+	return api.storagedeals.Put(storageDeal)
+}


I like where this is going. In the future, we could consider adding a DealRemove or DealDelete for letting users define their own data retention policies around deals.

We briefly discussed expiration policies around deals when we initially added deal persistence, but we never came up with a great solution for it. With this, we could potentially have an automatic delete after a few days for most users and let larger miners with capacity and interest in holding records for longer define their own policies through manual commands.

phritz

Generally LGTM, see comments re deal put/ls in plumbing which i conclude is not a great home for them in the limit.

node/node.go

phritz · 2019-03-06T05:41:49Z

plumbing/api.go

@@ -120,7 +125,17 @@ func (api *API) BlockGet(ctx context.Context, id cid.Cid) (*types.Block, error)
 	return api.chain.GetBlock(ctx, id)
 }

-// MessagePoolPending lists messages in the pool.
+// DealsLs a slice of all storagedeals in the local datastore and possibly an error


unclear to me whether these deal calls belong in plumbing long term. they are used only by the storage protocol i think, and put at least should probably be a private interface that it uses. fine here for now but @shannonwells can you ack that you'll consider moving this as part of your work? they might just want to be private dependencies of the storage protocol instead of public calls part of the plumbing.

Yes, if we are going to make storage mining part of protocol then it will need to be moved out of here.

ACK'd, comment I made below is heavily related to this thread too.

phritz · 2019-03-06T05:45:15Z

plumbing/strgdls/store.go

+
+	results, err := store.dealsDs.Query(query.Query{Prefix: "/" + StorageDealPrefix})
+	if err != nil {
+		return deals, errors.Wrap(err, "failed to query deals from datastore")


if this comment is accurate i would not expect this to be an error, i would expect it to just return empty results

This err case is distinct from the query returning an empty result set, so I thought it might be wise to point it out.

plumbing/strgdls/store.go

porcelain/storagedeals.go

protocol/storage/client.go

plumbing/strgdls/store.go

protocol/storage/client.go

phritz · 2019-03-06T06:06:17Z

protocol/storage/storagedeal/state.go

@@ -1,13 +1,15 @@
-package storage
+package storagedeal


blocking: why do we have a new package for this? i suspect an import cycle which speaks to a deeper problem. i think the issue is likely that storage protocol depends on deal put/ls plumbing, which depend on storage protocol. this is a strong signal that deal put/ls do not belong in plumbing. the putter and lser should be direct, private dependencies of the storage protocol and should not be in the plumbing interface. commands that need deal ls should get it from the deal lser the storage protocol uses, which can be surfaced to them in storage protocol control api which will be separate from plumbing because it is higher level, which is part of #2030. for now we can keep it but @shannonwells can you please ack (and @rkowalick please ensure she does) that undoing this -- moving the deal back into the storage package as storage.deal and removing deal put/ls from plumbing -- is part of the scope for 2030? sorry to ramble here, i'm going ooo. and sorry @rkowalick this is not really something you could have forseen, we are underprepared to move protocol specific calls at this point, we'll have to play catch up.

Yes it makes sense to have everything that's interacting with storage deals use the upcoming protocol api, if that's what you're asking.

@phritz I agree entirely that this is the core of the problem. It seems the protocol refactor stuff that @shannonwells will likely reorganize this stuff, so it seems like wasted effort to pull this stuff out of plumbing right now.

We coalesce the storage deal stores for both miners and clients so they can easily be queried by each. This allows miners to query the vouchers they have for various storage deals, along with the state of those deals. We also push the deal storage mechanism into plumbing, though it may be moved shortly with the upcoming protocol refactor. There is also some simple cleanup such as: - Removing some unused contexts - Removing the in-memory cache of the deal store. Badger is really fast and this simplifies the code tremendously

phritz reviewed Feb 8, 2019

View reviewed changes

whyrusleeping reviewed Feb 12, 2019

View reviewed changes

lkowalick force-pushed the feat/miner-payment-management branch from 0b2f9c7 to eb2ead4 Compare February 27, 2019 19:52

lkowalick commented Mar 1, 2019

View reviewed changes

porcelain/deals.go Outdated Show resolved Hide resolved

lkowalick changed the title ~~WIP miner payment management~~ Miner payment management Mar 1, 2019

lkowalick requested review from phritz and rosalinekarr March 1, 2019 21:02

phritz reviewed Mar 2, 2019

View reviewed changes

api/impl/client.go Outdated Show resolved Hide resolved

phritz reviewed Mar 2, 2019

View reviewed changes

commands/client.go Outdated Show resolved Hide resolved

lkowalick force-pushed the feat/miner-payment-management branch from fd19d26 to 51a0023 Compare March 4, 2019 22:50

rosalinekarr approved these changes Mar 5, 2019

View reviewed changes

lkowalick force-pushed the feat/miner-payment-management branch from 51a0023 to 1c13a56 Compare March 5, 2019 16:23

phritz approved these changes Mar 6, 2019

View reviewed changes

lkowalick force-pushed the feat/miner-payment-management branch from fb05742 to 305970f Compare March 6, 2019 17:55

lkowalick force-pushed the feat/miner-payment-management branch from bf8bab1 to 115352d Compare March 7, 2019 15:30

lkowalick merged commit 87f1b55 into master Mar 7, 2019

lkowalick deleted the feat/miner-payment-management branch March 7, 2019 16:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Miner payment management #1845

Miner payment management #1845

lkowalick commented Feb 8, 2019 •

edited

Loading

phritz Feb 8, 2019

phritz Feb 12, 2019

rosalinekarr Mar 1, 2019

phritz Feb 8, 2019

lkowalick Feb 8, 2019

phritz Feb 12, 2019

whyrusleeping Feb 12, 2019

whyrusleeping Feb 12, 2019

phritz commented Feb 12, 2019

rosalinekarr left a comment

rosalinekarr Mar 5, 2019

rosalinekarr Mar 5, 2019

phritz left a comment

phritz Mar 6, 2019

shannonwells Mar 6, 2019

lkowalick Mar 7, 2019

phritz Mar 6, 2019

lkowalick Mar 6, 2019

phritz Mar 6, 2019 •

edited

Loading

shannonwells Mar 6, 2019

lkowalick Mar 7, 2019

Miner payment management #1845

Miner payment management #1845

Conversation

lkowalick commented Feb 8, 2019 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phritz commented Feb 12, 2019

rosalinekarr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phritz 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phritz Mar 6, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lkowalick commented Feb 8, 2019 •

edited

Loading

phritz Mar 6, 2019 •

edited

Loading