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

Fix (backing) relying on para ids being unique #3144

Closed
Tracked by #1829
eskimor opened this issue Jan 30, 2024 · 4 comments · Fixed by #3231
Closed
Tracked by #1829

Fix (backing) relying on para ids being unique #3144

eskimor opened this issue Jan 30, 2024 · 4 comments · Fixed by #3231
Assignees

Comments

@eskimor
Copy link
Member

eskimor commented Jan 30, 2024

Here we assign a ParaId to a validator group as if that were a unique mapping. This needs to be fixed. Any other occurrences of this assumption need to be fixed as well. Goal should be that a parachain being scheduled on multiple cores does not brick it.

Without full elastic scaling implemented only one core will be successfully occupied in the end and all other potentially produced blocks won't make it (wasted effort), but the parachain should still be able to get some blocks included.

@eskimor eskimor changed the title Fix backing relying on para ids being unique Fix (backing) relying on para ids being unique Jan 30, 2024
@sandreim sandreim self-assigned this Jan 31, 2024
@sandreim sandreim moved this from Backlog to In Progress in parachains team board Jan 31, 2024
@sandreim
Copy link
Contributor

sandreim commented Feb 1, 2024

Both node side and runtime make assumptions around a 1:1 mapping between parachain and backing group.

Problem

On the node side the statement-table crate implements a generic Table which is designed with the said assumption in mind. The table is used to manage own and received statements of different candidates under a given relay chain block. To instantiate the table we need to provide all the backing groups, and in the case of multiple cores assigned to the same para, only the last core group would be retained. The statement table will fail to import statements and generate BackedCandidates. Statements for a given candidate will be checked based on the backing group info - number of required votes, validator indices and authority ids. One can easily see how this fails if candidate is build on a core for which we don't have the backing group info.

On the runtime side things are even worse, we cannot reason about the CoreIndex, hence know the backing group when processing a backed candidate . So, even if only one core is occupied by a para with multiple cores assigned we wouldn't know the backing group, unless we test indices across all of them which raises a security issue as there is no way to ensure backers attest only their own backed candidate and not also other candidates on other cores.

Proper Solution

The solution I recommend is to introduce the CoreIndex in the CandidateDescriptor (or maybe in the outputs of the candidate) which will then allow us to improve the statement table to support multiple cores assigned to same. This would also allow to make the necessary changes in runtime inherent processing as we'll have the precise backing group information.

No easy hack available

I've been considering to get away without CoreIndex, but reality has hit pretty hard. While we can deal with candidates that are backed by the local node (then we know the core_index) we cannot deal with incoming statements unless we change the StatementDistribution protocol such that CoreIndex is specified with the statement. Then we can easily check if the ValidatorIndex is in the backing group assigned to CoreIndex. However this won't work with the runtime where the CoreIndex is not available.

That being said, I'd want to hear some thoughts on this. I'd argue that we should craft an RFC to for an extended CandidateReceipt, CandidateDescriptor and dependent structs as well as future proof it for further additions. We could support both the old and new formats to not break parachain teams, but we'll still have the pain to upgrade the StatementDistribution network protocol.

@sandreim
Copy link
Contributor

sandreim commented Feb 1, 2024

Oh, or maybe there is a way to always be able to know for sure the core, based on validator index in the statement. We can always search all the backing groups and then we'd know the core. Would be a bit slower to process these, but will work in runtime and client.

@eskimor
Copy link
Member Author

eskimor commented Feb 1, 2024

As discussed, for sure the proper fix is to have the CoreIndex as part of the candidate commitments, which we plan to launch this year. In the meantime accepting backing groups to attest to multiple candidates seems acceptable, as it only affects paras that actually make use of elastic scaling and the threat is only reward stealing and only easily achievable if the backers are also block producers: Otherwise the legitimate backers should usually win on a first come first serve basis.

Having this interim solution would not only benefit parachains needing this soon, but also allows us to parallelize work more: We could already have an elastic scaling solution that works and can fix issues/improve performance, while we are still rolling out/developing the candidate commitment solution.

@eskimor
Copy link
Member Author

eskimor commented Feb 1, 2024

Actually. Not even reward stealing should be possible.
I think it should be possible that this is totally fine: We just need to ensure that a backing group can not testify on more than one core per block.

github-merge-queue bot pushed a commit that referenced this issue Feb 22, 2024
…3229)

First step in implementing
#3144

### Summary of changes
- switch statement `Table` candidate mapping from `ParaId` to
`CoreIndex`
- introduce experimental `InjectCoreIndex`  node feature.
- determine and assume a `CoreIndex` for a candidate based on statement
validator index. If the signature is valid it means validator controls
the validator that index and we can easily map it to a validator
group/core.
- introduce a temporary provisioner fix until we fully enable elastic
scaling in the subystem. The fix ensures we don't fetch the same
backable candidate when calling `get_backable_candidate` for each core.

TODO:
- [x] fix backing tests
- [x] fix statement table tests
- [x] add new test

---------

Signed-off-by: Andrei Sandu <[email protected]>
Signed-off-by: alindima <[email protected]>
Co-authored-by: alindima <[email protected]>
fellowship-merge-bot bot pushed a commit to polkadot-fellows/runtimes that referenced this issue Feb 22, 2024
<!-- Remember that you can run `/merge` to enable auto-merge in the PR
-->

<!-- Remember to modify the changelog. If you don't need to modify it,
you can check the following box.
Instead, if you have already modified it, simply delete the following
line. -->

Will be needed for enabling the fix for
paritytech/polkadot-sdk#3144 (unbricking
paraids which acquire multiple cores via coretime). Will also be needed
in the future for enabling other features, like
paritytech/polkadot-sdk#1644 and
paritytech/polkadot-sdk#628

