Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Define gRPC api call rate constraints #5103

Merged
merged 2 commits into from
Feb 10, 2021

Conversation

ghubstan
Copy link
Contributor

The general rule is limit calls that change p2p data to 1/minute, others to 1/second. An exception is made to set/remove wallet password methods (1/5s), due to the latency of writing wallet changes to disk.

This change may affect api testing in the future. If that happens, further changes to the call rate metering interceptor may be made to loosen the constraints when running in regtest/dev mode.

The general rule is limit calls that change p2p data to 1/minute,
others to 1/second.  An exception is made to set/remove wallet password
methods (1/5s), due to the latency of writing wallet changes to disk.

This change may affect api testing in the future.  If that happens,
further changes to the call rate metering interceptor may be made to loosen
the constraints when running in regtest/dev mode.
// Trying to set or remove a wallet password several times before the 1st attempt has time to
// persist the change to disk may corrupt the wallet, so allow only 1 attempt per 5 seconds.
put("setWalletPassword", new GrpcCallRateMeter(1, SECONDS, 5));
put("removeWalletPassword", new GrpcCallRateMeter(1, SECONDS, 5));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems like a race condition that can have severe consequences (wallet corruption). Maybe further reduce the rate to be on the safe side?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe. I was advised to set it to 1/sec, but bumped it to 5/sec for the reason you give here. Let's see what others have to say after they take a look at this.

Move this into a static CallRateMeteringInterceptor.valueOf(Map) method.
Copy link
Member

@sqrrm sqrrm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK

@sqrrm sqrrm merged commit 57e0bbc into bisq-network:master Feb 10, 2021
@ghubstan ghubstan deleted the 01-apply-rate-meters branch February 10, 2021 19:53
@ripcurlx ripcurlx added this to the v1.6.0 milestone Feb 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants