branch bound limits #1621
branch bound limits #1621
Conversation
larskuhtz
commented
Mar 10, 2023
•
larskuhtz
commented
- To see the specific tasks where the Asana app for GitHub is being used, see below:
- https://app.asana.com/0/0/1204717400575174
| | length (_branchBoundsUpper bounds) > defaultBoundsLimit = throwError $ err400Msg $ | ||
| "upper branch bound limit exceeded. Only " <> show defaultBoundsLimit <> " values are supported." | ||
| | length (_branchBoundsLower bounds) > defaultBoundsLimit = throwError $ err400Msg $ | ||
| "upper branch bound limit exceeded. Only " <> show defaultBoundsLimit <> " values are supported." |
There was a problem hiding this comment.
Should this say "lower branch bound limit exceeded"?
| @@ -156,6 +180,8 @@ branchHeadersHandler | |||
| :: TreeDb db | |||
| => ToJSON (DbKey db) | |||
| => db | |||
| -> Int | |||
There was a problem hiding this comment.
How about we start using new types for these?
| | length (_branchBoundsUpper bounds) > boundsLimit = throwError $ err400Msg $ | ||
| "upper branch bound limit exceeded. Only " <> show boundsLimit <> " values are supported." | ||
| | length (_branchBoundsLower bounds) > boundsLimit = throwError $ err400Msg $ | ||
| "upper branch bound limit exceeded. Only " <> show boundsLimit <> " values are supported." |
There was a problem hiding this comment.
Same question as above here.
src/Chainweb/Chainweb.hs
Outdated
| -- | ||
| p2pRequestSizeLimit :: Middleware | ||
| p2pRequestSizeLimit = requestSizeLimitMiddleware $ | ||
| setMaxLengthForRequest (\_req -> pure $ Just $ 8 * 1024) -- 8KB |
There was a problem hiding this comment.
Would it be better to make these tunable values via the config file? Or do we want to specifically discourage that?
There was a problem hiding this comment.
I think we don't want to encourage configuration here because a strict enough limit may inadvertently disable the mempool for larger txs.
There was a problem hiding this comment.
As discussed let's make the limit the same as that for the service API so that we can get these branch bound limits in.
edmundnoble
force-pushed
the
lars/bound-limits
branch
from
552c641
to
10df3c3
Compare
src/Chainweb/Chainweb.hs
Outdated
| -- | ||
| p2pRequestSizeLimit :: Middleware | ||
| p2pRequestSizeLimit = requestSizeLimitMiddleware $ | ||
| setMaxLengthForRequest (\_req -> pure $ Just $ 8 * 1024) -- 8KB |
There was a problem hiding this comment.
I think we don't want to encourage configuration here because a strict enough limit may inadvertently disable the mempool for larger txs.
src/Chainweb/Chainweb.hs
Outdated
| -- | ||
| p2pRequestSizeLimit :: Middleware | ||
| p2pRequestSizeLimit = requestSizeLimitMiddleware $ | ||
| setMaxLengthForRequest (\_req -> pure $ Just $ 8 * 1024) -- 8KB |
There was a problem hiding this comment.
As discussed let's make the limit the same as that for the service API so that we can get these branch bound limits in.
|
Where are the tests for this work? |
|
Testing of rate limits for the P2P should be done in a systematic and uniform way for all endpoints. We have already a script that systematically tests all endpoints for rate limits. The results from running that script is the basis for this PR. We are currently considering options to integrate that with into the CI pipeline. |
|
Possible unit tests could check the limits of the API, although, again I think, it would be good to do that more systematically instead of just adding individual tests on an ad-hoc basis. We used to have comprehensive tests of REST parameters. But I don't think that has been maintained over time. But the tests cases and the test setup should still exist -- so maybe we could update that to the latest APIs? |
|
This file could be a starting point for more comprehensive API tests: https://github.com/kadena-io/chainweb-node/blob/master/test/Chainweb/Test/RestAPI.hs |
|
Currently depends on https://github.com/kadena-io/chainweb-openapi/pull/20/files |
This is now merged, updated here as well |
* limit number of bounds in branch requests * tighten request size limit for p2p endpoints * Address review * add tests for bounds limits * point to other openapi spec * openapi spec: switch back to main --------- Co-authored-by: Edmund Noble <[email protected]> Co-authored-by: chessai <[email protected]> Co-authored-by: Edmund Noble <[email protected]> Co-authored-by: chessai <[email protected]>
