Unsigned Validation best practices (paritytech#5563)

* Configurable Unsigned Priority.

* Use the new builder.

* Fix tests.

* Fix benches.

* Remove unused import.

* Rename for_pallet

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 {

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()
 		}

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>;

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()
 		}

frame/im-online/src/mock.rs

@@ -160,13 +160,18 @@ 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 = ();
 	type Call = Call;
 	type SubmitTransaction = SubmitTransaction;
 	type ReportUnresponsiveness = OffenceHandler;
 	type SessionDuration = Period;
+	type UnsignedPriority = UnsignedPriority;
 }
 
 /// Im Online module.

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 {}

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()
 		}

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, ()>;

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,