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

Add Paras authorize_code_hash + apply_authorized_code feature #7592

Open
wants to merge 42 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 15 commits
Commits
Show all changes
42 commits
Select commit Hold shift + click to select a range
90fa441
Added `authorize_force_set_current_code_hash` feature
bkontur Feb 17, 2025
693c277
Add `force_set_current_code` test
bkontur Feb 17, 2025
1a9dba4
Update from bkontur running command 'fmt'
github-actions[bot] Feb 17, 2025
8dc2779
Merge remote-tracking branch 'origin/master' into bko-paras-authorize…
bkontur Feb 17, 2025
e14f331
Fix Rococo/Westend
bkontur Feb 17, 2025
a3d1f3c
Merge remote-tracking branch 'origin/master' into bko-paras-authorize…
bkontur Feb 17, 2025
9bb38f3
Merge remote-tracking branch 'origin/bko-paras-authorize-set-current-…
bkontur Feb 17, 2025
9eea06f
Update from bkontur running command 'prdoc --audience runtime_dev --b…
github-actions[bot] Feb 17, 2025
caff716
Update prdoc/pr_7592.prdoc
bkontur Feb 17, 2025
4d746d2
Update from bkontur running command 'bench --pallet polkadot_runtime_…
github-actions[bot] Feb 17, 2025
bde0c00
Apply suggestions from code review
bkontur Feb 18, 2025
6062032
Update polkadot/runtime/parachains/src/paras/mod.rs
bkontur Feb 18, 2025
3243e63
Dedicated enum variant `NotAuthorizedCode`
bkontur Feb 18, 2025
1aeb6b3
Merge branch 'master' into bko-paras-authorize-set-current-code
bkontur Feb 18, 2025
77463f4
Update from bkontur running command 'fmt'
github-actions[bot] Feb 18, 2025
a1225cb
Update prdoc/pr_7592.prdoc
bkontur Feb 18, 2025
a699970
Merge remote-tracking branch 'origin/master' into bko-paras-authorize…
bkontur Feb 21, 2025
ad37325
Align error codes with other pallets
bkontur Feb 21, 2025
d0c6e45
Merge branch 'master' into bko-paras-authorize-set-current-code
bkontur Feb 21, 2025
65ec72b
Merge branch 'master' into bko-paras-authorize-set-current-code
bkontur Feb 22, 2025
eaf9a78
Merge remote-tracking branch 'origin/master' into bko-paras-authorize…
bkontur Feb 26, 2025
f10b0a8
Merge remote-tracking branch 'origin/master' into bko-paras-authorize…
bkontur Feb 26, 2025
841f4e4
Refactor finished
bkontur Feb 27, 2025
560c6bb
Merge remote-tracking branch 'origin/master' into bko-paras-authorize…
bkontur Feb 27, 2025
83abc82
Update from bkontur running command 'fmt'
github-actions[bot] Feb 27, 2025
1eb117e
Update from bkontur running command 'bench --pallet polkadot_runtime_…
github-actions[bot] Feb 27, 2025
8b1a483
Update from bkontur running command 'prdoc --audience runtime_dev --b…
github-actions[bot] Feb 27, 2025
795d59a
Apply suggestions from code review
bkontur Feb 27, 2025
5bde2fd
Apply suggestions from code review
bkontur Mar 3, 2025
ad907ab
Merge branch 'master' into bko-paras-authorize-set-current-code
bkontur Mar 3, 2025
7c386ec
Add `validate_unsigned` validation for `apply_authorized_code`
bkontur Mar 3, 2025
033f908
Merge remote-tracking branch 'origin/master' into bko-paras-authorize…
bkontur Mar 3, 2025
f6981d0
Update from bkontur running command 'fmt'
github-actions[bot] Mar 3, 2025
ab3a823
Merge remote-tracking branch 'origin/master' into bko-paras-authorize…
bkontur Mar 4, 2025
d5800fc
Revert
bkontur Mar 4, 2025
15552cc
Revert and simplify
bkontur Mar 4, 2025
6e0cccb
Update from bkontur running command 'prdoc --audience runtime_dev --b…
github-actions[bot] Mar 4, 2025
3cdb61e
Update from bkontur running command 'fmt'
github-actions[bot] Mar 4, 2025
3157c46
Update prdoc/pr_7592.prdoc
bkontur Mar 4, 2025
9bd8c32
Update prdoc/pr_7592.prdoc
bkontur Mar 4, 2025
4ef9b78
ehm
bkontur Mar 4, 2025
b4fdadc
Update from bkontur running command 'bench --pallet polkadot_runtime_…
github-actions[bot] Mar 4, 2025
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
24 changes: 24 additions & 0 deletions polkadot/runtime/parachains/src/paras/benchmarking.rs
Original file line number Diff line number Diff line change
Expand Up @@ -249,6 +249,30 @@ mod benchmarks {
}
}

