-
Notifications
You must be signed in to change notification settings - Fork 660
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
FRAME: Reintroduce TransactionExtension
as a replacement for SignedExtension
#3685
base: master
Are you sure you want to change the base?
Conversation
Signed-off-by: georgepisaltu <[email protected]>
Signed-off-by: georgepisaltu <[email protected]>
Signed-off-by: georgepisaltu <[email protected]>
Signed-off-by: georgepisaltu <[email protected]>
Signed-off-by: georgepisaltu <[email protected]>
Signed-off-by: georgepisaltu <[email protected]>
TransactionExtension
as a replacement for SignedExtension
Signed-off-by: georgepisaltu <[email protected]>
Signed-off-by: georgepisaltu <[email protected]>
Signed-off-by: georgepisaltu <[email protected]>
Signed-off-by: georgepisaltu <[email protected]>
Signed-off-by: georgepisaltu <[email protected]>
Signed-off-by: georgepisaltu <[email protected]>
Signed-off-by: georgepisaltu <[email protected]>
Signed-off-by: georgepisaltu <[email protected]>
Signed-off-by: georgepisaltu <[email protected]>
Signed-off-by: georgepisaltu <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@georgepisaltu
Looks good so far for the Bridges part. I like the deduplication of the validate vs prepare stuff here. Could you please check: #5906? There are just some small docs nits.
Thanks to FakeDispatchable
, I am able to compile the relayer part.
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 PJS-API
error Unsupported unsigned extrinsic version 5
- full stack:
2024-10-02 15:05:21 REGISTRY: Unknown signed extensions StorageWeightReclaim, CheckMetadataHash found, treating them as no-effect
2024-10-02 15:05:21 API/INIT: RPC methods not decorated: chainHead_v1_body, chainHead_v1_call, chainHead_v1_continue, chainHead_v1_follow, chainHead_v1_header, chainHead_v1_stopOperation, chainHead_v1_storage, chainHead_v1_unfollow, chainHead_v1_unpin, chainSpec_v1_chainName, chainSpec_v1_genesisHash, chainSpec_v1_properties, transactionWatch_v1_submitAndWatch, transactionWatch_v1_unwatch, transaction_v1_broadcast, transaction_v1_stop
2024-10-02 15:05:21 API/INIT: statemine/1015000: Not decorating runtime apis without matching versions: Core/5 (1/2/3/4 known)
2024-10-02 15:05:21 API/INIT: statemine/1015000: Not decorating unknown runtime apis: 0xd7bdd8a272ca0d65/1, 0x6ff52ee858e6c5bd/1, 0x91b1c8b16328eb92/1, 0x9ffb505aa738d69c/1, 0x695c80446b8b3d4e/1, 0xfbc577b9d747efd6/1
Error: createType(ExtrinsicUnknown):: Unsupported unsigned extrinsic version 5
at createTypeUnsafe (/home/bparity/parity/polkadot-sdk/bridges/testing/framework/utils/generate_hex_encoded_call/node_modules/@polkadot/types-create/cjs/create/type.js:54:22)
at TypeRegistry.createTypeUnsafe (/home/bparity/parity/polkadot-sdk/bridges/testing/framework/utils/generate_hex_encoded_call/node_modules/@polkadot/types/cjs/create/registry.js:230:52)
at newFromValue (/home/bparity/parity/polkadot-sdk/bridges/testing/framework/utils/generate_hex_encoded_call/node_modules/@polkadot/types/cjs/extrinsic/Extrinsic.js:25:21)
at decodeExtrinsic (/home/bparity/parity/polkadot-sdk/bridges/testing/framework/utils/generate_hex_encoded_call/node_modules/@polkadot/types/cjs/extrinsic/Extrinsic.js:33:16)
at new GenericExtrinsic (/home/bparity/parity/polkadot-sdk/bridges/testing/framework/utils/generate_hex_encoded_call/node_modules/@polkadot/types/cjs/extrinsic/Extrinsic.js:186:25)
at new Submittable (/home/bparity/parity/polkadot-sdk/bridges/testing/framework/utils/generate_hex_encoded_call/node_modules/@polkadot/api/cjs/submittable/createClass.js:55:13)
at /home/bparity/parity/polkadot-sdk/bridges/testing/framework/utils/generate_hex_encoded_call/node_modules/@polkadot/api/cjs/submittable/createSubmittable.js:7:27
at Object.decorated [as forceCreate] (/home/bparity/parity/polkadot-sdk/bridges/testing/framework/utils/generate_hex_encoded_call/node_modules/@polkadot/api/cjs/base/Decorate.js:491:42)
at /home/bparity/parity/polkadot-sdk/bridges/testing/framework/utils/generate_hex_encoded_call/index.js:99:38
at process.processTicksAndRejections (node:internal/process/task_queues:95:5)
Basically, we use PJS-API in the scripts to construct encoded call bytes for xcm::Transact
and this PJS-API call exactly fails with error above:
const call = api.tx.foreignAssets.forceCreate(JSON.parse(assetId), assetOwnerAccountId, isSufficient, minBalance);
writeHexEncodedBytesToOutput(call.method, outputFile);
I am using testnet runtimes from this branch, so it looks like that actual runtimes returns something to the PJS-API, that causes the error above? I don't know. I will investigate and open an issue with PJS-API.
frame_metadata_hash_extension::CheckMetadataHash::new(false), | ||
cumulus_primitives_storage_weight_reclaim::StorageWeightReclaim::new(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cumulus_primitives_storage_weight_reclaim::StorageWeightReclaim::new(), | |
cumulus_primitives_storage_weight_reclaim::StorageWeightReclaim::new(), |
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. |
Some of those changes could not be suggested in the main: [PR](#3685).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will approve it, with some last remarks. Maybe some can be done in a follow up.
I'm still on the fence when it comes to treat RawOrigin::None
as a "forbidden" origin. This will implicitly change a lot of code and I'm not a very big fan of this. 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).
Also please rename CreateInherent
and do not abuse it for general transactions (aka old unsigned transactions), can probably be a follow up.
T: Send + Sync, | ||
<T as Config>::RuntimeCall: From<frame_system::Call<T>>, | ||
<T as frame_system::Config>::RuntimeCall: Dispatchable<Info = DispatchInfo> + GetDispatchInfo, | ||
<<T as frame_system::Config>::RuntimeCall as Dispatchable>::PostInfo: Default, | ||
<<T as frame_system::Config>::RuntimeCall as Dispatchable>::RuntimeOrigin: | ||
AsSystemOriginSigner<T::AccountId> + AsTransactionAuthorizedOrigin + Clone, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
T: Send + Sync, | |
<T as Config>::RuntimeCall: From<frame_system::Call<T>>, | |
<T as frame_system::Config>::RuntimeCall: Dispatchable<Info = DispatchInfo> + GetDispatchInfo, | |
<<T as frame_system::Config>::RuntimeCall as Dispatchable>::PostInfo: Default, | |
<<T as frame_system::Config>::RuntimeCall as Dispatchable>::RuntimeOrigin: | |
AsSystemOriginSigner<T::AccountId> + AsTransactionAuthorizedOrigin + Clone, | |
<T as Config>::RuntimeCall: From<frame_system::Call<T>> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All of the bounds are required.
These are bounds specified in the extension impl itself.
T: Send + Sync,
<T as frame_system::Config>::RuntimeCall: Dispatchable<Info = DispatchInfo>,
<<T as frame_system::Config>::RuntimeCall as Dispatchable>::RuntimeOrigin:
AsSystemOriginSigner<T::AccountId> + Clone,
This is used to be able to give a PostInfo
object to test_run
<<T as frame_system::Config>::RuntimeCall as Dispatchable>::PostInfo: Default,
/// Weight information for the extensions of this pallet. | ||
type ExtensionsWeightInfo = (); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we not also just merge this into the default weights of the pallet?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could but the system weights don't really need these extension weights defined there. This approach lets us better configure the mocks, especially for tests in E2E extrinsic runs like in frame_executive
. It also doesn't force you to benchmark the weights if you don't care about the extensions. The cost to pay for this modularity is minimal.
} | ||
|
||
/// Interface for creating an inherent. | ||
pub trait CreateInherent<LocalCall>: CreateTransactionBase<LocalCall> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here
CreateInherent
which IMHO should be namedCreateBare
is to create a bare extrinsic from a call only, without transaction extension.
In the future we want to have these "old unsigned tx" be general transactions or not? So, this should be called CreateGeneral
?
fn weight(&self, _call: &Call) -> Weight { | ||
Weight::zero() | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO this function should not provide a default implementation. Otherwise it is probably overseen to implement this, while this is quite important.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, I moved the default impl to an explicit call in impl_tx_ext_default!
and updated the docs.
} | ||
|
||
/// New instance of an old-school signed transaction on extrinsic format version 4. | ||
pub fn new_signed_legacy( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need this? Probably for tests? If yes, can we put it behind cfg(test)
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need this? Probably for tests? If yes, can we put it behind
cfg(test)
?
I think we will need it for the P/K bridge relayer, because Polkadot and Kusama runtime upgrades are asynchronous. At some point, KusamaBridgeHub will be upgraded to the new format (5), while PolkadotBridgeHub will still use the older format (4). Our relayer will need to support both new_signed
and new_signed_legacy
, for example here.
Can we deprecate it and remove it by Q1-Q2/25?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, we should probably just mark it as deprecated until all of the tooling migrates to v5.
ext.encode_to(dest); | ||
}, | ||
_ => { | ||
// unreachable, versions are checked in the constructor |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At least add a debug_assert!
that this is never hit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added an error log instead, that should be visible all the time while not being dangerous in production.
substrate/primitives/runtime/src/generic/unchecked_extrinsic.rs
Outdated
Show resolved
Hide resolved
@@ -0,0 +1,70 @@ | |||
[package] | |||
name = "pallet-verify-signature" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
name = "pallet-verify-signature" | |
name = "frame-verify-signature-extension" |
Not really a pallet. Also doesn't really need the Config
trait etc. This can just be a standalone extension like CheckMetadataHash
extension.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe #3685 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, we use the pallet to make use of the benchmarking infrastructure. I guess we could rename it as well in a follow up, but there is precedent with pallet-transaction-payment
, pallet-asset-tx-payment
and pallet-asset-conversion-tx-payment
.
pub enum Val<T: Config> { | ||
Charge { | ||
tip: BalanceOf<T>, | ||
// who paid the fee | ||
who: T::AccountId, | ||
// transaction fee | ||
fee: BalanceOf<T>, | ||
}, | ||
NoCharge, | ||
} | ||
|
||
/// The info passed between the prepare and post-dispatch steps for the `ChargeAssetTxPayment` | ||
/// extension. | ||
pub enum Pre<T: Config> { | ||
Charge { | ||
tip: BalanceOf<T>, | ||
// who paid the fee | ||
who: T::AccountId, | ||
// imbalance resulting from withdrawing the fee | ||
imbalance: <<T as Config>::OnChargeTransaction as OnChargeTransaction<T>>::LiquidityInfo, | ||
}, | ||
NoCharge { | ||
// weight initially estimated by the extension, to be refunded | ||
refund: Weight, | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Both could just be an Option
that with None
refunds the weight.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one could be an option because we know there is only one weight this extension can have, but for example in the case of both extensions in pallet-asset-tx-payment
and pallet-asset-conversion-tx-payment
this wouldn't work because there would be no way for you to figure out what weight you used before dispatch if you don't provide it in Pre
.
Anyway this is only used internally in the extension code and is just a bit more explicit while being consistent with other extensions, I'd keep as is.
_len: usize, | ||
) -> TransactionValidity { | ||
Ok(ValidTransaction::default()) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just realize that this function is not implemented for tuples. How can people make use of AsTransactionExtension
for signed extensions?
Isn't the convertion invalid?
Maybe we should implement some stuff for tuples, or just remove the AsTransactionExtension
, it seems more error prone than helpful, do I miss something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's ok, it's not meant to be used in tuples. It's just a temporary way to allow you to take your SignedExtension
tuple definition and do an .into()
on it to make it TransactionExtension
compatible while you migrate.
With the fix in polkadot-js/api#5976 and also in |
Signed-off-by: georgepisaltu <[email protected]>
if !auth.first.1.verify(&msg[..], &first_account) { | ||
Err(InvalidTransaction::Custom(100))? | ||
} | ||
if !auth.second.1.verify(&msg[..], &second_account) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is like a fully state-less, offchain, 2/2 multisig
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, the pallet functionality isn't really useful, this example just demonstrates how to use the origin mutation and the inherited_implication. Hopefully this example shows how
TransactionExtension` can be used for some actually useful signature authorization schemes.
@@ -921,6 +942,7 @@ pub mod pallet { | |||
|
|||
/// Total length (in bytes) for all extrinsics put together, for the current block. | |||
#[pallet::storage] | |||
#[pallet::whitelist_storage] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch!
pub use pallet::*; | ||
|
||
#[frame_support::pallet] | ||
pub mod pallet { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was initially a bit lost as to why this has to be a pallet. You need access to a few types like Signature
, but you can also get those from generics on a standalone extension.
But I now see that there is a also the matter of benchmarking. We don't really have the machinery to benchmark extensions outside of a pallet.
Am I missing any other points?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right. It's a benchmarking thing, though I personally like moving the generics from the extension definition to the pallet itself, I think it's cleaner.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no major issues found, but I stumbled upon a lot of open conversations, some of which are still valid. Good to do one final pass on all open conversations before merging.
PR deserves a revival of our old buy-this-man-a berr
label.
Signed-off-by: georgepisaltu <[email protected]>
Signed-off-by: georgepisaltu <[email protected]>
Signed-off-by: georgepisaltu <[email protected]>
Signed-off-by: georgepisaltu <[email protected]>
Signed-off-by: georgepisaltu <[email protected]>
Signed-off-by: georgepisaltu <[email protected]>
Signed-off-by: georgepisaltu <[email protected]>
Signed-off-by: georgepisaltu <[email protected]>
Signed-off-by: georgepisaltu <[email protected]>
Original PR #2280 reverted in #3665
This PR reintroduces the reverted functionality with additional changes, related effort here. Description is copied over from the original PR
First part of Extrinsic Horizon
Introduces a new trait
TransactionExtension
to replaceSignedExtension
. Introduce the idea of transactions which obey the runtime's extensions and have according Extension data (né Extra data) yet do not have hard-coded signatures.Deprecate the terminology of "Unsigned" when used for transactions/extrinsics owing to there now being "proper" unsigned transactions which obey the extension framework and "old-style" unsigned which do not. Instead we have General for the former and Bare for the latter. (Ultimately, the latter will be phased out as a type of transaction, and Bare will only be used for Inherents.)
Types of extrinsic are now therefore:
ValidateUnsigned
(deprecated) and the_bare_compat
bits ofTransactionExtension
(deprecated).ProvideInherent
.TransactionExtension
.TransactionExtension
differs fromSignedExtension
because:pre_dispatch
is renamed toprepare
and need not contain the checks present invalidate
.validate
andprepare
is passed anOrigin
rather than aAccountId
.validate
may pass arbitrary information intoprepare
via a new user-specifiable typeVal
.AdditionalSigned
/additional_signed
is renamed toImplicit
/implicit
. It is encoded for the entire transaction and passed in to each extension as a new argument tovalidate
. This facilitates the ability of extensions to acts as underlying crypto.There is a new
DispatchTransaction
trait which contains only default function impls and is impl'ed for anyTransactionExtension
impler. It provides several utility functions which reduce some of the tedium from usingTransactionExtension
(indeed, none of its regular functions should now need to be called directly).Three transaction version discriminator ("versions") are now permissible (RFC here) in extrinsic version 5:
For the New-school General Transaction, it becomes trivial for authors to publish extensions to the mechanism for authorizing an Origin, e.g. through new kinds of key-signing schemes, ZK proofs, pallet state, mutations over pre-authenticated origins or any combination of the above.
UncheckedExtrinsic
still maintains encode/decode backwards compatibility with extrinsic version 4, where the first byte was encoded as:Now,
UncheckedExtrinsic
contains aPreamble
and the actual call. ThePreamble
describes the type of extrinsic as follows:Code Migration
NOW: Getting it to build
Wrap your
SignedExtension
s inAsTransactionExtension
. This should be accompanied by renaming your aggregate type in line with the new terminology. E.g. Before:After:
You'll also need to alter any transaction building logic to add a
.into()
to make the conversion happen. E.g. Before:After:
SOON: Migrating to
TransactionExtension
Most
SignedExtension
s can be trivially converted to become aTransactionExtension
. There are a few things to know.SignedExtension
, you should now implement two traits individually:TransactionExtensionBase
andTransactionExtension
.fn weight
.TransactionExtensionBase
This trait takes care of anything which is not dependent on types specific to your runtime, most notably
Call
.AdditionalSigned
/additional_signed
is renamed toImplicit
/implicit
.weight
function. If your extension is associated with a pallet, you'll probably want to do this via the pallet's existing benchmarking infrastructure.TransactionExtension
Generally:
pre_dispatch
is nowprepare
and you should not reexecute thevalidate
functionality in there!AsSystemOriginSigner::as_system_origin_signer
.Pre
, calledVal
. This defines data which is passed fromvalidate
intoprepare
. This is important since you should not be duplicating logic fromvalidate
toprepare
, you need a way of passing your working from the former into the latter. This is it.Call
type parameter.Call
is the runtime call type which used to be an associated type; you can just move it to become a type parameter for your trait impl.AccountId
associated type any more. Just remove it.Regarding
validate
:validate
; all can be ignored when migrating fromSignedExtension
.validate
returns a tuple on success; the second item in the tuple is the new ticket typeSelf::Val
which gets passed in toprepare
. If you use any information extracted duringvalidate
(off-chain and on-chain, non-mutating) inprepare
(on-chain, mutating) then you can pass it through with this. For the tuple's last item, just return theorigin
argument.Regarding
prepare
:pre_dispatch
, but there is one change:validate
!!SignedExtension
which was required to run the same checks inpre_dispatch
as invalidate
.)Regarding
post_dispatch
:TransactionExtension
,Pre
is always defined, so the first parameter isSelf::Pre
rather thanOption<Self::Pre>
.If you make use of
SignedExtension::validate_unsigned
orSignedExtension::pre_dispatch_unsigned
, then:origin
isNone
.TransactionExtension
s' data.ValidateUnsigned
can still be used (for now) if you need to be able to construct transactions which contain none of the extension data, however these will be phased out in stage 2 of the Transactions Horizon, so you should consider moving to an extension-centric design.