polkadot-sdk icon indicating copy to clipboard operation
polkadot-sdk copied to clipboard

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

Open georgepisaltu opened this issue 1 year ago • 17 comments

Original PR https://github.com/paritytech/polkadot-sdk/pull/2280 reverted in https://github.com/paritytech/polkadot-sdk/pull/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.

georgepisaltu avatar Mar 13 '24 19:03 georgepisaltu

A comment on my comment https://github.com/paritytech/polkadot-sdk/issues/2415#issuecomment-1989480626 would also be nice.

I will propose some changes to the initial issue, which should address that comment as well.

georgepisaltu avatar Jul 30 '24 12:07 georgepisaltu

By the way I think it is actually fine to deny any non signed origin in extensions like CheckWeight, CheckNonce etc...

I have the feeling that the final design of pipeline will not be a single pipe where checks are concurent but instead it will be branching.

Instead of this:

(SignatureStyle1, SignatureStyle2, CheckOnce, CheckWeight, FilterNoneToAcceptedUnsignedCall<SkipIfFeeLess<ChargeTransaction>>)

I expect a pipeline like this:

(
  OneOf2<
    FilterNoneToAcceptedUnsignedCall,
    (
      OneOf2<VerifySignatureStyle1, VerifySignatureStyle2>,
      CheckNonce,
      CheckWeight,
      OneOf2<ChargeTransaction, UseSpecialPrivilege>,
    )
)

with OneOf2 being a simple enum with 2 variants.

gui1117 avatar Aug 02 '24 08:08 gui1117

~ValidateUnsigned::validate_unsigned function takes source: TransactionSource as a parameter, this will be needed to be passed in some way to the transaction extension.~

~EDIT: or do I miss something like unsigned transaction with source local should be now considered inherent?~

gui1117 avatar Aug 10 '24 09:08 gui1117

Working on stage 2. The pre dispatch weight of a transaction extension would be more accurate if it had access to call information, explicit payload (and maybe even implicit payload, but I am not sure of this one).

In stage 2 we want a transaction extension which match the call variant and execute some authorization logic on some variant and allow them to dispatch.

The pre dispatch weight for this is the maximum of all call variant authorization logic. But if we had access to call then we could compute a more accurate weight. Calls themselves have access to their call instance when calculating the weight.

It can be fine because the weight of call variant authorization logic should be small enough to not DDOS, and refund will be accurate.

But when people look at the upfront cost of a basic transfer this effect will not be negligible. And it is an important metric I guess.

Also giving just access to the explicit payload can be good enough because then we could allow transactions to opt out of this authorization. (The only bad consequence is that the transaction extension format will be breaking, asking for one more byte for this explicit payload).

Personally I think I would give the call and the explicit payload to the weight function.

gui1117 avatar Aug 13 '24 05:08 gui1117

The CI pipeline was cancelled due to failure one of the required jobs. Job name: test-linux-stable 1/3 Logs: https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/7244840

paritytech-cicd-pr avatar Sep 03 '24 17:09 paritytech-cicd-pr

bot bench-all substrate

georgepisaltu avatar Sep 13 '24 13:09 georgepisaltu

@georgepisaltu "$PIPELINE_SCRIPTS_DIR/commands/bench-all/bench-all.sh" --target_dir=substrate (https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/7344421) was cancelled in https://github.com/paritytech/polkadot-sdk/pull/3685#issuecomment-2349615123

command-bot[bot] avatar Sep 13 '24 13:09 command-bot[bot]

bot cancel 5-478ae05b-f12d-4078-a562-184d6fbfbb48

georgepisaltu avatar Sep 13 '24 17:09 georgepisaltu

@georgepisaltu Command "$PIPELINE_SCRIPTS_DIR/commands/bench-all/bench-all.sh" --target_dir=substrate has finished. Result: https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/7344421 has finished. If any artifacts were generated, you can download them from https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/7344421/artifacts/download.

command-bot[bot] avatar Sep 13 '24 17:09 command-bot[bot]

bot bench-all substrate

georgepisaltu avatar Sep 13 '24 18:09 georgepisaltu

@georgepisaltu https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/7346730 was started for your command "$PIPELINE_SCRIPTS_DIR/commands/bench-all/bench-all.sh" --target_dir=substrate. Check out https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/pipelines?page=1&scope=all&username=group_605_bot to know what else is being executed currently.

Comment bot cancel 2-5d79bfe9-88ee-4d07-b1ec-c76f92205de0 to cancel this command or bot cancel to cancel all commands in this pull request.

command-bot[bot] avatar Sep 13 '24 18:09 command-bot[bot]

@georgepisaltu Command "$PIPELINE_SCRIPTS_DIR/commands/bench-all/bench-all.sh" --target_dir=substrate has finished. Result: https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/7346730 has finished. If any artifacts were generated, you can download them from https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/7346730/artifacts/download.