#[benchmark]
fn authorize_force_set_current_code_hash() {
let para_id = ParaId::from(1000);
let code_hash = [0; 32].into();

#[extrinsic_call]
_(RawOrigin::Root, para_id, code_hash);

assert_last_event::<T>(Event::CodeAuthorized { para_id, code_hash }.into());
}

#[benchmark]
fn apply_authorized_force_set_current_code(c: Linear<MIN_CODE_SIZE, MAX_CODE_SIZE>) {
let new_code = ValidationCode(vec![0; c as usize]);
let para_id = ParaId::from(c as u32);
AuthorizedCodeHash::<T>::insert(&para_id, new_code.hash());
generate_disordered_pruning::<T>();

#[extrinsic_call]
_(RawOrigin::Root, para_id, new_code);

assert_last_event::<T>(Event::CurrentCodeUpdated(para_id).into());
}

impl_benchmark_test_suite!(
Pallet,
crate::mock::new_test_ext(Default::default()),
Expand Down
94 changes: 89 additions & 5 deletions polkadot/runtime/parachains/src/paras/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,10 @@ use alloc::{collections::btree_set::BTreeSet, vec::Vec};
use bitvec::{order::Lsb0 as BitOrderLsb0, vec::BitVec};
use codec::{Decode, Encode};
use core::{cmp, mem};
use frame_support::{pallet_prelude::*, traits::EstimateNextSessionRotation, DefaultNoBound};
use frame_support::{
dispatch::PostDispatchInfo, pallet_prelude::*, traits::EstimateNextSessionRotation,
DefaultNoBound,
};
use frame_system::pallet_prelude::*;
use polkadot_primitives::{
ConsensusLog, HeadData, Id as ParaId, PvfCheckStatement, SessionIndex, UpgradeGoAhead,
Expand Down Expand Up @@ -551,6 +554,8 @@ pub trait WeightInfo {
fn include_pvf_check_statement_finalize_onboarding_accept() -> Weight;
fn include_pvf_check_statement_finalize_onboarding_reject() -> Weight;
fn include_pvf_check_statement() -> Weight;
fn authorize_force_set_current_code_hash() -> Weight;
fn apply_authorized_force_set_current_code(c: u32) -> Weight;
}

pub struct TestWeightInfo;
Expand Down Expand Up @@ -596,6 +601,12 @@ impl WeightInfo for TestWeightInfo {
// This special value is to distinguish from the finalizing variants above in tests.
Weight::MAX - Weight::from_parts(1, 1)
}
fn authorize_force_set_current_code_hash() -> Weight {
Weight::MAX
}
fn apply_authorized_force_set_current_code(_c: u32) -> Weight {
Weight::MAX
}
}

#[frame_support::pallet]
Expand Down Expand Up @@ -649,6 +660,8 @@ pub mod pallet {
pub enum Event {
/// Current code has been updated for a Para. `para_id`
CurrentCodeUpdated(ParaId),
/// New code hash has been authorized for a Para.
CodeAuthorized { code_hash: ValidationCodeHash, para_id: ParaId },
/// Current head has been updated for a Para. `para_id`
CurrentHeadUpdated(ParaId),
/// A code upgrade has been scheduled for a Para. `para_id`
Expand Down Expand Up @@ -696,6 +709,8 @@ pub mod pallet {
CannotUpgradeCode,
/// Invalid validation code size.
InvalidCode,
/// The given code is not authorized.
NotAuthorizedCode,
}

/// All currently active PVF pre-checking votes.
Expand Down Expand Up @@ -791,6 +806,11 @@ pub mod pallet {
#[pallet::storage]
pub type FutureCodeHash<T: Config> = StorageMap<_, Twox64Concat, ParaId, ValidationCodeHash>;

/// The code hash of a para which is authorized.
#[pallet::storage]
pub(super) type AuthorizedCodeHash<T: Config> =
StorageMap<_, Twox64Concat, ParaId, ValidationCodeHash>;

/// This is used by the relay-chain to communicate to a parachain a go-ahead with in the upgrade
/// procedure.
///
Expand Down Expand Up @@ -895,10 +915,7 @@ pub mod pallet {
new_code: ValidationCode,
) -> DispatchResult {
ensure_root(origin)?;
let new_code_hash = new_code.hash();
Self::increase_code_ref(&new_code_hash, &new_code);
Self::set_current_code(para, new_code_hash, frame_system::Pallet::<T>::block_number());
Self::deposit_event(Event::CurrentCodeUpdated(para));
Self::do_force_set_current_code_update(para, new_code);
Ok(())
}

Expand Down Expand Up @@ -1149,6 +1166,65 @@ pub mod pallet {
MostRecentContext::<T>::insert(&para, context);
Ok(())
}

/// Sets the storage for the authorized current code hash of the parachain.
///
/// This can be useful when we want to trigger `Paras::force_set_current_code(para, code)`
/// from a different chain than the one where the `Paras` pallet is deployed.
///
/// The main reason is to avoid transferring the entire `new_code` wasm blob between chains.
/// Instead, we authorize `new_code_hash` with `root`, which can later be applied by
/// `Paras::apply_authorized_force_set_current_code(para, new_code)` by anyone.
#[pallet::call_index(9)]
#[pallet::weight(<T as Config>::WeightInfo::authorize_force_set_current_code_hash())]
pub fn authorize_force_set_current_code_hash(
origin: OriginFor<T>,
para: ParaId,
new_code_hash: ValidationCodeHash,
) -> DispatchResult {
ensure_root(origin)?;

// insert authorized code hash.
AuthorizedCodeHash::<T>::insert(&para, new_code_hash);
Self::deposit_event(Event::CodeAuthorized { para_id: para, code_hash: new_code_hash });

Ok(())
}

/// Applies the already authorized current code for the parachain,
/// triggering the same functionality as `force_set_current_code`.
#[pallet::call_index(10)]
#[pallet::weight(<T as Config>::WeightInfo::apply_authorized_force_set_current_code(new_code.0.len() as u32))]
pub fn apply_authorized_force_set_current_code(
Copy link
Member

Choose a reason for hiding this comment

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

Could use the feeless_if attribute.

Copy link
Contributor Author

@bkontur bkontur Feb 18, 2025

Choose a reason for hiding this comment

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

Could use the feeless_if attribute.

I don't see much example of feeless_if, but iiuc it can do calculation just based on the inputs?
Or should I do something like:

#[pallet::feeless_if(|_: &OriginFor<T>, para: ParaId, validation_code: ValidationCode| -> bool {
            // check that the code was applied
            Self::current_code(para) == Some(validation_code.hash())
})]

But this way, we allow spamming the chain for free, because I cannot check Self::current_code(para) before and after.

Copy link
Member

Choose a reason for hiding this comment

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

because I cannot check Self::current_code(para) before and after.

What you mean by 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.

If I understand correctly, the purpose of using feeless_if here was to allow free execution when the code is actually applied, essentially the same effect as the PostDispatchInfo hack at the end (which should be removed when using feeless_if):

let post = PostDispatchInfo {
    // Consume the rest of the block to prevent further transactions
    actual_weight: Some(T::BlockWeights::get().max_block),
    // No fee for a valid upgrade
    pays_fee: Pays::No,
};
Ok(post)

However, in this case, the PostDispatchInfo hack is applied after validations, when do_force_set_current_code_update is actually executed.

Is this really enough? Is it ok to access storage and calculate hash again inside feeless_if?

#[pallet::feeless_if(|_: &OriginFor<T>, para: ParaId, validation_code: ValidationCode| -> bool {
            // check that the code was applied
            Self::current_code(para) == Some(validation_code.hash())
})]

