Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove the prospective-parachains subsystem from collators #4471

Merged

Conversation

alindima
Copy link
Contributor

@alindima alindima commented May 15, 2024

Implements #4429

Collators only need to maintain the implicit view for the paraid they are collating on.
In this case, bypass prospective-parachains entirely. It's still useful to use the GetMinimumRelayParents message from prospective-parachains for validators, because the data is already present there.

This enables us to entirely remove the subsystem from collators, which consumed resources needlessly

Aims to resolve #4167

TODO:

  • fix unit tests

@alindima alindima added R0-silent Changes should not be mentioned in any release notes T8-polkadot This PR/Issue is related to/affects the Polkadot network. T9-cumulus This PR/Issue is related to cumulus. labels May 15, 2024
@alindima alindima linked an issue May 15, 2024 that may be closed by this pull request
rx.await.map_err(|_| FetchError::ProspectiveParachainsUnavailable)
}

// Request the min relay parent for a single para, directly using ChainApi.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seem to return the leaf_number - allowed_ancestry_len no matter what para_id. Don't we have to constrain this to be the minimum between this value and the best backable parachain block relay parent ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would complicate the code for collators, since they'll no longer have the prospective-parachains subsystem to track the backed candidates. Also, they don't receive backing statements. I guess we could use the persisted_validation_data runtime API which would return the relay chain block height of the last backed or included candidate.

I wouldn't bother with this for now, as it preserves the existing behaviour (which doesn't seem to cause issues) and the collator should anyway be smart enough to not produce collations that move the relay parents backwards.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the name of the function though is not really correct

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changed the function name

Copy link
Contributor

@sandreim sandreim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like we only use the min relay parent for sanity checking before advertising collations, I would propose to remove this check instead and rely on validators rejecting it and disconnecting the collator.

There are two situations when this would happen:

  • somebody writes a bad collator that authors on ancient relay parents which is a bug, and having a strong signal as getting disconnected/banned is better than relying on this sanity check as the effect is the same - parachain doesn't make progress at all
  • the collator is so far behind in syncing the relay chain such that this check would not actually fail at all, and it will still be the validator rejecting the collation

@skunert
Copy link
Contributor

skunert commented May 16, 2024

I reran the original experiments from #4167 and this PR indeed fixes the issue!

@alindima
Copy link
Contributor Author

It looks like we only use the min relay parent for sanity checking before advertising collations, I would propose to remove this check instead and rely on validators rejecting it and disconnecting the collator.

There are two situations when this would happen:

  • somebody writes a bad collator that authors on ancient relay parents which is a bug, and having a strong signal as getting disconnected/banned is better than relying on this sanity check as the effect is the same - parachain doesn't make progress at all
  • the collator is so far behind in syncing the relay chain such that this check would not actually fail at all, and it will still be the validator rejecting the collation

As discussed offline, we need to keep the implicit view on the collator side regardless of whether we query it before advertising. We still need to query the allowed ancestry so that collators are able to build and advertise collations that are built on older relay parents and for pruning these collations when the relay parents go out of scope.

So we could at most remove one line of code, the one that re-checks the ancestry before advertising. But I don't think it's worth it. It's a sanity check that'll save us from advertising outdated collations

@paritytech-cicd-pr
Copy link

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: test-linux-stable 3/3
Logs: https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/6245613

@alindima alindima removed the R0-silent Changes should not be mentioned in any release notes label May 20, 2024
@alindima alindima requested a review from sandreim May 20, 2024 14:53
@alindima alindima added this pull request to the merge queue May 21, 2024
Merged via the queue into master with commit 278486f May 21, 2024
154 of 156 checks passed
@alindima alindima deleted the alindima/remove-prospective-parachains-from-collators branch May 21, 2024 08:38
Ank4n pushed a commit that referenced this pull request May 21, 2024
Implements #4429

Collators only need to maintain the implicit view for the paraid they
are collating on.
In this case, bypass prospective-parachains entirely. It's still useful
to use the GetMinimumRelayParents message from prospective-parachains
for validators, because the data is already present there.

This enables us to entirely remove the subsystem from collators, which
consumed resources needlessly

Aims to resolve #4167 

TODO:
- [x] fix unit tests
hitchhooker pushed a commit to ibp-network/polkadot-sdk that referenced this pull request Jun 5, 2024
…h#4471)

Implements paritytech#4429

Collators only need to maintain the implicit view for the paraid they
are collating on.
In this case, bypass prospective-parachains entirely. It's still useful
to use the GetMinimumRelayParents message from prospective-parachains
for validators, because the data is already present there.

This enables us to entirely remove the subsystem from collators, which
consumed resources needlessly

Aims to resolve paritytech#4167 

TODO:
- [x] fix unit tests
TarekkMA pushed a commit to moonbeam-foundation/polkadot-sdk that referenced this pull request Aug 2, 2024
…h#4471)

Implements paritytech#4429

Collators only need to maintain the implicit view for the paraid they
are collating on.
In this case, bypass prospective-parachains entirely. It's still useful
to use the GetMinimumRelayParents message from prospective-parachains
for validators, because the data is already present there.

This enables us to entirely remove the subsystem from collators, which
consumed resources needlessly

Aims to resolve paritytech#4167 

TODO:
- [x] fix unit tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T8-polkadot This PR/Issue is related to/affects the Polkadot network. T9-cumulus This PR/Issue is related to cumulus.
Projects
None yet
5 participants