Introduce TransactionExtensionPipeline to use instead of tuple for pipeline with more than 12 elements

[gui1117]

Fix #6569

Introduce a new type TransactionExtensionPipeline that has 32 generics that default to ().
This type accepts up to 32 transaction extensions which is better than the limit of 12 for a tuple.

As stated in the linked issue, the issue with tuple is that tuple of tuple is not the same as a single tuple. The inherited implication is changing. But it is impossible to find out the order in the metadata because the metadata is just a vec of transaction extension metadata.

[gui1117]

another solution is to implement a wrapper of tuple, that manually implements Debug, PartialEq, Eq, and also implements TransactionExtension.

This way the type may look less like a hack.

[georgepisaltu]

This would work, we just need to make sure we expose it properly in the metadata so that it's not transparent, right?

[gui1117]

I am not sure I understand, the metadata is anyway just a vec of TransactionExtensionMetadata.

What I am thinking is we can implement do either:

• like this PR currently, a type with 32 generics being () by default.
• or a type with only 1 generic, and do impl<A> Debug for Pipeline<(A,)> .., impl<A, B> Debug for Pipeline<(A, B)> .. etc...
  Both implementation would be transparent

[georgepisaltu]

Looks good, can you also add extra tests with the new TransactionExtensionPipeline in substrate/frame/support/src/dispatch.rs in the extension_weight_tests module to make sure the weight calculation and refund is working properly?

[gui1117]

TransactionExtensionPipeline take 32 generics that defaults to (), so I inlined methods here so hopefully the code gets optimized by the compiler.

[gui1117]

Looks good, can you also add extra tests with the new TransactionExtensionPipeline in substrate/frame/support/src/dispatch.rs in the extension_weight_tests module to make sure the weight calculation and refund is working properly?

I added test for the refund of weights in post_dispatch and in bare_post_dispatch (there was already a test for post_dispatch_details) in 56854f7
Do you think we need more?

EDIT: I will also do the test you suggest

[gui1117]

Looks good, can you also add extra tests with the new TransactionExtensionPipeline in substrate/frame/support/src/dispatch.rs in the extension_weight_tests module to make sure the weight calculation and refund is working properly?

I added the test in b3e4503

[paritytech-workflow-stopper]

All GitHub workflows were cancelled due to failure one of the required jobs.
Failed workflow url: https://github.com/paritytech/polkadot-sdk/actions/runs/12211927288
Failed job name: test-doc

[georgepisaltu]

LGTM

[gui1117]

For the record another solution is to add a new validate_pipeline function with a different signature:

	fn validate_pipeline(
		&self,
		origin: <Call as Dispatchable>::RuntimeOrigin,
		call: &Call,
		info: &DispatchInfoOf<Call>,
		len: usize,
		self_implicit: Self::Implicit,
		inherited_implicit_implication: &impl Encode,
		inherited_explicit_implication: &impl Encode,
		inherited_final_implication: &impl Encode,
		source: TransactionSource,
	) -> Result<
		(ValidTransaction, Self::Val, <Call as Dispatchable>::RuntimeOrigin),
		TransactionValidityError,
	> {
		let valid = ValidTransaction::default();
		let val = ();
		let following_explicit_implications = for_tuples!( ( #( &self.Tuple ),* ) );
		let following_implicit_implications = self_implicit;

		for_tuples!(#(
			// Implication of this pipeline element not relevant for later items, so we pop it.
			let (_item, following_explicit_implications) = following_explicit_implications.pop_front();
			let (item_implicit, following_implicit_implications) = following_implicit_implications.pop_front();
			let (item_valid, item_val, origin) = {
				let implications = (
					// The first is the implications born of the fact we return the mutated
					// origin.
					inherited_final_implication,
					// This is the explicitly made implication born of the fact the new origin is
					// passed into the next items in this pipeline-tuple.
					&following_explicit_implications,
					inherited_explicit_implication,
					// This is the implicitly made implication born of the fact the new origin is
					// passed into the next items in this pipeline-tuple.
					&following_implicit_implications,
					inherited_implicit_implication,
				);
				Tuple.validate(origin, call, info, len, item_implicit, final, (following_explicit, inheritied_explicit), (following_implicit, inherited_implicit), source)?
			};
			let valid = valid.combine_with(item_valid);
			let val = val.push_back(item_val);
		)* );
		Ok((valid, val, origin))
	}

TransactionExtension trait already has some invalid associated items for the pipeline: like IDENTIFIER is only valid for a leaf but not for a pipeline, similarly validate would return always invalid for a pipeline and would only be valid for a leaf.

I mention this solution because I see in the doc people likes to have tuples of tuples, like here in the doc:

polkadot-sdk/docs/sdk/packages/guides/first-runtime/src/lib.rs

Lines 64 to 69 in 5abdc5c

	pub(super) type SignedExtra = (
	// `frame` already provides all the signed extensions from `frame-system`. We just add the
	// one related to tx-payment here.
	frame::runtime::types_common::SystemTransactionExtensionsOf<Runtime>,
	pallet_transaction_payment::ChargeTransactionPayment<Runtime>,
	);

And also bridges stuff do it.

EDIT: we can 2 functions or just one general function with all the 3 arguments and people will have to use it correctly.

[gui1117]

I looked deeper into the alternative, I think the alternative is better and cleaner. I implemented in #7028

I will consider this PR superseded by #7028