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
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
77 changes: 3 additions & 74 deletions cumulus/pallets/parachain-system/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -53,9 +53,6 @@ use polkadot_runtime_parachains::FeeTracker;
use scale_info::TypeInfo;
use sp_runtime::{
traits::{Block as BlockT, BlockNumberProvider, Hash},
transaction_validity::{
InvalidTransaction, TransactionSource, TransactionValidity, ValidTransaction,
},
BoundedSlice, FixedU128, RuntimeDebug, Saturating,
};
use xcm::{latest::XcmHash, VersionedLocation, VersionedXcm};
Expand Down Expand Up @@ -193,7 +190,7 @@ pub mod ump_constants {
pub mod pallet {
use super::*;
use frame_support::pallet_prelude::*;
use frame_system::{pallet_prelude::*, WeightInfo as SystemWeightInfo};
use frame_system::pallet_prelude::*;

#[pallet::pallet]
#[pallet::storage_version(migration::STORAGE_VERSION)]
Expand Down Expand Up @@ -653,52 +650,8 @@ pub mod pallet {
Ok(())
}

/// Authorize an upgrade to a given `code_hash` for the runtime. The runtime can be supplied
/// later.
///
/// The `check_version` parameter sets a boolean flag for whether or not the runtime's spec
/// version and name should be verified on upgrade. Since the authorization only has a hash,
/// it cannot actually perform the verification.
///
/// This call requires Root origin.
#[pallet::call_index(2)]
#[pallet::weight(<T as frame_system::Config>::SystemWeightInfo::authorize_upgrade())]
#[allow(deprecated)]
#[deprecated(
note = "To be removed after June 2024. Migrate to `frame_system::authorize_upgrade`."
)]
pub fn authorize_upgrade(
origin: OriginFor<T>,
code_hash: T::Hash,
check_version: bool,
) -> DispatchResult {
ensure_root(origin)?;
frame_system::Pallet::<T>::do_authorize_upgrade(code_hash, check_version);
Ok(())
}

/// Provide the preimage (runtime binary) `code` for an upgrade that has been authorized.
///
/// If the authorization required a version check, this call will ensure the spec name
/// remains unchanged and that the spec version has increased.
///
/// Note that this function will not apply the new `code`, but only attempt to schedule the
/// upgrade with the Relay Chain.
///
/// All origins are allowed.
#[pallet::call_index(3)]
#[pallet::weight(<T as frame_system::Config>::SystemWeightInfo::apply_authorized_upgrade())]
#[allow(deprecated)]
#[deprecated(
note = "To be removed after June 2024. Migrate to `frame_system::apply_authorized_upgrade`."
)]
pub fn enact_authorized_upgrade(
_: OriginFor<T>,
code: Vec<u8>,
) -> DispatchResultWithPostInfo {
let post = frame_system::Pallet::<T>::do_apply_authorize_upgrade(code)?;
Ok(post)
}
// WARNING: call indices 2 and 3 were used in a former version of this pallet. Using them
// again will require to bump the transaction version of runtimes using this pallet.
}

#[pallet::event]
Expand Down Expand Up @@ -951,30 +904,6 @@ pub mod pallet {
sp_io::storage::set(b":c", &[]);
}
}

#[pallet::validate_unsigned]
impl<T: Config> sp_runtime::traits::ValidateUnsigned for Pallet<T> {
type Call = Call<T>;

fn validate_unsigned(_source: TransactionSource, call: &Self::Call) -> TransactionValidity {
if let Call::enact_authorized_upgrade { ref code } = call {
if let Ok(hash) = frame_system::Pallet::<T>::validate_authorized_upgrade(&code[..])
{
return Ok(ValidTransaction {
priority: 100,
requires: Vec::new(),
provides: vec![hash.as_ref().to_vec()],
longevity: TransactionLongevity::max_value(),
propagate: true,
})
}
}
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.

return Ok(Default::default())
}
Err(InvalidTransaction::Call.into())
}
}
}

impl<T: Config> Pallet<T> {
Expand Down
6 changes: 3 additions & 3 deletions cumulus/pallets/parachain-system/src/mock.rs
Original file line number Diff line number Diff line change
Expand Up @@ -49,9 +49,9 @@ type Block = frame_system::mocking::MockBlock<Test>;

frame_support::construct_runtime!(
pub enum Test {
System: frame_system::{Pallet, Call, Config<T>, Storage, Event<T>},
ParachainSystem: parachain_system::{Pallet, Call, Config<T>, Storage, Inherent, Event<T>, ValidateUnsigned},
MessageQueue: pallet_message_queue::{Pallet, Call, Storage, Event<T>},
System: frame_system,
ParachainSystem: parachain_system,
MessageQueue: pallet_message_queue,
}
);

Expand Down
6 changes: 2 additions & 4 deletions cumulus/pallets/parachain-system/src/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1127,10 +1127,8 @@ fn upgrade_version_checks_should_work() {
let new_code = vec![1, 2, 3, 4];
let new_code_hash = H256(sp_crypto_hashing::blake2_256(&new_code));

#[allow(deprecated)]
let _authorize = ParachainSystem::authorize_upgrade(RawOrigin::Root.into(), new_code_hash, true);
#[allow(deprecated)]
let res = ParachainSystem::enact_authorized_upgrade(RawOrigin::None.into(), new_code);
let _authorize = System::authorize_upgrade(RawOrigin::Root.into(), new_code_hash);
let res = System::apply_authorized_upgrade(RawOrigin::None.into(), new_code);

assert_eq!(expected.map_err(DispatchErrorWithPostInfo::from), res);
});
Expand Down
2 changes: 1 addition & 1 deletion cumulus/pallets/xcmp-queue/src/mock.rs
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ frame_support::construct_runtime!(
System: frame_system::{Pallet, Call, Config<T>, Storage, Event<T>},
Balances: pallet_balances::{Pallet, Call, Storage, Config<T>, Event<T>},
ParachainSystem: cumulus_pallet_parachain_system::{
Pallet, Call, Config<T>, Storage, Inherent, Event<T>, ValidateUnsigned,
Pallet, Call, Config<T>, Storage, Inherent, Event<T>,
},
XcmpQueue: xcmp_queue::{Pallet, Call, Storage, Event<T>},
}
Expand Down
16 changes: 16 additions & 0 deletions prdoc/pr_5439.prdoc
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
title: "Deprecated calls removed in cumulus parachain system pallet"

doc:
- audience: [Runtime Dev, Runtime User]
description: |
Call `authorize_upgrade` in parachain system pallet `cumulus-pallet-parachain-system` has
been removed, use `authorize_upgrade` or `authorize_upgrade_without_checks` calls in system
pallet `frame-system` instead.
Call `enact_authorized_upgrade` in parachain system pallet `cumulus-pallet-parachain-system`
has been removed, use `apply_authorized_upgrade` call in system pallet `frame-system` instead.

crates:
- name: cumulus-pallet-parachain-system
bump: major
- name: cumulus-pallet-xcmp-queue
bump: none
Loading