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.

Sign up for GitHub

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

Jump to bottom

CandidateValidationMessage::ValidateFromChainState isn't used in production #5643

Closed
AndreiEres opened this issue Sep 9, 2024 · 4 comments · Fixed by #5707
Closed

CandidateValidationMessage::ValidateFromChainState isn't used in production #5643

AndreiEres opened this issue Sep 9, 2024 · 4 comments · Fixed by #5707
Assignees
@AndreiEres

Comments

@AndreiEres
Copy link
Contributor

We use it in examples and malus. If we cut it, it can simplify the logic.
cc @s0me0ne-unkn0wn
@s0me0ne-unkn0wn
Copy link
Contributor

CC @ordian @pepyakin

I don't see any obstacles to removing it, but neither do I remember the reason for introducing it in the first place, so it would be good to have more eyes on it. I wonder when it was removed? Most probably it was used in disputes and maybe approvals. @alexggh @alindima do you remember removing it from there?

@alexggh
Copy link
Contributor

alexggh commented Sep 9, 2024

@alexggh @alindima do you remember removing it from there

Nope, but I always thought it was added for malus.

@ordian
Copy link
Member

ordian commented Sep 9, 2024

It seems to be removed in the initial async backing impl, in particular paritytech/polkadot#5557. I don't recall the reason for it though, but it seems like it's not really needed anymore for production.

@sandreim
Copy link
Contributor

The way I understand it:

  • ValidateFromChainState was to be used by backing
  • ValidateFromExhaustive was to be used by approval checking

The only difference is that ValidateFromChainState also performs the runtime acceptance criteria checks, so it feels more like an optimization, but is redundant because in chain also verifies this so create inherent will filter any candidates that don't pass the criteria checks anyway.

I think it should be ok to remove it.

@AndreiEres AndreiEres self-assigned this Sep 13, 2024
@AndreiEres AndreiEres mentioned this issue Sep 13, 2024
Merged
github-merge-queue bot pushed a commit that referenced this issue Oct 1, 2024
@AndreiEres
Remove ValidateFromChainState (#5707)
1617852 
# Description

This PR removes the
`CandidateValidationMessage::ValidateFromChainState`, which was
previously used by backing, but is no longer relevant since initial
async backing implementation
paritytech/polkadot#5557.

Fixes #5643

## Integration

This change should not affect downstream projects since
`ValidateFromChainState` was already unused.

## Review Notes

- Removed all occurrences of `ValidateFromChainState`.
- Moved utility functions, previously used in candidate validation tests
and malus, exclusively to candidate validation tests as they are no
longer used in malus.
- Deleted the
`polkadot_parachain_candidate_validation_validate_from_chain_state`
metric from Prometheus.
- Removed `Spawner` from `ReplaceValidationResult` in malus’
interceptors.
- `fake_validation_error` was only used for `ValidateFromChainState`
handling, while other cases directly used
`InvalidCandidate::InvalidOutputs`. It has been replaced with
`fake_validation_error`, with a fallback to
`InvalidCandidate::InvalidOutputs`.
- Updated overseer’s minimal example to replace `ValidateFromChainState`
with `ValidateFromExhaustive`.
paritytech-ci pushed a commit that referenced this issue Oct 1, 2024
@AndreiEres
Remove ValidateFromChainState (#5707)
a3ec838 
# Description

This PR removes the
`CandidateValidationMessage::ValidateFromChainState`, which was
previously used by backing, but is no longer relevant since initial
async backing implementation
paritytech/polkadot#5557.

Fixes #5643

## Integration

This change should not affect downstream projects since
`ValidateFromChainState` was already unused.

## Review Notes

- Removed all occurrences of `ValidateFromChainState`.
- Moved utility functions, previously used in candidate validation tests
and malus, exclusively to candidate validation tests as they are no
longer used in malus.
- Deleted the
`polkadot_parachain_candidate_validation_validate_from_chain_state`
metric from Prometheus.
- Removed `Spawner` from `ReplaceValidationResult` in malus’
interceptors.
- `fake_validation_error` was only used for `ValidateFromChainState`
handling, while other cases directly used
`InvalidCandidate::InvalidOutputs`. It has been replaced with
`fake_validation_error`, with a fallback to
`InvalidCandidate::InvalidOutputs`.
- Updated overseer’s minimal example to replace `ValidateFromChainState`
with `ValidateFromExhaustive`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@AndreiEres AndreiEres

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Remove ValidateFromChainState paritytech/polkadot-sdk
5 participants
@ordian @AndreiEres @s0me0ne-unkn0wn @alexggh @sandreim