Refactory of Fork-Tree data structure

[davxy]

WARNING

This is a critical structure used by both GRANDPA and BABE consensus algorithms.
As such, a rigorous review is required

---

A good part of the methods where rewritten in order to:

• provide a non-recursive implementation of all the methods
• simplification of the overall implementation
• rebalance only when strictly required (traversal is not free)
• removal of some code without altering the structure api

---

A couple of well circumscribed unsafe operations were introduced.

In particular, to traverse the tree with the intent to mutate some parts of it, in an iterative implementation we need to maintain multiple mutable references to some nodes (in a stack).

This is not allowed in safe Rust.

Here we're resorting to raw pointers in a couple of places with extra attention to only use them for data that has a provable stable address (not subject to relocation/deletion)

[koute]

Regarding the unsafe parts - I haven't looked at the code very closely, but it might be worthwhile to run the tests through MIRI, and for a core data structure like this one it'd also be nice (if possible) to hook it up to a fuzzying harness and see if it finds anything (if we want to be absolutely sure it's correct).

[davxy]

Regarding the unsafe parts - I haven't looked at the code very closely, but it might be worthwhile to run the tests through MIRI, and for a core data structure like this one it'd also be nice (if possible) to hook it up to a fuzzying harness and see if it finds anything (if we want to be absolutely sure it's correct).

Unsafe code has been removed at all. For map I've used @koute gist, for drain_filter I've used a similar method ;-)

[koute]

Can you also add a test (or modify an existing one) which tests that the map returns the roots in the right order?

(self.roots.into_iter().rev().map(|node| (usize::MAX, node)).collect(); -> self.roots.into_iter().map(|node| (usize::MAX, node)).collect(); should make it fail; last time I checked adding/deleting the rev() made no difference in which tests were passing)

[davxy]

Can you also add a test (or modify an existing one) which tests that the map returns the roots in the right order?

(self.roots.into_iter().rev().map(|node| (usize::MAX, node)).collect(); -> self.roots.into_iter().map(|node| (usize::MAX, node)).collect(); should make it fail; last time I checked adding/deleting the rev() made no difference in which tests were passing)

@koute done

[koute]

Still missing a comment that the iteration order in unspecified. (:

Besides that LGTM!

[andresilva]

Overall LGTM. Just one thing that might be missing is a custom implementation of Decode/Encode since the generated codec will likely use recursion as well. I guess this can be tested by creating a very deep tree and then trying to decode/encode.

[davxy]

Overall LGTM. Just one thing that might be missing is a custom implementation of Decode/Encode since the generated codec will likely use recursion as well. I guess this can be tested by creating a very deep tree and then trying to decode/encode.

@andresilva I will check that and if that is the case I'll open one issue as a reminder to implement it in a separate PR