Rewrite fork tree functions using loops.

[expenses]

This PR is intended to fix #5998. Flaming Fir currently panics when --light is used on the debug build, but not on the release build.

import is done, find_node_index_where is in progress.

[expenses]

@andresilva do you think you could help me set up a set of tests for find_node_index_where? I'm struggling to understand why something like:

#[test]
fn find_index_where() {
	let (mut tree, is_descendent_of) = test_fork_tree();

	assert_eq!(
		tree.find_node_index_where(&"A", &1, &is_descendent_of, &|&()| true),
		Ok(Some(vec![0]))
	);
}

panics with:

---- test::find_index_where stdout ----
thread 'test::find_index_where' panicked at 'assertion failed: `(left == right)`
  left: `Ok(None)`,
 right: `Ok(Some([0]))`', utils/fork-tree/src/lib.rs:1004:9
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

[expenses]

I've written out some tests for that now.

[bkchr]

Why did you remove the custom decode implementation?

[bkchr]

(The one that you have written)

[expenses]

Why did you remove the custom decode implementation?

I haven't seen a node crash because of that yet, but I'll put it if I encounter that.

[bkchr]

Please bring it back. I have seen this. First the node crashes because of import and on restart the decode fails.

[expenses]

I've been rewriting the find_node_index_where function and have been fuzzing the modified implementation against the original to check for bugs. In doing so, I've come across a situation where I think my implementation handles correctly while the original does not. I've added it as a test find_node_where_specific_value:

#[test]
fn find_node_where_specific_value() {
    let mut tree = ForkTree::new();

		//
	// A - B
	//  \
	//   — C
	//
	let is_descendent_of = |base: &&str, block: &&str| -> Result<bool, TestError> {
		match (*base, *block) {
			("A", b) => Ok(b == "B" || b == "C" || b == "D"),
			("B", b) | ("C", b) => Ok(b == "D"),
			("0", _) => Ok(true),
			_ => Ok(false),
		}
	};

	tree.import("A", 1, 1, &is_descendent_of).unwrap();
	tree.import("B", 2, 2, &is_descendent_of).unwrap();
	tree.import("C", 2, 4, &is_descendent_of).unwrap();

	assert_eq!(
		tree.find_node_where(&"D", &3, &is_descendent_of, &|&n| n == 4)
			.map(|opt| opt.map(|node| node.hash)),
		Ok(Some("C"))
	);
}

and it fails with:

---- test::find_node_where_specific_value stdout ----
thread 'test::find_node_where_specific_value' panicked at 'assertion failed: `(left == right)`
  left: `Ok(None)`,
 right: `Ok(Some("C"))`', utils/fork-tree/src/lib.rs:1755:9
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

at present. However I believe this test should pass, as D is a descendant of C. @andresilva What do you think?

[andresilva]

@expenses In that example D is a child of both B and C which is not possible since B and C are on different branches.

let is_descendent_of = |base: &&str, block: &&str| -> Result<bool, TestError> {
	match (*base, *block) {
		("A", b) => Ok(b == "B" || b == "C" || b == "D"),
		("C", b) => Ok(b == "D"),
		("0", _) => Ok(true),
		_ => Ok(false),
	}
};

I think the current implementation will early exit after checking B since it exploits the fact that D can only be a child of one branch.

[expenses]

@andresilva so this is caused by a bad is_descendent_of? I think we can ignore that for now then.

[andresilva]

Neat :)

[expenses]

On master, cargo run --release -- --light sync at roughly 1250 bps. On this branch, it syncs at roughly 16 bps, so something is clearly wrong here.

I've implemented the suggested changes in #7337 (comment) and #7337 (comment) and this issue seems to have been resolved 🎉

[andresilva]

Can we add a test for encoding/decoding and then making sure both structures are equal?

We will need to do a resync from scratch again of Polkadot and Kusama (with a full node), but we might as well do it just before we are ready to merge. It's a bit annoying but this touches consensus critical code and we really need to be sure we aren't breaking anything.

[andresilva]

Could you expand this proof, or maybe add it as a comment on why there must be at least 2 items on the stack. My understanding is that the only situation where we'd only have one item on the stack is if the root node has no children, otherwise we'll always have at least 2 nodes which are the root and whatever current children (or grandchildren..) we're currently handling.

