Feat/deal protocol #108

whyrusleeping · 2018-03-12T06:38:15Z

The previous design was pretty poor. No nice way of resumption, expected a channel to stay open during the entire transfer, had poorly defined states and no story for multiple transfers saturating bandwidth.

This should be a little better. I'm torn on whether it should be a single libp2p protocol, or two as implemented here. But, probably good to get some feedback before thinking about it too much more.

whyrusleeping · 2018-03-12T06:42:40Z

Oh, and the cborutil stuff i wrote before it was a simple request response, its technically not needed now and i'll probably remove it.

whyrusleeping · 2018-03-12T06:44:54Z

node/deal_protocol.go

+	}
+
+	vals, err := abi.ToValues([]interface{}{
+		big.NewInt(0).SetUint64(propose.Deal.Ask),


ToValues should probably be smart enough to handle ints... maybe

it should be able to handle uint32 and uint64 explicitly, but not uint to avoid accidentally introducing dependencies on build/architecture

I mean that it should be able to convert normal ints into the big int 'Integer' type.

not sure then, because that could lead to ambiguous cases if we support regular ints at some point

dignifiedquire

I am missing the entry point for starting the actual process, how would this whole process get started if I had a deal I want to propose?

dignifiedquire · 2018-03-12T10:47:20Z

node/deal_protocol.go

+	pdata, err := abi.EncodeValues(vals)
+	if err != nil {
+		return nil, errors.Wrap(err, "failed to encode abi values")
+	}


you can use abi.ToEncodedValues() instead of calling ToValues and EncodeValues individually

That wasnt merged when I wrote this.

dignifiedquire · 2018-03-12T10:49:30Z

node/deal_protocol.go

+	return &storage, nil
+}
+
+func (sm *StorageMarket) GetAsk(id uint64) (*core.Ask, error) {


while I understand the convenience of the current implementation, this at least needs a TODO to switch over to message sending instead of relying on being able to execute the code of the actor outside the VM

This would be good c5 exercise to give some exposure to the vm, even if they are not going to be working on it. If you put a TODO you can add a c5-tagged issue to make the change.

I think we always want to be able to directly access contract state for performance reasons. Things that interact with the market are going to want to do lots of interesting queries, and we don't want each request to have to spin up a whole vm just to get it.

Hmmm this could get pretty complicated once the actors are written in a different language, also this means we need to make actors thread safe, as currently I am pretty sure this could get into race conditions depending on how things are wired up.

What? race conditions? You take a hash of the actors memory, load it up in some way, and get a race condition how?

I guess it is synced on bestblock to avoid any races

dignifiedquire · 2018-03-12T10:49:52Z

node/deal_protocol.go

+	return ask, nil
+}
+
+func (sm *StorageMarket) GetBid(id uint64) (*core.Bid, error) {


Same as GetAsk

dignifiedquire · 2018-03-12T10:51:22Z

core/storage_market.go

@@ -46,6 +45,9 @@ type Bid struct {
 	//Coding ???
 	Owner types.Address
 	ID    uint64
+
+	// Used indicates whether or not this bid is in use by a deal
+	Used bool


We could store a reference to the deal here, which could make certain lookups cheaper

dignifiedquire · 2018-03-12T10:54:57Z

node/deal_protocol.go

+	if err := sm.fetchData(context.TODO(), propose.Deal.DataRef); err != nil {
+		return nil, errors.Wrap(err, "fetching data failed")
+	}
+


Can you add a dummy method to store the data & mark it for sealing, so it's clear where & how to plug that part in?

dignifiedquire · 2018-03-12T10:56:11Z

node/deal_protocol.go

+)
+
+const MakeDealProtocolID = protocol.ID("/fil/deal/mk/1.0.0")
+const CheckDealProtocolID = protocol.ID("/fil/deal/chk/1.0.0")


Is there any benefit on the libp2p side to making it two distinct protocols? Otherwise I am having a hard time seeing the benefit of splitting them into two.

Well, doing things this way makes the protocols themselves cleaner. Switching to a single protocol means we need to add extra fields to the 'request' object to tell what type of request it is (propose vs check)

We should keep them separate -- they are two different things and I don't see a lot of benefit from combination...

porcuquine · 2018-03-13T01:47:04Z

node/deal_protocol.go

+	cbor.RegisterCborType(DealPropose{})
+	cbor.RegisterCborType(DealResponse{})
+	cbor.RegisterCborType(DealQuery{})
+}


[I initially inadvertently first left this comment at the draft spec (https://github.com/filecoin-project/specs/pull/63). However, it was hidden there and also didn't make perfect sense outside the context of the API here.]

I think it would be a good idea to fully standardize the naming so the hypothetical verbosity is at least consistent and meaningful. However, the obvious way to do that makes the reuse of DealResponse somewhat problematic -- since we would then want (Storage)DealProposalResponse and Check(Storage)DealResponse.

If, instead, we decide that it does make sense to reuse the same type, it might make sense to name it in a way which conveys its shared function. For example, I could see using (Storage)DealStatus in both cases.

In this case, I think I prefer the current design with just changed naming (to …Status). However, I acknowledge that this potentially adds a layer of complexity to the message naming language. I think I'm comfortable with that, as long as we remain thoughtful and consistent about naming choices.

To expand a little:

generally I think we should name responses that are specific to a request as {...}Response, so FooRequest should have FooResponse.

if a response is not scoped to a specific request then as @clkunzang says yes I think DealStatus is much more clear

however having said that, right now it's clear that we can re-use the status but we are going to find it annoying if we want to evolve the responses for the different requests in different ways, for example if we want to enable a batch query and then the response should have a set of statuses. i think it makes sense to have separate responses for separate requests, even if they both just embed a dealstatus for now (so ProposeDealResponse and CheckDealResponse just embed DealStatus for now). in general seems like the safer and more future proof thing to do.

BTW I think DealProposal as a noun might be more clear than DealPropose

Regarding DealProposal vs DealPropose. There are two separate issues here. I'll illustrate by referring to DealProposal as the noun form, and DealPropose as the verb form.

If we want to use the verb form, then we should put the verb first and use ProposeDeal.

Independently, we should decide if we have a global preference for noun or verb form. I have a mild preference for verb form on linguistic grounds; and a slightly stronger preference on the basis that forcing verb choice will help document the API's intention more clearly. Whatever we choose, we should be consistent.

Based on the above, my concrete suggestion would be:

ProposeDeal -> ProposeDealResponse (embeds DealStatus).

CheckDeal -> CheckDealResponse (embeds DealStatus).

Then we should make sure our naming all eventually conforms with this pattern (or amend/extend it). Ideally, our naming consistency would extend to reusing verbs wherever appropriate. So if we have another pair of messages following an initiate/poll pattern, we could use <Initiate><Noun>, <Initiate><Noun>Response, Check<Noun>, and Check<Noun>Response -- embedding <Noun>Status as a default implementation, but only if that makes sense.

I'm not sure naming the types after the action makes a lot of sense to me. The operations are the verb, the object is the concrete thing.

Thats kinda like calling my wallet the SpendMoney.

You're right. So noun forms for structs, verb forms for handlers, and try to have the correspondences be as consistent as possible -- so ProposeDeal and DealProposal, etc.

Does that make sense?

Yeah, that makes the most sense to me

phritz · 2018-03-13T17:00:17Z

node/deal_protocol.go

+	}
+}
+
+func (sm *StorageMarket) CheckDeal(id [32]byte) (*DealResponse, error) {


Would be easier to understand if either Check or Query and were consistent with it. Right now we have a DealQuery which executes CheckDeal. Let's pick Check or Query and stick with it?

This is one of those "implement it both ways and hope someone has an opinion on which way is right" sort of things.

I guess i'll pick Query?

Or just say what it means: GetStatus? If not that then just to be obtuse I pick Check.

phritz · 2018-03-13T19:13:08Z

node/deal_protocol.go

+	cbor.RegisterCborType(DealPropose{})
+	cbor.RegisterCborType(DealResponse{})
+	cbor.RegisterCborType(DealQuery{})
+}


To expand a little:

generally I think we should name responses that are specific to a request as {...}Response, so FooRequest should have FooResponse.

if a response is not scoped to a specific request then as @clkunzang says yes I think DealStatus is much more clear

however having said that, right now it's clear that we can re-use the status but we are going to find it annoying if we want to evolve the responses for the different requests in different ways, for example if we want to enable a batch query and then the response should have a set of statuses. i think it makes sense to have separate responses for separate requests, even if they both just embed a dealstatus for now (so ProposeDealResponse and CheckDealResponse just embed DealStatus for now). in general seems like the safer and more future proof thing to do.

BTW I think DealProposal as a noun might be more clear than DealPropose

phritz · 2018-03-13T19:13:45Z

node/deal_protocol.go

+)
+
+const MakeDealProtocolID = protocol.ID("/fil/deal/mk/1.0.0")
+const CheckDealProtocolID = protocol.ID("/fil/deal/chk/1.0.0")


