Relay rate limits #906

cody-littley · 2024-11-18T17:12:13Z

Why are these changes needed?

Adds server side rate limiting to the relay.

Checks

I've made sure the tests are passing. Note that there might be a few flaky tests, in that case, please comment that they are not relevant.
I've checked the new test coverage and the coverage percentage didn't drop.
Testing Strategy
- Unit tests
- Integration tests
- This PR is not tested :(

Signed-off-by: Cody Littley <cody@eigenlabs.org>

cody-littley · 2024-11-18T17:14:40Z

relay/config.go

+	// MaxKeysPerGetChunksRequest is the maximum number of keys that can be requested in a single GetChunks request.
+	// Default is 1024. // TODO should this be the max batch size? What is that?
+	MaxKeysPerGetChunksRequest int


@ian-shim what is the maximum number of keys we should permit in a single request?

~~I think we can start with something like 512? It will probably hit the grpc limit anyways if it goes beyond that.~~
Actually let's leave this with 1024 and talk about this offline.

cody-littley · 2024-11-18T17:15:48Z

relay/relay_test_utils.go

@@ -177,7 +177,7 @@ func buildChunkStore(t *testing.T, logger logging.Logger) (chunkstore.ChunkReade

 func randomBlob(t *testing.T) (*v2.BlobHeader, []byte) {

-	data := tu.RandomBytes(128)
+	data := tu.RandomBytes(225) // TODO talk to Ian about this


@ian-shim a blob size of 128 bytes results in 5 commitment length of 5, which the method to determine the length of a chunk says is illegal because it's not a power of 2. Can you explain this to me?

Yeah this is a requirement v2 started imposing - to force the blob length to be a power of 2. GetCommitments method should be refactored so that it automatically outputs a length in power of 2. @mooselumph is working on that.

Signed-off-by: Cody Littley <cody@eigenlabs.org>

ian-shim · 2024-11-18T22:28:29Z

relay/config.go

+	MaxKeysPerGetChunksRequest int
+
+	// RateLimits contains configuration for rate limiting.
+	RateLimits limiter.Config
 }

 // DefaultConfig returns the default configuration for the relay Server.
 func DefaultConfig() *Config {


nit: can we move this to test file? actual default values for optional fields can be set in flags.go

ian-shim · 2024-11-18T22:38:06Z

relay/config.go

+	// MaxKeysPerGetChunksRequest is the maximum number of keys that can be requested in a single GetChunks request.
+	// Default is 1024. // TODO should this be the max batch size? What is that?
+	MaxKeysPerGetChunksRequest int


~~I think we can start with something like 512? It will probably hit the grpc limit anyways if it goes beyond that.~~
Actually let's leave this with 1024 and talk about this offline.

ian-shim · 2024-11-18T22:46:11Z

relay/limiter/blob_rate_limiter.go

+		return fmt.Errorf("global concurrent request limit exceeded for getBlob operations, try again later")
+	}
+
+	allowed := l.opLimiter.AllowN(now, 1)


is this different from opLimiter.Allow()?

The only difference is that the time is supplied from the outside context. This allows me to write unit tests that aren't wall clock dependent.

ian-shim · 2024-11-18T22:47:07Z

relay/limiter/blob_rate_limiter.go

+	countInFlight := l.operationsInFlight.Add(1)
+	if countInFlight > int64(l.config.MaxConcurrentGetBlobOps) {
+		l.operationsInFlight.Add(-1)
+		return fmt.Errorf("global concurrent request limit exceeded for getBlob operations, try again later")
+	}


should this whole sequence be atomic? otherwise, can operationsInFlight go over MaxConcurrentGetBlobOps and potentially reject valid operations?

The situation where this logic may fail is if the request rate is so high that there are always MaxConcurrentGetBlobOps or more goroutines that happen to be inside this one block of code, which could in theory prevent any actual operations from being served.

Although such a scenario is unlikely, probably better to err on the side of safety. This now uses a mutex to guarantee atomicity.

Playing around with this a little, the code gets really ugy when I try and to low resolution locking. Methods in this class (as well as ChunkRateLimiter) are just doing basic arithmetic... let's just use a single mutext to protect the entire class for the sake of simplicity.

ian-shim · 2024-11-18T22:50:13Z

relay/relay_test_utils.go

@@ -177,7 +177,7 @@ func buildChunkStore(t *testing.T, logger logging.Logger) (chunkstore.ChunkReade

 func randomBlob(t *testing.T) (*v2.BlobHeader, []byte) {

-	data := tu.RandomBytes(128)
+	data := tu.RandomBytes(225) // TODO talk to Ian about this


Yeah this is a requirement v2 started imposing - to force the blob length to be a power of 2. GetCommitments method should be refactored so that it automatically outputs a length in power of 2. @mooselumph is working on that.

ian-shim · 2024-11-18T23:08:03Z

relay/server.go

+			"too many chunk requests provided, max is %d", s.config.MaxKeysPerGetChunksRequest)
+	}
+
+	// Future work: client IDs will be fixed when authentication is implemented


nit: // TODO(cody-littley): ...

ian-shim · 2024-11-18T23:10:36Z

relay/server.go

+	}
+
+	// Future work: client IDs will be fixed when authentication is implemented
+	clientID := fmt.Sprintf("%d", request.RequesterId)


do we allow unauthenticated traffic or all requests need to be signed (for MVP)?

My plan was to only permit authenticated traffic for GetChunks(). Is there any use case for allowing unauthenticated traffic? I can add it if you think it would be helpful.

ian-shim · 2024-11-18T23:12:25Z

relay/server.go

+
+		if req.GetByIndex() != nil {
+			key := v2.BlobKey(req.GetByIndex().GetBlobKey())
+			metadata = mMap[key]


check if the key exists in the map?

Should not be possible, since process will fail before this point if there is missing metadata. But it's a useful safety check, so I don't mind adding it.

relay/server.go

relay/metadata_provider.go

Signed-off-by: Cody Littley <cody@eigenlabs.org>

ian-shim

lgtm

ian-shim · 2024-11-19T17:26:47Z

relay/limiter/chunk_rate_limiter.go

+	}
+
+	if l.globalOperationsInFlight >= l.config.MaxConcurrentGetChunkOps {
+		return fmt.Errorf("global concurrent request limit exceeded for GetChunks operations, try again later")


nit: I think it would be useful to include the values (globalOperationsInFlight & MaxConcurrentGetChunkOps) in the error messages

added to this error message and all similar error messages

ian-shim · 2024-11-19T17:28:21Z

relay/limiter/chunk_rate_limiter.go

+		return fmt.Errorf("global rate limit exceeded for GetChunk bandwidth, try again later")
+	}
+
+	allowed = l.perClientBandwidthLimiter[requesterID].AllowN(now, bytes)


I know l.perClientBandwidthLimiter[requesterID] should exist because this method always gets called after BeginGetChunkOperation, but this library probably shouldn't assume that. Let's check if l.perClientBandwidthLimiter[requesterID] exists

added sanity check

ian-shim · 2024-11-19T17:31:11Z

relay/metadata_provider.go

@@ -153,8 +155,15 @@ func (m *metadataProvider) fetchMetadata(key v2.BlobKey) (*blobMetadata, error)
 		}
 	}