because I cannot check Self::current_code(para) before and after.

Forget about it, I'm probably overthinking. My idea was to check the result of the extrinsic inside #[pallet::feeless_if], something like (but that's not possible):

let code_before = Self::current_code(para);
.. wait extrinsic to do the job
let code_after = Self::current_code(para);

if code_before ! code_after {
   PaysFee::No
} else {
   PaysFee::Yes
}

In other words, I’m not sure how to properly use feeless_if here. 😅

Copy link
Member

Choose a reason for hiding this comment

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

Is this really enough? Is it ok to access storage and calculate hash again inside feeless_if?

Yes that would be enough.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

IIUC, for feeless_if, we need to add the SkipCheckIfFeeless extension wrapper and configure the pallet accordingly:
https://github.com/paritytech/polkadot-sdk/blob/master/substrate/frame/transaction-payment/skip-feeless-payment/src/lib.rs#L16-L35

impl pallet_skip_feeless_payment::Config for Runtime {
    type RuntimeEvent = RuntimeEvent;
}

However, we don’t currently use SkipCheckIfFeeless anywhere in polkadot-sdk (except in kitchensink), polkadot-fellows, or other runtimes.


IIUC, all charge payment extensions below use type Val = Val<T>; and type Pre = Pre<T>;, but SkipCheckIfFeeless instead uses type Val = Intermediate<S::Val, ...> and type Pre = Intermediate<S::Pre, ...>

