Prospective Parachains: track candidate parent nodes in a fragment tree

[slumber]

Needed for #5054

We'll need to make use of GetHypotheticalDepths from #4913
...
We should only fetch & second collations that are either built on top of the root of some fragment tree or have a parent which is backed. We could relax this somewhat to fetch & second collations that are built on top of a Seconded candidate which we've validated locally, but that'd be more complex and the 'backed' rule should work OK although it's not as efficient.

Previous version of GetHypotheticalDepths doesn't give any info about parent node being seconded/backed. Now:

1. Fragment tree tracks candidate hash of a parent node for each depth
2. Prospective parachains looks up this candidate status in candidate storage (0 depth is special)

Also forward CandidateSeconded notifications to collator protocol

[slumber]

At this point it should be guaranteed by collator protocol that at least 1 depth corresponds to backed parent, and we do occupy the rest too. This seems to be correct

[slumber]

is_backed would return false for candidate not present in storage, but we do expect fragment trees and candidate storage to be updated correspondingly, thus this should never happen

[slumber]

@rphmeier while this PR makes it possible to implement the proposal from the collator protocol issue,
I would suggest to extract it into a separate task since it requires some discussion (and also think about spam protection here)

For now, to finish #5740 we could go with a temporary "accept anything with an non-empty depth in a fragment tree"
wdyt?

[rphmeier]

We should only fetch & second collations that are either built on top of the root of some fragment tree or have a parent which is backed.

I don't understand the logical step from this requirement to tracking candidate parent hashes of nodes in the fragment tree. That is, if we get a collation announcement from a peer, we should expect that there is no such node in the fragment tree (it can exist, but only if one of our group's other members has seconded the candidate and we've gotten it via backing).

What we actually need to check in order to enforce this condition is that there exists a node where hash(head_data) == advertisement.parent_head_data_hash and the node is backed. We could address this just by adding another request to the prospective parachains system which does this search.

[slumber]

We should only fetch & second collations that are either built on top of the root of some fragment tree or have a parent which is backed.

I don't understand the logical step from this requirement to tracking candidate parent hashes of nodes in the fragment tree. That is, if we get a collation announcement from a peer, we should expect that there is no such node in the fragment tree (it can exist, but only if one of our group's other members has seconded the candidate and we've gotten it via backing).

What we actually need to check in order to enforce this condition is that there exists a node where hash(head_data) == advertisement.parent_head_data_hash and the node is backed. We could address this just by adding another request to the prospective parachains system which does this search.

Tracking parents is indeed unnecessary
Candidate hashes are returned because we may want to keep advertisements in memory until we receive CandidateBacked event.
I think we should discuss it in #5923

[rphmeier]

This doesn't make sense, conceptually. Candidates don't refer to their parent by hash, therefore there can be multiple parents. If this is introduced, it should be a Vec, but I also don't think this makes sense to include in the first place.

[slumber]

It refers to some particular depth, not membership in general

[rphmeier]

Candidate hashes are returned because we may want to keep advertisements in memory until we receive CandidateBacked event.

There's already logic for pruning advertisements (quoting from #5054 ):

1. We'd use GetMinimumRelayParent on both sides to track which collations are acceptable in a view, so each active-leaf comes with an implied ancestry of a few relay-parents allowed for announcements.
2. We should allow collators to make a few (up to MAX_DEPTH?) announcements per relay-parent

Therefore, if we stick to what has already been described in the github issue description, we already limit the amount of incoming announcements based on the amount of collator peers we accept at any given time and we prune when those announcements leave the implicit view.

So this PR seems totally unnecessary.

[slumber]

1. We'd use GetMinimumRelayParent on both sides to track which collations are acceptable in a view, so each active-leaf comes with an implied ancestry of a few relay-parents allowed for announcements.
2. We should allow collators to make a few (up to MAX_DEPTH?) announcements per relay-parent

Therefore, if we stick to what has already been described in the github issue description, we already limit the amount of incoming announcements based on the amount of collator peers we accept at any given time and we prune when those announcements leave the implicit view.

So this PR seems totally unnecessary.

This PR targets a different issue, when a collator advertises a collation built on top seconded para head.
We would want to keep such advertisements in memory until it either goes out of view or parent head gets backed, only
then fetch it.

[rphmeier]

This PR targets a different issue, when a collator advertises a collation built on top seconded para head.
We would want to keep such advertisements in memory until it either goes out of view or parent head gets backed, only
then fetch it.

The code changes here don't relate to this issue as far as I can tell and is introducing complexity without a clear benefit. We already keep announcements in memory until they go out of view - we keep up to MAX_DEPTH announcements per collator per relay-parent, regardless of our opinion of the status of the parent head.

The way it should be implemented is to check whether GetHypotheticalDepths or some comparable request yields at least one parent which is backed. If not, we keep the announcement and listen for notifications of candidates being seen as backed. We can compare those notifications to any announcements we haven't yet acted upon and then fetch if necessary

