Fix leaf block removal in the backend

[davxy]

This PR introduces a new remove method for the leaves set maintained by the backend.

The remove, import, and finalization methods return types were named/renamed

• RemoveOutcome
• ImportDisplaced -> ImportOutcome
• FinalizationDisplaced -> FinalizationOutcome

This naming/renaming has been done to better reflect what the operations are actually doing.
That is, for example:

• an import always adds a new leaf, but not always removes its parent from the leaves set (the parent may not be a leaf already)
• a remove not always removes a leaf and not always adds its parent to the set (it should be the last child to add the parent as a leaf).

The naming also better fits in the undo operations that were improved and better tested.

---

The new remove method has been immediately used to fix the remove_leaf_block backend method.
Before it was using revert and that has not the effect we actually need here.

[davxy]

@skunert applied your suggestion. Thank you.

@bkchr fixed another little bug in the remove_leaf_block.
On block removal the persisted children list was not updated.

This is the new fix

substrate/client/db/src/lib.rs

Lines 2230 to 2253 in 39a9f30

	let children: Vec<_> = self
	.blockchain()
	.children(hdr.parent)?
	.into_iter()
	.filter(|child_hash| child_hash != hash)
	.collect();
	let parent_leaf = if children.is_empty() {
	children::remove_children(
	&mut transaction,
	columns::META,
	meta_keys::CHILDREN_PREFIX,
	hdr.parent,
	);
	Some(hdr.parent)
	} else {
	children::write_children(
	&mut transaction,
	columns::META,
	meta_keys::CHILDREN_PREFIX,
	hdr.parent,
	children,
	);
	None
	};

[paritytech-cicd-pr]

The CI pipeline was cancelled due to failure one of the required jobs.
The job name - cargo-check-benches
The job logs - https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/1749976

[skunert]

I think this check is still broken, we now always insert the parent 🤔.
https://github.com/paritytech/substrate/pull/12005/files#diff-d631f4b5aa83ad9f2088c7d13236ad51b6a4ffdb870927f071532d9904bc2c5fR142

#[test]
fn removal_breaks() {
	let mut set = LeafSet::new();
	set.import(10_1u32, 10u32, 0u32);

	set.import(11_2, 11, 10_1);
	set.import(12_2, 12, 11_2);
	set.import(12_3, 12, 11_2);

	let outcome = set.remove(12_3, 12, Some(11_2)).unwrap();
	assert_eq!(outcome.removed.hash, 12_3);
	assert_eq!(outcome.inserted, None);
}

Here 12_3 is removed but sibling 12_2 still exists, so the parent 11_2 should not be inserted into the leave set. I think we need to check if any children of the parent node are still present.

[davxy]

I think this check is still broken, we now always insert the parent thinking. https://github.com/paritytech/substrate/pull/12005/files#diff-d631f4b5aa83ad9f2088c7d13236ad51b6a4ffdb870927f071532d9904bc2c5fR142

@skunert that test is expected to fail. See this (new) comment here:

substrate/client/api/src/leaves.rs

Lines 121 to 128 in 8dada46

	/// Update the leaf list on removal.
	///
	/// Note that the leaves set structure doesn't have the information to decide if the
	/// leaf we're removing is the last children of the parent. Follows that this method requires
	/// the caller to check this condition and optionally pass the `parent_hash` if `hash` is
	/// its last child.
	///
	/// Returns `None` if no modifications are applied.

Within the LeavesSet structure we don't have any information to infer if the removed leaf is the last child of a parent.
(at the moment is a fairly primitive low level "dumb" structure)
The caller should pass this information.

In your test case the caller should call:

    let outcome = set.remove(12_3, 12, None).unwrap();

Fortunately this function usage is not so widespread... remove has been just introduced by this PR. I'm the first user :-D

substrate/client/db/src/lib.rs

Line 2255 in 8dada46

let remove_outcome = leaves.remove(*hash, hdr.number, parent_leaf);

[skunert]

Ah sorry, totally missed this, makes sense now 👍.