+	blobSize := uint32(cert.BlobHeader.BlobCommitments.Length)


Ah my bad. I meant BlobMetadata. Otherwise, we can also multiply Length by BYTES_PER_SYMBOL, but this will overestimate the size

cody-littley added 7 commits November 15, 2024 11:09

Added rate limiting.

02e9069

Signed-off-by: Cody Littley <cody@eigenlabs.org>

Properly handle blob sizes.

dc39c21

Signed-off-by: Cody Littley <cody@eigenlabs.org>

Incremental progress.

d99fd39

Signed-off-by: Cody Littley <cody@eigenlabs.org>

Incremental progress.

e39eaf9

Signed-off-by: Cody Littley <cody@eigenlabs.org>

Merge branch 'master' into relay-rate-limits2

876ec69

Signed-off-by: Cody Littley <cody@eigenlabs.org>

unit tests

9500f4f

Signed-off-by: Cody Littley <cody@eigenlabs.org>

Unit tests.

ffe18c6

Signed-off-by: Cody Littley <cody@eigenlabs.org>

cody-littley requested review from jianoaix and ian-shim November 18, 2024 17:12

cody-littley self-assigned this Nov 18, 2024

Fix tests.

79d9614

Signed-off-by: Cody Littley <cody@eigenlabs.org>

