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 for (force_)set_current_code feature #7592

Open
wants to merge 20 commits into
base: master
Choose a base branch
from

Conversation

bkontur
Copy link
Contributor

@bkontur bkontur commented Feb 17, 2025

Closes: #7574
Relates to: #7591

This feature 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.

Open questions

  • Do we need something like poke_authorized_code_hash? E.g. in case that we authorize code hash, but nobody would apply it and the parachain starts working with old/other_new code? Is this possible?

@bkontur
Copy link
Contributor Author

bkontur commented Feb 17, 2025

/cmd fmt

@bkontur bkontur added T14-system_parachains This PR/Issue is related to system parachains. A4-needs-backport Pull request must be backported to all maintained releases. labels Feb 17, 2025
@bkontur
Copy link
Contributor Author

bkontur commented Feb 17, 2025

/cmd prdoc --audience runtime_dev --bump patch

@bkontur
Copy link
Contributor Author

bkontur commented Feb 17, 2025

/cmd bench --pallet polkadot_runtime_parachains::paras --runtime westend rococo

Copy link
Contributor

Command "bench --pallet polkadot_runtime_parachains::paras --runtime westend rococo" has started 🚀 See logs here

Copy link
Contributor

Command "bench --pallet polkadot_runtime_parachains::paras --runtime westend rococo" has finished ✅ See logs here

Subweight results:
File Extrinsic Old New Change [%]
cumulus/pallets/collator-selection/src/weights.rs leave_intent - - ERROR
cumulus/pallets/collator-selection/src/weights.rs new_session - - ERROR
cumulus/pallets/collator-selection/src/weights.rs register_as_candidate - - ERROR
cumulus/pallets/collator-selection/src/weights.rs set_invulnerables - - ERROR
cumulus/pallets/collator-selection/src/weights.rs take_candidate_slot - - ERROR
cumulus/pallets/collator-selection/src/weights.rs update_bond - - ERROR
cumulus/parachains/runtimes/assets/asset-hub-rococo/src/weights/pallet_collator_selection.rs take_candidate_slot - - ERROR
cumulus/parachains/runtimes/assets/asset-hub-rococo/src/weights/pallet_collator_selection.rs update_bond - - ERROR
cumulus/parachains/runtimes/assets/asset-hub-westend/src/weights/pallet_collator_selection.rs take_candidate_slot - - ERROR
cumulus/parachains/runtimes/assets/asset-hub-westend/src/weights/pallet_collator_selection.rs update_bond - - ERROR
cumulus/parachains/runtimes/bridge-hubs/bridge-hub-rococo/src/weights/pallet_collator_selection.rs take_candidate_slot - - ERROR
cumulus/parachains/runtimes/bridge-hubs/bridge-hub-rococo/src/weights/pallet_collator_selection.rs update_bond - - ERROR
cumulus/parachains/runtimes/bridge-hubs/bridge-hub-westend/src/weights/pallet_collator_selection.rs take_candidate_slot - - ERROR
cumulus/parachains/runtimes/bridge-hubs/bridge-hub-westend/src/weights/pallet_collator_selection.rs update_bond - - ERROR
cumulus/parachains/runtimes/collectives/collectives-westend/src/weights/pallet_collator_selection.rs take_candidate_slot - - ERROR
cumulus/parachains/runtimes/collectives/collectives-westend/src/weights/pallet_collator_selection.rs update_bond - - ERROR
cumulus/parachains/runtimes/collectives/collectives-westend/src/weights/pallet_preimage.rs ensure_updated - - ERROR
cumulus/parachains/runtimes/people/people-rococo/src/weights/pallet_collator_selection.rs take_candidate_slot - - ERROR
cumulus/parachains/runtimes/people/people-rococo/src/weights/pallet_collator_selection.rs update_bond - - ERROR
cumulus/parachains/runtimes/people/people-westend/src/weights/pallet_collator_selection.rs take_candidate_slot - - ERROR
cumulus/parachains/runtimes/people/people-westend/src/weights/pallet_collator_selection.rs update_bond - - ERROR
polkadot/runtime/rococo/src/weights/polkadot_runtime_parachains_paras.rs add_trusted_validation_code - - ERROR
polkadot/runtime/rococo/src/weights/polkadot_runtime_parachains_paras.rs force_note_new_head - - ERROR
polkadot/runtime/rococo/src/weights/polkadot_runtime_parachains_paras.rs force_schedule_code_upgrade - - ERROR
polkadot/runtime/rococo/src/weights/polkadot_runtime_parachains_paras.rs force_set_current_code - - ERROR
polkadot/runtime/rococo/src/weights/polkadot_runtime_parachains_paras.rs force_set_current_head - - ERROR
polkadot/runtime/westend/src/weights/pallet_preimage.rs ensure_updated - - ERROR
polkadot/runtime/westend/src/weights/polkadot_runtime_parachains_paras.rs add_trusted_validation_code - - ERROR
polkadot/runtime/westend/src/weights/polkadot_runtime_parachains_paras.rs force_note_new_head - - ERROR
polkadot/runtime/westend/src/weights/polkadot_runtime_parachains_paras.rs force_schedule_code_upgrade - - ERROR
polkadot/runtime/westend/src/weights/polkadot_runtime_parachains_paras.rs force_set_current_code - - ERROR
polkadot/runtime/westend/src/weights/polkadot_runtime_parachains_paras.rs force_set_current_head - - ERROR
substrate/frame/election-provider-support/src/weights.rs phragmen - - ERROR
substrate/frame/election-provider-support/src/weights.rs phragmms - - ERROR
polkadot/runtime/westend/src/weights/polkadot_runtime_parachains_paras.rs force_set_most_recent_context 110.16us 103.91us -5.67
polkadot/runtime/westend/src/weights/polkadot_runtime_parachains_paras.rs include_pvf_check_statement_finalize_onboarding_accept 1.21ms 1.03ms -14.87
polkadot/runtime/westend/src/weights/polkadot_runtime_parachains_paras.rs authorize_force_set_current_code_hash 137.18us Added
polkadot/runtime/westend/src/weights/polkadot_runtime_parachains_paras.rs apply_authorized_force_set_current_code 40.31ms Added
polkadot/runtime/rococo/src/weights/polkadot_runtime_parachains_paras.rs authorize_force_set_current_code_hash 137.30us Added
polkadot/runtime/rococo/src/weights/polkadot_runtime_parachains_paras.rs apply_authorized_force_set_current_code 38.43ms Added
Command output:

