FRAME: Reintroduce `TransactionExtension` as a replacement for `SignedExtension` #3685

georgepisaltu · 2024-03-13T19:39:08Z

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 replace SignedExtension. 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:

Bare (no hardcoded signature, no Extra data; used to be known as "Unsigned")
- Bare transactions (deprecated): Gossiped, validated with ValidateUnsigned (deprecated) and the _bare_compat bits of TransactionExtension (deprecated).
- Inherents: Not gossiped, validated with ProvideInherent.
Extended (Extra data): Gossiped, validated via TransactionExtension.
- Signed transactions (with a hardcoded signature).
- General transactions (without a hardcoded signature).

TransactionExtension differs from SignedExtension because:

A signature on the underlying transaction may validly not be present.
It may alter the origin during validation.
pre_dispatch is renamed to prepare and need not contain the checks present in validate.
validate and prepare is passed an Origin rather than a AccountId.
validate may pass arbitrary information into prepare via a new user-specifiable type Val.
AdditionalSigned/additional_signed is renamed to Implicit/implicit. It is encoded for the entire transaction and passed in to each extension as a new argument to validate. 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 any TransactionExtension impler. It provides several utility functions which reduce some of the tedium from using TransactionExtension (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:

0b00000101: Bare (used to be called "Unsigned"): contains Signature or Extra (extension data). After bare transactions are no longer supported, this will strictly identify an Inherents only.
0b10000101: Old-school "Signed" Transaction: contains Signature, Extra (extension data) and an extension version byte, introduced as part of RFC99.
0b01000101: New-school "General" Transaction: contains Extra (extension data) and an extension version byte, as per RFC99, but no Signature.

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:

0b00000100 - Unsigned transactions
0b10000100 - Old-school Signed transactions, without the extension version byte

Now, UncheckedExtrinsic contains a Preamble and the actual call. The Preamble describes the type of extrinsic as follows:

/// A "header" for extrinsics leading up to the call itself. Determines the type of extrinsic and
/// holds any necessary specialized data.
#[derive(Eq, PartialEq, Clone)]
pub enum Preamble<Address, Signature, Extension> {
	/// An extrinsic without a signature or any extension. This means it's either an inherent or
	/// an old-school "Unsigned" (we don't use that terminology any more since it's confusable with
	/// the general transaction which is without a signature but does have an extension).
	///
	/// NOTE: In the future, once we remove `ValidateUnsigned`, this will only serve Inherent
	/// extrinsics and thus can be renamed to `Inherent`.
	Bare(ExtrinsicVersion),
	/// An old-school transaction extrinsic which includes a signature of some hard-coded crypto.
	Signed(Address, Signature, ExtensionVersion, Extension, ExtrinsicVersion),
	/// A new-school transaction extrinsic which does not include a signature by default. The
	/// origin authorization, through signatures or other means, is performed by the transaction
	/// extension in this extrinsic.
	General(ExtensionVersion, Extension),
}

Code Migration

NOW: Getting it to build

Wrap your SignedExtensions in AsTransactionExtension. This should be accompanied by renaming your aggregate type in line with the new terminology. E.g. Before:

/// The SignedExtension to the basic transaction logic.
pub type SignedExtra = (
	/* snip */
	MySpecialSignedExtension,
);
/// Unchecked extrinsic type as expected by this runtime.
pub type UncheckedExtrinsic =
	generic::UncheckedExtrinsic<Address, RuntimeCall, Signature, SignedExtra>;

After:

/// The extension to the basic transaction logic.
pub type TxExtension = (
	/* snip */
	AsTransactionExtension<MySpecialSignedExtension>,
);
/// Unchecked extrinsic type as expected by this runtime.
pub type UncheckedExtrinsic =
	generic::UncheckedExtrinsic<Address, RuntimeCall, Signature, TxExtension>;

You'll also need to alter any transaction building logic to add a .into() to make the conversion happen. E.g. Before:

fn construct_extrinsic(
		/* snip */
) -> UncheckedExtrinsic {
	let extra: SignedExtra = (
		/* snip */
		MySpecialSignedExtension::new(/* snip */),
	);
	let payload = SignedPayload::new(call.clone(), extra.clone()).unwrap();
	let signature = payload.using_encoded(|e| sender.sign(e));
	UncheckedExtrinsic::new_signed(
		/* snip */
		Signature::Sr25519(signature),
		extra,
	)
}

After:

fn construct_extrinsic(
		/* snip */
) -> UncheckedExtrinsic {
	let tx_ext: TxExtension = (
		/* snip */
		MySpecialSignedExtension::new(/* snip */).into(),
	);
	let payload = SignedPayload::new(call.clone(), tx_ext.clone()).unwrap();
	let signature = payload.using_encoded(|e| sender.sign(e));
	UncheckedExtrinsic::new_signed(
		/* snip */
		Signature::Sr25519(signature),
		tx_ext,
	)
}

SOON: Migrating to `TransactionExtension`

Most SignedExtensions can be trivially converted to become a TransactionExtension. There are a few things to know.

Instead of a single trait like SignedExtension, you should now implement two traits individually: TransactionExtensionBase and TransactionExtension.
Weights are now a thing and must be provided via the new function 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 to Implicit/implicit.
Weight must be returned by implementing the 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 now prepare and you should not reexecute the validate functionality in there!
You don't get an account ID any more; you get an origin instead. If you need to presume an account ID, then you can use the trait function AsSystemOriginSigner::as_system_origin_signer.
You get an additional ticket, similar to Pre, called Val. This defines data which is passed from validate into prepare. This is important since you should not be duplicating logic from validate to prepare, you need a way of passing your working from the former into the latter. This is it.
This trait takes a 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.
There's no AccountId associated type any more. Just remove it.

Regarding validate:

You get three new parameters in validate; all can be ignored when migrating from SignedExtension.
validate returns a tuple on success; the second item in the tuple is the new ticket type Self::Val which gets passed in to prepare. If you use any information extracted during validate (off-chain and on-chain, non-mutating) in prepare (on-chain, mutating) then you can pass it through with this. For the tuple's last item, just return the origin argument.

Regarding prepare:

This is renamed from pre_dispatch, but there is one change:
FUNCTIONALITY TO VALIDATE THE TRANSACTION NEED NOT BE DUPLICATED FROM validate!!
(This is different to SignedExtension which was required to run the same checks in pre_dispatch as in validate.)

Regarding post_dispatch:

Since there are no unsigned transactions handled by TransactionExtension, Pre is always defined, so the first parameter is Self::Pre rather than Option<Self::Pre>.

If you make use of SignedExtension::validate_unsigned or SignedExtension::pre_dispatch_unsigned, then:

Just use the regular versions of these functions instead.
Have your logic execute in the case that the origin is None.
Ensure your transaction creation logic creates a General Transaction rather than a Bare Transaction; this means having to include all TransactionExtensions' 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.

Signed-off-by: georgepisaltu <george.pisaltu@parity.io>

… for `SignedExtension` (#2280)" (#3665)" This reverts commit bbd51ce.

Signed-off-by: georgepisaltu <george.pisaltu@parity.io>

…tx-ext

Signed-off-by: georgepisaltu <george.pisaltu@parity.io>

carlosala · 2024-10-07T15:31:33Z

substrate/primitives/runtime/src/generic/unchecked_extrinsic.rs

+/// Type to represent the version of the [Extension](TransactionExtension) used in this extrinsic.
+pub type ExtensionVersion = u8;
+/// Type to represent the extrinsic format version which defines an [UncheckedExtrinsic].
+pub type ExtrinsicVersion = u8;


These two versions should go to the metadata (I propose Version type, found at System.Version constant).

The extrinsic version (soon to be a vec of supported versions in V16 metadata) is already in there.

The extension version we should add in some way. I believe it changes when the set of transaction extensions in a runtime changes? In which case we either want to add one extension version to the metadata to go with the list of "current" transaction extensions, or we could to have a vec of (extension_version, list_of_extensions) to reflect the current sets of extensions that the runtime supports and the version of each, if applicable.

Probably adding it to System.Version is the way to proceed. Afterwards we can add it to V16 metadata as you propose.

@gui1117 @georgepisaltu thoughts here?

The extension version we should add in some way. I believe it changes when the set of transaction extensions in a runtime changes?

The extension version here doesn't change if a runtime add or remove some transaction extensions.
It is 0 for all runtime currently.
If a transaction extension get added or removed, I think we should bump the transaction version in the runtime version (RuntimeVersion struct, transaction_version field), because it changes the execution of the extrinsic.
I don't exactly know when we would bump it but IIUC extension version is associated with the way transaction extensions are used in an extrinsic, in the implementation in sp-runtime.

I see UncheckedExtrinsic::new_transaction assume extension version is 0, I guess putting it into parameter would make more sense.

RFC99 defines the transaction extension version. This PR only adds encoding/decoding support for this to not break the format twice (as is menitoned in the RFC as well). Full support for the extension version is out of scope for this PR and will be added later. Metadata work is also a follow-up on this PR.

I still think the extension version should be part of the inherited implication, so it can be signed in some way.
Otherwise supporting multiple version can lead to extrinsic being valid in some non-intended context.
See : #3685 (comment)

Replied here and I think it has merit but I stand by my earlier comment above about this being out of scope for this PR.

Yes I agree we should do in a follow-up.

substrate/primitives/runtime/src/generic/unchecked_extrinsic.rs

…tx-ext

carlosala · 2024-10-08T12:08:45Z

substrate/frame/verify-signature/src/extension.rs

+		// Construct the payload that the signature will be validated against. The inherited
+		// implication contains the encoded bytes of the call and all of the extension data of the
+		// extensions that follow in the `TransactionExtension` pipeline.
+		//
+		// In other words:
+		// - extensions that precede this extension are ignored in terms of signature validation;
+		// - extensions that follow this extension are included in the payload to be signed (as if
+		//   they were the entire `SignedExtension` pipeline in the traditional signed transaction
+		//   model).
+		//
+		// The encoded bytes of the payload are then hashed using `blake2_256`.
+		let msg = inherited_implication.using_encoded(blake2_256);


Until now the payloads were only hashed if they exceed 256 bytes of length. Changing this now to always hash might be messy for tooling, signers, etc.
If done at least it has to be spec-ed like https://spec.polkadot.network/id-extrinsics#defn-extrinsic-signature or in an RFC.
Is there any reason why this should be changed now? I don't see any particular advantage.
WDYT @bkchr?

An extrinsic format version bump always involves a breaking change, so tooling has to make an active effort to support new versions anyways. V5 extrinsics also introduce the extension version byte, so this would have been a breaking change even without the change to payload construction, we just bundled them together.

Even so, we provide compatibility with v4 extrinsics regarding encoding/decoding, so tools that don't yet offer support for this should use v4 extrinsics.

Before we release, we will introduce a new entry in the spec for v5 extrinsics, in addition to the one in v4 that you linked.

Until now the payloads were only hashed if they exceed 256 bytes of length. Changing this now to always hash might be messy for tooling, signers, etc.

This was overseen in the RFC and should still be added in some extra RFC to clarify this.

jsdw · 2024-10-08T15:39:33Z

@josepot and @carlosala raised a couple of good points in a call earlier re this PR, which I want to raise here:

Extrinsic version number in metadata: keep it at 4

Currently, the metadata (eg V14 and V15) has an extrinsic.version field which is currently 4. I couldn't immediately see in the code whether this changes to 5 or not, but wanted to suggest that we keep that version set to 4 in this PR.

The reason for this is for backward compatibility: after merging this PR, any apps that check that field in order to know which extrinsic version is supported by the chain will still see "4", and will therefore still be happy to continue submitting V4 extrinsics.

If we update the number to 5, then there is a chance that apps that only know how to construct V4 extrinsics will assume that they no longer can submit them and start failing.

When V16 metadata lands, we should support a vec of versions, and so can properly communicate to apps all of the versions that are supported.

The Transaction Extension version field: keep it at 0 until V16 metadata lands.

My understanding (and please correct if wrong) is that the transaction extension version will be incremented each time the set of transaction extensions changes.

In V14 and V15 metadata, we currently only see the "latest" set of transaciton extensions. This means that if I submit an extrinsic using eg v0 transaction extensions when we are currently on v1, then I will not know how to decode this extrinsic using the metadata: I can decode the v1 list of extensions but do not know what the v0 list looks like.

As long as the extension version stays at 0, we are ok, because there is only one possible list of extensions that is valid in an extrinsic anyway, and this list is available in the metadata.

When V16 metadata lands, the extensions list should be something like Vec<(version_u8, list_of_extensions_at_this_version)>. ie, the list of extensions for each supported extension version number. This will provide enough information for any extrinsic to be decoded. (See #5285)

I hope both of those points are reasonable; please @josepot and @carlosala feel free to elaborate on anything I might have missed! \cc @lexnv

gui1117 · 2024-10-08T17:09:06Z

About supporting multiple set of transaction extension and having in the metadata: Vec<(version_u8, list_of_extensions_at_this_version)>.
I think then we must put the extension version (version_u8) in the inherited implication. So that it is signed alongside the call and other transaction extensions.

The reason is that 2 extrinsic could be valid for 2 set of transaction extension.
Let's say we have a transaction extension with implicit being (tip_to_block_author_u8, tip_to_treasury_u8), which is replaced in another version by the a similar transaction with implicit being the opposite: (tip_to_treasury_u8, tip_to_block_author_u8), then the extrinsic has a different meaning depending on which extension version we use, and the signature never sign the extension version.

Also we should probably use a compact and not a u8.

georgepisaltu · 2024-10-08T18:24:55Z

About supporting multiple set of transaction extension and having in the metadata: Vec<(version_u8, list_of_extensions_at_this_version)>. I think then we must put the extension version (version_u8) in the inherited implication. So that it is signed alongside the call and other transaction extensions.

The reason is that 2 extrinsic could be valid for 2 set of transaction extension. Let's say we have a transaction extension with implicit being (tip_to_block_author_u8, tip_to_treasury_u8), which is replaced in another version by the a similar transaction with implicit being the opposite: (tip_to_treasury_u8, tip_to_block_author_u8), then the extrinsic has a different meaning depending on which extension version we use, and the signature never sign the extension version.

Also we should probably use a compact and not a u8.

This is already covered by the current set of extensions that we're running, namely CheckGenesis and CheckSpecVersion. The only situation where the situation you're describing could happen would be with 2 different chains or same chain after a runtime upgrade, both of which are prevented by the extensions above. Any changes introduced to a runtime's extension, breaking or backwards compatible, can only be done through a runtime upgrade, which means a spec version bump.

The point of the extension version, as described in the RFC, is to enable the runtime to introduce changes to the extensions used while supporting the old one in a backwards compatible way.

georgepisaltu · 2024-10-08T18:38:58Z

My understanding (and please correct if wrong) is that the transaction extension version will be incremented each time the set of transaction extensions changes.

In V14 and V15 metadata, we currently only see the "latest" set of transaciton extensions. This means that if I submit an extrinsic using eg v0 transaction extensions when we are currently on v1, then I will not know how to decode this extrinsic using the metadata: I can decode the v1 list of extensions but do not know what the v0 list looks like.

As long as the extension version stays at 0, we are ok, because there is only one possible list of extensions that is valid in an extrinsic anyway, and this list is available in the metadata.

As mentioned in other comments, the extension version is:

runtime specific
supported only as far as encode/decode in this PR

There will be another PR altogether adding full support for versioned extensions and the version will not be hardcoded for all UncheckedExtrinsics. It will start at 0 for all runtimes in metadata v16, which will be released along the same timeline as polkadot-sdk in December 2024 as far as my current understanding goes. Then, with subsequent upgrades it can be incremented for some runtimes.

The rest of the points about the metadata changes/additions should probably be discussed separately, maybe on the metadata v16 issue or a new one (@lexnv). We should focus on the core runtime functionality aspect in this PR and get it through, and address the metadata stuff in follow up efforts, according to the release timeline stated earlier.

gui1117 · 2024-10-08T18:39:10Z

About supporting multiple set of transaction extension and having in the metadata: Vec<(version_u8, list_of_extensions_at_this_version)>. I think then we must put the extension version (version_u8) in the inherited implication. So that it is signed alongside the call and other transaction extensions.
The reason is that 2 extrinsic could be valid for 2 set of transaction extension. Let's say we have a transaction extension with implicit being (tip_to_block_author_u8, tip_to_treasury_u8), which is replaced in another version by the a similar transaction with implicit being the opposite: (tip_to_treasury_u8, tip_to_block_author_u8), then the extrinsic has a different meaning depending on which extension version we use, and the signature never sign the extension version.
Also we should probably use a compact and not a u8.

This is already covered by the current set of extensions that we're running, namely CheckGenesis and CheckSpecVersion. The only situation where the situation you're describing could happen would be with 2 different chains or same chain after a runtime upgrade, both of which are prevented by the extensions above. Any changes introduced to a runtime's extension, breaking or backwards compatible, can only be done through a runtime upgrade, which means a spec version bump.

The point of the extension version, as described in the RFC, is to enable the runtime to introduce changes to the extensions used while supporting the old one in a backwards compatible way.

check genesis is signing for a blockchain, check spec version is signing for a runtime specification.

We are talking about supporting multiple set of transaction extension in one runtime of one blockchain.

Example: in one runtime we have:

extension version 1: (other extensions..., Ext1(tip_for_block_author_u8), Ext2(tip_for_treasury_u8))
extension version 2: (other extensions..., Ext1(tip_for_treasury_u8), Ext2(tip_for_block_author_u8))

I send my transaction. How can I ensure my tip goes to the correct destination, and nobody can change the meaning of my intended signature.
If the extension version is not signed, or some uuid of extensions, then anybody can just decompose/recompose my transaction to a different intent.

I know such situation will be rare, but better to avoid automatically, rather than adding another burden on runtime developer.

carlosala · 2024-10-08T18:48:03Z

Absolutely. Since that extensionVersion could change the behavior on runtime of the extrinsic it has to be signed.

georgepisaltu · 2024-10-08T18:51:14Z

@gui1117 I see now what corner case you're talking about. I personally think this would be extremely improbable, but if we want to address it, we should just provide an extension at the frame_system level that would include this version into the payload. I think it would be the cleanest way to do it, especially after we deprecate SignedPayload and we lose the ability to generate custom extrinsic signing payloads. That's how we handle all of the other similar corner cases, like different chain or different version etc.

gui1117 · 2024-10-08T19:12:43Z

@gui1117 I see now what corner case you're talking about. I personally think this would be extremely improbable, but if we want to address it, we should just provide an extension at the frame_system level that would include this version into the payload. I think it would be the cleanest way to do it, especially after we deprecate SignedPayload and we lose the ability to generate custom extrinsic signing payloads. That's how we handle all of the other similar corner cases, like different chain or different version etc.

I was worried it would be difficult to get access to the extension version. But we can always leverage note_extrinsic in system to also get this information.
Then we can have another regular transaction extension.

So OK, let's do in follow-up

carlosala · 2024-10-08T20:09:05Z

If we were to use a System pallet tx extension, we would need to ensure it is the first extension and stable across versions; otherwise, decoding is not directly possible.

bkchr · 2024-10-08T20:33:26Z

Yeah generally the extension_version stuff should only be added by this pr to the format of the extrinsic and then later implemented properly.

bkchr · 2024-10-08T20:36:22Z

Regarding the signing of the extension version. I would not want to have an extra extension for this... For signed transactions it isn't such a problem and we could directly include it into the signature. However, when it comes to general transaction we will have a problem. Or we pass it down as inherited implication?

georgepisaltu · 2024-10-09T09:29:04Z

For signed transactions it isn't such a problem and we could directly include it into the signature. However, when it comes to general transaction we will have a problem. Or we pass it down as inherited implication?

We can definitely come up with something to include it in the inherited implication. First idea, carry the version over from UncheckedExtrinsic into CheckedExtrinsic and then constructing the initial inherited implication used in validate_only as (version, call) instead of call.

The problems if we do it like this are:

We have to do both solutions for Signed and General, and they are different impls, more to maintain.
We have to do another update to polkadot-js/api to use this new (hardcoded) payload structure and go through that release process again. Without polkadot-js/api support, the zombienet CI checks will fail.

I like the extension solution and I like the inherited implication solution, but we need to check with the polkadot-js/api team how long it would take to implement this after we make the required changes in this PR.

Another idea would be to merge this as is with the current polkadot-js/api support and do the inherited implication change in another PR while they do the change on their end and release a new polkadot-js/api version, and merge ours when they release that, all of this before our polkadot-sdk December 2024 release obviously. WDYT @bkchr @gui1117

carlosala · 2024-10-09T10:59:37Z

I think before all of that; we need to come up with a structure that allows the changes proposed in this PR without having flaws in terms of security, ability to decode, etc.
Summarizing, we miss:

An RFC that defines what is the signing payload and signing process (hashing the payload, with which hash, etc) for Signed extrinsics. General extrinsics do not need this RFC since all the crypto information will come as a Tx Extension, and therefore the potential future extensions will need their own RFC (like the one created for CheckMetadataHash.
Come up with an idea to keep the backwards compatibility of Extension using extensionVersion for General extrinsics. There is no straight-forward solution ATM. The backwards compatibility for Signed extrinsics is more or less straight-forward.
Ensure extensionVersion is in the metadata (I propose as a constant inside System.Version), and does not exceed 0 (i.e. it can't be bumped) until Metadata V16 is available in the chain to ensure that extrinsics are always "decodeable" just using the latest available metadata.

If needed I can work on the first draft of the RFC.

georgepisaltu · 2024-10-09T12:55:35Z

An RFC that defines what is the signing payload and signing process (hashing the payload, with which hash, etc) for Signed extrinsics. General extrinsics do not need this RFC since all the crypto information will come as a Tx Extension, and therefore the potential future extensions will need their own RFC (like the one created for CheckMetadataHash.

The solutions proposed have different payload schemes, different encoding and different assumptions built into them. This is not just for Signed extrinsics, all extrinsic types need to be captured within this solution, regardless of the approach we choose. V4 is still supported but doesn't change. We need one RFC (for v5), and we need to agree on an approach first before being able to write anything, which is the point of this comment, because the RFC needs to define the encoding format which is dependent on the chosen solution.

Come up with an idea to keep the backwards compatibility of Extension using extensionVersion for General extrinsics. There is no straight-forward solution ATM. The backwards compatibility for Signed extrinsics is more or less straight-forward.

As mentioned multiple times, this PR only introduces the encode/decode logic for the extension version. Full support is to be added later in subsequent PRs.

Ensure extensionVersion is in the metadata (I propose as a constant inside System.Version), and does not exceed 0 (i.e. it can't be bumped) until Metadata V16 is available in the chain to ensure that extrinsics are always "decodeable" just using the latest available metadata.

Again the remaining metadata work is out of scope for this PR. Metadata v16 is a separate effort, but with the same release schedule. Moreover, both this PR with extrinsic v5 as well as metadata v16 are unreleased and they will release together. Extrinsic Horizon should be an incremental effort and the code in this PR doesn't break any tests. We should track the metadata related issues in the metadata v16 meta issue, not here.

It's good that we're identifying issues and things we need to improve before the release, but we need to stop the scope creep in this PR.

carlosala · 2024-10-09T13:53:22Z

It's good that we're identifying issues and things we need to improve before the release, but we need to stop the scope creep in this PR.

Let's ensure Westend and/or Rococo do not get released with these new changes then 👍🏻
Runtimes are getting modified in this PR.

gui1117 · 2024-10-09T14:44:58Z

About metadata can we just put extrinsic version 4 in the metadata, and not have anything else until metadata v16.
blockchains will contain some extrinsic v5, and tools will have to be able to decode them, but they don't need to build them.

About signature of extension version. Having new transaction extension seems hard. We would need to pass the extension version as an argument in validate. Or enforce the extrinsic user to notify frame-system about the extension version before calling UncheckedExtrinsic::check, then frame-system would put this into storage and a transaction extension can have it part of its implicit.(statefull solution are not a good design, but it is doable in frame-executive to do this operation).

Without transaction extension, IMO the solution is to have in the inherited implication (other solution is to have it in a parameter in validate).
Signed transaction will also need to have it part of its payload but it is fine IMO, the payload will always need to be specified manually, until signed transaction are deprecated.

bkchr · 2024-10-09T18:15:39Z

2. We have to do another update to polkadot-js/api to use this new (hardcoded) payload structure and go through that release process again. Without polkadot-js/api support, the zombienet CI checks will fail.

As already proposed here multiple times, the metadata should return version 4. This way we don't need any fixes in pjs right now until the stuff is sorted out. This is also much better, because it allows us to still change some things with version 5. TBH, I don't see this getting anywhere ready for having extrinsics with version 5 being stable until december. They go into the release, but we will continue to use version 4 "by default".

2. Come up with an idea to keep the backwards compatibility of Extension using extensionVersion for General extrinsics. There is no straight-forward solution ATM. The backwards compatibility for Signed extrinsics is more or less straight-forward.

I don't get the problem? General transactions will be backwards compatible like signed transactions when it comes to extensions. Or do you mean how to ensure that extensionVersion is not "changed"?

Without transaction extension, IMO the solution is to have in the inherited implication (other solution is to have it in a parameter in validate).

Still think this is the best way. Any kind of general transaction that will need some kind of authorization will use a signature anyway that will then also include the extension version in the signed data, like we should do it for signed transactions.

Signed transaction will also need to have it part of its payload but it is fine IMO, the payload will always need to be specified manually, until signed transaction are deprecated.

Signed transactions are getting deprecated?

3. Ensure extensionVersion is in the metadata (I propose as a constant inside System.Version), and does not exceed 0 (i.e. it can't be bumped) until Metadata V16 is available in the chain to ensure that extrinsics are always "decodeable" just using the latest available metadata.

The extension version should be put next to the already existing extrinsic version. Don't see a reason to include this in System.Version.

gui1117 · 2024-10-09T19:04:23Z

substrate/primitives/runtime/src/generic/checked_extrinsic.rs

+				// TODO: Remove below once `ValidateUnsigned` is deprecated and the only `Bare`
+				// extrinsics left are inherents.
+				#[allow(deprecated)]
+				Extension::post_dispatch_bare_compat(info, &post_info, len, &pd_res)?;


Is there something wrong here?

Formerly signed extensions post_dispatch was executed for unsigned extrinsics. This is important for CheckWeight which registers the used weight inside the post dispatch.
But now it no longer executes it.
AFAICT nothing register the weight for unsigned transactions and inherents.

in polkadot-sdk only CheckWeight and StorageWeightReclaim needs to execute some logic in before and after dispatch of inherents and unsigned transactions. AFAICT.

Maybe we can move both in frame-system or frame-executive.
Like have some extrinsic hook with pre-dispatch and post-dispatch.

EDIT: Or the other direction: runtime provides an instance of transaction extension to be used for inherents and unsigned transaction. And they are validated, prepared and post-dispatch is executed.
The validation for unsigned transaction is simply the ValidateUnsigned logic.
The validation for inherent is: source is local or in-block and also ProvideInherent::is_inherent returns true.

gui1117 · 2024-10-09T19:28:43Z

Signed transaction will also need to have it part of its payload but it is fine IMO, the payload will always need to be specified manually, until signed transaction are deprecated.

Signed transactions are getting deprecated?

I meant it could be replace with a general transaction with a transaction extension which contains the payload. In the far future.

bkchr · 2024-10-09T19:36:06Z

I meant it could be replace with a general transaction with a transaction extension which contains the payload. In the far future.

Just discussed this with @georgepisaltu via DM. Generally I think we should directly remove it. There is no real benefit in adding support for it and then removing it again. Changing the format is quite hard in the ecosystem and we should not do this again. This would also require extrinsic version 6. We need support for extrinsic version and then we could remove signed transactions directly from v5.

Nevertheless, the pr can be merged as it is right now as long as we expose v4 in the metadata.

carlosala · 2024-10-10T09:42:19Z

I don't get the problem? General transactions will be backwards compatible like signed transactions when it comes to extensions. Or do you mean how to ensure that extensionVersion is not "changed"?

With general transactions, we don't have a way to specify which extensionVersion we are submitting since there is no signing payload to append extensionVersion. extensionVersion could be modified without the user knowing it was modified, altering the intent.

The extension version should be put next to the already existing extrinsic version. Don't see a reason to include this in System.Version.

If we keep version 4 in metadata v14/15 I agree. If version 5 gets ever to metadata v14/15 then we need to add that byte somewhere, otherwise we can't construct extrinsics from outside of the runtime.

georgepisaltu added 8 commits March 8, 2024 13:29

Fixes and refactoring after tx ext introduction

e7c6f57

Signed-off-by: georgepisaltu <george.pisaltu@parity.io>

Remove bench helper trait from claims

2fa52c8

Signed-off-by: georgepisaltu <george.pisaltu@parity.io>

Rename additional_signed to implicit in IR

90819a5

Signed-off-by: georgepisaltu <george.pisaltu@parity.io>

Fix imports

9206bea

Signed-off-by: georgepisaltu <george.pisaltu@parity.io>

Fix metadata test

611c165

Signed-off-by: georgepisaltu <george.pisaltu@parity.io>

Fix unchecked extrinsic signing in tests

6fd6b25

Signed-off-by: georgepisaltu <george.pisaltu@parity.io>

Revert "Revert "FRAME: Create TransactionExtension as a replacement…

53c0379

… for `SignedExtension` (#2280)" (#3665)" This reverts commit bbd51ce.

Merge branch 'tx-ext-fixes' into george-tx-ext

ed40294

georgepisaltu requested review from gavofyork and bkchr March 13, 2024 19:39

georgepisaltu requested review from athei, acatangiu, andresilva, cheme and a team as code owners March 13, 2024 19:39

paritytech-review-bot bot requested a review from a team March 13, 2024 19:39

georgepisaltu changed the title ~~George/restore gav tx ext~~ FRAME: Reintroduce TransactionExtension as a replacement for SignedExtension Mar 13, 2024

georgepisaltu added the T1-FRAME This PR/Issue is related to core FRAME, the framework. label Mar 13, 2024

georgepisaltu mentioned this pull request Mar 13, 2024

TransactionExtension follow up refactoring and fixes #3623

Closed

TarikGul mentioned this pull request Mar 13, 2024

[Tracking upstream] Addition of TransactionExtension polkadot-js/api#5820

Open

sinzii mentioned this pull request Mar 14, 2024

Support extrinsic format version v5 dedotdev/dedot#55

Open

georgepisaltu added 8 commits March 14, 2024 15:02

Add docs for impl_tx_ext_default

b6fba4a

Signed-off-by: georgepisaltu <george.pisaltu@parity.io>

Remove TransactionExtensionInterior

455b762

Signed-off-by: georgepisaltu <george.pisaltu@parity.io>

Merge remote-tracking branch 'origin/master' into george/restore-gav-…

5f6f63b

…tx-ext

Minor fixes for TransactionExtensionInterior

a896a50

Signed-off-by: georgepisaltu <george.pisaltu@parity.io>

Fix metadata test

26ea26f

Signed-off-by: georgepisaltu <george.pisaltu@parity.io>

Bump tx version in bridges

cde2b1e

Signed-off-by: georgepisaltu <george.pisaltu@parity.io>

Missed file in previous commit

4c6f654

Signed-off-by: georgepisaltu <george.pisaltu@parity.io>

Merge remote-tracking branch 'origin' into george/restore-gav-tx-ext

c4b522b

Signed-off-by: georgepisaltu <george.pisaltu@parity.io>

taplo fix

a60dd86

carlosala reviewed Oct 7, 2024

View reviewed changes

substrate/primitives/runtime/src/generic/unchecked_extrinsic.rs Show resolved Hide resolved

gui1117 added 4 commits October 7, 2024 18:57

Merge remote-tracking branch 'origin/master' into george/restore-gav-…

4b5dd2c

…tx-ext

prdoc: remove outdated crates

62e961c

rename in ExtrinsicMetadata: SignedExtensions to TransactionExtensions

9e7216e

add debug assert

00c4406

carlosala reviewed Oct 8, 2024

View reviewed changes

carlosala mentioned this pull request Oct 9, 2024

feat: add support for extrinsic v5 polkadot-api/polkadot-api#777

Draft

gui1117 reviewed Oct 9, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FRAME: Reintroduce `TransactionExtension` as a replacement for `SignedExtension` #3685

FRAME: Reintroduce `TransactionExtension` as a replacement for `SignedExtension` #3685

georgepisaltu commented Mar 13, 2024 •

edited

Loading

carlosala Oct 7, 2024

jsdw Oct 8, 2024

carlosala Oct 8, 2024 •

edited

Loading

carlosala Oct 8, 2024

gui1117 Oct 8, 2024 •

edited

Loading

georgepisaltu Oct 8, 2024

gui1117 Oct 8, 2024 •

edited

Loading

georgepisaltu Oct 8, 2024

gui1117 Oct 8, 2024

carlosala Oct 8, 2024 •

edited

Loading

georgepisaltu Oct 8, 2024

bkchr Oct 8, 2024

jsdw commented Oct 8, 2024 •

edited

Loading

gui1117 commented Oct 8, 2024 •

edited

Loading

georgepisaltu commented Oct 8, 2024

georgepisaltu commented Oct 8, 2024

gui1117 commented Oct 8, 2024

carlosala commented Oct 8, 2024

georgepisaltu commented Oct 8, 2024

gui1117 commented Oct 8, 2024

carlosala commented Oct 8, 2024

bkchr commented Oct 8, 2024

bkchr commented Oct 8, 2024

georgepisaltu commented Oct 9, 2024

carlosala commented Oct 9, 2024 •

edited

Loading

georgepisaltu commented Oct 9, 2024

carlosala commented Oct 9, 2024 •

edited

Loading

gui1117 commented Oct 9, 2024

bkchr commented Oct 9, 2024

gui1117 Oct 9, 2024 •

edited

Loading

gui1117 Oct 10, 2024 •

edited

Loading

gui1117 commented Oct 9, 2024

bkchr commented Oct 9, 2024

carlosala commented Oct 10, 2024

FRAME: Reintroduce TransactionExtension as a replacement for SignedExtension #3685

Are you sure you want to change the base?

FRAME: Reintroduce TransactionExtension as a replacement for SignedExtension #3685

Conversation

georgepisaltu commented Mar 13, 2024 • edited Loading

Code Migration

NOW: Getting it to build

SOON: Migrating to TransactionExtension

TransactionExtensionBase

TransactionExtension

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carlosala Oct 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gui1117 Oct 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gui1117 Oct 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carlosala Oct 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsdw commented Oct 8, 2024 • edited Loading

Extrinsic version number in metadata: keep it at 4

The Transaction Extension version field: keep it at 0 until V16 metadata lands.

gui1117 commented Oct 8, 2024 • edited Loading

georgepisaltu commented Oct 8, 2024

georgepisaltu commented Oct 8, 2024

gui1117 commented Oct 8, 2024

carlosala commented Oct 8, 2024

georgepisaltu commented Oct 8, 2024

gui1117 commented Oct 8, 2024

carlosala commented Oct 8, 2024

bkchr commented Oct 8, 2024

bkchr commented Oct 8, 2024

georgepisaltu commented Oct 9, 2024

carlosala commented Oct 9, 2024 • edited Loading

georgepisaltu commented Oct 9, 2024

carlosala commented Oct 9, 2024 • edited Loading

gui1117 commented Oct 9, 2024

bkchr commented Oct 9, 2024

gui1117 Oct 9, 2024 • edited Loading

Choose a reason for hiding this comment

gui1117 Oct 10, 2024 • edited Loading

Choose a reason for hiding this comment

gui1117 commented Oct 9, 2024

bkchr commented Oct 9, 2024

carlosala commented Oct 10, 2024

FRAME: Reintroduce `TransactionExtension` as a replacement for `SignedExtension` #3685

FRAME: Reintroduce `TransactionExtension` as a replacement for `SignedExtension` #3685

georgepisaltu commented Mar 13, 2024 •

edited

Loading

SOON: Migrating to `TransactionExtension`

`TransactionExtensionBase`

`TransactionExtension`

carlosala Oct 8, 2024 •

edited

Loading

gui1117 Oct 8, 2024 •

edited

Loading

gui1117 Oct 8, 2024 •

edited

Loading

carlosala Oct 8, 2024 •

edited

Loading

jsdw commented Oct 8, 2024 •

edited

Loading

gui1117 commented Oct 8, 2024 •

edited

Loading

carlosala commented Oct 9, 2024 •

edited

Loading

carlosala commented Oct 9, 2024 •

edited

Loading

gui1117 Oct 9, 2024 •

edited

Loading

gui1117 Oct 10, 2024 •

edited

Loading