RPC: don't cap page size in unsafe mode

[gotjoshua]

actually belongs in a config file i guess, but this hard coded max creates weird behavior in downstream clis:
regen-network/regen-ledger#272

[codecov]

Codecov Report

Merging #6329 (2c476a9) into master (3a69056) will decrease coverage by 0.02%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #6329      +/-   ##
==========================================
- Coverage   60.80%   60.77%   -0.03%     
==========================================
  Files         282      282              
  Lines       26865    26865              
==========================================
- Hits        16334    16326       -8     
- Misses       8818     8836      +18     
+ Partials     1713     1703      -10     

Impacted Files	Coverage Δ
rpc/core/env.go	60.00% <100.00%> (ø)
consensus/reactor.go	66.25% <0.00%> (-4.96%)	⬇️
consensus/peer_state.go	78.94% <0.00%> (-3.95%)	⬇️
evidence/reactor.go	73.45% <0.00%> (-1.77%)	⬇️
p2p/transport_memory.go	85.13% <0.00%> (-1.36%)	⬇️
p2p/router.go	77.16% <0.00%> (-0.49%)	⬇️
consensus/state.go	66.75% <0.00%> (-0.10%)	⬇️
statesync/snapshots.go	91.59% <0.00%> (ø)
p2p/switch.go	60.37% <0.00%> (+0.46%)	⬆️
statesync/syncer.go	80.40% <0.00%> (+0.80%)	⬆️
... and 6 more

[tac0turtle]

Instead of changing the max value here, we should allow the maxPerPage to be overridden by what the user sets in the request.

[cmwaters]

I thought that the maxPerPage limit was to avoid clients requesting enormous amounts of data and thus slowing the node down. i.e. if we didn't have maxPerPage, a client could just make a query for all txs above height 0 and the node would have to load every tx (maybe there are other rate limiting mechanisms that I am not aware of).

we should allow the maxPerPage to be overridden by what the user sets in the request.

If the user can override it then we don't really need a max right?

[tac0turtle]

I thought that the maxPerPage limit was to avoid clients requesting enormous amounts of data and thus slowing the node down. i.e. if we didn't have maxPerPage, a client could just make a query for all txs above height 0 and the node would have to load every tx (maybe there are other rate limiting mechanisms that I am not aware of).

we should allow the maxPerPage to be overridden by what the user sets in the request.

If the user can override it then we don't really need a max right?

Agree here. Was trying to compromise, but clients should implement handling pagination

[gotjoshua]

clients should implement handling pagination

agreed, but only when it is really needed... a hard coded limit of 100 seems way too low,
with the default of 40, 1000 seems like a reasonable max... thus my proposal

[tac0turtle]

clients should implement handling pagination

agreed, but only when it is really needed... a hard coded limit of 100 seems way too low,
with the default of 40, 1000 seems like a reasonable max... thus my proposal

I would agree, if the RPC wasn't such a large DOS vector. There a known issues with mutex contention that slows down nodes when the RPC is used. My take is, I want to avoid defaulting behavior that could expose a larger dos vector.

Note: the SDK has the same default https://github.com/cosmos/cosmos-sdk/blob/master/types/query/pagination.go#L16.

[gotjoshua]

such a large DOS vector

hmmmm. excellent point. perhaps the large pages could be allowed only from localhost, or otherwise configurable in a safe way.

During the regen testnet, I ran an extra node just for data queries and stats calculations. It was not even exposed. In this case and other cases with other security measures in place to prevent DOS, this hard coded value is not cool.

Perhaps this PR is far too simple and dangerous, but could you imagine to add a config option in the future?

[cmwaters]

I think the best option, at least in the short term, may be to use the Unsafe config variable. When this is set to true, we remove this upper bound and let the client decide the perPage value in their request else if Unsafe is false, we keep with the 100 maxPerPage upper bound.

tendermint/config/config.go

Line 398 in 3a69056

Unsafe bool `mapstructure:"unsafe"`

[tac0turtle]

utACK