cody-littley commented Nov 18, 2024

View reviewed changes

Cleanup.

c1d83e6

Signed-off-by: Cody Littley <cody@eigenlabs.org>

cody-littley changed the title ~~Relay rate limits2~~ Relay rate limits Nov 18, 2024

cody-littley added 2 commits November 18, 2024 14:58

Fix test issue.

b5dc37c

Signed-off-by: Cody Littley <cody@eigenlabs.org>

Cleanup

b420cca

Signed-off-by: Cody Littley <cody@eigenlabs.org>

ian-shim reviewed Nov 18, 2024

View reviewed changes

cody-littley added 4 commits November 19, 2024 10:15

Merge branch 'master' into relay-rate-limits2

517b78e

Signed-off-by: Cody Littley <cody@eigenlabs.org>

Convert config to flag pattern.

7982aec

Signed-off-by: Cody Littley <cody@eigenlabs.org>

Simplify rate limiter classes.

366fdf7

Signed-off-by: Cody Littley <cody@eigenlabs.org>

Made suggested changes.

f2c10e4

Signed-off-by: Cody Littley <cody@eigenlabs.org>

ian-shim approved these changes Nov 19, 2024

View reviewed changes

cody-littley merged commit afd5894 into Layr-Labs:master Nov 19, 2024
6 checks passed

cody-littley deleted the relay-rate-limits2 branch November 19, 2024 20:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Relay rate limits #906

Relay rate limits #906

cody-littley commented Nov 18, 2024

cody-littley Nov 18, 2024

ian-shim Nov 18, 2024 •

edited

Loading

cody-littley Nov 18, 2024 •

edited

Loading

ian-shim Nov 18, 2024

ian-shim Nov 18, 2024

cody-littley Nov 19, 2024

ian-shim Nov 18, 2024 •

edited

Loading

ian-shim Nov 18, 2024

cody-littley Nov 19, 2024

ian-shim Nov 18, 2024

cody-littley Nov 19, 2024

cody-littley Nov 19, 2024

ian-shim Nov 18, 2024

ian-shim Nov 18, 2024

cody-littley Nov 19, 2024

ian-shim Nov 18, 2024

cody-littley Nov 19, 2024

ian-shim Nov 18, 2024

cody-littley Nov 19, 2024

ian-shim left a comment

ian-shim Nov 19, 2024

cody-littley Nov 19, 2024

ian-shim Nov 19, 2024

cody-littley Nov 19, 2024

ian-shim Nov 19, 2024

Relay rate limits #906

Relay rate limits #906

Conversation

cody-littley commented Nov 18, 2024

Why are these changes needed?

Checks

Choose a reason for hiding this comment

ian-shim Nov 18, 2024 • edited Loading

Choose a reason for hiding this comment

cody-littley Nov 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ian-shim Nov 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ian-shim left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ian-shim Nov 18, 2024 •

edited

Loading

cody-littley Nov 18, 2024 •

edited

Loading

ian-shim Nov 18, 2024 •

edited

Loading