-
Notifications
You must be signed in to change notification settings - Fork 646
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
FRAME: Reintroduce TransactionExtension
as a replacement for SignedExtension
#3685
base: master
Are you sure you want to change the base?
Conversation
Signed-off-by: georgepisaltu <[email protected]>
Signed-off-by: georgepisaltu <[email protected]>
Signed-off-by: georgepisaltu <[email protected]>
Signed-off-by: georgepisaltu <[email protected]>
Signed-off-by: georgepisaltu <[email protected]>
Signed-off-by: georgepisaltu <[email protected]>
TransactionExtension
as a replacement for SignedExtension
Signed-off-by: georgepisaltu <[email protected]>
Signed-off-by: georgepisaltu <[email protected]>
Signed-off-by: georgepisaltu <[email protected]>
Signed-off-by: georgepisaltu <[email protected]>
Signed-off-by: georgepisaltu <[email protected]>
Signed-off-by: georgepisaltu <[email protected]>
Signed-off-by: georgepisaltu <[email protected]>
@georgepisaltu Command |
bot bench-all substrate |
@georgepisaltu https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/7346730 was started for your command Comment |
@georgepisaltu Command |
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.
Still did not reviewed everything. However, wanting to give you this feedback already to give you time to fix them.
There are a lot of comments that you don't have addressed, from other peoples before. This needs to be done!
I found some extensions which are still written incorrectly, basically rejecting any kid of tx that is not using a system origin
.
substrate/primitives/runtime/src/traits/transaction_extension/mod.rs
Outdated
Show resolved
Hide resolved
substrate/primitives/runtime/src/traits/transaction_extension/mod.rs
Outdated
Show resolved
Hide resolved
substrate/primitives/runtime/src/traits/transaction_extension/mod.rs
Outdated
Show resolved
Hide resolved
substrate/primitives/runtime/src/traits/transaction_extension/mod.rs
Outdated
Show resolved
Hide resolved
substrate/frame/transaction-payment/asset-tx-payment/src/lib.rs
Outdated
Show resolved
Hide resolved
substrate/primitives/runtime/src/traits/transaction_extension/mod.rs
Outdated
Show resolved
Hide resolved
/// Interface to differentiate between Runtime Origins authorized to include a transaction into the | ||
/// block, and those who aren't. | ||
/// | ||
/// Typically, upon validation or application of a transaction, the origin resulting from the | ||
/// transaction extension (see [`TransactionExtension`]) is checked for authorization. Transaction | ||
/// is then rejected or applied. | ||
/// | ||
/// In FRAME, an authorized origin is either an `Origin::Signed` System origin or a custom origin | ||
/// authorized in a `TransactionExtension`. | ||
pub trait AsAuthorizedOrigin { | ||
/// Whether the origin is authorized to include the transaction in a block. | ||
/// | ||
/// In FRAME returns `false` if the origin is a System `Origin::None` variant, `true` otherwise, | ||
/// meaning only signed or custom origin resulting from the transaction extension pipeline are | ||
/// authorized. | ||
fn is_authorized(&self) -> bool; | ||
} |
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.
Do we really need this? Maybe just using Option
or a custom enum to represent the initial origin
and then at the end just check if this was set?
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 because at the substrate primitives level where TransactionExtension
is defined, there is no concept of what an origin is, it's just a type. This trait is the way to differentiate between what the implementer decides is the "default" origin for their substrate runtime (if they're using a frame_system
based runtime, it will probably be frame_system::RawOrigin::None
BUT not necessarily). frame_system
and frame_support
are at a higher level in terms of dependency and we can't use concepts in those crates. As you can see in the VerifyMultiSignature
implementation now, this trait is useful.
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.
Yeah, but as I said the initial origin could just be a None
? This way, the origin can stay generic and we don't need this trait. Or what do I miss?
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 see one drawbacks in using Option<Origin>
, transaction extensions can't add stuff on the origin prior to mutate it to an authorized origin.
Like an extension adding some filter.
Or if Origin
type get more complex, some additional information on the origin.
That said such extension could always be put after authorizing transaction extensions in the pipeline.
So Option<Origin>
is also ok to me.
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 think Option<Origin>
is redundant because all origin types used in practice, including frame_system
as well as downstream custom ones, must have a value equivalent to "no origin" already. This would just introduce a "no origin for extension" (None
) and then possibly a "no origin for call" (Some(frame_system::RawOrigin::None)
).
In addition to what @gui1117 said, it's just more overhead for people writing extensions because it's explicit in the fn arguments and you have to "unwrap" it all the time and then get your relevant origin. With the trait, the impl is done once and you call it only if you care about this origin being authorized or not. Also, the trait allows users to have more origin values that are not authorized, or values that are authorized only in certain conditions, it's only limited by the trait impl.
Since the origin itself is a generic type, it makes sense we'd have a unifying interface for determining what's authorized and what's not. The trait is simple, the docs are clear, the impl is done for you and you bring it in context only if you use it, I don't see why not use 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.
When we remove ValidateUnsigned
we get: bare and general extrinsics start with frame system none origin. Bare extrinsics are only inherents, so they are inherently valid, and nodes include them into a block.
General extrinsics needs to be authorized to be valid and get into a block.
Seems coherent.
EDIT: I agree AsAuthorizedOrigin
trait is not so much more complicated and probably more future proof.
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.
RawOrigin::None
can be a valid origin for inherents for example, after validation.
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.
Inherents are not validated by TransactionExtension
s and for inherents it's not the origin that is "authorized", it's the call itself that is "authorized".
This trait is not about an origin being valid, that's decided by the call logic. An authorized origin means an origin that was authorized by a part of the system through validation and is independent of what the call is, it's a property of the origin itself.
Anyway it feels like this is just a wording issue. I can update the docs to make them more comprehensive or do some renames if necessary.
…extension PR (#5440) * rename metadata stuff from `additional_signed` to `implicit` (doesn't break metadata, only temporary representation in the code) * removed some unused associated type `SignaturePayload` * remove `From<u64>` from transaction payment transaction extension * remove `TransactionExtensionBase`, merged into `TransactionExtension` --------- Co-authored-by: georgepisaltu <[email protected]>
Signed-off-by: georgepisaltu <[email protected]>
Signed-off-by: georgepisaltu <[email protected]>
Signed-off-by: georgepisaltu <[email protected]>
…tx-ext Signed-off-by: georgepisaltu <[email protected]>
Signed-off-by: georgepisaltu <[email protected]>
Signed-off-by: georgepisaltu <[email protected]>
Signed-off-by: georgepisaltu <[email protected]>
Signed-off-by: georgepisaltu <[email protected]>
Signed-off-by: georgepisaltu <[email protected]>
Signed-off-by: georgepisaltu <[email protected]>
Signed-off-by: georgepisaltu <[email protected]>
Signed-off-by: georgepisaltu <[email protected]>
Signed-off-by: georgepisaltu <[email protected]>
I think everything from other reviewers (so far) has been addressed now.
As outlined in other comments on the issue, I thought that we would merge this PR without support for "other" custom origins and we would add support for that later. We are not currently using this in our codebase and we will only when we merge phase 2 of Extrinsic Horizon, but regardless I will work on allowing custom origins in all these extensions because it shouldn't be a problem now that we reject transactions without an origin by default. |
IMO, if we touch the code now any way and rewrite them as transaction extension, we should make them work properly. Otherwise we will miss them later. |
substrate/frame/support/procedural/src/construct_runtime/expand/metadata.rs
Outdated
Show resolved
Hide resolved
substrate/primitives/runtime/src/generic/unchecked_extrinsic.rs
Outdated
Show resolved
Hide resolved
pub fn new_unsigned(function: Call) -> Self { | ||
Self { signature: None, function } | ||
/// Returns `true` if this extrinsic instance is an inherent, `false`` otherwise. | ||
pub fn is_inherent(&self) -> bool { |
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.
For the record this naming is not meaningful until we remove ValidateUnsigned
.
We could name it is_bare
during the transition period. But it is also ok to use the final naming.
/// Create new `SignedPayload` from raw components. | ||
pub fn from_raw(call: Call, tx_ext: Extension, implicit: Extension::Implicit) -> Self { | ||
Self((call, tx_ext, implicit), EXTRINSIC_FORMAT_VERSION) | ||
} | ||
|
||
/// Deconstruct the payload into it's components. | ||
pub fn deconstruct(self) -> (Call, Extension, Extension::Implicit) { | ||
self.0 | ||
} |
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 way this type is used is not so ideal to me.
Like here, executing deconstruct
and then from_raw
can change the actual value.
And in Encode
we also lose the extrinsic format version.
Maybe having 2 types: SignedPayloadV4
and SignedPayloadV5
would make the code safer.
or new_v4
, new_v5
etc..
But maybe it is fine as it is.
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.
Code is good to me.
But before merging we should find consensus on those discussions:
-
make all transaction extensions allow not signed origin to go through in this PR.
IMO a follow-up is good. We can have a tracking issue, and it shouldn't be difficult. -
AsAuthorizedOrigin
trait or just usingOption<Origin>
in transaction extension pipeline.
IMOAsAuthorizedOrigin
trait is good, frame systemNone
origin fits our usecase, and it allows us to do more stuff with origin in the pipeline. But I don't mindOption<Origin>
either. -
in frame system offchain renameCreateInherent
toCreateBare
or something, and alsois_inherent
tois_bare
?
IMO:overall I don't mind.CreateBare
but
And some minor ones:
-
merge PR docs into one for this PR.
-
Extrinsic failed and extrinsic success events: use a type to avoid too much breaking change for people using events.
- name: frame | ||
bump: major |
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.
- name: frame | |
bump: major | |
- name: polkadot-sdk-frame | |
bump: major |
For CI
- name: minimal-runtime | ||
bump: major | ||
- name: node-template | ||
bump: major | ||
- name: node-template-runtime | ||
bump: major |
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.
- name: minimal-runtime | |
bump: major | |
- name: node-template | |
bump: major | |
- name: node-template-runtime | |
bump: major | |
- name: minimal-template-runtime | |
bump: major | |
- name: minimal-template-node | |
bump: major | |
- name: solochain-template-runtime | |
bump: major |
CI say those crates don't exists.
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.
This rename is totally unneeded.
/// Actually dispatch this call and return the result of it. | ||
fn dispatch(self, origin: Self::RuntimeOrigin) | ||
-> crate::DispatchResultWithInfo<Self::PostInfo>; | ||
} | ||
|
||
/// Shortcut to reference the `Origin` type of a `Dispatchable`. | ||
pub type OriginOf<T> = <T as Dispatchable>::RuntimeOrigin; |
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.
pub type OriginOf<T> = <T as Dispatchable>::RuntimeOrigin; | |
pub type DispatchOriginOf<T> = <T as Dispatchable>::RuntimeOrigin; |
Otherwise it is a quite generic name.
|
||
/// A mock type for account, identifies a u64 and consider any signature valid. | ||
#[derive(Clone, Debug, Encode, Decode, PartialEq, Eq, PartialOrd, Ord, scale_info::TypeInfo)] | ||
pub struct AccountU64(u64); |
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.
We already got UintAuthorityId
. We don't need this. You can just implement Verify
for it.
#[codec(encode_bound())] | ||
#[codec(decode_bound())] |
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.
#[codec(encode_bound())] | |
#[codec(decode_bound())] |
Why? The derive macro should be smart enough to do this.
where | ||
V: Codec + Debug + Sync + Send + Clone + Eq + PartialEq + StaticTypeInfo, | ||
<V::Signer as IdentifyAccount>::AccountId: | ||
Codec + Debug + Sync + Send + Clone + Eq + PartialEq + StaticTypeInfo, |
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.
where | |
V: Codec + Debug + Sync + Send + Clone + Eq + PartialEq + StaticTypeInfo, | |
<V::Signer as IdentifyAccount>::AccountId: | |
Codec + Debug + Sync + Send + Clone + Eq + PartialEq + StaticTypeInfo, |
Also not required
where | ||
V: Codec + Debug + Sync + Send + Clone + Eq + PartialEq + StaticTypeInfo, | ||
<V::Signer as IdentifyAccount>::AccountId: | ||
Codec + Debug + Sync + Send + Clone + Eq + PartialEq + StaticTypeInfo, |
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.
where | |
V: Codec + Debug + Sync + Send + Clone + Eq + PartialEq + StaticTypeInfo, | |
<V::Signer as IdentifyAccount>::AccountId: | |
Codec + Debug + Sync + Send + Clone + Eq + PartialEq + StaticTypeInfo, |
Same
// TODO: create a pallet to benchmark this weight. | ||
Weight::zero() |
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.
Needs to be done.
)] | ||
#[codec(encode_bound())] | ||
#[codec(decode_bound())] | ||
pub enum VerifyMultiSignature<V: Verify> |
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.
pub enum VerifyMultiSignature<V: Verify> | |
pub enum VerifySignature<V: Verify> |
No need to call it Multi
/// Extension that, if enabled, validates a signature type against the payload constructed from the | ||
/// call and the rest of the transaction extension pipeline. This extension provides the | ||
/// functionality that traditionally signed transactions had with the implicit signature checking | ||
/// implemented in [`Checkable`](sp_runtime::traits::Checkable). It is meant to be placed ahead of | ||
/// any other extensions that do authorization work in the [`TransactionExtension`] pipeline. |
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.
Documentation is not that great. I mean UncheckedExtrinsic
for Signed
still has a signature. So, this documentation first reads a little bit confusing.
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.
Also maybe giving an usage example.
0..=LOWEST_SUPPORTED_EXTRINSIC_FORMAT_VERSION => | ||
SignedPayload::new_legacy(self.function, tx_ext)?, |
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.
0..=LOWEST_SUPPORTED_EXTRINSIC_FORMAT_VERSION => | |
SignedPayload::new_legacy(self.function, tx_ext)?, | |
LOWEST_SUPPORTED_EXTRINSIC_FORMAT_VERSION => | |
SignedPayload::new_legacy(self.function, tx_ext)?, |
You can not just accept 0..4
as valid tx versions.
…line not rejecting not signed origins (#5612) Based on #3685 Update transaction extensions so that not signed origins are passing throught and correctly refunded. * payments transaction extensions is done with refund. Ideally the transaction extension should be an option, so that we don't need to refund and instead we don't charge when the transaction extension is not used. * `PrevalidateAttests` is refactor to have the weight at the correct place. * `CheckNonce` is refunded for when nonce is not checked. Same ideally `Nonce` should be `Option<Nonce>`, so that we can have proper weight ahead of time and not need to do refund. * bridges transaction extensions, allow not signed origins. But benchmark and weight will be done in another PR I'm thinking we should be able to support multiple version of transaction extension at the same time in the runtime. So we can easily extend the capability of the transaction extensions. Then we can have optional transaction payment and optional check nonce easily. @georgepisaltu
Original PR #2280 reverted in #3665
This PR reintroduces the reverted functionality with additional changes, related effort here. Description is copied over from the original PR
First part of Extrinsic Horizon
Introduces a new trait
TransactionExtension
to replaceSignedExtension
. Introduce the idea of transactions which obey the runtime's extensions and have according Extension data (né Extra data) yet do not have hard-coded signatures.Deprecate the terminology of "Unsigned" when used for transactions/extrinsics owing to there now being "proper" unsigned transactions which obey the extension framework and "old-style" unsigned which do not. Instead we have General for the former and Bare for the latter. (Ultimately, the latter will be phased out as a type of transaction, and Bare will only be used for Inherents.)
Types of extrinsic are now therefore:
ValidateUnsigned
(deprecated) and the_bare_compat
bits ofTransactionExtension
(deprecated).ProvideInherent
.TransactionExtension
.TransactionExtension
differs fromSignedExtension
because:pre_dispatch
is renamed toprepare
and need not contain the checks present invalidate
.validate
andprepare
is passed anOrigin
rather than aAccountId
.validate
may pass arbitrary information intoprepare
via a new user-specifiable typeVal
.AdditionalSigned
/additional_signed
is renamed toImplicit
/implicit
. It is encoded for the entire transaction and passed in to each extension as a new argument tovalidate
. This facilitates the ability of extensions to acts as underlying crypto.There is a new
DispatchTransaction
trait which contains only default function impls and is impl'ed for anyTransactionExtension
impler. It provides several utility functions which reduce some of the tedium from usingTransactionExtension
(indeed, none of its regular functions should now need to be called directly).Three transaction version discriminator ("versions") are now permissible (RFC here) in extrinsic version 5:
For the New-school General Transaction, it becomes trivial for authors to publish extensions to the mechanism for authorizing an Origin, e.g. through new kinds of key-signing schemes, ZK proofs, pallet state, mutations over pre-authenticated origins or any combination of the above.
UncheckedExtrinsic
still maintains encode/decode backwards compatibility with extrinsic version 4, where the first byte was encoded as:Code Migration
NOW: Getting it to build
Wrap your
SignedExtension
s inAsTransactionExtension
. This should be accompanied by renaming your aggregate type in line with the new terminology. E.g. Before:After:
You'll also need to alter any transaction building logic to add a
.into()
to make the conversion happen. E.g. Before:After:
SOON: Migrating to
TransactionExtension
Most
SignedExtension
s can be trivially converted to become aTransactionExtension
. There are a few things to know.SignedExtension
, you should now implement two traits individually:TransactionExtensionBase
andTransactionExtension
.fn weight
.TransactionExtensionBase
This trait takes care of anything which is not dependent on types specific to your runtime, most notably
Call
.AdditionalSigned
/additional_signed
is renamed toImplicit
/implicit
.weight
function. If your extension is associated with a pallet, you'll probably want to do this via the pallet's existing benchmarking infrastructure.TransactionExtension
Generally:
pre_dispatch
is nowprepare
and you should not reexecute thevalidate
functionality in there!AsSystemOriginSigner::as_system_origin_signer
.Pre
, calledVal
. This defines data which is passed fromvalidate
intoprepare
. This is important since you should not be duplicating logic fromvalidate
toprepare
, you need a way of passing your working from the former into the latter. This is it.Call
type parameter.Call
is the runtime call type which used to be an associated type; you can just move it to become a type parameter for your trait impl.AccountId
associated type any more. Just remove it.Regarding
validate
:validate
; all can be ignored when migrating fromSignedExtension
.validate
returns a tuple on success; the second item in the tuple is the new ticket typeSelf::Val
which gets passed in toprepare
. If you use any information extracted duringvalidate
(off-chain and on-chain, non-mutating) inprepare
(on-chain, mutating) then you can pass it through with this. For the tuple's last item, just return theorigin
argument.Regarding
prepare
:pre_dispatch
, but there is one change:validate
!!SignedExtension
which was required to run the same checks inpre_dispatch
as invalidate
.)Regarding
post_dispatch
:TransactionExtension
,Pre
is always defined, so the first parameter isSelf::Pre
rather thanOption<Self::Pre>
.If you make use of
SignedExtension::validate_unsigned
orSignedExtension::pre_dispatch_unsigned
, then:origin
isNone
.TransactionExtension
s' data.ValidateUnsigned
can still be used (for now) if you need to be able to construct transactions which contain none of the extension data, however these will be phased out in stage 2 of the Transactions Horizon, so you should consider moving to an extension-centric design.