-
Notifications
You must be signed in to change notification settings - Fork 238
fix: Check bounds before precomputing SRS table #1846
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
Conversation
The latest Buf updates on your PR. Results from workflow Buf Proto / buf (pull_request).
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
I wonder if this is cacheable and load it or something ? |
We do cache a lot of the tables already. The problem is when the operator state changes in V1 we end up with some weird parameters like (DimE=4, CosetSize=2097152) which causes the encoders to panic and crash loop. The mitigation to the crashloop so far has been to generate that table on a separate machine and add it to the SRSTables folder for each encoder. This change would just allow us to emit an error message rather than crash loop the encoders. |
This is just V1 so doesn't really matter I guess, but going forward I've been trying to install that we start using https://joeduffyblog.com/2016/02/07/the-error-model/#bugs-arent-recoverable-errors instead of returning errors. Are we sure that this error is being sent back to the user as a 500 for example? A panic and recover (using https://pkg.go.dev/github.com/grpc-ecosystem/go-grpc-middleware/recovery) would give us that for free. |
Why are these changes needed?
This PR adds a defensive check to ensure that trying to generate tables > loaded SRS size will not panic but instead return an error.
Original issue:
2025/08/05 07:39:49 Table with params: DimE=4 CosetSize=2097152 does not exist
leads to:This is because we set the following configuration
The root issue is that if an SRS table is not precomputed we will try to compute it and store it. Computing an SRS table requires more points to be loaded so we hit a panic due to overflowing the buffer.
Options are:
This issue is not present on the V2 encoder since numChunks is fixed to 8192 which limits the amount of SRSTables we need precomputed. V1 encoding parameters are determined based on the operator state and so we've started to hit this issue due to instability on the Holesky operator state.
Checks