I'm going to close this PR - happy to reopen if it helps with implementing the protocol as described in the issue.

[rphmeier]

I amended some language in #5054 which may have been the cause of this confusion:

previously:

We should allow collators to make a few (up to MAX_DEPTH?) announcements per relay-parent and we'll ignore ones that we don't recognize with GetHypotheticalDepths

changed to:

We should allow collators to make a few (up to MAX_DEPTH?) announcements per relay-parent and we'll keep but not act on announcements that we don't recognize with GetHypotheticalDepths

[slumber]

The code changes here don't relate to this issue as far as I can tell and is introducing complexity without a clear benefit. We already keep announcements in memory until they go out of view - we keep up to MAX_DEPTH announcements per collator per relay-parent, regardless of our opinion of the status of the parent head.

The way it should be implemented is to check whether GetHypotheticalDepths or some comparable request yields at least one parent which is backed. If not, we keep the announcement and listen for notifications of candidates being seen as backed. We can compare those notifications to any announcements we haven't yet acted upon and then fetch if necessary

GetHypotheticalDepths doesn't give any info about parent nodes being backed

The plan with this PR was that the response for this request not only gives us vector of possible depths, but also returns a status of parent nodes (again, for each depth)
Then, if there exists some backed parent, we fetch the collation right away
Otherwise, put each seconded candidate hash in a queue and wait for its backing

What's unnecessary in this PR is tracking, but the overall idea should match the one you described.

[rphmeier]

GetHypotheticalDepths doesn't give any info about parent nodes being backed

Yeah, it always either needed to change or to have some alternative request introduced which serves the same purpose. I mentioned that in a few places.

I'll reopen, but it'd need to be almost completely changed, because the current changes to GetHypotheticalDepths and the tree don't make any sense conceptually. I think we should alter it to something like this:

return depths: Vec<(usize, ParentHeadDataState)>, // or something more compact

enum ParentHeadDataState {
  // i.e. the head data hash is the 'current head' of the `BaseConstraints`: this is the case for _all_ depths when depth 0 is a valid hypothetical depth
  CommittedToRelayChain, 
  // A candidate committing to the parent's head-data has been backed.
  Backed,
  // A candidate committing to the parent's head-data has been seconded,
  Seconded,
}
// unknown state is not necessary, because this is used under hypothetical depths which have already passed a 'parent head known' filter.

The implementation of GetHypotheticalDepths needs to account for the possibility that there are multiple valid parents at a depth and that they have different statuses.

[slumber]

@rphmeier
What do you think about introducing a simpler endpoint:

pub enum ProspectiveParachainsMessage {
    // ...
    IsSecondingAllowed {
        para_id: ParaId,
        parent_head_data_hash: Hash,
        sender: oneshot::Sender<bool>,
    }
    // ...
}

suited specifically for collator protocol. It would traverse para's fragment trees for each active leaf
and look for node such that

• it's height less than MAX_CANDIDATE_DEPTH + 1
• it's head data hash matches the one from request
• the corresponding candidate is marked as Backed

If found, send true back or false otherwise.
Also, once a candidate gets backed, send the para head data hash and para id to collator protocol
to unblock advertisements.

Then we don't need to re-request hypothetical depth at new activations and can simply wait for
CandidateBacked events (or until candidate's relay parent goes out of view).

The thing is, collator protocol doesn't track which depths are vacant in the tree and probably shouldn't.

[rphmeier]

I would prefer a more general API which can be used by multiple subsystems.

This IsSecondingAllowed is not enough for the logic that the collator needs to do, which is to avoid seconding more than one candidate per depth per relay-parent. Candidate-backing currently defends against it but collator-protocol doesn't retry.

Both statement distribution and collator-protocol need to receive notifications about new backed and seconded candidates.

[slumber]

I would prefer a more general API which can be used by multiple subsystems.

Which subsystem other than collator protocol will look into parent node status?

This IsSecondingAllowed is not enough for the logic that the collator needs to do, which is to avoid seconding more than one candidate per depth per relay-parent. Candidate-backing currently defends against it but collator-protocol doesn't retry.

I was re-reading through conversation in this PR and assumed that was your intention here too:

• What we actually need to check in order to enforce this condition is that there exists a node where hash(head_data) == advertisement.parent_head_data_hash and the node is backed. We could address this just by adding another request to the prospective parachains system which does this search.

We would then track depths twice and I don't really like it.

Both statement distribution and collator-protocol need to receive notifications about new backed and seconded candidates.

Collator protocol would only care about those seconded locally. But anyway, this is out of scope of the issue.

[slumber]

Backing is actually capable of telling whether we can second a candidate. It just needs to track parent head data hash for each candidate in seconded_at_depth (easy) + be aware of para head committed to relay chain (comes from validity constraints so not easy)