[bisq-network/bisq] Prevent excessive api calls (#4966)

[sqrrm]

@sqrrm requested changes on this pull request.

Mainly I would like to see a moving time window for the rate limit.

Also, I might have missed but is the api only supposed to work on *nix systems? Seems very specific in many places.

> +    private void handleInterceptorConfigErrorAndCloseCall(ServerCall<?, ?> serverCall)
+            throws StatusRuntimeException {
+        String methodName = getRateMeterKey(serverCall);
+        String msg = format("%s's rate metering interceptor is incorrectly configured;"
+                        + "  its rate meter cannot be found ",
+                methodName);
+        log.error(StringUtils.capitalize(msg) + ".");
+        serverCall.close(FAILED_PRECONDITION.withDescription(msg), new Metadata());
+    }
+
+    private void handlePermissionDeniedErrorAndCloseCall(String methodName,
+                                                         GrpcCallRateMeter rateMeter,
+                                                         ServerCall<?, ?> serverCall)
+            throws StatusRuntimeException {
+        String msg = getDefaultRateExceededError(methodName, rateMeter);
+        log.error(StringUtils.capitalize(msg) + ".");

This should probably be a `log.warn` as it's not an error but rather expected when the rate limit is exceeded. Might consider changing the method name as well for the same reason.

> +import java.util.concurrent.TimeUnit;
+
+import lombok.Getter;
+import lombok.extern.slf4j.Slf4j;
+
+import javax.annotation.Nullable;
+
+import static java.lang.String.format;
+
+ at Slf4j
+public final class GrpcCallRateMeter {
+
+    @Getter
+    private final long allowedCallsPerTimeUnit;
+    @Getter
+    private final TimeUnit timeUnit;

I find this a bit odd. I think it makes more sense to set the time window in seconds and just have a `int` with number of seconds in the time window.

> +    @Getter
+    private boolean isRunning;
+
+    @Nullable
+    private Timer timer;
+
+    public GrpcCallRateMeter(long allowedCallsPerTimeUnit, TimeUnit timeUnit) {
+        this.allowedCallsPerTimeUnit = allowedCallsPerTimeUnit;
+        this.timeUnit = timeUnit;
+    }
+
+    public void start() {
+        if (timer != null)
+            timer.stop();
+
+        timer = UserThread.runPeriodically(() -> callsCount = 0, 1, timeUnit);

My expectation is to have a moving time window with the rate limit. The way it's done now I can send double the max amount if I send all at the end of the first period and then continues at the beginning of the second period.

> +                                     TimeUnit timeUnit) {
+            log.info("Adding call rate metering definition {}.{} ({}/{}).",
+                    grpcServiceClassName,
+                    methodName,
+                    maxCalls,
+                    StringUtils.chop(timeUnit.name().toLowerCase()));
+            rateMeterConfigs.stream().filter(c -> c.isConfigForGrpcService(grpcServiceClassName))
+                    .findFirst().ifPresentOrElse(
+                    (config) -> config.addMethodCallRateMeter(methodName, maxCalls, timeUnit),
+                    () -> rateMeterConfigs.add(new GrpcServiceRateMeteringConfig(grpcServiceClassName)
+                            .addMethodCallRateMeter(methodName, maxCalls, timeUnit)));
+        }
+
+        public File build() {
+            File tmpFile = serializeRateMeterDefinitions();
+            File configFile = new File("/tmp/ratemeters.json");

Not sure this is right unless it's only supposed to work on *nix.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/bisq-network/bisq/pull/4966#pullrequestreview-557040093
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.bisq.network/pipermail/bisq-github/attachments/20201222/48f213a2/attachment-0001.htm>