Avoid Unstable Sort

[shawntabrizi]

Following the norm set by: #6754

Undoes regressions caused by: #11154 (comment)

[bkchr]

Nothing you changed here is in any way consensus critical. The only place that is used on chain is in contracts and that has a comment above the code that explains why it is safe.

[mrcnski]

Was this the offending lint? Might want to set it to allow.

clippy::stable_sort_primitive
https://rust-lang.github.io/rust-clippy/master/index.html

[shawntabrizi]

Nothing you changed here is in any way consensus critical. The only place that is used on chain is in contracts and that has a comment above the code that explains why it is safe.

Understood, but as a rule, we should just not have it in the code base. If anything, we can put rules to explicit and easily audit for it. Else, someone needs to check each one each time to make sure it is not consensus critical.

[ordian]

Unstable sorting is indistinguishable from stable sorting when sorting primitive integers

Doesn't that lint apply only to a vec of primitive types? In that case, it's safe to use even in consensus-critical code.

Besides that, sort_unstable is deterministic, so it could only cause consensus problems if there are validators with native and (native or wasm) runtimes compiled with different versions of stdlib with different behavior (e.g. different seeds).
With the removal of the native runtime, I don't see how it could be a problem in the future. Maybe best not to cargo-cult some rules.

That being said, I do agree a ban for now is warranted as the risks outweigh the potential benefits.

[shawntabrizi]

Besides that, sort_unstable is deterministic

It is deterministic, but not between rust versions, which is the problem we ran into.

There was some optimization made, and it changed the final order between rust versions.

[shawntabrizi]

bot merge

[athei]

It is deterministic, but not between rust versions, which is the problem we ran into.

But why does this matter if the state transition function is defined as wasm? It was about the native runtime?

[bkchr]

AFAIR, we had this report about unstable_sort at some point, but we never had any problem on a live network because of it.

It was about the native runtime?

As always ;)

What @shawntabrizi probably means is this: https://medium.com/polkadot-network/a-polkadot-postmortem-24-05-2021-e96b25c184db and this was about binary_search_by.

But yeah, if we got the native runtime removed it should be easier. However, I could still imagine that host code is may leading to a consensus problem, but it would be much harder to provoke.