Venus: Storage client should use the Has method from go-datastore

[rosalinekarr]

Description

Currently, isMaybeDupDeal in protocol/storage/client.go calls the DealsLs plumbing method and iterates over results to check for the existence of a deal. This is inefficient and doesn't take full advantage of the API exposed by go-datastore.

We should add a DealsHas plumbing method to plumbing/strgdls/store.go which uses go-datastore's underlying Has method and can be used in the storage client in place of isMaybeDupDeal.

Acceptance criteria

1. A DealsHas method is added to plumbing/strgdls for exposing go-datastore's Has method
2. DealHas is tested
3. protocol/storage/client.go is changed to use the new DealsHas method
4. isMaybeDupDeal is removed from protocol/storage/client.go

Risks + pitfalls

Where to begin

plumbing/strgdls/store.go

[byoulten]

Hi @rosalinekarr

I just want to put in a little disclaimer that I am new to this but am looking to learn and contribute, so if my questions are kind of elementary then I apologize. I am hoping to get up to speed pretty quickly.

So in looking at implementing this, I have run into a little bit of a problem and perhaps you can tell me if I am on the right track or if I am missing the point entirely.

From my understanding, the idea is to use the native "Has" function in exposed by the go-datastore. But, from what I can see, the "Has" function will only take the "Key" as the parameter.

The current "isMaybeDupDeal" method might be a little misnamed, because we are not actually checking on the Deal at this stage. We are only checking if the PROPOSAL contains all the same elements as a previous Deal. The way in which it is done is by checking the PieceRef (which is a CID that I am assuming, but could be wrong, is the same if all attributes of the Proposal are the same, ie, the CID of the proposal), and checking the Miner Address. This would indicate to me that 2 identical deals can be proposed if they are for different miners, but not if they are for the same miner, which makes sense.

Where I come unstuck is how this logic gets incorporated into the Has method. If the Has method is checking the Deal CID, which is just the Proposal CID with a prefix, and we only receive the Proposal CID as part of the response to a proposal going out to a miner, then we can't do the check on the key itself because we don't have it yet. From what I can see though, the key is the only parameter the Has function takes.

I am taking a look at using the Query function to perhaps perform this "Has" functionality for deals/proposals, as it is also part of the go-datastore API and will probably offer better response times than retrieving and looping. The method would still be "Has" is store.go, but would -

1) Check if another Deals CID matches (using Has from the go-datastore)
...and if there is no Deal CID...
2) Query whether another deal exists where the proposals PieceRef and Miner Address combination matches

Again, please let me know if I am interpreting the spec correctly or if I am missing something (or a lot of things).

Thanks :slightly_smiling_face:

[frrist]

Hey @byoulten, I can give you a hand with this.

PieceRef is the hash of the data stored by a deal -- i.e. if a client made a deal to store file foo.txt with minerA and minerB, the PieceRef of those deals would be identical as it is the hash of foo.txt. For more details on this see the Storage Deal Make Spec

If the Has method is checking the Deal CID, which is just the Proposal CID with a prefix, and we only receive the Proposal CID as part of the response to a proposal going out to a miner, then we can't do the check on the key itself because we don't have it yet.

Given a deal proposal, we can calculate it's CID at any time. I would expect the Has method to look similar to:

import (
   "github.com/filecoin-project/go-filecoin/util/convert"
)

func (store *Store) Has(storageProposal *storagedeal.Proposal) (bool, error) {
    proposalCid, err := convert.ToCid(storageProposal)
    if err != nil {
        return false, err
    }
    key := datastore.KeyWithNamespaces([]string{StorageDealPrefix, proposalCid.String()})

    exists, err := store.dealsDs.Has(key)
  // query deals using the above `key` and compare their miner address

It sounds like you are on the right track, it also might be worth checking out #2790 for some inspiration.

[sahilnanda1995]

Hi guys,
I wanted to confirm is this assigned to someone or is it up for grabs.
Thanks

[anorth]

It's not assigned, but I suggest against taking it up right away. A large
amount of deal-related code is set to be deleted shortly to make way for
integrating a shared storage market module, so the fix may not be valuable.

On Thu, 12 Dec 2019 at 05:33, Sahil Nanda [e-mail] wrote:

Hi guys,
I wanted to confirm is this assigned to someone or is it up for grabs.

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
https://github.com/filecoin-project/go-filecoin/issues/2763?email_source=notifications&email_token=AADMW6WNABN5KHJYGQRKXHTQYG5JPA5CNFSM4HL5H37KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEGVNWGY#issuecomment-564845339,
or unsubscribe
https://github.com/notifications/unsubscribe-auth/AADMW6S2OQPLY552XFBCLBLQYG5JPANCNFSM4HL5H37A
.

[sahilnanda1995]

Thank you @anorth

Are the following issues still relevant

1. 3329: _Generalize node shutdown hooks_
2. 2757: _Prometheus Metric for Storage Power_
   (don't want to tag the issues here as Github will link it)

If not can you recommend any issue to start with that would be valuable to fix.
Thanks again 🙂

It's not assigned, but I suggest against taking it up right away. A large amount of deal-related code is set to be deleted shortly to make way for integrating a shared storage market module, so the fix may not be valuable.
…
On Thu, 12 Dec 2019 at 05:33, Sahil Nanda @.*> wrote: Hi guys, I wanted to confirm is this assigned to someone or is it up for grabs. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#2763?email_source=notifications&email_token=AADMW6WNABN5KHJYGQRKXHTQYG5JPA5CNFSM4HL5H37KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEGVNWGY#issuecomment-564845339>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AADMW6S2OQPLY552XFBCLBLQYG5JPANCNFSM4HL5H37A .