Implement server-side rate limits for Key Images on FLR #3314
Conversation
awygle
force-pushed
the
feature/fog-ledger-router
branch
3 times, most recently
from
e024ac6 to
fee2eb8
Compare
awygle
force-pushed
the
awygle/flr-rate-limit
branch
from
a905af2 to
5802dbb
Compare
awygle
force-pushed
the
awygle/flr-rate-limit
branch
from
5802dbb to
996a1ee
Compare
nick-mobilecoin
left a comment
nick-mobilecoin
left a comment
There was a problem hiding this comment.
main blocker is the test, and local manipulation of the test, doesn't seem to indicate it works correctly.
I haven't reviewed new dependencies. I like the idea of the interface for the governor crate, I wish it didn't pull so many deps in.
| } | ||
|
|
||
| /// Resource exhausted error may be returned if a rate limit is exceeded. | ||
| #[inline] |
There was a problem hiding this comment.
🔧 I'm guessing this use of #[inline] is more for consistency with the rest of the file.
I would vote for not using #[inline] over keeping the file consistent, unless it can be proven to be required for performance.
While inline is a technically a hint to the compiler, implementation wise, it's treated as "almost always inline". Looking at these functions/macros I've got a strong hunch function call overhead is a drop in the bucket compared to the other work happening. Inline can have negative side effects like; binary code bloat and developers reluctant to refactor since inline can often indicate performance critical code.
There was a problem hiding this comment.
I'm fine with removing #[inline] (I do agree it's not needed in these cases), but I should mention that "it's treated as 'almost always inline'" is LLVM's default mode (the joke on the LLVM developer IRC is "LLVM's inlining heuristic is 'yes'").
| rate_limit_burst_period: test_config | ||
| .rate_limit_period | ||
| .unwrap_or(Duration::from_secs(10)), | ||
| rate_limit_max_burst: test_config | ||
| .rate_limit_max | ||
| .unwrap_or(NonZeroU32::new(80).unwrap()), |
There was a problem hiding this comment.
🔧 This feels a little odd that the test config is set to None and then here it decides to use a default based on None. The best wording I can think is that this spreads the "default" logic around (of the TestConfig) instead of keeping it in one place.
Suggest creating a new() method for TestConfig that the other tests use to initialize with default limits,
And then if desired create a new with limits for the limit test.
| pub async fn check_key_images( | ||
| &mut self, | ||
| key_images: &[KeyImage], | ||
| ) -> Result<CheckKeyImagesResponse, Error> { |
There was a problem hiding this comment.
❓ Double checking. This was removed because it's redundant with the following?
.with_context(create_context(&tracer, "check_key_images"))
This PR implements server-side rate limits for Key Images on FLR
Motivation
This is needed because of bad clients locking up our servers.
Future Work
This implementation isn't what we want to end up with long-term. We ultimately want to return rate limit messages in-band, with a higher hard rate limit for clients that don't respect them.