FRAME: Create TransactionExtension as a replacement for SignedExtension

[gavofyork]

Closes #2160

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:

• 0b000000100: 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.
• 0b100000100: Old-school "Signed" Transaction: contains Signature and Extra (extension data).
• 0b010000100: New-school "General" Transaction: contains Extra (extension data), 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.

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 two type parameters: Call and Context. 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. Context is not currently used and you can safely implement over it as an unbounded type.
• 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.

TODO

• Introduce CheckSignature impl of TransactionExtension to ensure it's possible to have crypto be done wholly in a TransactionExtension.
• Deprecate SignedExtension and move all uses in codebase to TransactionExtension.
  • ChargeTransactionPayment
  • DummyExtension
  • ChargeAssetTxPayment (asset-tx-payment)
  • ChargeAssetTxPayment (asset-conversion-tx-payment)
  • CheckWeight
  • CheckTxVersion
  • CheckSpecVersion
  • CheckNonce
  • CheckNonZeroSender
  • CheckMortality
  • CheckGenesis
  • CheckOnlySudoAccount
  • WatchDummy
  • PrevalidateAttests
  • GenericSignedExtension
  • SignedExtension (chain-polkadot-bulletin)
  • RefundSignedExtensionAdapter
• Implement fn weight across the board.
• Go through all pre-existing extensions which assume an account signer and explicitly handle the possibility of another kind of origin.
  • CheckNonce should probably succeed in the case of a non-account origin.
  • CheckNonZeroSender should succeed in the case of a non-account origin.
  • ChargeTransactionPayment and family should fail in the case of a non-account origin.
  • [ ]
• Fix any broken tests.

[xlc]

Should we have a RFC to describe the extrinsic format change? Yeah I can read the diff but I prefer to read a spec in this case.

The API changes can be considered internal details and no need to be spec'ed but extrinsic format is part of external interface and should be discussed and documented.

[gavofyork]

Should we have a RFC to describe the extrinsic format change? Yeah I can read the diff but I prefer to read a spec in this case.

The API changes can be considered internal details and no need to be spec'ed but extrinsic format is part of external interface and should be discussed and documented.

This PR makes no alterations to the protocol of the Polkadot Relay or system chains as defined by the extrinsic datagrams which are acceptable or their meaning.

Furthermore, we do not (yet) have a convention of blanket requiring preapproval via an RFC on arbitrarily small changes which would alter transaction formats used by the Polkadot Relay and system chains. If you feel that such a move would be in the interests of Polkadot, I suggest you consider writing an RFC to propose it.

[kianenigma]

I suppose this goes back to the new tx extensions being able to alter origin?

[kianenigma]

If this is the killer feature, it should be documented in reference_docs::transaction_extensions.

[georgepisaltu]

This enables TransactionExtensions to alter origins during validation/preparation. This should be documented in an example of new-school transaction impl using TransactionExtension. Opened #3593 to address this.

[kianenigma]

I see that you have made follow-up issues, but don't see these being addressed in any of them. Would be good to have an issue for them too, even if they are small. They can be grouped into one issue.

[georgepisaltu]

This is a bit tough because the next step in Extrinsic Horizon is to get rid of unsigned transactions, and ValidateUnsigned would have to be removed and its checks moved somewhere else. With step 1 done and before step 2, renaming ValidateUnsigned to ValidateInherent wouldn't really make sense because unsigned transactions still exist and would be validated using this trait. This will probably just be removed in step 2, but we should clear up these todo's, which basically just describe a subset of the changes required in step 2.

[kianenigma]

for these, you can make issues with the deprecation label and use the process as per https://github.com/paritytech/polkadot-sdk/blob/master/docs/contributor/DEPRECATION_CHECKLIST.md

[georgepisaltu]

Again, this should happen when we do step 2 of #2415 .

[kianenigma]

Are the docs for this type up to date still? Especially in regard to extrinsic types etc.

We reference this type + the diagram linked to it a lot as the source of truth for "types of extrinsic"

[georgepisaltu]

This is still true for old-school extrinsics but incomplete given the general transaction type. This doc comment along with the mermaid file need to be updated. Opened #3592 for this.

[bkchr]

I'm not yet through all the changes, but here are already some comments.

[bkchr]

You changed the implementation.

It was before:

		Self::validate_unsigned(call, info, len).map(|_| ()).map_err(Into::into)

