paritytech / polkadot-sdk

The Parity Polkadot Blockchain SDK
https://polkadot.network/
1.78k stars 635 forks source link

Extrinsic Horizon #2415

Open gavofyork opened 9 months ago

gavofyork commented 9 months ago

Goals

Explanation

Stage 1: TransactionExtension

New transaction type - General

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.

Types of extrinsic are now therefore:

Three extrinsic version discriminators ("versions") are now permissible, according to RFC84:

Origin mutation at the extension level

The introduction of the TransactionExtension interface over SignedExtension would now allow any origin, not just the signed/unsigned origin supported in frame_system. Furthermore, the origin can be mutated by any extension in the pipeline during the validation step, as now extensions receive a RuntimeOrigin as input instead of an AccountId, which is the de facto frame_system origin type.

Authorization of non-standard origins

With a TransactionExtension, for both New-school General and Old-school Signed Transactions, 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.

Free transactions

As of today, there are 2 specific requirements for users who want to run any (signed) transaction:

  1. payment for the nonce, either through acquiring ED or having some on-chain entity provide for your account
  2. payment for the transaction fees, which even if ultimately refunded, a user still needs to have enough funds to pay in the worst case scenario

Nonce storage is a concept inherently tied to the account model and authorization currently in use in substrate based chains. However, with the introduction of TransactionExtensions, any form of authorization that fits in the validate + prepare flow and mutates the origin is allowed. This means that the nonce will be checked and incremented only for traditional signed origins.

Charging transaction fees makes sense only when the origin is an account, as only an account can hold currency. Fees protect the chain against spam, but an arbitrary origin is incapable of paying fees. Therefore, as with the nonce, transaction fees should be charged only when the origin is a traditional signed origin.

Therefore, the responsibility of protection against replay attacks (nonce increments), Sybil resistance (storage of nonce on chain) and on-chain spam (transaction fees) fall on the extension authorizing the non-standard origin.

This implies a much greater compute burden on the validation phase of extensions, which is also variable depending on what authorization is needed for a particular call, so the current weightless model of extensions cannot work. TransactionExtensions now provide a fn weight function to factor in the cost of this computation. Also, the validation done by an extension followed by authorization through origin mutation should generally mean that the call requiring said special authorization does not do the same checks again during the actual dispatch of the call.

N.B. It becomes apparent that the use case for what are now unsigned transactions, which are naturally free and currently handled via ValidateUnsigned, fits nicely into non-standard origin types validated by the TransactionExtension interface.

Stage 2: Begin removal of concept of Unsigned transactions:

Unsigned transactions should be validated by the TransactionExtension pipeline just like any other transaction. This will allow "bare" transactions to just mean inherents.

Current state of ValidateUnsigned

Each extrinsic call that is now validated using ValidateUnsigned instead of the SignedExtension pipeline has a particular set of arbitrary checks that are performed, described by the implementation of ValidateUnsigned::validate_unsigned.

Moving the logic of ValidateUnsigned into an instance of TransactionExtension

In principle, the logic of each ValidateUnsigned::validate_unsigned and ValidateUnsigned::pre_dispatch can be done in TransactionExtension::validate and TransactionExtension::prepare respectively. The call validation logic can be injected into the functions in TransactionExtension similarly to how SkipCheckIfFeeless is using the fn feeless_if closure decorator over pallet calls.

This validation logic must:

Creating bespoke origins for each call within a pallet that wants to use this new "unsigned" interface can be done during the pallet macro expansion. It could either be a single origin per pallet with a predefined name, such as Origin::Preauthorized, or it could be one origin variant for each call that uses the interface, named after the call name, e.g. fn foo_bar(origin: OriginFor<T>) -> DispatchResult -> Origin::FooBar.

In short, an extension that validates calls that were previously unsigned now validates free calls that have a non-standard origin. The extension should run the validation logic defined in the pallet call closure and mutate the origin to a pre-approved pallet origin, to be later checked during dispatch. Most of this can be hidden away behind procedural macros so users don't have to manually define and interact these extensions and origins.

All transactions have an origin

With the change to fee payment and ValidateUnsigned, all valid transactions must reach the call dispatch phase of extrinsic application with some origin, as no origin would mean that nobody authorized this transaction.

In order to do this, there could either be an extension in the pipeline invalidating transactions without an origin. The extension could be anywhere in the pipeline as long as there are no possible origin mutations after its own validate cycle, so somewhere towards the end of it. Alternatively, this can be enforced in the implementation of Applyable::apply, but it would need special handling when validating transaction for the transaction pool and, given that extensions have versioning now through RFC99, it's probably cleaner overall to have it as an extension, as any changes to this logic would be reflected in a version bump.

Other action items

Stage 2.5: Integrated semantic description

Transactions should include a metadata extension VerifyMetadata

Stage 3 PR: New APIs, deprecate old-school.

All extrinsics are now validated either by ProvideInherent (which is an aggregated type of the runtime) or TransactionExtension (which is an explicitly configured type of the runtime).

Bare transactions are removed, so:

This leaves us with three types/versions of Extrinsics:

Action items:

Remove deprecated items:

Tidy up language:

Action items done in PR3685

Move test code to regular types:

Remove all needless reliance on traits with a defunct model of transactions:

Stage 4: Remove rest of deprecated API

Other notes:

bkchr commented 9 months ago

Transactions should include a metadata extension VerifyMetadata

Wouldn't this also not just be a TransactionExtension?

ggwpez commented 9 months ago

Remove or migrate uses of validate_unsigned: Frontier: migrate. Claims: remove.

What is the criteria for migration vs removal in the other pallets that use it?

gavofyork commented 9 months ago

If the feature is out of use (as I expected claims was), then they can be deprecated and removed. But since claim was actually used only 5 days ago last, I guess we ought to keep it around 😕.