---------

Signed-off-by: alindima <[email protected]>
Co-authored-by: Bastian Köcher <[email protected]>
github-merge-queue bot pushed a commit that referenced this issue Feb 23, 2024
…rent cores (#3231)

Fixes #3144

Builds on top of #3229

### Summary
Some preparations for Runtime to support elastic scaling, guarded by
config node features bit `FeatureIndex::ElasticScalingMVP`. This PR
introduces a per-candidate `CoreIndex` but does it in a hacky way to
avoid changing `CandidateCommitments`, `CandidateReceipts` primitives
and networking protocols.

#### Including `CoreIndex` in `BackedCandidate`
If the `ElasticScalingMVP` feature bit is enabled then
`BackedCandidate::validator_indices` is extended by 8 bits.
The value stored in these bits represents the assumed core index for the
candidate.

It is temporary solution which works by creating a mapping from
`BackedCandidate` to `CoreIndex` by assuming the `CoreIndex` can be
discovered by checking in which validator group the validator that
signed the statement is.

TODO:
- [x] fix tests
- [x] add new tests
- [x] Bump runtime API for Kusama, so we have that node features thing!
-> polkadot-fellows/runtimes#194

---------

Signed-off-by: Andrei Sandu <[email protected]>
Signed-off-by: alindima <[email protected]>
Co-authored-by: alindima <[email protected]>
@github-project-automation github-project-automation bot moved this from In Progress to Completed in parachains team board Feb 23, 2024
eskimor pushed a commit that referenced this issue Feb 23, 2024
…3229)

First step in implementing
#3144

- switch statement `Table` candidate mapping from `ParaId` to
`CoreIndex`
- introduce experimental `InjectCoreIndex`  node feature.
- determine and assume a `CoreIndex` for a candidate based on statement
validator index. If the signature is valid it means validator controls
the validator that index and we can easily map it to a validator
group/core.
- introduce a temporary provisioner fix until we fully enable elastic
scaling in the subystem. The fix ensures we don't fetch the same
backable candidate when calling `get_backable_candidate` for each core.

TODO:
- [x] fix backing tests
- [x] fix statement table tests
- [x] add new test

---------

Signed-off-by: Andrei Sandu <[email protected]>
Signed-off-by: alindima <[email protected]>
Co-authored-by: alindima <[email protected]>
eskimor pushed a commit that referenced this issue Feb 23, 2024
…rent cores (#3231)

Fixes #3144

Builds on top of #3229

Some preparations for Runtime to support elastic scaling, guarded by
config node features bit `FeatureIndex::ElasticScalingMVP`. This PR
introduces a per-candidate `CoreIndex` but does it in a hacky way to
avoid changing `CandidateCommitments`, `CandidateReceipts` primitives
and networking protocols.

If the `ElasticScalingMVP` feature bit is enabled then
`BackedCandidate::validator_indices` is extended by 8 bits.
The value stored in these bits represents the assumed core index for the
candidate.

It is temporary solution which works by creating a mapping from
`BackedCandidate` to `CoreIndex` by assuming the `CoreIndex` can be
discovered by checking in which validator group the validator that
signed the statement is.

TODO:
- [x] fix tests
- [x] add new tests
- [x] Bump runtime API for Kusama, so we have that node features thing!
-> polkadot-fellows/runtimes#194

---------

Signed-off-by: Andrei Sandu <[email protected]>
Signed-off-by: alindima <[email protected]>
Co-authored-by: alindima <[email protected]>
eskimor pushed a commit that referenced this issue Feb 26, 2024
…3229)

First step in implementing
#3144

### Summary of changes
- switch statement `Table` candidate mapping from `ParaId` to
`CoreIndex`
- introduce experimental `InjectCoreIndex`  node feature.
- determine and assume a `CoreIndex` for a candidate based on statement
validator index. If the signature is valid it means validator controls
the validator that index and we can easily map it to a validator
group/core.
- introduce a temporary provisioner fix until we fully enable elastic
scaling in the subystem. The fix ensures we don't fetch the same
backable candidate when calling `get_backable_candidate` for each core.

TODO:
- [x] fix backing tests
- [x] fix statement table tests
- [x] add new test

---------

Signed-off-by: Andrei Sandu <[email protected]>
Signed-off-by: alindima <[email protected]>
Co-authored-by: alindima <[email protected]>
eskimor pushed a commit that referenced this issue Feb 26, 2024
…rent cores (#3231)

Fixes #3144

Builds on top of #3229

### Summary
Some preparations for Runtime to support elastic scaling, guarded by
config node features bit `FeatureIndex::ElasticScalingMVP`. This PR
introduces a per-candidate `CoreIndex` but does it in a hacky way to
avoid changing `CandidateCommitments`, `CandidateReceipts` primitives
and networking protocols.

#### Including `CoreIndex` in `BackedCandidate`
If the `ElasticScalingMVP` feature bit is enabled then
`BackedCandidate::validator_indices` is extended by 8 bits.
The value stored in these bits represents the assumed core index for the
candidate.

It is temporary solution which works by creating a mapping from
`BackedCandidate` to `CoreIndex` by assuming the `CoreIndex` can be
discovered by checking in which validator group the validator that
signed the statement is.

TODO:
- [x] fix tests
- [x] add new tests
- [x] Bump runtime API for Kusama, so we have that node features thing!
-> polkadot-fellows/runtimes#194

---------

Signed-off-by: Andrei Sandu <[email protected]>
Signed-off-by: alindima <[email protected]>
Co-authored-by: alindima <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Completed
2 participants