Merge branch 'master' into oty-prdoc-check

polkadot/node/network/bridge/src/rx/mod.rs

@@ -962,6 +962,21 @@ fn update_our_view<Context>(
 		)
 	};
 
+	let our_view = OurView::new(
+		live_heads.iter().take(MAX_VIEW_HEADS).cloned().map(|a| (a.hash, a.span)),
+		finalized_number,
+	);
+
+	dispatch_validation_event_to_all_unbounded(
+		NetworkBridgeEvent::OurViewChange(our_view.clone()),
+		ctx.sender(),
+	);
+
+	dispatch_collation_event_to_all_unbounded(
+		NetworkBridgeEvent::OurViewChange(our_view),
+		ctx.sender(),
+	);
+
 	let v1_validation_peers =
 		filter_by_peer_version(&validation_peers, ValidationVersion::V1.into());
 	let v1_collation_peers = filter_by_peer_version(&collation_peers, CollationVersion::V1.into());
@@ -1007,21 +1022,6 @@ fn update_our_view<Context>(
 		metrics,
 		notification_sinks,
 	);
-
-	let our_view = OurView::new(
-		live_heads.iter().take(MAX_VIEW_HEADS).cloned().map(|a| (a.hash, a.span)),
-		finalized_number,
-	);
-
-	dispatch_validation_event_to_all_unbounded(
-		NetworkBridgeEvent::OurViewChange(our_view.clone()),
-		ctx.sender(),
-	);
-
-	dispatch_collation_event_to_all_unbounded(
-		NetworkBridgeEvent::OurViewChange(our_view),
-		ctx.sender(),
-	);
 }
 
 // Handle messages on a specific v1 peer-set. The peer is expected to be connected on that

prdoc/pr_5356.prdoc

@@ -0,0 +1,18 @@
+# Schema: Polkadot SDK PRDoc Schema (prdoc) v1.0.0
+# See doc at https://raw.githubusercontent.com/paritytech/polkadot-sdk/master/prdoc/schema_user.json
+
+title: Fix OurViewChange small race
+
+doc:
+  - audience: Node Dev
+    description: |
+      Always queue OurViewChange event before we send view changes to our peers, because otherwise we risk 
+      the peers sending us a message that can be processed by our subsystems before OurViewChange.
+      Normally, this is not really a problem because the latency of the ViewChange we send to our peers 
+      is way higher than that of our subsystem processing OurViewChange, however on testnets like versi 
+      where CPUs are sometimes overcommitted this race gets triggered occasionally, so let's fix it by 
+      sending the messages in the right order.
+
+crates:
+  - name: polkadot-network-bridge
+    bump: minor

prdoc/pr_5359.prdoc

