-
Notifications
You must be signed in to change notification settings - Fork 856
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
bkontur
wants to merge
42
commits into
master
Choose a base branch
from
bko-paras-authorize-set-current-code
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
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 693c277
Add `force_set_current_code` test
bkontur 1a9dba4
Update from bkontur running command 'fmt'
github-actions[bot] 8dc2779
Merge remote-tracking branch 'origin/master' into bko-paras-authorize…
bkontur e14f331
Fix Rococo/Westend
bkontur a3d1f3c
Merge remote-tracking branch 'origin/master' into bko-paras-authorize…
bkontur 9bb38f3
Merge remote-tracking branch 'origin/bko-paras-authorize-set-current-…
bkontur 9eea06f
Update from bkontur running command 'prdoc --audience runtime_dev --b…
github-actions[bot] caff716
Update prdoc/pr_7592.prdoc
bkontur 4d746d2
Update from bkontur running command 'bench --pallet polkadot_runtime_…
github-actions[bot] bde0c00
Apply suggestions from code review
bkontur 6062032
Update polkadot/runtime/parachains/src/paras/mod.rs
bkontur 3243e63
Dedicated enum variant `NotAuthorizedCode`
bkontur 1aeb6b3
Merge branch 'master' into bko-paras-authorize-set-current-code
bkontur 77463f4
Update from bkontur running command 'fmt'
github-actions[bot] a1225cb
Update prdoc/pr_7592.prdoc
bkontur a699970
Merge remote-tracking branch 'origin/master' into bko-paras-authorize…
bkontur ad37325
Align error codes with other pallets
bkontur d0c6e45
Merge branch 'master' into bko-paras-authorize-set-current-code
bkontur 65ec72b
Merge branch 'master' into bko-paras-authorize-set-current-code
bkontur eaf9a78
Merge remote-tracking branch 'origin/master' into bko-paras-authorize…
bkontur f10b0a8
Merge remote-tracking branch 'origin/master' into bko-paras-authorize…
bkontur 841f4e4
Refactor finished
bkontur 560c6bb
Merge remote-tracking branch 'origin/master' into bko-paras-authorize…
bkontur 83abc82
Update from bkontur running command 'fmt'
github-actions[bot] 1eb117e
Update from bkontur running command 'bench --pallet polkadot_runtime_…
github-actions[bot] 8b1a483
Update from bkontur running command 'prdoc --audience runtime_dev --b…
github-actions[bot] 795d59a
Apply suggestions from code review
bkontur 5bde2fd
Apply suggestions from code review
bkontur ad907ab
Merge branch 'master' into bko-paras-authorize-set-current-code
bkontur 7c386ec
Add `validate_unsigned` validation for `apply_authorized_code`
bkontur 033f908
Merge remote-tracking branch 'origin/master' into bko-paras-authorize…
bkontur f6981d0
Update from bkontur running command 'fmt'
github-actions[bot] ab3a823
Merge remote-tracking branch 'origin/master' into bko-paras-authorize…
bkontur d5800fc
Revert
bkontur 15552cc
Revert and simplify
bkontur 6e0cccb
Update from bkontur running command 'prdoc --audience runtime_dev --b…
github-actions[bot] 3cdb61e
Update from bkontur running command 'fmt'
github-actions[bot] 3157c46
Update prdoc/pr_7592.prdoc
bkontur 9bd8c32
Update prdoc/pr_7592.prdoc
bkontur 4ef9b78
ehm
bkontur b4fdadc
Update from bkontur running command 'bench --pallet polkadot_runtime_…
github-actions[bot] File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
But this way, we allow spamming the chain for free, because I cannot check
Self::current_code(para)
before and after.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What you mean by this?
There was a problem hiding this comment.
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 thePostDispatchInfo
hack at the end (which should be removed when usingfeeless_if
):However, in this case, the
PostDispatchInfo
hack is applied after validations, whendo_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
?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):In other words, I’m not sure how to properly use
feeless_if
here. 😅There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes that would be enough.
There was a problem hiding this comment.
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 theSkipCheckIfFeeless
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
However, we don’t currently use
SkipCheckIfFeeless
anywhere inpolkadot-sdk
(except inkitchensink
),polkadot-fellows
, or other runtimes.IIUC, all charge payment extensions below use
type Val = Val<T>;
andtype Pre = Pre<T>;
, butSkipCheckIfFeeless
instead usestype Val = Intermediate<S::Val, ...>
andtype Pre = Intermediate<S::Pre, ...>
IIUC, based on the
kitchensink
extras, which includeSkipCheckIfFeeless
, and the usage ofget_eth_extension
, it should be safe to add without breaking anything.@gupnik @georgepisaltu @bkchr
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 existingTransactionExtensions
, or cause issues for clients submitting extrinsics (e.g., requiring them to change the format or anything else)?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?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.I don't know audit status but otherwise I don't see anything that would break from this.