validate ip parameter in set_bans rpc call

[eversinc33]

The set_bans method does not validate if there is actually a host or ip passed as a parameter. If a faulty RPC call fails to provide one of those, it falls back to banning 0.0.0.0:

IMO the better way to handle this would be to provide an error message upon a missing parameter, which I added here:

[eversinc33]

Theres quite a few places where, if wrong parameters are supplied or parameters are missing, a faulty error message is thrown or, as in this case, an action is taken that is plain wrong. If param names change or a developer simply mistypes a parameter name in their RPC client, a message such as "param x not supplied" would be much more helpful, too. If this is merged and OK as a fix, I would go for fixing the other bugs as well.

[iamamyth]

In most cases, I think the API should validate the data prior to performing any action, otherwise you end up with partially completed actions and the caller has no idea as to the effect of the call. For example, in this case, if you pass in 10 IPs, and the 5th one is rejected, you get the first four banned, and the rest not, but the error response would be the same with zero bans, or 9. Because this PR didn't introduce the "partial ban" behavior, I think it's OK, but, as a whole, the RPC API could benefit from a consistent approach with batching (e.g. return a status for each element in the batch, or pre-validate the whole batch and do nothing if invalid).

[eversinc33]

In most cases, I think the API should validate the data prior to performing any action, otherwise you end up with partially completed actions and the caller has no idea as to the effect of the call. For example, in this case, if you pass in 10 IPs, and the 5th one is rejected, you get the first four banned, and the rest not, but the error response would be the same with zero bans, or 9. Because this PR didn't introduce the "partial ban" behavior, I think it's OK, but, as a whole, the RPC API could benefit from a consistent approach with batching (e.g. return a status for each element in the batch, or pre-validate the whole batch and do nothing if invalid).

That is actually a very valid point - in this specific case, maybe ignoring the faulty element and moving on is the better approach.

While a simple for loop to validate all entries before would "work" in this case, I guess a proper way to fix this would involve thinking some more about a good way to design this, so that adding proper validation is not something that needs to be done manually for each call, but rather through some abstraction. That is definitely beyond the scope of this bugfix tho.

[iamamyth]

That is actually a very valid point - in this specific case, maybe ignoring the faulty element and moving on is the better approach.

Yes, I think what you're doing here makes sense because it's the backwards-compatible choice.