bkchr commented 9 months ago

We should also change SignedPayload to just always hash the data that we sign/verify. I don't really see the advantage of doing it this way.

bkchr commented 6 months ago
    fn validate(
        &self,
        origin: OriginOf<Call>,
        call: &Call,
        info: &DispatchInfoOf<Call>,
        len: usize,
        context: &mut Context,
        self_implicit: Self::Implicit,
        inherited_implication: &impl Encode,
    ) -> ValidateResult<Self::Val, Call>;

    pub type ValidateResult<Val, Call> =
        Result<(ValidTransaction, Val, OriginOf<Call>), TransactionValidityError>;

Instead of this, I think it would be better to change the interface to:

    fn validate(
        &self,
        origin: Option<OriginOf<Call>>,
                signer: Option<Account>,
        call: &Call,
        info: &DispatchInfoOf<Call>,
        len: usize,
        context: &mut Context,
        self_implicit: Self::Implicit,
        inherited_implication: &impl Encode,
    ) -> ValidateResult<Self::Val, Call>;

    pub type ValidateResult<Val, Call> =
        Result<(ValidTransaction, Val, Option<OriginOf<Call>>), TransactionValidityError>;

So, basically we would make the origin an Option and go back to passing the signer. This would bring us the advantage of chaining logic, but first let's look at the current structure. If I want to use the SkipFeelessIf with the TransactionPayment extension, I need to nest them. Aka SkipFeelessIf<TransactionPayment>. Now let's assume we actually want to have General transactions that are created by an offchain worker for example. The problem would be that we need to skip at least TransactionPayment. So, we would need to nest again CustomExt<SkipFeelessIf<TransactionPayment>>>. (We can probably continue this for multiple different extensions).

IMO it would be better to do this like:

Exts = (
     CustomExt,
     SkipFeelessIf,
     TransactionPayment,
);

At the beginning origin would be None. The extensions can set the origin to Some, which would signal the following extensions like TransactionPayment to not try to deduct any fees. If at the end of executing all extensions, origin would still be None, we would reject the transaction. This way we would also get back the old behavior of rejecting unsigned/general transactions "by default".

Polkadot-Forum commented 4 months ago

This issue has been mentioned on Polkadot Forum. There might be relevant details there:

https://forum.polkadot.network/t/2024-04-23-technical-fellowship-opendev-call/7592/1

gui1117 commented 1 month ago

Now let's assume we actually want to have General transactions that are created by an offchain worker for example. The problem would be that we need to skip at least TransactionPayment. So, we would need to nest again CustomExt<SkipFeelessIf<TransactionPayment>>>. (We can probably continue this for multiple different extensions).

Maybe TransactionPayment should accept with no-op any origin which is not a signed origin.

CustomExt would transform the None origin into a pallet origin it relates to: PalletA::Origin::OffchainOrigin. And the triggered call would just ensure the origin is PalletA::Origin::OffchainOrigin.

This would allow different pipeline to be concurrent. Signed origin pipeline and RuntimeOrigin::PalletA(PalletA::Origin::OffchainOrigin) pipeline would be concurrent.

And we can have a DenyNone transaction extension as a final transaction extension. To avoid General Transaction to be able to call the deprecated unsigned transaction logics.

Alternatively we could also have some more complex pipeline structure with a top-level transaction extension which redirect to different pipeline depending on the call variant and the origin variant (signed or none).

Both seem doable with the current PR design.

gui1117 commented 1 week ago

Now that transaction extension get a lot of custom logic, it could be great to be able to give custom error for invalid transaction.

There is InvalidTransaction::Custom(u8) but it is rather quite limited.

I think we should introduce the same variant as in DispatchError: DispatchError::Module(ModuleError).

e.g.:

@@ -82,6 +83,8 @@ pub enum InvalidTransaction {
        MandatoryValidation,
        /// The sending address is disabled or known to be invalid.
        BadSigner,
+       /// A custom error in a module
+       Module(ModuleError),
 }

Then in the pallet we can just use the pallet error. It is already a type which cover multiple situation: the many different call of a pallet, so for the transaction extension situation, even if it is a different situation in nature, I think it is ok.

bkchr commented 1 week ago

And we can have a DenyNone transaction extension as a final transaction extension. To avoid General Transaction to be able to call the deprecated unsigned transaction logics.

This sounds like a footgun. We should keep the logic that an unknown origin is rejected by default.

georgepisaltu commented 1 week ago

an unknown origin is rejected by default

This can be implemented in the Applyable::apply and Applyable::validate logic of CheckedExtrinsic by rejecting transactions without an origin when trying to dispatch the call.

However, we should provide a DenyNone extension as part of substrate because other implementers might be using transaction extensions but some other extrinsic type with a custom Applyable impl.

bkchr commented 6 days ago

However, we should provide a DenyNone extension as part of substrate because other implementers might be using transaction extensions but some other extrinsic type with a custom Applyable impl.

I bet with you that they would never use this. This should be really not optional. Rejecting None should be the default. Then don't use Option to pass the origin and instead use a custom enum that can represent this.

gui1117 commented 6 days ago

I agree we don't need DenyNone and we should make it by design.

I think we can make CheckedExtrinsic deny the None origin after the transaction extension checks.

So that bare extrinsic are dispatched with None, and signed and general transactions are validated with transaction extension then if the returned origin is none it is invalid otherwise dispatched with the returned origin.

transaction extension is responsible to validate the transaction by giving it a sensible origin different from None.

georgepisaltu commented 4 days ago

I will implement the check in Applyable::apply and Applyable::validate logic of CheckedExtrinsic and add the necessary documentation around the trait to warn custom impls that they need to check the origin before call dispatch when applying extrinsics.