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 deprecated calls in cumulus-parachain-system #5439

Merged
merged 9 commits into from
Aug 27, 2024

Conversation

gui1117
Copy link
Contributor

@gui1117 gui1117 commented Aug 22, 2024

Calls were written to be removed after June 2024. This PR removes them.

This PR will break users using those calls. The call won't be decodable by the runtime, so it should fail early with no consequences. The functionality must be same as before, users will just need to use the calls in System.

@paritytech-cicd-pr
Copy link

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

@gui1117 gui1117 changed the title Remove deprecated calls in cumulus-parachain-system WIP: Remove deprecated calls in cumulus-parachain-system Aug 22, 2024
@gui1117 gui1117 changed the title WIP: Remove deprecated calls in cumulus-parachain-system Remove deprecated calls in cumulus-parachain-system Aug 22, 2024
Copy link
Member

@ggwpez ggwpez left a comment

Choose a reason for hiding this comment

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

🙏

PS: The semver check is buggy, but the prdoc looks good here.

@ggwpez ggwpez added the T2-pallets This PR/Issue is related to a particular pallet. label Aug 22, 2024
})
}
}
if let Call::set_validation_data { .. } = call {
Copy link
Contributor

Choose a reason for hiding this comment

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

We need this (=

Copy link
Contributor Author

Choose a reason for hiding this comment

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

set_validation_data is an inherent, inherent should not be validated by validate_unsigned, they only go through pre_dispatch.

The default implementation (by construct_runtime) is to consider all calls invalid in validate_unsigned and all calls valid in pre_dispatch. So the default implementation is what we want here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Indeed the behavior in construct runtime is allow calls in pre_dispatch and automatically reject any calls that don't have ValidateUnsigned::validate_unsigned impl.

On a different note, inherents are never "validated" as transactions and never enter the transaction pool, they are just included in the block and applied along with other regular transactions. Specifically when inherents are applied, the Applyable impl of UncheckedExtrinsic calls on ValidateUnsigned::pre_dispatch. ValidateUnsigned::validate_unsigned is called only when validating a checked extrinsic in frame_executive, which is called when a transaction tries to enter the transaction pool, which should never happen for inherents.

set_validation_data must be called exactly once per block, as an inherent, at the beginning of the block. This means it was never correct to have a ValidateUnsigned validation for it. It should never be allowed as an unsigned transaction, only as an inherent.

Also, for peace of mind, all of the tests, including zombienet tests, are passing with the changes in this PR. The chain doesn't work without calls to set_validation_data every block, so the fact that the tests pass reinforces the reasoning.

Copy link
Contributor

@lexnv lexnv left a comment

Choose a reason for hiding this comment

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

Thanks for contributing once again @gui1117 🙏

@gui1117
Copy link
Contributor Author

gui1117 commented Aug 27, 2024

CI is good, non-mandatory gitlab-subsystem-benchmark-approval-voting is failing, but it must not be related to the PR.

@ggwpez ggwpez added this pull request to the merge queue Aug 27, 2024
Merged via the queue into paritytech:master with commit f0323d5 Aug 27, 2024
188 of 189 checks passed
ordian added a commit that referenced this pull request Aug 27, 2024
* master: (36 commits)
  Bump the ci_dependencies group across 1 directory with 2 updates (#5401)
  Remove deprecated calls in cumulus-parachain-system (#5439)
  Make the PR template a default for new PRs (#5462)
  Only log the propagating transactions when they are not empty (#5424)
  [CI] Fix SemVer check base commit (#5361)
  Sync status refactoring (#5450)
  Add build options to the srtool build step (#4956)
  `MaybeConsideration` extension trait for `Consideration` (#5384)
  Skip slot before creating inherent data providers during major sync (#5344)
  Add symlinks for code of conduct and contribution guidelines (#5447)
  pallet-collator-selection: correctly register weight in `new_session` (#5430)
  Derive `Clone` on `EncodableOpaqueLeaf` (#5442)
  Moving `Find FAIL-CI` check to GHA (#5377)
  Remove panic, as proof is invalid. (#5427)
  Reactive syncing metrics (#5410)
  [bridges] Prune messages from confirmation tx body, not from the on_idle (#5006)
  Change the chain to Rococo in the parachain template Zombienet config (#5279)
  Improve the appearance of crates on `crates.io` (#5243)
  Add initial version of `pallet_revive` (#5293)
  Update OpenZeppelin template documentation (#5398)
  ...
@gui1117 gui1117 deleted the gui-parachain-system-remove-calls branch August 27, 2024 11:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T2-pallets This PR/Issue is related to a particular pallet.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants