Throttle related panic cleanup

[shaspitz]

Description

For every panic in throttle.go or closely related files, this PR either:

1. Clarifies with comments why the panic is still included
2. Removes the panic in favor of an error log to prevent undesirable binary behavior

Linked issues

Works toward #621

Type of change

• Fix: Changes and/or adds code behavior, specifically to fix a bug
• Docs: Adds documentation

New behavior tests

n/a, fix was small and only related to queries

[mpoke]

Nice work @smarshall-spitzbart. See my comments below. Also, the following throttling methods are called from EndBlock:

k.CheckForSlashMeterReplenishment(ctx)
k.HandleLeadingVSCMaturedPackets(ctx)
k.HandleThrottleQueues(ctx)

Could you add for each one a docstring describing the scenarios when they panic?

[danwt]

Suggestion: prefix every func which calls panic directly with Must
Tiny nit: some of the comments are a bit verbose ('Explanation:', 'Further', 'In this case'). They are very clear though so that's good

Overall good job!

[mpoke]

prefix every func which calls panic directly with Must

@danwt Prefixing getters with Must may imply that the key must exist. I think it's okay to go ahead with the pattern that getters and setters panic if marshaling / unmarshaling fails. We just need to make sure that the data for setters is valid.

[shaspitz]

Nice work @smarshall-spitzbart. See my comments below. Also, the following throttling methods are called from EndBlock:

k.CheckForSlashMeterReplenishment(ctx)
k.HandleLeadingVSCMaturedPackets(ctx)
k.HandleThrottleQueues(ctx)

Could you add for each one a docstring describing the scenarios when they panic?

Yup! See 19f6afa

[mpoke]

Nice work @smarshall-spitzbart

[MSalopek]

Overall, LGTM!
I am in favor of adding a Must prefix to functions that perform logic other than Get/Set since the pattern of panicking inside Set/Get is widespread in cosmos-sdk.

Maybe we could aggregate errors that happened during queue iteration and return them as part of the query result in another PR?

Besides that, queries should use pagination to avoid dumping large number of records from the throttle queues. Not sure, but that could also be a very easy DoS attack vector. I doubt it would affect validator nodes (provided the validators use sentry nodes) but the RPC/proxy/sentry nodes could be affected.

[danwt]

Great work!

[shaspitz]

Re "must" naming conventions, see #650. We can then address this for the entire repo

Maybe we could aggregate errors that happened during queue iteration and return them as part of the query result in another PR?

Seems like a good idea 👍, thanks for the input @MSalopek. Could you make an issue for this, as you may have more context as to whether this error accumulation pattern could be applied to other queries as well

[MSalopek]

Re "must" naming conventions, see #650. We can then address this for the entire repo

Maybe we could aggregate errors that happened during queue iteration and return them as part of the query result in another PR?

Seems like a good idea 👍, thanks for the input @MSalopek. Could you make an issue for this, as you may have more context as to whether this error accumulation pattern could be applied to other queries as well

Issue opened: #651

Thanks!