Improved deals & retrieval record tracking & new indices and records API & support verified deals #772
Conversation
jsign
changed the title
jsign
changed the title
There was a problem hiding this comment.
This all looks technically very very good. Some great new features an improvements. I left a handful of comments and questions, but only one think to really consider before merging. Other thing I noticed is there's no updates to the cli, just wondering if that was an omission or coming in the future?
I kept up to date the existing commands with some new flag that I added. Regarding new APIs, yes, they don't have CLI commands and that won't happen in this PR. At the same time of doing this PR, I was also working in the I think your point is part of a bigger topic. At many places, we call "having a CLI command" just printing the raw proto as a JSON output. At some other places, we experimented using other libraries that are more interactive or similar. I could add those CLI commands easily, but I'm not totally convinced that's calling it a day in UX. In summary, I'd say that this falls into the "coming in the future", but I feel that we should decide if the CLI should have some minimum bar greater than printing a JSON. Not strong thoughts now, but I feel we went back and forth a bit before on that before. |
Signed-off-by: Ignacio Hagopian <[email protected]>
Signed-off-by: Ignacio Hagopian <[email protected]>
Signed-off-by: Ignacio Hagopian <[email protected]>
Signed-off-by: Ignacio Hagopian <[email protected]>
Signed-off-by: Ignacio Hagopian <[email protected]>
Signed-off-by: Ignacio Hagopian <[email protected]>
Signed-off-by: Ignacio Hagopian <[email protected]>
Signed-off-by: Ignacio Hagopian <[email protected]>
Signed-off-by: Ignacio Hagopian <[email protected]>
Signed-off-by: Ignacio Hagopian <[email protected]>
Signed-off-by: Ignacio Hagopian <[email protected]>
Signed-off-by: Ignacio Hagopian <[email protected]>
Signed-off-by: Ignacio Hagopian <[email protected]>
Signed-off-by: Ignacio Hagopian <[email protected]>
Signed-off-by: Ignacio Hagopian <[email protected]>
Signed-off-by: Ignacio Hagopian <[email protected]>
Signed-off-by: Ignacio Hagopian <[email protected]>
Signed-off-by: Ignacio Hagopian <[email protected]>
Signed-off-by: Ignacio Hagopian <[email protected]>
Signed-off-by: Ignacio Hagopian <[email protected]>
Signed-off-by: Ignacio Hagopian <[email protected]>
Signed-off-by: Ignacio Hagopian <[email protected]>
Signed-off-by: Ignacio Hagopian <[email protected]>
Signed-off-by: Ignacio Hagopian <[email protected]>
Signed-off-by: Ignacio Hagopian <[email protected]>
Signed-off-by: Ignacio Hagopian <[email protected]>
Signed-off-by: Ignacio Hagopian <[email protected]>
Signed-off-by: Ignacio Hagopian <[email protected]>
Signed-off-by: Ignacio Hagopian <[email protected]>
Signed-off-by: Ignacio Hagopian <[email protected]>
| FROM golang:1.15.5-buster as builder | ||
| FROM golang:1.16-buster as builder |
There was a problem hiding this comment.
Bumping Go base-image.
| DataTransferStart: timestamppb.New(time.Unix(r.DataTransferStart, 0)), | ||
| DataTransferEnd: timestamppb.New(time.Unix(r.DataTransferEnd, 0)), | ||
| SealingStart: timestamppb.New(time.Unix(r.SealingStart, 0)), | ||
| SealingEnd: timestamppb.New(time.Unix(r.SealingEnd, 0)), |
There was a problem hiding this comment.
Using @asutula suggestion.
| SealingStart: timestamppb.New(time.Unix(r.SealingStart, 0)), | ||
| SealingEnd: timestamppb.New(time.Unix(r.SealingEnd, 0)), | ||
| ErrMsg: r.ErrMsg, | ||
| UpdatedAt: timestamppb.New(time.Unix(0, r.UpdatedAt)), |
There was a problem hiding this comment.
Yep, UpdatedAt has nano-granularity.
| // The index stores at the nano precision. | ||
| lock.Lock() | ||
| updatedAtTime := time.Unix(rec.Time, nanoCounter).UnixNano() | ||
| nanoCounter++ | ||
| lock.Unlock() | ||
|
|
||
| var indexKey datastore.Key | ||
| if strings.HasPrefix(r.Key, "/deals/storage-") { | ||
| indexKey = datastore.NewKey("/deals/updatedatidx/storage").ChildString(strconv.FormatInt(updatedAtTime, 10)) | ||
| var sdr deals.StorageDealRecord | ||
| if err := json.Unmarshal(r.Value, &sdr); err != nil { | ||
| lock.Lock() | ||
| errors = append(errors, fmt.Sprintf("unmarshaling storage deal record: %s", r.Key)) | ||
| lock.Unlock() | ||
| return | ||
| } | ||
| sdr.UpdatedAt = updatedAtTime | ||
| buf, err := json.Marshal(sdr) | ||
| if err != nil { | ||
| lock.Lock() | ||
| errors = append(errors, fmt.Sprintf("marshaling storage deal record: %s", r.Key)) | ||
| lock.Unlock() | ||
| return | ||
| } | ||
| r.Value = buf | ||
| } else if strings.HasPrefix(r.Key, "/deals/retrieval") { | ||
| indexKey = datastore.NewKey("/deals/updatedatidx/retrieval").ChildString(strconv.FormatInt(updatedAtTime, 10)) | ||
| var rr deals.RetrievalDealRecord | ||
| if err := json.Unmarshal(r.Value, &rr); err != nil { | ||
| lock.Lock() | ||
| errors = append(errors, fmt.Sprintf("unmarshaling retrieval record: %s", r.Key)) | ||
| lock.Unlock() | ||
| return | ||
| } | ||
| rr.UpdatedAt = updatedAtTime | ||
| buf, err := json.Marshal(rr) | ||
| if err != nil { | ||
| lock.Lock() | ||
| errors = append(errors, fmt.Sprintf("marshaling retrieval record: %s", r.Key)) | ||
| lock.Unlock() | ||
| return | ||
| } | ||
| r.Value = buf |
There was a problem hiding this comment.
So, what changed here?
Recall that in this migration we're populating the new /deals/updatedatidx for existing records. The format of the key is /deals/updatedatidx/<updated-at-nano> that it's usually the value of the field UpdatedAt.
After doing some migration tests in our internal stack, I discovered two things to correct:
- For old records, I used
Timeas the<updated-at-nano>since it's the only time that exists in these records (UpdatedAtis a new field). But, a subtle thing happens here.Timehas seconds granularity, and in some cases it had collisions. I did some quick trick, and had a counter to use as the nano-part... so even if same things exists for the same second, they won't collide in the index. In new records this shouldn't happen, sinceUpdateAthas nano precision. - As things were, old records would have the
UpdatedAtnew field set to 0 (as being the default value). So I set those values as to have some coherency.
Maybe the code structure seems odd, but that's mostly because we're iterating keys that might be from different types. So, in each case we do the appropriate thing and update r.Value. Below, unconditionally, the original key value is updated (thing that wasn't there before, since this migration only populated /deals/updatedatidx).
| if err := ds.Put(datastore.NewKey(r.Key), r.Value); err != nil { | ||
| lock.Lock() | ||
| errors = append(errors, fmt.Sprintf("saving updated-at field: %s", err)) | ||
| lock.Unlock() | ||
| return | ||
| } |
There was a problem hiding this comment.
This is the Put that didn't exist before. Is mostly updating records with populated UpdatedAt field.
| // With TrimPrefix since the Key should be relative | ||
| // to the namespace of the store, and not the full-key. | ||
| if err := ds.Put(indexKey, []byte(strings.TrimPrefix(r.Key, "/deals"))); err != nil { |
There was a problem hiding this comment.
This TrimPrefix logic wasn't here before. This fixes an important missed thing.
The Store of records, store in /deals/updatedatidx values the key of the record that was created/updated.
BUT, that key is scoped in the Store namespace... not the global one. Since the Store is created in a key-wrapped-namespace /deals, it's important to take that into account when populating it.
Catched this while doing some testing in our internal stack.
| message GetUpdatedStorageDealRecordsSinceRequest { | ||
| google.protobuf.Timestamp since = 1; | ||
| int32 limit = 2; | ||
| } | ||
|
|
||
| message GetUpdatedStorageDealRecordsSinceResponse { | ||
| repeated powergate.user.v1.StorageDealRecord records = 1; | ||
| } | ||
|
|
||
|
|
||
| message GetUpdatedRetrievalRecordsSinceRequest { | ||
| google.protobuf.Timestamp since = 1; | ||
| int32 limit = 2; | ||
| } |
There was a problem hiding this comment.
Also changed GetUpdatedXXXX requests to google.protobuf.Timestamp.
| AskPrice: strconv.FormatUint(lastAsk.Price, 10), | ||
| AskVerifiedPrice: strconv.FormatUint(lastAsk.VerifiedPrice, 10), |
There was a problem hiding this comment.
Oh, also switching prices to string in the API.
We still need to change things internally to avoid using uint64 too, to basically avoided the same problem.
Created #776 to track.
This PR includes new features and improvements to Powergate.
Lots of things happened here. To improve visibility and review having a mental map of changes, I'll describe the big-picture of changes in different sections.
Most of these changes are motivated by this Textile PR
We add verified-deals support in StorageConfigs. A new attribute VerifiedDeal indicates that new deals will be marked as verified in the deal proposal. Of course, the wallet address making the deal should be a verified-client and have enough data-cap to create the verified-deal.
When a verified-deal deal is done, all existing miner-selectors use the verified-price from miners instead of the usual price for unverified data. The semantics of MaxPrice still hold, being verified or unverified, it will block prices that are above MaxPrice.
A new group of APIs was created to expose indices data for miners.
These APIs are:
GetMiners: which allows getting a list of all miner-addresses (and no more information) that satisfy some filters (e.g: have power >0 in the network).GetMinersInfo: Provided with a list of miner-addresses, it will return all known information from indices from these miners, such as ask-price, verified-ask price, relative power, sector-size, min/max piece size, etc.If an external application wants to update information about miners, it can:
GetMinersto get a list of miner-address of all known miners (this output won't be big since only their address is included).GetMinersInfoto avoid making big queries. Say, fetching in batches of 30 or 40 miners.Also, my intention is to let
GetMinersInfoto include information from the miners index, ask index, or any other internal index. I think this is good to simply have in a single API call consolidated information about the miner. Doing more granular APIs for the different indices doesn't sound like something needed, and also explodes the API surface.A new group of APIs was created to expose deals record fetching, as to understand made deals and retrievals executed by Powergate.
The idea behind these APIs is that an application can fetch incrementally new records that are created or updated with new information:
GetUpdatedRetrievalRecordsSince(ctx context.Context, sinceNano int64, limit int)GetUpdatedStorageDealRecordsSince(ctx context.Context, sinceNano int64, limit int)With these APIs, the application can fetch which records changed since some last known
sinceNanotime, and get the updated information. It can also have some safety-bounds usinglimitto avoid very big outputs if there's too much data to be discovered. If that's the case, it can continue asking for the next set of updated records in a batching mode (sinceNano+limit).The way this is implemented is every time a record is created or touched, it's included in an updated-index.
This change needed to create a migration to allow existing records to be discovered by these new APIs. If no migration is done, querying for the old records would result in zero results. To allow all (old and new) records to be discovered by the same new system, I created a migration that sets the update-index modified timestamp to the existing records' timestamp. This allows you not to have special cases where you can't rely on the updated-index or having to create "GetAllRecords" style APIs to get old-things or similar.
I took advantage and refactored the deals module in mulitiple files, and simplified the
WatchAPI.Also, the
storeof records now it's in its own package with clearly exported methods. Also, the API of this store was simplified.Before this PR, if a deal failed, it's the corresponding deal-storage record (
Pending: true) was deleted. That's not the case now. A new fieldFailedis included, and if the deal fails (or timeouts), it would be set totrue.This allows to not lose information about attempted deals that failed. Any attempt to make a deal will be recorded.
Retrieval records now also include their
IDas an attribute. This also needed some work as a migration since existing records didn't have anIDvalue.The main argument for doing this is because when clients receive retrieval records since their values don't have an ID, it is tough to merge that data with existing data the client has fetched before. Saying it differently, if you don't have an ID... how you "join" to update?
Of course, we could ask the client to do the MD5 of the concatenated 4 values we use internally... but we are not evil with our clients. :)
The Ask index now also includes the Verified Ask Price. Before, we only stored the general storage price, but miners also advertise the price if you're making a verified deal.