respect the rate limit from cid gravity #66
Conversation
merlinran
force-pushed
the
merlin/cid-gravity-rate-limit
branch
from
6990b70 to
9dc50c2
Compare
Signed-off-by: Merlin Ran <merlinran@gmail.com>
merlinran
force-pushed
the
merlin/cid-gravity-rate-limit
branch
from
9dc50c2 to
e381b84
Compare
| rules = r.(*rawRules) | ||
|
|
||
| if rules.DealRateLimit > 0 && rules.CurrentDealRate >= rules.DealRateLimit { | ||
| return |
There was a problem hiding this comment.
Q: How does the caller know if prices has a correct value? In this line valid = true and prices seems to have the default struct values. Maybe the default struct ResolvedPrices is interpreted as "Do not bid?"
(Same question applies to Maintenance Mode, etc; so probably making a dumb question).
There was a problem hiding this comment.
gosh I think you found a bug. valid==true means the result is valid, and https://github.com/textileio/bidbot/blob/main/service/service.go#L361 just uses the default prices if the VerifiedPriceValid or UnverifiedPriceValid is false, rather than stop bidding. huge huge bug. will fix
There was a problem hiding this comment.
fixed in 27c5850. I tested via make up with a real CID gravity token, and made sure that bidbot didn't bid if in maintenance mode. good catch! wonder why I didn't test that in the first place.
There was a problem hiding this comment.
@merlinran, one question left here: how does rules.DealRateLimit work here? I don't see this variable set anywhere but only in tests; maybe missing something? Shouldn't be updated when bidbot wins an auction?
There was a problem hiding this comment.
Or maybe is automatically fed by Lotus into cid-gravity.. so mostly outside of bidbot scope.
In theory whenever it wins a deal, we should propose a deal.. so that is fed by some Lotus cid-gravity plugin, and it would be updated that way?
There was a problem hiding this comment.
DealRateLimit is configured by the provider on the cid gravity console, and yeah, CurrentDealRate is updated by cid gravity server when it accepts a new deal by the lotus cid-gravity plugin.
It may be helpful to register one account on https://app.cidgravity.com to have a sense what it looks like.
| case http.StatusTooManyRequests: | ||
| reset := resp.Header.Get("X-Ratelimit-Reset") | ||
| ts, err := strconv.ParseInt(reset, 10, 0) | ||
| if err != nil { | ||
| return fmt.Errorf("parsing X-Ratelimit-Reset: %v", err) | ||
| } | ||
| t := time.Unix(ts, 0) | ||
| cg.apiRateLimitReset.Store(t) | ||
| return fmt.Errorf("CID gravity API rate limit is hit: %s/%s, won't reset until %v", | ||
| resp.Header.Get("X-Ratelimit-Remaining"), | ||
| resp.Header.Get("X-Ratelimit-Limit"), | ||
| t) |
There was a problem hiding this comment.
Quite impressed they already set a rate-limiting strategy; which is totally fine and good practice. 👍
I think would simply rate limit from our side to some safe limit like once per minute or similar since I wouldn't expect finer than that would be much relevant and maybe could simplify impl a bit.
There was a problem hiding this comment.
Yeah cidGravityCachePeriod is set to one minute so the API is hit at most once per minute. Mostly try to good behaving and log something if rate limit happens.
| // Use cached rules if they are loaded no earlier than this period. | ||
| cidGravityCachePeriod = time.Minute | ||
| ) | ||
|
|
||
| type rawRules struct { | ||
| Blocked bool | ||
| MaintenanceMode bool | ||
| DealRateLimit int | ||
| CurrentDealRate int |
There was a problem hiding this comment.
cid gravity allows the deal as long as CurrentDealRate <= DealRateLimit on the server side.
| rules atomic.Value // *CIDGravityRules | ||
| rulesLastUpdated atomic.Value // time.Time | ||
| rulesETag atomic.Value // string | ||
| apiRateLimitReset atomic.Value // time.Time |
There was a problem hiding this comment.
Not to be confused with the deal rate limit. It's the limit hitting CID gravity API.
| if rules.(*rawRules).Blocked { | ||
| rules = r.(*rawRules) | ||
|
|
||
| if rules.DealRateLimit > 0 && rules.CurrentDealRate >= rules.DealRateLimit { |
There was a problem hiding this comment.
Note >= here. Because the deal rate check happens on CID gravity server, if rules.CurrentDealRate is already rules.DealRateLimit, the next check by the lotus node would fail.
| @@ -127,48 +140,17 @@ func (cg *clientRules) maybeReloadRules(url string, timeout time.Duration, cache | |||
| if time.Since(lastUpdated) < cachePeriod { | |||
| return true | |||
| } | |||
| if t := cg.apiRateLimitReset.Load(); t != nil { | |||
| reset := t.(time.Time) | |||
| if time.Now().Before(reset) { | |||
There was a problem hiding this comment.
slight optimization to avoid hitting API server if API rate limit is hit.
| // proceed | ||
| case http.StatusNotModified: | ||
| return nil | ||
| case http.StatusTooManyRequests: |
There was a problem hiding this comment.
The status code and headers are confirmed with Florian.
Signed-off-by: Merlin Ran <merlinran@gmail.com>
the API will be live in one day or two.