@@ -0,0 +1,21 @@
+title: Make ticket non-optional and add ensure_successful method to Consideration trait
+
+doc:
+  - audience: Runtime Dev
+    description: |
+      Make ticket non-optional and add ensure_successful method to Consideration trait.
+
+      Reverts the optional return ticket type for the new function introduced in 
+      [polkadot-sdk/4596](https://github.com/paritytech/polkadot-sdk/pull/4596) and adds a helper
+      `ensure_successful` function for the runtime benchmarks.
+      Since the existing FRAME pallet represents zero cost with a zero balance type rather than 
+      `None` in an option, maintaining the ticket type as a non-optional balance is beneficial 
+      for backward compatibility and helps avoid unnecessary migrations.
+
+crates:
+  - name: frame-support
+    bump: major
+  - name: pallet-preimage
+    bump: major
+  - name: pallet-balances
+    bump: patch

substrate/frame/balances/src/tests/fungible_tests.rs

@@ -518,24 +518,25 @@ fn freeze_consideration_works() {
 			let who = 4;
 			// freeze amount taken somewhere outside of our (Consideration) scope.
 			let extend_freeze = 15;
+			let zero_ticket = Consideration::new(&who, Footprint::from_parts(0, 0)).unwrap();
 			assert_eq!(Balances::balance_frozen(&TestId::Foo, &who), 0);
 
-			let ticket = Consideration::new(&who, Footprint::from_parts(10, 1)).unwrap().unwrap();
+			let ticket = Consideration::new(&who, Footprint::from_parts(10, 1)).unwrap();
 			assert_eq!(Balances::balance_frozen(&TestId::Foo, &who), 10);
 
-			let ticket = ticket.update(&who, Footprint::from_parts(4, 1)).unwrap().unwrap();
+			let ticket = ticket.update(&who, Footprint::from_parts(4, 1)).unwrap();
 			assert_eq!(Balances::balance_frozen(&TestId::Foo, &who), 4);
 
 			assert_ok!(Balances::increase_frozen(&TestId::Foo, &who, extend_freeze));
 			assert_eq!(Balances::balance_frozen(&TestId::Foo, &who), 4 + extend_freeze);
 
-			let ticket = ticket.update(&who, Footprint::from_parts(8, 1)).unwrap().unwrap();
+			let ticket = ticket.update(&who, Footprint::from_parts(8, 1)).unwrap();
 			assert_eq!(Balances::balance_frozen(&TestId::Foo, &who), 8 + extend_freeze);
 
-			assert_eq!(ticket.update(&who, Footprint::from_parts(0, 0)).unwrap(), None);
+			assert_eq!(ticket.update(&who, Footprint::from_parts(0, 0)).unwrap(), zero_ticket);
 			assert_eq!(Balances::balance_frozen(&TestId::Foo, &who), 0 + extend_freeze);
 
-			let ticket = Consideration::new(&who, Footprint::from_parts(10, 1)).unwrap().unwrap();
+			let ticket = Consideration::new(&who, Footprint::from_parts(10, 1)).unwrap();
 			assert_eq!(Balances::balance_frozen(&TestId::Foo, &who), 10 + extend_freeze);
 
 			let _ = ticket.drop(&who).unwrap();
@@ -560,24 +561,26 @@ fn hold_consideration_works() {
 			let who = 4;
 			// hold amount taken somewhere outside of our (Consideration) scope.
 			let extend_hold = 15;
+
+			let zero_ticket = Consideration::new(&who, Footprint::from_parts(0, 0)).unwrap();
 			assert_eq!(Balances::balance_on_hold(&TestId::Foo, &who), 0);
 
-			let ticket = Consideration::new(&who, Footprint::from_parts(10, 1)).unwrap().unwrap();
+			let ticket = Consideration::new(&who, Footprint::from_parts(10, 1)).unwrap();
 			assert_eq!(Balances::balance_on_hold(&TestId::Foo, &who), 10);
 
-			let ticket = ticket.update(&who, Footprint::from_parts(4, 1)).unwrap().unwrap();
+			let ticket = ticket.update(&who, Footprint::from_parts(4, 1)).unwrap();
 			assert_eq!(Balances::balance_on_hold(&TestId::Foo, &who), 4);
 
 			assert_ok!(Balances::hold(&TestId::Foo, &who, extend_hold));
 			assert_eq!(Balances::balance_on_hold(&TestId::Foo, &who), 4 + extend_hold);
 
-			let ticket = ticket.update(&who, Footprint::from_parts(8, 1)).unwrap().unwrap();
+			let ticket = ticket.update(&who, Footprint::from_parts(8, 1)).unwrap();
 			assert_eq!(Balances::balance_on_hold(&TestId::Foo, &who), 8 + extend_hold);
 
-			assert_eq!(ticket.update(&who, Footprint::from_parts(0, 0)).unwrap(), None);
+			assert_eq!(ticket.update(&who, Footprint::from_parts(0, 0)).unwrap(), zero_ticket);
 			assert_eq!(Balances::balance_on_hold(&TestId::Foo, &who), 0 + extend_hold);
 
-			let ticket = Consideration::new(&who, Footprint::from_parts(10, 1)).unwrap().unwrap();
+			let ticket = Consideration::new(&who, Footprint::from_parts(10, 1)).unwrap();
 			assert_eq!(Balances::balance_on_hold(&TestId::Foo, &who), 10 + extend_hold);
 
 			let _ = ticket.drop(&who).unwrap();
@@ -600,21 +603,22 @@ fn lone_freeze_consideration_works() {
 			>;
 
 			let who = 4;
+			let zero_ticket = Consideration::new(&who, Footprint::from_parts(0, 0)).unwrap();
 			assert_eq!(Balances::balance_frozen(&TestId::Foo, &who), 0);
 
-			let ticket = Consideration::new(&who, Footprint::from_parts(10, 1)).unwrap().unwrap();
+			let ticket = Consideration::new(&who, Footprint::from_parts(10, 1)).unwrap();
 			assert_eq!(Balances::balance_frozen(&TestId::Foo, &who), 10);
 
 			assert_ok!(Balances::increase_frozen(&TestId::Foo, &who, 5));
 			assert_eq!(Balances::balance_frozen(&TestId::Foo, &who), 15);
 
-			let ticket = ticket.update(&who, Footprint::from_parts(4, 1)).unwrap().unwrap();
+			let ticket = ticket.update(&who, Footprint::from_parts(4, 1)).unwrap();
 			assert_eq!(Balances::balance_frozen(&TestId::Foo, &who), 4);
 
-			assert_eq!(ticket.update(&who, Footprint::from_parts(0, 0)).unwrap(), None);
+			assert_eq!(ticket.update(&who, Footprint::from_parts(0, 0)).unwrap(), zero_ticket);
 			assert_eq!(Balances::balance_frozen(&TestId::Foo, &who), 0);
 
-			let ticket = Consideration::new(&who, Footprint::from_parts(10, 1)).unwrap().unwrap();
+			let ticket = Consideration::new(&who, Footprint::from_parts(10, 1)).unwrap();
 			assert_eq!(Balances::balance_frozen(&TestId::Foo, &who), 10);
 
 			let _ = ticket.drop(&who).unwrap();
@@ -637,21 +641,22 @@ fn lone_hold_consideration_works() {
 			>;
 
 			let who = 4;
+			let zero_ticket = Consideration::new(&who, Footprint::from_parts(0, 0)).unwrap();
 			assert_eq!(Balances::balance_on_hold(&TestId::Foo, &who), 0);
 
-			let ticket = Consideration::new(&who, Footprint::from_parts(10, 1)).unwrap().unwrap();
+			let ticket = Consideration::new(&who, Footprint::from_parts(10, 1)).unwrap();
 			assert_eq!(Balances::balance_on_hold(&TestId::Foo, &who), 10);
 
 			assert_ok!(Balances::hold(&TestId::Foo, &who, 5));
 			assert_eq!(Balances::balance_on_hold(&TestId::Foo, &who), 15);
 
-			let ticket = ticket.update(&who, Footprint::from_parts(4, 1)).unwrap().unwrap();
+			let ticket = ticket.update(&who, Footprint::from_parts(4, 1)).unwrap();
 			assert_eq!(Balances::balance_on_hold(&TestId::Foo, &who), 4);
 
-			assert_eq!(ticket.update(&who, Footprint::from_parts(0, 0)).unwrap(), None);
+			assert_eq!(ticket.update(&who, Footprint::from_parts(0, 0)).unwrap(), zero_ticket);
 			assert_eq!(Balances::balance_on_hold(&TestId::Foo, &who), 0);
 
-			let ticket = Consideration::new(&who, Footprint::from_parts(10, 1)).unwrap().unwrap();
+			let ticket = Consideration::new(&who, Footprint::from_parts(10, 1)).unwrap();
 			assert_eq!(Balances::balance_on_hold(&TestId::Foo, &who), 10);
 
 			let _ = ticket.drop(&who).unwrap();

substrate/frame/preimage/src/benchmarking.rs

@@ -116,7 +116,7 @@ benchmarks! {
 		T::ManagerOrigin::try_successful_origin().map_err(|_| BenchmarkError::Weightless)?,
 		hash
 	) verify {
-		let ticket = TicketOf::<T>::new(&noter, Footprint { count: 1, size: MAX_SIZE as u64 }).unwrap().unwrap();
+		let ticket = TicketOf::<T>::new(&noter, Footprint { count: 1, size: MAX_SIZE as u64 }).unwrap();
 		let s = RequestStatus::Requested { maybe_ticket: Some((noter, ticket)), count: 1, maybe_len: Some(MAX_SIZE) };
 		assert_eq!(RequestStatusFor::<T>::get(&hash), Some(s));
 	}

substrate/frame/preimage/src/lib.rs

@@ -124,8 +124,6 @@ pub mod pallet {
 		type ManagerOrigin: EnsureOrigin<Self::RuntimeOrigin>;
 
 		/// A means of providing some cost while data is stored on-chain.
-		///
-		/// Should never return a `None`, implying no cost for a non-empty preimage.
 		type Consideration: Consideration<Self::AccountId, Footprint>;
 	}
 
@@ -162,8 +160,6 @@ pub mod pallet {
 		TooMany,
 		/// Too few hashes were requested to be upgraded (i.e. zero).
 		TooFew,
-		/// No ticket with a cost was returned by [`Config::Consideration`] to store the preimage.
-		NoCost,
 	}
 
 	/// A reason for this pallet placing a hold on funds.
@@ -274,10 +270,10 @@ impl<T: Config> Pallet<T> {
 				// unreserve deposit
 				T::Currency::unreserve(&who, amount);
 				// take consideration
-				let Ok(Some(ticket)) =
+				let Ok(ticket) =
 					T::Consideration::new(&who, Footprint::from_parts(1, len as usize))
+						.defensive_proof("Unexpected inability to take deposit after unreserved")
 				else {
-					defensive!("None ticket or inability to take deposit after unreserved");
 					return true
 				};
 				RequestStatus::Unrequested { ticket: (who, ticket), len }
@@ -288,10 +284,12 @@ impl<T: Config> Pallet<T> {
 					T::Currency::unreserve(&who, deposit);
 					// take consideration
 					if let Some(len) = maybe_len {
-						let Ok(Some(ticket)) =
+						let Ok(ticket) =
 							T::Consideration::new(&who, Footprint::from_parts(1, len as usize))
+								.defensive_proof(
+									"Unexpected inability to take deposit after unreserved",
+								)
 						else {
-							defensive!("None ticket or inability to take deposit after unreserved");
 							return true
 						};
 						Some((who, ticket))
@@ -351,8 +349,7 @@ impl<T: Config> Pallet<T> {
 				RequestStatus::Requested { maybe_ticket: None, count: 1, maybe_len: Some(len) },
 			(None, Some(depositor)) => {
 				let ticket =
-					T::Consideration::new(depositor, Footprint::from_parts(1, len as usize))?
-						.ok_or(Error::<T>::NoCost)?;
+					T::Consideration::new(depositor, Footprint::from_parts(1, len as usize))?;
 				RequestStatus::Unrequested { ticket: (depositor.clone(), ticket), len }
 			},
 		};

substrate/frame/support/src/traits/storage.rs

@@ -201,7 +201,7 @@ where
 }
 
 /// Some sort of cost taken from account temporarily in order to offset the cost to the chain of
-/// holding some data `Footprint` (e.g. [`Footprint`]) in state.
+/// holding some data [`Footprint`] in state.
 ///
 /// The cost may be increased, reduced or dropped entirely as the footprint changes.
 ///
@@ -217,16 +217,14 @@ pub trait Consideration<AccountId, Footprint>:
 	Member + FullCodec + TypeInfo + MaxEncodedLen
 {
 	/// Create a ticket for the `new` footprint attributable to `who`. This ticket *must* ultimately
-	/// be consumed through `update` or `drop` once the footprint changes or is removed. `None`
-	/// implies no cost for a given footprint.
-	fn new(who: &AccountId, new: Footprint) -> Result<Option<Self>, DispatchError>;
+	/// be consumed through `update` or `drop` once the footprint changes or is removed.
+	fn new(who: &AccountId, new: Footprint) -> Result<Self, DispatchError>;
 
 	/// Optionally consume an old ticket and alter the footprint, enforcing the new cost to `who`
-	/// and returning the new ticket (or an error if there was an issue). `None` implies no cost for
-	/// a given footprint.
+	/// and returning the new ticket (or an error if there was an issue).
 	///
 	/// For creating tickets and dropping them, you can use the simpler `new` and `drop` instead.
-	fn update(self, who: &AccountId, new: Footprint) -> Result<Option<Self>, DispatchError>;
+	fn update(self, who: &AccountId, new: Footprint) -> Result<Self, DispatchError>;
 
 	/// Consume a ticket for some `old` footprint attributable to `who` which should now been freed.
 	fn drop(self, who: &AccountId) -> Result<(), DispatchError>;
@@ -239,18 +237,24 @@ pub trait Consideration<AccountId, Footprint>:
 	fn burn(self, _: &AccountId) {
 		let _ = self;
 	}
+	/// Ensure that creating a ticket for a given account and footprint will be successful if done
+	/// immediately after this call.
+	#[cfg(feature = "runtime-benchmarks")]
+	fn ensure_successful(who: &AccountId, new: Footprint);
 }
 
 impl<A, F> Consideration<A, F> for () {
-	fn new(_: &A, _: F) -> Result<Option<Self>, DispatchError> {
-		Ok(Some(()))
+	fn new(_: &A, _: F) -> Result<Self, DispatchError> {
+		Ok(())
 	}
-	fn update(self, _: &A, _: F) -> Result<Option<Self>, DispatchError> {
-		Ok(Some(()))
+	fn update(self, _: &A, _: F) -> Result<(), DispatchError> {
+		Ok(())
 	}
 	fn drop(self, _: &A) -> Result<(), DispatchError> {
 		Ok(())
 	}
+	#[cfg(feature = "runtime-benchmarks")]
+	fn ensure_successful(_: &A, _: F) {}
 }
 
 macro_rules! impl_incrementable {