pallet_skip_feeless_payment::SkipCheckIfFeeless<Runtime, 
        pallet_transaction_payment::ChargeTransactionPayment<Runtime>,
>

pallet_skip_feeless_payment::SkipCheckIfFeeless<Runtime, 
        pallet_asset_conversion_tx_payment::ChargeAssetTxPayment<Runtime>,
>

pallet_skip_feeless_payment::SkipCheckIfFeeless<Runtime, 
        pallet_asset_tx_payment::ChargeAssetTxPayment<Runtime>,
>

IIUC, based on the kitchensink extras, which include SkipCheckIfFeeless, and the usage of get_eth_extension, it should be safe to add without breaking anything.


@gupnik @georgepisaltu @bkchr

  1. Could you help confirm whether it is safe to add the SkipCheckIfFeeless wrapper to existing (relay and/or parachain) runtimes? Specifically, would this change affect backwards compatibility for existing TransactionExtensions, or cause issues for clients submitting extrinsics (e.g., requiring them to change the format or anything else)?

  2. If it is safe, I can manage/handle adding SkipCheckIfFeeless to all runtimes. However, should/want we apply it to all runtimes, only relay chains, or just system parachains?

Copy link
Contributor

Choose a reason for hiding this comment

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

1. Could you help confirm whether it is safe to add the `SkipCheckIfFeeless` wrapper to existing (relay and/or parachain) runtimes? Specifically, would this change affect backwards compatibility for existing `TransactionExtensions`, or cause issues for clients submitting extrinsics (e.g., requiring them to change the format or anything else)?

