-
Notifications
You must be signed in to change notification settings - Fork 13
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
market: Real-world fixes #319
Conversation
de2dde8
to
486cd9a
Compare
The DenyUnknownClients appears to be flipped, at least in the webui |
bc622a3
to
ab418de
Compare
029ceb4
to
5ed280a
Compare
c05a071
to
95cad2d
Compare
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.
Looks good with some minor changes.
@@ -160,20 +153,35 @@ func StartHTTPServer(ctx context.Context, d *deps.Deps) error { | |||
Addr: cfg.ListenAddress, | |||
Handler: libp2pConnMiddleware(loggingMiddleware(compressionMw(chiRouter))), // Attach middlewares | |||
ReadTimeout: cfg.ReadTimeout, | |||
WriteTimeout: cfg.WriteTimeout, | |||
WriteTimeout: time.Hour * 2, |
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.
Can we use default here instead of a hardcoded value?
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.
So WriteTimeout generally isn't useful for mitigating much, unless you're trying to mitigate successful retrievals.
Later we could bring a much more advanced write rate enforcer from ribs so that /piece uploads have reasonable bounds, but for now setting a low write timeout guarantees that retrievals will fail
Ref https://github.com/FilOzone/ribs/blob/main/ributil/minratewriter.go
c7bcaa1
to
8b8bf64
Compare
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.
Looks good. We need couple of changes to make tests happy.
Don't merge yet, keeping this around until it mostly works