Unsigned Validation best practices (#5563)

* Configurable Unsigned Priority.

* Use the new builder.

* Fix tests.

* Fix benches.

* Remove unused import.

* Rename for_pallet

substrate/bin/node/runtime/src/lib.rs

@@ -35,7 +35,7 @@ use sp_runtime::{
 	impl_opaque_keys, generic, create_runtime_str,
 };
 use sp_runtime::curve::PiecewiseLinear;
-use sp_runtime::transaction_validity::{TransactionValidity, TransactionSource};
+use sp_runtime::transaction_validity::{TransactionValidity, TransactionSource, TransactionPriority};
 use sp_runtime::traits::{
 	self, BlakeTwo256, Block as BlockT, StaticLookup, SaturatedConversion,
 	ConvertInto, OpaqueKeys,
@@ -361,6 +361,7 @@ impl pallet_staking::Trait for Runtime {
 	type Call = Call;
 	type SubmitTransaction = TransactionSubmitterOf<()>;
 	type MaxNominatorRewardedPerValidator = MaxNominatorRewardedPerValidator;
+	type UnsignedPriority = StakingUnsignedPriority;
 }
 
 parameter_types! {
@@ -540,6 +541,9 @@ impl pallet_sudo::Trait for Runtime {
 
 parameter_types! {
 	pub const SessionDuration: BlockNumber = EPOCH_DURATION_IN_SLOTS as _;
+	pub const ImOnlineUnsignedPriority: TransactionPriority = TransactionPriority::max_value();
+	/// We prioritize im-online heartbeats over phragmen solution submission.
+	pub const StakingUnsignedPriority: TransactionPriority = TransactionPriority::max_value() / 2;
 }
 
 impl pallet_im_online::Trait for Runtime {
@@ -549,6 +553,7 @@ impl pallet_im_online::Trait for Runtime {
 	type SubmitTransaction = TransactionSubmitterOf<Self::AuthorityId>;
 	type SessionDuration = SessionDuration;
 	type ReportUnresponsiveness = Offences;
+	type UnsignedPriority = ImOnlineUnsignedPriority;
 }
 
 impl pallet_offences::Trait for Runtime {

substrate/frame/example-offchain-worker/src/lib.rs

@@ -53,9 +53,10 @@ use sp_runtime::{
 	traits::Zero,
 	transaction_validity::{
 		InvalidTransaction, ValidTransaction, TransactionValidity, TransactionSource,
+		TransactionPriority,
 	},
 };
-use sp_std::{vec, vec::Vec};
+use sp_std::vec::Vec;
 use lite_json::json::JsonValue;
 
 #[cfg(test)]
@@ -106,6 +107,12 @@ pub trait Trait: frame_system::Trait {
 	///
 	/// This ensures that we only accept unsigned transactions once, every `UnsignedInterval` blocks.
 	type UnsignedInterval: Get<Self::BlockNumber>;
+
+	/// A configuration for base priority of unsigned transactions.
+	///
+	/// This is exposed so that it can be tuned for particular runtime, when
+	/// multiple pallets send unsigned transactions.
+	type UnsignedPriority: Get<TransactionPriority>;
 }
 
 decl_storage! {
@@ -537,32 +544,33 @@ impl<T: Trait> frame_support::unsigned::ValidateUnsigned for Module<T> {
 				.map(|price| if &price > new_price { price - new_price } else { new_price - price })
 				.unwrap_or(0);
 
-			Ok(ValidTransaction {
+			ValidTransaction::with_tag_prefix("ExampleOffchainWorker")
 				// We set base priority to 2**20 to make sure it's included before any other
 				// transactions in the pool. Next we tweak the priority depending on how much
 				// it differs from the current average. (the more it differs the more priority it
 				// has).
-				priority: (1 << 20) + avg_price as u64,
+				.priority(T::UnsignedPriority::get().saturating_add(avg_price as _))
 				// This transaction does not require anything else to go before into the pool.
 				// In theory we could require `previous_unsigned_at` transaction to go first,
 				// but it's not necessary in our case.
-				requires: vec![],
+				//.and_requires()
+
 				// We set the `provides` tag to be the same as `next_unsigned_at`. This makes
 				// sure only one transaction produced after `next_unsigned_at` will ever
 				// get to the transaction pool and will end up in the block.
 				// We can still have multiple transactions compete for the same "spot",
 				// and the one with higher priority will replace other one in the pool.
-				provides: vec![codec::Encode::encode(&(KEY_TYPE.0, next_unsigned_at))],
+				.and_provides(next_unsigned_at)
 				// The transaction is only valid for next 5 blocks. After that it's
 				// going to be revalidated by the pool.
-				longevity: 5,
+				.longevity(5)
 				// It's fine to propagate that transaction to other peers, which means it can be
 				// created even by nodes that don't produce blocks.
 				// Note that sometimes it's better to keep it for yourself (if you are the block
 				// producer), since for instance in some schemes others may copy your solution and
 				// claim a reward.
-				propagate: true,
-			})
+				.propagate(true)
+				.build()
 		} else {
 			InvalidTransaction::Call.into()
 		}

substrate/frame/example-offchain-worker/src/tests.rs

@@ -94,6 +94,7 @@ impl frame_system::offchain::CreateTransaction<Test, Extrinsic> for Test {
 parameter_types! {
 	pub const GracePeriod: u64 = 5;
 	pub const UnsignedInterval: u64 = 128;
+	pub const UnsignedPriority: u64 = 1 << 20;
 }
 
 impl Trait for Test {
@@ -103,6 +104,7 @@ impl Trait for Test {
 	type SubmitUnsignedTransaction = SubmitTransaction;
 	type GracePeriod = GracePeriod;
 	type UnsignedInterval = UnsignedInterval;
+	type UnsignedPriority = UnsignedPriority;
 }
 
 type Example = Module<Test>;

substrate/frame/im-online/src/lib.rs

@@ -247,6 +247,12 @@ pub trait Trait: frame_system::Trait + pallet_session::historical::Trait {
 			IdentificationTuple<Self>,
 			UnresponsivenessOffence<IdentificationTuple<Self>>,
 		>;
+
+	/// A configuration for base priority of unsigned transactions.
+	///
+	/// This is exposed so that it can be tuned for particular runtime, when
+	/// multiple pallets send unsigned transactions.
+	type UnsignedPriority: Get<TransactionPriority>;
 }
 
 decl_event!(
@@ -658,13 +664,14 @@ impl<T: Trait> frame_support::unsigned::ValidateUnsigned for Module<T> {
 				return InvalidTransaction::BadProof.into();
 			}
 
-			Ok(ValidTransaction {
-				priority: TransactionPriority::max_value(),
-				requires: vec![],
-				provides: vec![(current_session, authority_id).encode()],
-				longevity: TryInto::<u64>::try_into(T::SessionDuration::get() / 2.into()).unwrap_or(64_u64),
-				propagate: true,
-			})
+			ValidTransaction::with_tag_prefix("ImOnline")
+				.priority(T::UnsignedPriority::get())
+				.and_provides((current_session, authority_id))
+				.longevity(TryInto::<u64>::try_into(
+					T::SessionDuration::get() / 2.into()
+				).unwrap_or(64_u64))
+				.propagate(true)
+				.build()
 		} else {
 			InvalidTransaction::Call.into()
 		}

substrate/frame/im-online/src/mock.rs

@@ -160,6 +160,10 @@ impl pallet_authorship::Trait for Runtime {
 	type EventHandler = ImOnline;
 }
 
+parameter_types! {
+	pub const UnsignedPriority: u64 = 1 << 20;
+}
+
 impl Trait for Runtime {
 	type AuthorityId = UintAuthorityId;
 	type Event = ();
@@ -167,6 +171,7 @@ impl Trait for Runtime {
 	type SubmitTransaction = SubmitTransaction;
 	type ReportUnresponsiveness = OffenceHandler;
 	type SessionDuration = Period;
+	type UnsignedPriority = UnsignedPriority;
 }
 
 /// Im Online module.

substrate/frame/session/benchmarking/src/mock.rs

@@ -146,6 +146,7 @@ pallet_staking_reward_curve::build! {
 parameter_types! {
 	pub const RewardCurve: &'static sp_runtime::curve::PiecewiseLinear<'static> = &I_NPOS;
 	pub const MaxNominatorRewardedPerValidator: u32 = 64;
+	pub const UnsignedPriority: u64 = 1 << 20;
 }
 
 pub type Extrinsic = sp_runtime::testing::TestXt<Call, ()>;
@@ -174,6 +175,7 @@ impl pallet_staking::Trait for Test {
 	type Call = Call;
 	type SubmitTransaction = SubmitTransaction;
 	type MaxNominatorRewardedPerValidator = MaxNominatorRewardedPerValidator;
+	type UnsignedPriority = UnsignedPriority;
 }
 
 impl crate::Trait for Test {}

substrate/frame/staking/src/lib.rs

@@ -292,7 +292,7 @@ use sp_runtime::{
 	},
 	transaction_validity::{
 		TransactionValidityError, TransactionValidity, ValidTransaction, InvalidTransaction,
-		TransactionSource,
+		TransactionSource, TransactionPriority,
 	},
 };
 use sp_staking::{
@@ -782,6 +782,12 @@ pub trait Trait: frame_system::Trait {
 	/// For each validator only the `$MaxNominatorRewardedPerValidator` biggest stakers can claim
 	/// their reward. This used to limit the i/o cost for the nominator payout.
 	type MaxNominatorRewardedPerValidator: Get<u32>;
+
+	/// A configuration for base priority of unsigned transactions.
+	///
+	/// This is exposed so that it can be tuned for particular runtime, when
+	/// multiple pallets send unsigned transactions.
+	type UnsignedPriority: Get<TransactionPriority>;
 }
 
 /// Mode of era-forcing.
@@ -3224,24 +3230,24 @@ impl<T: Trait> frame_support::unsigned::ValidateUnsigned for Module<T> {
 				era,
 			);
 
-			Ok(ValidTransaction {
+			ValidTransaction::with_tag_prefix("StakingOffchain")
 				// The higher the score[0], the better a solution is.
-				priority: score[0].saturated_into(),
-				// no requires.
-				requires: vec![],
+				.priority(T::UnsignedPriority::get().saturating_add(score[0].saturated_into()))
 				// Defensive only. A single solution can exist in the pool per era. Each validator
 				// will run OCW at most once per era, hence there should never exist more than one
 				// transaction anyhow.
-				provides: vec![("StakingOffchain", era).encode()],
+				.and_provides(era)
 				// Note: this can be more accurate in the future. We do something like
 				// `era_end_block - current_block` but that is not needed now as we eagerly run
 				// offchain workers now and the above should be same as `T::ElectionLookahead`
 				// without the need to query more storage in the validation phase. If we randomize
 				// offchain worker, then we might re-consider this.
-				longevity: TryInto::<u64>::try_into(T::ElectionLookahead::get()).unwrap_or(DEFAULT_LONGEVITY),
+				.longevity(TryInto::<u64>::try_into(
+						T::ElectionLookahead::get()).unwrap_or(DEFAULT_LONGEVITY)
+				)
 				// We don't propagate this. This can never the validated at a remote node.
-				propagate: false,
-			})
+				.propagate(false)
+				.build()
 		} else {
 			InvalidTransaction::Call.into()
 		}

substrate/frame/staking/src/mock.rs

@@ -272,6 +272,7 @@ parameter_types! {
 	pub const BondingDuration: EraIndex = 3;
 	pub const RewardCurve: &'static PiecewiseLinear<'static> = &I_NPOS;
 	pub const MaxNominatorRewardedPerValidator: u32 = 64;
+	pub const UnsignedPriority: u64 = 1 << 20;
 }
 
 impl Trait for Test {
@@ -293,6 +294,7 @@ impl Trait for Test {
 	type Call = Call;
 	type SubmitTransaction = SubmitTransaction;
 	type MaxNominatorRewardedPerValidator = MaxNominatorRewardedPerValidator;
+	type UnsignedPriority = UnsignedPriority;
 }
 
 pub type Extrinsic = TestXt<Call, ()>;

substrate/frame/staking/src/tests.rs

@@ -3165,7 +3165,7 @@ mod offchain_phragmen {
 					&inner,
 				),
 				TransactionValidity::Ok(ValidTransaction {
-					priority: 1125, // the proposed slot stake.
+					priority: (1 << 20) + 1125, // the proposed slot stake.
 					requires: vec![],
 					provides: vec![("StakingOffchain", active_era()).encode()],
 					longevity: 3,

substrate/primitives/runtime/src/transaction_validity.rs

@@ -264,6 +264,17 @@ impl Default for ValidTransaction {
 }
 
 impl ValidTransaction {
+	/// Initiate `ValidTransaction` builder object with a particular prefix for tags.
+	///
+	/// To avoid conflicts between different parts in runtime it's recommended to build `requires`
+	/// and `provides` tags with a unique prefix.
+	pub fn with_tag_prefix(prefix: &'static str) -> ValidTransactionBuilder {
+		ValidTransactionBuilder {
+			prefix: Some(prefix),
+			validity: Default::default(),
+		}
+	}
+
 	/// Combine two instances into one, as a best effort. This will take the superset of each of the
 	/// `provides` and `requires` tags, it will sum the priorities, take the minimum longevity and
 	/// the logic *And* of the propagate flags.
@@ -278,6 +289,104 @@ impl ValidTransaction {
 	}
 }
 
+/// `ValidTransaction` builder.
+///
+///
+/// Allows to easily construct `ValidTransaction` and most importantly takes care of
+/// prefixing `requires` and `provides` tags to avoid conflicts.
+#[derive(Default, Clone, RuntimeDebug)]
+pub struct ValidTransactionBuilder {
+	prefix: Option<&'static str>,
+	validity: ValidTransaction,
+}
+
+impl ValidTransactionBuilder {
+	/// Set the priority of a transaction.
+	///
+	/// Note that the final priority for `FRAME` is combined from all `SignedExtension`s.
+	/// Most likely for unsigned transactions you want the priority to be higher
+	/// than for regular transactions. We recommend exposing a base priority for unsigned
+	/// transactions as a runtime module parameter, so that the runtime can tune inter-module
+	/// priorities.
+	pub fn priority(mut self, priority: TransactionPriority) -> Self {
+		self.validity.priority = priority;
+		self
+	}
+
+	/// Set the longevity of a transaction.
+	///
+	/// By default the transaction will be considered valid forever and will not be revalidated
+	/// by the transaction pool. It's recommended though to set the longevity to a finite value
+	/// though. If unsure, it's also reasonable to expose this parameter via module configuration
+	/// and let the runtime decide.
+	pub fn longevity(mut self, longevity: TransactionLongevity) -> Self {
+		self.validity.longevity = longevity;
+		self
+	}
+
+	/// Set the propagate flag.
+	///
+	/// Set to `false` if the transaction is not meant to be gossiped to peers. Combined with
+	/// `TransactionSource::Local` validation it can be used to have special kind of
+	/// transactions that are only produced and included by the validator nodes.
+	pub fn propagate(mut self, propagate: bool) -> Self {
+		self.validity.propagate = propagate;
+		self
+	}
+
+	/// Add a `TransactionTag` to the set of required tags.
+	///
+	/// The tag will be encoded and prefixed with module prefix (if any).
+	/// If you'd rather add a raw `require` tag, consider using `#combine_with` method.
+	pub fn and_requires(mut self, tag: impl Encode) -> Self {
+		self.validity.requires.push(match self.prefix.as_ref() {
+			Some(prefix) => (prefix, tag).encode(),
+			None => tag.encode(),
+		});
+		self
+	}
+
+	/// Add a `TransactionTag` to the set of provided tags.
+	///
+	/// The tag will be encoded and prefixed with module prefix (if any).
+	/// If you'd rather add a raw `require` tag, consider using `#combine_with` method.
+	pub fn and_provides(mut self, tag: impl Encode) -> Self {
+		self.validity.provides.push(match self.prefix.as_ref() {
+			Some(prefix) => (prefix, tag).encode(),
+			None => tag.encode(),
+		});
+		self
+	}
+
+	/// Augment the builder with existing `ValidTransaction`.
+	///
+	/// This method does add the prefix to `require` or `provides` tags.
+	pub fn combine_with(mut self, validity: ValidTransaction) -> Self {
+		self.validity = core::mem::take(&mut self.validity).combine_with(validity);
+		self
+	}
+
+	/// Finalize the builder and produce `TransactionValidity`.
+	///
+	/// Note the result will always be `Ok`. Use `Into` to produce `ValidTransaction`.
+	pub fn build(self) -> TransactionValidity {
+		self.into()
+	}
+}
+
+impl From<ValidTransactionBuilder> for TransactionValidity {
+	fn from(builder: ValidTransactionBuilder) -> Self {
+		Ok(builder.into())
+	}
+}
+
+impl From<ValidTransactionBuilder> for ValidTransaction {
+	fn from(builder: ValidTransactionBuilder) -> Self {
+		builder.validity
+	}
+}
+
+
 #[cfg(test)]
 mod tests {
 	use super::*;
@@ -301,4 +410,26 @@ mod tests {
 		// decode back
 		assert_eq!(TransactionValidity::decode(&mut &*encoded), Ok(v));
 	}
+
+	#[test]
+	fn builder_should_prefix_the_tags() {
+		const PREFIX: &str = "test";
+		let a: ValidTransaction = ValidTransaction::with_tag_prefix(PREFIX)
+			.and_requires(1)
+			.and_requires(2)
+			.and_provides(3)
+			.and_provides(4)
+			.propagate(false)
+			.longevity(5)
+			.priority(3)
+			.priority(6)
+			.into();
+		assert_eq!(a, ValidTransaction {
+			propagate: false,
+			longevity: 5,
+			priority: 6,
+			requires: vec![(PREFIX, 1).encode(), (PREFIX, 2).encode()],
+			provides: vec![(PREFIX, 3).encode(), (PREFIX, 4).encode()],
+		});
+	}
 }