command-bot[bot] avatar Sep 13 '24 23:09 command-bot[bot]

There are a lot of comments that you don't have addressed, from other peoples before.

I think everything from other reviewers (so far) has been addressed now.

I found some extensions which are still written incorrectly, basically rejecting any kid of tx that is not using a system origin.

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.

georgepisaltu avatar Sep 18 '24 17:09 georgepisaltu

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.

bkchr avatar Sep 18 '24 20:09 bkchr

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.

This is done.

georgepisaltu avatar Sep 25 '24 10:09 georgepisaltu

I am also running the bridges zombienet tests. For example, continuous-integration/gitlab-zombienet-bridges-0001-asset-transfer-works fails, but it seems this needs more investigation, as I might be encountering a probably PAPI error Unsupported unsigned extrinsic version 5 - full stack

We are aware of this zombienet issue. The error seems to be coming from here and there is an effort in polkadot-js/api to support v5 extrinsics. I am not aware of the exact dependency tree in those scripts. We need to support the v5 extrinsic format in PAPI as well anyway.

georgepisaltu avatar Oct 02 '24 13:10 georgepisaltu

I am also running the bridges zombienet tests. For example, continuous-integration/gitlab-zombienet-bridges-0001-asset-transfer-works fails, but it seems this needs more investigation, as I might be encountering a probably PAPI error Unsupported unsigned extrinsic version 5 - full stack

We are aware of this zombienet issue. The error seems to be coming from here and there is an effort in polkadot-js/api to support v5 extrinsics. I am not aware of the exact dependency tree in those scripts. We need to support the v5 extrinsic format in PAPI as well anyway.

With the fix in https://github.com/polkadot-js/api/pull/5976 and also in PJS/api-cli https://github.com/polkadot-js/api/pull/5976#issuecomment-2391292647, the bridges zombienet tests now work locally. I need to release a new relayer with support for ext. version 4/5, so I will prepare another PR with fixed zombienet tests.

bkontur avatar Oct 03 '24 13:10 bkontur

IMO the not allowed origin should not be able to appear in the pallet code at all. Otherwise pallet functions that ignore the origin could run into problems (like batch for example).

This is how it will work actually. I think once we implement phase 2 of Extrinsic Horizon it will be clearer. When we deprecate ValidateUnsigned, the pallets will not be using the RawOrigin::None origin anymore but their own custom origins for currently unsigned transactions. That's the end goal, but the current approach is still solid, even while we're in this transition period.

This will implicitly change a lot of code and I'm not a very big fan of this.

This whole PR changed a lot of implicit assumptions about substrate primitives and how they work, but it was done to enable new usecases. I am confident that origins and the way they are used in pallets will ultimately be better than what we have now once we complete Extrinsic Horizon.

We already have a POC in the works for phase 2. If, for whatever reason, we find that this approach doesn't work, we can always come back and rethink the design.

georgepisaltu avatar Oct 07 '24 08:10 georgepisaltu

IMO the not allowed origin should not be able to appear in the pallet code at all. Otherwise pallet functions that ignore the origin could run into problems (like batch for example).

This is how it will work actually. I think once we implement phase 2 of Extrinsic Horizon it will be clearer. When we deprecate ValidateUnsigned, the pallets will not be using the RawOrigin::None origin anymore but their own custom origins for currently unsigned transactions. That's the end goal, but the current approach is still solid, even while we're in this transition period.

I agree, the system::None origin, will still be a valid origin for inherent. But it won't be a valid origin for unsigned transaction anymore. This is great, we can easily separate inherent extrinsics from other extrinsics by looking at the origin.

Then we can remove ProvideInherent::is_inherent and force all extrinsics from origin system::None to be validated by the inherent checks. This should also solve the issue with call such as batch. As inherent calls won't be able to be called from an unsigned transaction.

gui1117 avatar Oct 07 '24 09:10 gui1117

@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 https://github.com/paritytech/polkadot-sdk/issues/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

jsdw avatar Oct 08 '24 15:10 jsdw

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.

gui1117 avatar Oct 08 '24 17:10 gui1117

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 avatar Oct 08 '24 18:10 georgepisaltu

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:

  1. runtime specific
  2. 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.

georgepisaltu avatar Oct 08 '24 18:10 georgepisaltu

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.

gui1117 avatar Oct 08 '24 18:10 gui1117

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

carlosala avatar Oct 08 '24 18:10 carlosala

@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.

georgepisaltu avatar Oct 08 '24 18:10 georgepisaltu

@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

gui1117 avatar Oct 08 '24 19:10 gui1117

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.

carlosala avatar Oct 08 '24 20:10 carlosala

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 avatar Oct 08 '24 20:10 bkchr

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?

bkchr avatar Oct 08 '24 20:10 bkchr