The encoding/decoding of transaction is not changed when adding SkipCheckIfFeeless.
In the metadata SkipCheckIfFeeless will not appear and instead only the inner transaction identifier, type and implicit will show up.

2. If it is safe, I can manage/handle adding `SkipCheckIfFeeless` to all runtimes. However, should/want we apply it to all runtimes, only relay chains, or just system parachains?

I don't know audit status but otherwise I don't see anything that would break from this.

_origin: OriginFor<T>,
para: ParaId,
new_code: ValidationCode,
) -> DispatchResultWithPostInfo {
// no need to ensure, anybody can do this

// Ensure `new_code` is authorized
if let Some(authorized_code_hash) = AuthorizedCodeHash::<T>::take(&para) {
if new_code.hash() != authorized_code_hash {
return Err(Error::<T>::NotAuthorizedCode.into());
}
} else {
return Err(Error::<T>::NotAuthorizedCode.into());
}

// TODO: FAIL-CI - more validations?

// set current code
Self::do_force_set_current_code_update(para, new_code);

// if ok, then allows "for free"
let post = PostDispatchInfo {
// consume the rest of the block to prevent further transactions
actual_weight: Some(T::BlockWeights::get().max_block),
// no fee for valid upgrade
pays_fee: Pays::No,
};
Ok(post)
}
}

#[pallet::validate_unsigned]
Expand Down Expand Up @@ -2159,6 +2235,14 @@ impl<T: Config> Pallet<T> {
weight + T::DbWeight::get().writes(1)
}

/// Force set the current code for the given parachain.
fn do_force_set_current_code_update(para: ParaId, new_code: ValidationCode) {
let new_code_hash = new_code.hash();
Self::increase_code_ref(&new_code_hash, &new_code);
Self::set_current_code(para, new_code_hash, frame_system::Pallet::<T>::block_number());
Self::deposit_event(Event::CurrentCodeUpdated(para));
}

/// Returns the list of PVFs (aka validation code) that require casting a vote by a validator in
/// the active validator set.
pub(crate) fn pvfs_require_precheck() -> Vec<ValidationCodeHash> {
Expand Down
149 changes: 149 additions & 0 deletions polkadot/runtime/parachains/src/paras/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2013,3 +2013,152 @@ fn parachains_cache_preserves_order() {
assert_eq!(Parachains::<Test>::get(), vec![a, c]);
});
}

