Add explicit tie-breaker in pending balance deposits sort

[jtraglia]

PR Description

Out of caution, this PR adds a thenComparing(index) call when sorting validators for pending balance deposits. See a screenshot of the relevant spec below. Because we're iterating from 0-len(vals), the current implementation should be fine but if this doesn't affect performance (which it shouldn't) I see no harm in adding this.

Documentation

• I thought about documentation and added the doc-change-required label to this PR if updates are required.

Changelog

• I thought about adding a changelog entry, and added one if I deemed necessary.

[mehdi-aouadi]

I'm not sure this won't trigger another sorting round.
The IntStream.range(0, validators.size()) already gave a sequential ordered stream as you mentioned, we could simply add a comment to make it clear.
Plus, since there is a filtering step in between, I'm almost sure that a second unnecessary sorting will be trigger anyway.
Parallelising this logic might be a good optimisation otherwise

[tbenr]

I think this doesn't trigger an additional round, should be indistinguishable from a perf perspective IMO.
Thus I'm for the change because it makes the correct sorting criteria explicit.

[tbenr]

LGTM

[jtraglia]

I'm almost sure that a second unnecessary sorting will be trigger anyway.

Hmm I don't think so. I made this simple test. See that b isn't used in the 2nd comparison.

@Test
void tieBreakerIsNotCalled() {
  record Item(String name, int foo, int bar) {}
  final List<Item> items = new ArrayList<>();
  items.add(new Item("a", 10, 100));
  items.add(new Item("b", 20, 200));
  items.add(new Item("c", 10, 150));

  final var sorted = IntStream.range(0, items.size()).boxed().sorted(
      Comparator.comparing(
          (Integer index) -> {
            System.out.println("Comparing " + items.get(index).name + ".foo: " + items.get(index).foo);
            return items.get(index).foo;
          }).thenComparing((Integer index) -> {
        System.out.println("Comparing " + items.get(index).name + ".bar: " + items.get(index).bar);
        assertThat(items.get(index).name).isNotEqualTo("b");
        return items.get(index).bar;
      }));

  sorted.forEach(System.out::println);
}

Parallelising this logic might be a good optimisation otherwise

Yes, this is good idea. But it will need the thenComparing call for that. Because with parallel streams, the sorted() operation does not guarantee that elements with equal sort keys will retain their original order.

[tbenr]

About the parallelism argument, we do parallel stream only in one case, (IIRC to go through all validators). To make it profitable you need to have a lot of elements to stream (or a complex processing over each element), otherwise you're just adding thread coordination overhead.
I don't think in this case we will have a deposit queue large enough to actually get some benefits. Not sure about the processing complexity though.

BTW queueEntireBalanceAndResetValidator needs to be executed in sequence due to the state.getPendingDeposits().append(deposit);