add configurable max blob size #675
Conversation
disperser/cmd/apiserver/main.go
Outdated
| return fmt.Errorf("configured max blob size is invalid %v", config.MaxBlobSize) | ||
| } | ||
|
|
||
| if int64(math.Log2(float64(config.MaxBlobSize))) == int64(math.Log2(float64(config.MaxBlobSize-1))) { |
There was a problem hiding this comment.
It's faster to use bit operation, config.MaxBlobSize > 0 && (config.MaxBlobSize&(config.MaxBlobSize-1)) == 0 should work
There was a problem hiding this comment.
this only runs once. I think it is more readable this way
There was a problem hiding this comment.
but your solution works
There was a problem hiding this comment.
I think there are utility functions to check if a number is power of 2 (in encoding or common package). Reusing that has no impact on readabilty.
There was a problem hiding this comment.
that function is in "github.com/Layr-Labs/eigenda/encoding/fft". I don't want to add a new strange dependancy, what do you think
There was a problem hiding this comment.
ideally we can pull it out to common/ and share across the codebase
disperser/cmd/apiserver/main.go
Outdated
| @@ -114,6 +115,14 @@ func RunDisperserServer(ctx *cli.Context) error { | |||
| ratelimiter = ratelimit.NewRateLimiter(reg, globalParams, bucketStore, logger) | |||
| } | |||
|
|
|||
| if config.MaxBlobSize < 0 || config.MaxBlobSize > 64*1024*1024 { | |||
There was a problem hiding this comment.
Should it fail if config.MaxBlobSize == 0?
There was a problem hiding this comment.
yeah, I will exclude 0
| @@ -94,6 +94,13 @@ var ( | |||
| EnvVar: common.PrefixEnvVar(envVarPrefix, "RATE_BUCKET_STORE_SIZE"), | |||
| Required: false, | |||
| } | |||
| MaxBlobSize = cli.IntFlag{ | |||
There was a problem hiding this comment.
It looks no issue if different disperser replicas are using different MaxBlobSize.
Should we add validation to the Node to check if the blob size is under a limit?
There was a problem hiding this comment.
I don't think there is issue. But we want it, it has to be different deployment, which the current devops repo has yet offered that.
I don't think we need to artificially constraint on blob size on the DA node size now, as far as the disperser isn't decentralized, but yes, once we got to the step. We need to have some rate limit or blob constraint. For now, I think we are good.
Empirically, there is no impact on validation speed with respect to the blob size.
disperser/cmd/apiserver/main.go
Outdated
| return fmt.Errorf("configured max blob size is invalid %v", config.MaxBlobSize) | ||
| } | ||
|
|
||
| if int64(math.Log2(float64(config.MaxBlobSize))) == int64(math.Log2(float64(config.MaxBlobSize-1))) { |
There was a problem hiding this comment.
I think there are utility functions to check if a number is power of 2 (in encoding or common package). Reusing that has no impact on readabilty.
disperser/cmd/apiserver/main.go
Outdated
| @@ -114,6 +115,14 @@ func RunDisperserServer(ctx *cli.Context) error { | |||
| ratelimiter = ratelimit.NewRateLimiter(reg, globalParams, bucketStore, logger) | |||
| } | |||
|
|
|||
| if config.MaxBlobSize <= 0 || config.MaxBlobSize > 64*1024*1024 { | |||
There was a problem hiding this comment.
So we want to support 64MiB?
There was a problem hiding this comment.
It is just a check, we might experiment with it
Why are these changes needed?
This PR makes max blob size a configurable parameters, validate if the blob is power of 2.
Checks