✅ Successful benchmarks of runtimes/pallets:
-- westend: ['polkadot_runtime_parachains::paras']
-- rococo: ['polkadot_runtime_parachains::paras']

Comment on lines 1195 to 1197
// TODO: FAIL-CI - more validations?
// do we need to check against `FutureCodeHash`, `CodeHashRef`,
// `PastCodeHash`,... code hashes?
Copy link
Member

Choose a reason for hiding this comment

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

I don't see a reason. force_set_code_hash will still do further checks and reject if the authorized code is incorrect.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, do_force_set_current_code_update or force_set_current_code does not do any validations.
So the only validation is here "if code is authorized".

I have the only doubt here:

Open questions

* [ ]  Do we need something like `poke_authorized_code_hash`? E.g. in case that we authorize code hash, but nobody would apply it and the parachain starts working with old/other_new code? Is this possible?

/// 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. 😅

bkontur and others added 2 commits February 18, 2025 10:27
Co-authored-by: Bastian Köcher <git@kchr.de>
Co-authored-by: Bastian Köcher <git@kchr.de>
@paritytech-workflow-stopper
Copy link

All GitHub workflows were cancelled due to failure one of the required jobs.
Failed workflow url: https://github.com/paritytech/polkadot-sdk/actions/runs/13387358936
Failed job name: fmt

@bkontur
Copy link
Contributor Author

bkontur commented Feb 18, 2025

/cmd fmt

@bkontur bkontur requested a review from bkchr February 18, 2025 10:05
@bkontur bkontur self-assigned this Feb 18, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A4-needs-backport Pull request must be backported to all maintained releases. T14-system_parachains This PR/Issue is related to system parachains.
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

paras pallet - add new extrinsincs for authorize/apply set_current_code
2 participants