#[test]
fn authorize_and_apply_set_current_code_works() {
new_test_ext(MockGenesisConfig::default()).execute_with(|| {
let para_a = ParaId::from(111);
let code_1 = ValidationCode(vec![1]);
let code_2 = ValidationCode(vec![2]);
let code_3 = ValidationCode(vec![3]);
let code_1_hash = code_1.hash();
let code_2_hash = code_2.hash();
let code_3_hash = code_3.hash();
let root = RuntimeOrigin::root();
let non_root = RuntimeOrigin::signed(1);

// check before
assert!(AuthorizedCodeHash::<Test>::get(para_a).is_none());
assert!(CurrentCodeHash::<Test>::get(para_a).is_none());
check_code_is_not_stored(&code_1);
check_code_is_not_stored(&code_2);
check_code_is_not_stored(&code_3);

// cannot apply non-authorized code
assert_err!(
Paras::apply_authorized_force_set_current_code(
non_root.clone(),
para_a,
code_1.clone()
),
Error::<Test>::NotAuthorizedCode,
);

// non-root user cannot authorize
assert_err!(
Paras::authorize_force_set_current_code_hash(non_root.clone(), para_a, code_1_hash),
DispatchError::BadOrigin,
);
// root can authorize
assert_ok!(Paras::authorize_force_set_current_code_hash(root.clone(), para_a, code_1_hash));

// check authorized code hash stored
assert_eq!(AuthorizedCodeHash::<Test>::get(para_a), Some(code_1_hash));
assert!(CurrentCodeHash::<Test>::get(para_a).is_none());
check_code_is_not_stored(&code_1);
check_code_is_not_stored(&code_2);
check_code_is_not_stored(&code_3);

// non-root cannot apply unauthorized code
assert_err!(
Paras::apply_authorized_force_set_current_code(
non_root.clone(),
para_a,
code_2.clone()
),
Error::<Test>::NotAuthorizedCode,
);
assert_eq!(AuthorizedCodeHash::<Test>::get(para_a), Some(code_1_hash));
assert!(CurrentCodeHash::<Test>::get(para_a).is_none());
check_code_is_not_stored(&code_1);
check_code_is_not_stored(&code_2);
check_code_is_not_stored(&code_3);

// non-root can apply authorized code
assert_ok!(Paras::apply_authorized_force_set_current_code(
non_root.clone(),
para_a,
code_1.clone()
));

// check authorized code was applied
assert!(AuthorizedCodeHash::<Test>::get(para_a).is_none());
assert_eq!(CurrentCodeHash::<Test>::get(para_a), Some(code_1_hash));
check_code_is_stored(&code_1);
check_code_is_not_stored(&code_2);
check_code_is_not_stored(&code_3);

// authorize multiple without apply:
// authorize code_2_hash
assert_ok!(Paras::authorize_force_set_current_code_hash(root.clone(), para_a, code_2_hash));
assert_eq!(AuthorizedCodeHash::<Test>::get(para_a), Some(code_2_hash));
assert_eq!(CurrentCodeHash::<Test>::get(para_a), Some(code_1_hash));
check_code_is_stored(&code_1);
check_code_is_not_stored(&code_2);
check_code_is_not_stored(&code_3);
// authorize code_3_hash
assert_ok!(Paras::authorize_force_set_current_code_hash(root, para_a, code_3_hash));
assert_eq!(AuthorizedCodeHash::<Test>::get(para_a), Some(code_3_hash));
assert_eq!(CurrentCodeHash::<Test>::get(para_a), Some(code_1_hash));
check_code_is_stored(&code_1);
check_code_is_not_stored(&code_2);
check_code_is_not_stored(&code_3);

// cannot apply older ones
assert_err!(
Paras::apply_authorized_force_set_current_code(
non_root.clone(),
para_a,
code_1.clone()
),
Error::<Test>::NotAuthorizedCode,
);
assert_err!(
Paras::apply_authorized_force_set_current_code(
non_root.clone(),
para_a,
code_2.clone()
),
Error::<Test>::NotAuthorizedCode,
);

// apply just authorized
assert_ok!(Paras::apply_authorized_force_set_current_code(
non_root.clone(),
para_a,
code_3.clone()
));
assert!(AuthorizedCodeHash::<Test>::get(para_a).is_none());
assert_eq!(CurrentCodeHash::<Test>::get(para_a), Some(code_3_hash));
check_code_is_stored(&code_1);
check_code_is_not_stored(&code_2);
check_code_is_stored(&code_3);
})
}

#[test]
fn force_set_current_code_works() {
new_test_ext(MockGenesisConfig::default()).execute_with(|| {
let para_a = ParaId::from(111);
let code_1 = ValidationCode(vec![1]);
let code_1_hash = code_1.hash();
let root = RuntimeOrigin::root();
let non_root = RuntimeOrigin::signed(1);

// check before
assert!(CurrentCodeHash::<Test>::get(para_a).is_none());
check_code_is_not_stored(&code_1);

// non-root user cannot execute
assert_err!(
Paras::force_set_current_code(non_root, para_a, code_1.clone()),
DispatchError::BadOrigin,
);
// root can execute
assert_ok!(Paras::force_set_current_code(root, para_a, code_1.clone()));

// check after
assert_eq!(CurrentCodeHash::<Test>::get(para_a), Some(code_1_hash));
check_code_is_stored(&code_1);
})
}
Loading
Loading