Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Remove candidate selection #3148

Merged

Conversation

Lldenaurois
Copy link
Contributor

@Lldenaurois Lldenaurois commented May 31, 2021

This diff removes the candidate selection subsystem and implements the same RunJob based logic as in the candidate selection subsystem via a FuturesUnordered construction. Therefore, collations are submitted directly to the validator side of the protocol. The validator side then awaits collations on a separate futures stream and subsequently backs the first candidate it receives for each relay parent.

In the future, we plan to expand on the candidate selection algorithm.

@Lldenaurois Lldenaurois added A3-in_progress Pull request is in progress. No review needed at this stage. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. labels May 31, 2021
@Lldenaurois Lldenaurois force-pushed the remove-candidate-selection branch 2 times, most recently from cb5138d to 5fb63d2 Compare May 31, 2021 20:43
Several approaches have been discussed, but all have some issues:

- The current approach is very straightforward. However, that protocol is vulnerable to a single collator which, as an attack or simply through chance, gets its block candidate to the node more often than its fair share of the time.
- It may be possible to do some BABE-like selection algorithm to choose an "Official" collator for the round, but that is tricky because the collator which produces the PoV does not necessarily actually produce the block.
Copy link
Contributor

Choose a reason for hiding this comment

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

We already have a de facto selection algorithm if collators produce blocks via Aura, BABE, or in future Sassafras, not sure why anyone else could submit the block for them in those cases.

- The current approach is very straightforward. However, that protocol is vulnerable to a single collator which, as an attack or simply through chance, gets its block candidate to the node more often than its fair share of the time.
- It may be possible to do some BABE-like selection algorithm to choose an "Official" collator for the round, but that is tricky because the collator which produces the PoV does not necessarily actually produce the block.
- We could use relay-chain BABE randomness to generate some delay `D` on the order of 1 second, +- 1 second. The collator would then second the first valid parablock which arrives after `D`, or in case none has arrived by `2*D`, the last valid parablock which has arrived. This makes it very hard for a collator to game the system to always get its block nominated, but it reduces the maximum throughput of the system by introducing delay into an already tight schedule.
- A variation of that scheme would be to randomly choose a number `I`, and have a fixed acceptance window `D` for parablock candidates. At the end of the period `D`, count `C`: the number of parablock candidates received. Second the one with index `I % C`. Its drawback is the same: it must wait the full `D` period before seconding any of its received candidates, reducing throughput.
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd worry about us building DoS attacks against ourselves, ala #2203 Races sound dangerous.

We've one grant applicant with storage ideas similar to my mixnet incentives design. Initially, they wanted their proof-of-useful-storage to actually produce blocks, I suggested they use Aura, etc. but run a sampling procedure that rewards storage separately, which seems easier to control.

@Lldenaurois Lldenaurois force-pushed the remove-candidate-selection branch 3 times, most recently from 6e76d3a to cff297a Compare May 31, 2021 23:46
@Lldenaurois Lldenaurois force-pushed the remove-candidate-selection branch from cff297a to 855ab51 Compare June 1, 2021 00:15
@Lldenaurois Lldenaurois marked this pull request as ready for review June 1, 2021 00:59
Copy link
Member

@ordian ordian left a comment

Choose a reason for hiding this comment

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

The code update looks mostly correct, the validator_side loop rewrite with select! macro is appreciated.

The implementer's guide needs an update and also the comments from Jeff to be resolved.

Copy link
Member

@ordian ordian left a comment

Choose a reason for hiding this comment

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

LGTM

@ordian ordian added A0-please_review Pull request needs code review. and removed A3-in_progress Pull request is in progress. No review needed at this stage. labels Jun 8, 2021
@Lldenaurois Lldenaurois merged commit a4dfdf1 into paritytech:master Jun 8, 2021

notify_candidate_selection(ctx, collator_id, relay_parent, para_id).await;
let future = async move {
((id, pending_collation), rx.timeout(COLLATION_FETCH_TIMEOUT).await)
Copy link
Member

Choose a reason for hiding this comment

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

We already have a timeout in the request-response system. That is 1 second currently. So, we don't really need this?

Copy link
Member

Choose a reason for hiding this comment

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

Ah, you're right

request_timeout: POV_REQUEST_TIMEOUT_CONNECTED,

Yes, it can be removed.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants