From 00946b10ab18331f959f5cbced7c433b6132b1cb Mon Sep 17 00:00:00 2001 From: Muharem Date: Wed, 14 Aug 2024 15:35:34 +0200 Subject: [PATCH 1/2] Make ticket non-optional and add ensure_successful method to Consideration trait (#5359) 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 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. --- prdoc/pr_5359.prdoc | 21 ++++ .../balances/src/tests/fungible_tests.rs | 41 ++++--- substrate/frame/preimage/src/benchmarking.rs | 2 +- substrate/frame/preimage/src/lib.rs | 17 ++- substrate/frame/support/src/traits/storage.rs | 26 ++-- .../support/src/traits/tokens/fungible/mod.rs | 112 ++++++++---------- 6 files changed, 115 insertions(+), 104 deletions(-) create mode 100644 prdoc/pr_5359.prdoc diff --git a/prdoc/pr_5359.prdoc b/prdoc/pr_5359.prdoc new file mode 100644 index 0000000000000..bf059129a4362 --- /dev/null +++ b/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 diff --git a/substrate/frame/balances/src/tests/fungible_tests.rs b/substrate/frame/balances/src/tests/fungible_tests.rs index 1a09303a6590d..e1c1be9b1335a 100644 --- a/substrate/frame/balances/src/tests/fungible_tests.rs +++ b/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(); diff --git a/substrate/frame/preimage/src/benchmarking.rs b/substrate/frame/preimage/src/benchmarking.rs index 2d3bec16b8183..3d0c5b900579d 100644 --- a/substrate/frame/preimage/src/benchmarking.rs +++ b/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::::new(¬er, Footprint { count: 1, size: MAX_SIZE as u64 }).unwrap().unwrap(); + let ticket = TicketOf::::new(¬er, 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::::get(&hash), Some(s)); } diff --git a/substrate/frame/preimage/src/lib.rs b/substrate/frame/preimage/src/lib.rs index 30056fc6d9a49..658e7fec53482 100644 --- a/substrate/frame/preimage/src/lib.rs +++ b/substrate/frame/preimage/src/lib.rs @@ -124,8 +124,6 @@ pub mod pallet { type ManagerOrigin: EnsureOrigin; /// 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; } @@ -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 Pallet { // 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 Pallet { 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 Pallet { 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::::NoCost)?; + T::Consideration::new(depositor, Footprint::from_parts(1, len as usize))?; RequestStatus::Unrequested { ticket: (depositor.clone(), ticket), len } }, }; diff --git a/substrate/frame/support/src/traits/storage.rs b/substrate/frame/support/src/traits/storage.rs index a954af14d259b..eb63ea59f6cb8 100644 --- a/substrate/frame/support/src/traits/storage.rs +++ b/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: 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, DispatchError>; + /// be consumed through `update` or `drop` once the footprint changes or is removed. + fn new(who: &AccountId, new: Footprint) -> Result; /// 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, DispatchError>; + fn update(self, who: &AccountId, new: Footprint) -> Result; /// 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: 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 Consideration for () { - fn new(_: &A, _: F) -> Result, DispatchError> { - Ok(Some(())) + fn new(_: &A, _: F) -> Result { + Ok(()) } - fn update(self, _: &A, _: F) -> Result, 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 { diff --git a/substrate/frame/support/src/traits/tokens/fungible/mod.rs b/substrate/frame/support/src/traits/tokens/fungible/mod.rs index f40e494b930d5..9b7c86971bb88 100644 --- a/substrate/frame/support/src/traits/tokens/fungible/mod.rs +++ b/substrate/frame/support/src/traits/tokens/fungible/mod.rs @@ -164,6 +164,8 @@ use codec::{Decode, Encode, MaxEncodedLen}; use core::marker::PhantomData; use frame_support_procedural::{CloneNoBound, EqNoBound, PartialEqNoBound, RuntimeDebugNoBound}; use scale_info::TypeInfo; +#[cfg(feature = "runtime-benchmarks")] +use sp_runtime::Saturating; use super::{ Fortitude::{Force, Polite}, @@ -209,38 +211,35 @@ pub struct FreezeConsideration(F::Balance, PhantomData ( where F: MutateFreeze; impl< - A: 'static, - F: 'static + MutateFreeze, + A: 'static + Eq, + #[cfg(not(feature = "runtime-benchmarks"))] F: 'static + MutateFreeze, + #[cfg(feature = "runtime-benchmarks")] F: 'static + MutateFreeze + Mutate, R: 'static + Get, D: 'static + Convert, Fp: 'static, > Consideration for FreezeConsideration { - fn new(who: &A, footprint: Fp) -> Result, DispatchError> { + fn new(who: &A, footprint: Fp) -> Result { let new = D::convert(footprint); - if new.is_zero() { - Ok(None) - } else { - F::increase_frozen(&R::get(), who, new)?; - Ok(Some(Self(new, PhantomData))) - } + F::increase_frozen(&R::get(), who, new)?; + Ok(Self(new, PhantomData)) } - fn update(self, who: &A, footprint: Fp) -> Result, DispatchError> { + fn update(self, who: &A, footprint: Fp) -> Result { let new = D::convert(footprint); if self.0 > new { F::decrease_frozen(&R::get(), who, self.0 - new)?; } else if new > self.0 { F::increase_frozen(&R::get(), who, new - self.0)?; } - if new.is_zero() { - Ok(None) - } else { - Ok(Some(Self(new, PhantomData))) - } + Ok(Self(new, PhantomData)) } fn drop(self, who: &A) -> Result<(), DispatchError> { F::decrease_frozen(&R::get(), who, self.0).map(|_| ()) } + #[cfg(feature = "runtime-benchmarks")] + fn ensure_successful(who: &A, fp: Fp) { + let _ = F::mint_into(who, F::minimum_balance().saturating_add(D::convert(fp))); + } } /// Consideration method using a `fungible` balance frozen as the cost exacted for the footprint. @@ -263,34 +262,27 @@ pub struct HoldConsideration( where F: MutateHold; impl< - A: 'static, - F: 'static + MutateHold, + A: 'static + Eq, + #[cfg(not(feature = "runtime-benchmarks"))] F: 'static + MutateHold, + #[cfg(feature = "runtime-benchmarks")] F: 'static + MutateHold + Mutate, R: 'static + Get, D: 'static + Convert, Fp: 'static, > Consideration for HoldConsideration { - fn new(who: &A, footprint: Fp) -> Result, DispatchError> { + fn new(who: &A, footprint: Fp) -> Result { let new = D::convert(footprint); - if new.is_zero() { - Ok(None) - } else { - F::hold(&R::get(), who, new)?; - Ok(Some(Self(new, PhantomData))) - } + F::hold(&R::get(), who, new)?; + Ok(Self(new, PhantomData)) } - fn update(self, who: &A, footprint: Fp) -> Result, DispatchError> { + fn update(self, who: &A, footprint: Fp) -> Result { let new = D::convert(footprint); if self.0 > new { F::release(&R::get(), who, self.0 - new, BestEffort)?; } else if new > self.0 { F::hold(&R::get(), who, new - self.0)?; } - if new.is_zero() { - Ok(None) - } else { - Ok(Some(Self(new, PhantomData))) - } + Ok(Self(new, PhantomData)) } fn drop(self, who: &A) -> Result<(), DispatchError> { F::release(&R::get(), who, self.0, BestEffort).map(|_| ()) @@ -298,6 +290,10 @@ impl< fn burn(self, who: &A) { let _ = F::burn_held(&R::get(), who, self.0, BestEffort, Force); } + #[cfg(feature = "runtime-benchmarks")] + fn ensure_successful(who: &A, fp: Fp) { + let _ = F::mint_into(who, F::minimum_balance().saturating_add(D::convert(fp))); + } } /// Basic consideration method using a `fungible` balance frozen as the cost exacted for the @@ -321,34 +317,28 @@ impl< #[codec(mel_bound())] pub struct LoneFreezeConsideration(PhantomData (A, Fx, Rx, D, Fp)>); impl< - A: 'static, - Fx: 'static + MutateFreeze, + A: 'static + Eq, + #[cfg(not(feature = "runtime-benchmarks"))] Fx: 'static + MutateFreeze, + #[cfg(feature = "runtime-benchmarks")] Fx: 'static + MutateFreeze + Mutate, Rx: 'static + Get, D: 'static + Convert, Fp: 'static, > Consideration for LoneFreezeConsideration { - fn new(who: &A, footprint: Fp) -> Result, DispatchError> { + fn new(who: &A, footprint: Fp) -> Result { ensure!(Fx::balance_frozen(&Rx::get(), who).is_zero(), DispatchError::Unavailable); - let new = D::convert(footprint); - if new.is_zero() { - Ok(None) - } else { - Fx::set_frozen(&Rx::get(), who, new, Polite).map(|_| Some(Self(PhantomData))) - } + Fx::set_frozen(&Rx::get(), who, D::convert(footprint), Polite).map(|_| Self(PhantomData)) } - fn update(self, who: &A, footprint: Fp) -> Result, DispatchError> { - let new = D::convert(footprint); - let _ = Fx::set_frozen(&Rx::get(), who, new, Polite)?; - if new.is_zero() { - Ok(None) - } else { - Ok(Some(Self(PhantomData))) - } + fn update(self, who: &A, footprint: Fp) -> Result { + Fx::set_frozen(&Rx::get(), who, D::convert(footprint), Polite).map(|_| Self(PhantomData)) } fn drop(self, who: &A) -> Result<(), DispatchError> { Fx::thaw(&Rx::get(), who).map(|_| ()) } + #[cfg(feature = "runtime-benchmarks")] + fn ensure_successful(who: &A, fp: Fp) { + let _ = Fx::mint_into(who, Fx::minimum_balance().saturating_add(D::convert(fp))); + } } /// Basic consideration method using a `fungible` balance placed on hold as the cost exacted for the @@ -372,30 +362,20 @@ impl< #[codec(mel_bound())] pub struct LoneHoldConsideration(PhantomData (A, Fx, Rx, D, Fp)>); impl< - A: 'static, - F: 'static + MutateHold, + A: 'static + Eq, + #[cfg(not(feature = "runtime-benchmarks"))] F: 'static + MutateHold, + #[cfg(feature = "runtime-benchmarks")] F: 'static + MutateHold + Mutate, R: 'static + Get, D: 'static + Convert, Fp: 'static, > Consideration for LoneHoldConsideration { - fn new(who: &A, footprint: Fp) -> Result, DispatchError> { + fn new(who: &A, footprint: Fp) -> Result { ensure!(F::balance_on_hold(&R::get(), who).is_zero(), DispatchError::Unavailable); - let new = D::convert(footprint); - if new.is_zero() { - Ok(None) - } else { - F::set_on_hold(&R::get(), who, new).map(|_| Some(Self(PhantomData))) - } + F::set_on_hold(&R::get(), who, D::convert(footprint)).map(|_| Self(PhantomData)) } - fn update(self, who: &A, footprint: Fp) -> Result, DispatchError> { - let new = D::convert(footprint); - let _ = F::set_on_hold(&R::get(), who, new)?; - if new.is_zero() { - Ok(None) - } else { - Ok(Some(Self(PhantomData))) - } + fn update(self, who: &A, footprint: Fp) -> Result { + F::set_on_hold(&R::get(), who, D::convert(footprint)).map(|_| Self(PhantomData)) } fn drop(self, who: &A) -> Result<(), DispatchError> { F::release_all(&R::get(), who, BestEffort).map(|_| ()) @@ -403,4 +383,8 @@ impl< fn burn(self, who: &A) { let _ = F::burn_all_held(&R::get(), who, BestEffort, Force); } + #[cfg(feature = "runtime-benchmarks")] + fn ensure_successful(who: &A, fp: Fp) { + let _ = F::mint_into(who, F::minimum_balance().saturating_add(D::convert(fp))); + } } From 05a8ba662f0afdd4a4e6e2f4e61e4ca2458d666c Mon Sep 17 00:00:00 2001 From: Alexandru Gheorghe <49718502+alexggh@users.noreply.github.com> Date: Wed, 14 Aug 2024 16:55:29 +0300 Subject: [PATCH 2/2] Fix OurViewChange small race (#5356) 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 that our subsystem processing OurViewChange, however on testnets like versi where CPU is sometimes overcommitted this race gets triggered occasionally, so let's fix it by sending the messages in the right order. --------- Signed-off-by: Alexandru Gheorghe --- polkadot/node/network/bridge/src/rx/mod.rs | 30 +++++++++++----------- prdoc/pr_5356.prdoc | 18 +++++++++++++ 2 files changed, 33 insertions(+), 15 deletions(-) create mode 100644 prdoc/pr_5356.prdoc diff --git a/polkadot/node/network/bridge/src/rx/mod.rs b/polkadot/node/network/bridge/src/rx/mod.rs index 56965ce6ba404..7745c42f78a17 100644 --- a/polkadot/node/network/bridge/src/rx/mod.rs +++ b/polkadot/node/network/bridge/src/rx/mod.rs @@ -962,6 +962,21 @@ fn update_our_view( ) }; + 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( 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 diff --git a/prdoc/pr_5356.prdoc b/prdoc/pr_5356.prdoc new file mode 100644 index 0000000000000..a306be3354407 --- /dev/null +++ b/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