Even when this is deprecated, you should not just change it.

[georgepisaltu]

As a default impl for a deprecated trait, I think it's ok to restore the previous behavior and call validate_unsigned 👍 .

[bkchr]

This should be removed.

[georgepisaltu]

I'll remove the comment, it will be addressed anyway in step 2 of #2415

[bkchr]

This can just be removed, as the type can be constructed using the extensions.

[bkchr]

These docs could be improved.

Generally people don't care what is old school or new school. It should mention an example what it is, IMO.

[bkchr]

Not sure why this is added here. Why not directly rename it to Inherent?

[georgepisaltu]

Because it still serves both unsigned extrinsics as well as inherents. Both are represented by this Bare variant but my understanding is that they are not equivalent.

[bkchr]

Missing docs on what this does.

[georgepisaltu]

I'll address this and this by opening an issue to document this extension.

[bkchr]

I don't really get why we need this trait at all. You have your default implementation, not sure why you can not directly use this one?

[bkchr]

Basically this breaks the runtime api interface, because the host will may fail to decode this error. However, as this is an error, it is "fine".

[bkchr]

It first validates and then prepares.

[georgepisaltu]

Will fix.

[bkchr]

You are partly explaining the code, aka "call can be passed as a reference". As someone wanting to use this trait, these information are quite useless.

[georgepisaltu]

@bkchr opened #3623 to address your comments

[bkchr]

Before it was said that you need to do the same operations in pre_dispatch_unsigned as in validate_unsigned. You now calling here both, which probably results in doing the same checks twice. I know that this is only for the compatibility, but still.

[bkchr]

This is called for all kinds of transactions, also General transactions which do not really pay for their execution.

[georgepisaltu]

General transactions indeed can't pay for their execution as things stand now. These transactions would either need to be feeless with some other means of approving/validating the origin to prevent spam or people who use this General format would need to write their own logic for a transaction payment extension.

[bkchr]

#2415 (comment)

[bkchr]

That the unsigned validation is not directly lifted to the TransactionExtension level is not good. IMO it is now just more confusing than before.

Another issue being that you call here just validate_only for the General transaction. How will you find out that a transaction is allowed being a General transaction? The old ValidateUnsigned logic required explicit approval for calls to be accepted as unsigned transaction. This will now be skipped here.

[bkchr]

This basically means that I could now DOS the network with General transaction that are invalid. (Currently it would be prevented by the broken transaction-payment transaction extension. However, this is nothing we should count on.)

[georgepisaltu]

That the unsigned validation is not directly lifted to the TransactionExtension level is not good. IMO it is now just more confusing than before.

This is step 2 of Extrinsic Horizon. I think it would have been really difficult to fit these changes in this PR too.

This basically means that I could now DOS the network with General transaction that are invalid. (Currently it would be prevented by the broken transaction-payment transaction extension.

Well, not now because this PR only introduces the framework for General transactions, but they are not really supported anywhere else. It's true that the transaction payment extension would need to change to accomodate for General transactions as you can't really pay for a transaction without an account. These changes should follow naturally when we do step 2 mentioned above, where we deal with unsigned transactions as well.

[bkchr]

but they are not really supported anywhere else

I can just construct them and send them to the chain. Any chain that will not use the tx-payment signed extension will (if they don't have any other schemes against these General transactions) be able to be dosed. Aka I can just send million of these transactions to the nodes and they will happily accept them.

[bkchr]

This is wrong. When this is called for a General transaction that has no signer, you will reject it.

[georgepisaltu]

It's true that the transaction payment extension only works with signed transactions right now and it needs to be changed to support General transactions. This check will be easier when we get to the point where Bare transactions are strictly inherents and everything else is a General transaction, where we will be able to probe into the origin type, account or pallet specific with specific validation.

[bkchr]

Same issue.

[bkchr]

Not really sure we should have this macro.

But if we want to have it, it should have proper docs.

[georgepisaltu]

It's just a convenience macro to avoid writing boilerplate. It should state what the default impl is, I agree.

[bkchr]

I mean I see what it does. However, users will not know what it does and what the expected syntax is.

[bkchr]

Why do we need this? I couldn't find anything where this is really required to be separate? Asking because I think that having two traits makes this more complicated to handle, as you need to implement both traits always.