We should keep them separate -- they are two different things and I don't see a lot of benefit from combination...

phritz · 2018-03-13T19:17:53Z

node/deal_protocol.go

+	// Rejected, the deal was rejected for some reason
+	Rejected
+
+	// Accepted, the deal was accepted but hasnt yet started


Can we define what "started" means -- does it mean a transfer is in progress? Below it says "started, the deal has started and the transfer is in progress" -- are those two different things?

I guess capitalizing the 'S' on started in this comment might make it more clear, but yes. Started means we're actively fetching the data. Accepted means we intend to accept this deal, but we're either busy, or havent gotten to it yet.

I think its a valuable distinction, but maybe they should be smashed into one?

Could be valuable I guess. AttemptingTransfer? Don't feel strongly.

phritz · 2018-03-13T19:19:06Z

node/deal_protocol.go

+	Started
+
+	// Failed, the deal has failed for some reason
+	Failed


Will we be able to distinguish why it failed at some point?

Thats what the message is for

phritz · 2018-03-13T19:22:17Z

node/deal_protocol.go

+	cbor.RegisterCborType(DealQuery{})
+}
+
+type DealPropose struct {


Probably worth a comment that says these are made by customers, not miners.

phritz · 2018-03-13T19:24:36Z

node/deal_protocol.go

+	if ok {
+		// TODO: maybe return current negotiation status instead?
+		return &DealResponse{
+			Message: "deal negotiation already in progress",


This is maybe an argument to separate the query response from propose deal response: because here we respond with rejected, but if they query they get an in progress status. If we just have one return type it's less clear that they have different semantics.

I'm actually thinking it should go the opposite direction. I think this should return whatever the current state of the existing negotiation is, but keep the message as is. It makes these calls a bit more idempotent.

that sounds good

phritz · 2018-03-13T19:32:20Z

node/deal_protocol.go

+	return &storage, nil
+}
+
+func (sm *StorageMarket) GetAsk(id uint64) (*core.Ask, error) {


This would be good c5 exercise to give some exposure to the vm, even if they are not going to be working on it. If you put a TODO you can add a c5-tagged issue to make the change.

phritz · 2018-03-16T19:20:21Z

node/deal_protocol.go

+	inet "gx/ipfs/QmXfkENeeBvh3zYA51MaSdGUdBjhQ99cP5WQe8zgr6wchG/go-libp2p-net"
+	"gx/ipfs/QmZNkThpqfVXs9GNbexPrfBbXSLNYeKrE7jwFM2oqHbyqN/go-libp2p-protocol"
+	"gx/ipfs/QmcZfnkapfECQGcLZaf9B79NRg7cRa9EnZh4LSbkCzwNvY/go-cid"
+


intentional spacing?

phritz · 2018-03-16T19:23:41Z

node/deal_protocol.go

+// creator of the proposal to select a bid and an ask and turn that into a
+// deal, add a reference to the data they want stored to the deal,  then add
+// their signature over the deal.
+type DealPropose struct {


OK, we're going with nouns and not a separate response for the separate protocol.

Yeah... Having separate types felt a bit weird to me. It seemed to encourage treating the response as logically different things, when they are really the same.

The ProposeDeal operation could really not return anything, and just require a followup Query call, and have the same effect.

Also, in the future, we're going to want the miner to be able to manually review proposals, or plug the decision making into a separate process. Given that, we might not even be able to say "Accepted" as a response from the ProposeDeal call.

phritz · 2018-03-16T19:41:37Z

node/deal_protocol.go

+	if err != nil {
+		s.Reset()
+		// TODO: metrics, more structured logging. This is fairly useful information
+		log.Infof("incoming deal proposal failed: %s", err)


incoming query deal failed

phritz · 2018-03-16T19:46:40Z

node/deal_protocol.go

+
+	ask, ok := stor.Orderbook.Asks[id]
+	if !ok {
+		return nil, fmt.Errorf("no such ask")


Is it a little bit funny to call this an actor when how it's used is dissimilar to "regular" actors that can be instantiated in called in the vm? For example when reading the code as it were an actor code my immediate reaction is hey the return signature is wrong. I admit I don't know what else to call it. Ask: tell me whether you think the name actor really works here.

(And to be clear if there is an ask along with an approval I'm happy to take an answer after merge or in slack or whatever)

yeah... I really struggled with the naming here. Any ideas?

mishmosh · 2018-04-17T20:23:14Z

Fixes #28.

whyrusleeping changed the base branch from master to feat/add-deal March 12, 2018 06:39

whyrusleeping force-pushed the feat/deal-protocol branch from b8d65f6 to a31d265 Compare March 12, 2018 06:39

whyrusleeping commented Mar 12, 2018

View reviewed changes

dignifiedquire reviewed Mar 12, 2018

View reviewed changes

This was referenced Mar 12, 2018

As a miner I accept every deal that come my way and receive the data before accepting the deal. #28

Closed

As a customer I can create via CLI a Deal between exactly one on-chain ask and one on-chain bid #27

Closed

whyrusleeping force-pushed the feat/deal-protocol branch from a31d265 to 7ac5b2b Compare March 12, 2018 23:20

porcuquine reviewed Mar 13, 2018

View reviewed changes

phritz reviewed Mar 13, 2018

View reviewed changes

whyrusleeping force-pushed the feat/add-deal branch from cf8173a to 066f3fe Compare March 14, 2018 23:07

whyrusleeping force-pushed the feat/deal-protocol branch from b2b3d7d to 27d06c0 Compare March 15, 2018 17:56

whyrusleeping changed the base branch from feat/add-deal to master March 15, 2018 17:59

dignifiedquire assigned phritz, dignifiedquire and porcuquine Mar 16, 2018

whyrusleeping force-pushed the feat/deal-protocol branch from 69da689 to e6c9244 Compare March 16, 2018 18:52

phritz previously approved these changes Mar 16, 2018

View reviewed changes

dignifiedquire previously approved these changes Mar 16, 2018

View reviewed changes

whyrusleeping dismissed stale reviews from dignifiedquire and phritz via c4261ae March 17, 2018 03:29

address code review

f970009

whyrusleeping force-pushed the feat/deal-protocol branch 2 times, most recently from ab99d7e to 665823a Compare March 17, 2018 03:33

implement deal protocol

575da6b

whyrusleeping force-pushed the feat/deal-protocol branch from 665823a to 575da6b Compare March 17, 2018 03:42

whyrusleeping merged commit 82a3442 into master Mar 17, 2018

Feat/deal protocol #108

Feat/deal protocol #108

Conversation

whyrusleeping commented Mar 12, 2018 • edited Loading

whyrusleeping commented Mar 12, 2018

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

dignifiedquire 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

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

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

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

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

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

mishmosh commented Apr 17, 2018

whyrusleeping commented Mar 12, 2018 •

edited

Loading