[expenses]

Done

[andresilva]

We should add a comment on what's going on in this test. IIRC this is the test with the funky is_descendent_of where D is a child on two different forks, what is the behavior we are testing here? Initially we wanted to test that we bail out early when we test against B and the predicate fails (i.e. we wouldn't test C since D was already a known descendent of B).

[expenses]

Yeah, after the changes I don't think we're testing for anything specific here, just correctness.

[expenses]

@andresilva Any suggestions on how I could fix the failing grandpa test?

[expenses]

When I tried to sync polkadot on the light client using this, I get the following at block 9,593:
2020-11-05 14:31:19 💔 Verification failed for block 0x3f276962de02b4ce592df31ddfa7fdc595637ab042515ea2e685d1ac5c0ff9bf received from peer: 12D3KooWKBAmuHPd8xFFszN7qFXLmTDmtffbk5wGRxkgrwnqEDaG, "Invalid author: Expected secondary author: Public(5e67b64cf07d4d258a47df63835121423551712844f5b67de68e36bb9a21e127 (138nJzAB...)), got: Public(6236877b05370265640c133fec07e64d7ca823db1dc56f2d3584b3d7c0f16158 (13DmthD9...))."

[andresilva]

I guess the implementations I posted have a bug, the error you're getting means that some transition wasn't applied correctly. And the grandpa test that's failing also indicates the same. I'll have to look into the grandpa test.

[andresilva]

Regarding the grandpa test the issue is with the test itself, we add two pending changes that are supposed to be on different forks but we pass an is_descendent_of function that will always return true. The reason this worked before is because the import method would verify the block number before checking on is_descendent_of, which isn't the case now. Either way this should be fine as the is_descendent_of used by the client should be correctly defined and would not exploit this behavior of checking block numbers to verify whether two blocks are on the same fork. Changing https://github.com/paritytech/substrate/blob/master/client/finality-grandpa/src/authorities.rs#L796-L797 to static_is_descendent_of(false) should make the test pass.

There's still something wrong though if you're getting that light client error when syncing. I wonder if it's related to rebalancing, maybe just commenting that code and trying to replicate the issue would allow us to pinpoint it.

[andresilva]

Nevermind I don't think it could be related to rebalance() as it doesn't alter the tree structure (only ordering of branches).

[expenses]

@cheme would it be a good idea to merge this branch: master...cheme:iter_forktree into this?

[cheme]

I am not against it, it depends how much we want to avoid unsafe code (for my part I find it fine, but it can be a vast debate).

There is also the option to keep recursive code since it is a bit more readable. (I think that the stack overflow is a limit case related to block not being finalized, so it could be an acceptable to skip the switch).
If keeping recursive code, I think we should still keep the new tests and add this fix: a9f13cc .

[andresilva]

No, let's not add unsafe code here, that seems a bit unnecessary IMO. I'm pretty sure the issue you have observed isn't related with the rebalance() method since the code I provided doesn't change the tree structure (the only mutation is sorting vecs of children), so it cannot affect correctness. This can be verified by just commenting all the code in rebalance() and observing that the crash still happens.

There is also the option to keep recursive code since it is a bit more readable. (I think that the stack overflow is a limit case related to block not being finalized, so it could be an acceptable to skip the switch).

For me recursive code is a lot easier to read, especially in tree structures, although I know it's not like that for everyone. It is true that this is an edge case that shouldn't happen under normal operation (i.e. if we finalize something every now and then), but it would still be nice if we didn't stack overflow. Given that we already spent some time on this I'd be in favor of still trying to merge these changes to make the algorithms iterative. I will try to debug this but I cannot make any promises right now.

[expenses]

Given that we already spent some time on this I'd be in favor of still trying to merge these changes to make the algorithms iterative.

Sunk cost fallacy 😉

All I want is to stop the stack overflow issue from happening so the light client is usable. How we solve this problem doesn't really matter. It'd probably better if we didn't use recursion because this issue could come back if we solve it in a different way, but we can keep this branch around for that case.

[expenses]

Oh hold for now.