Fix telemetry rpc getting stuck if all nodes have bandwidth set to 0

[pwojcikdev]

I encountered this bug when I was doing some testing on private network, where all nodes have bandwidth set to 0. In that case the 'bandwidths' set is empty and std::next() has undefined behavior (gets stuck in infinite loop).

[thsfs]

what are the steps to reproduce this issue?

[pwojcikdev]

1. Create network of at least 10 nodes with config option 'bandwidth_limit = 0'
2. Try to call 'telemetry' rpc action
3. RPC never completes, IO thread stuck in infinite loop

[dsiganos]

I think this bug is caused by this if statement, which removes the zero bandwidth values.

if (telemetry_data.bandwidth_cap != 0)
{
	bandwidths.insert (telemetry_data.bandwidth_cap);
}

If all bandwidth_cap are set 0 then counts inside strip_outliers_and_sum when calculating bandwidth_sum will be empty.

Therefore, this should be reproducible by a unit test with 10 nodes having bandwidth set to 0.

@fikumikudev are you willing to write such a unit test?

[dsiganos]

Also, I do not think this bug is fully fixed by the proposed change. The function strip_outliers_and_sum expects to receive a list of at least 2 elements to operate correctly and this check only checks for empty list.

[pwojcikdev]

Yes, it looks like my fix is not complete. std::next will move the iterator past the end of a container, there are no checks there. I also think that this bug could potentially affect mainnet even, if someone set up a lot of nodes with bandwidth_limit = 0, so that num_either_side_to_remove is greater than the number of nodes with bandwidth limit set to non zero value, then any node that handles telemetry rpc would have all its io threads stuck, effectively taking down that node.

[pwojcikdev]

There are two ways to fix this I can think of, first is to rewrite strip_outliers_and_sum to properly respect container bounds, the second is to not skip zero bandwidth values, but instead replace them with something predefined, like 1Gbps maybe?

[dsiganos]

I think improving strip_outliers_and_sum to handle any number of elements is the right way to do it.

[pwojcikdev]

@dsiganos I fixed the strip_outliers_and_sum logic and added a unit test for this bug case. Please check if it looks good to you.

[dsiganos]

Yeap, I'll look at it now. I pushed a PR to your election scheduler RPC PR branch.
pwojcikdev#1

thank you for your contributions, they are great!