pallet-bounties: allow bounties to never expire (#7723)

# Description

Fixes polkadot-fellows/runtimes#509

The Bounties expiration and renewal heavily depends on manual
interactions through UI. This PR refactors the duration of the bounty to
be an optional configuration constant. If set to None, bounties remain
active indefinitely, removing the need for calling`extend_bounty_expiry`
and preventing automatic curator slashing for inactivity, which often
penalises unnecessarily.

## Integration

Remove
[BountyUpdatePeriod](https://github.com/polkadot-fellows/runtimes/blob/db4bb534cb411c0d6a2fe57eb331e6ec93ace825/relay/polkadot/src/lib.rs#L774)

## Review Notes

Modifies how bounty expiry is handled
<details>
<summary>🔍 Code Diff Summary</summary>

```diff
- #[pallet::constant]
- type BountyUpdatePeriod: Get<BlockNumberFor<Self, I>>;
+ #[pallet::constant]
+ type BountyUpdatePeriod: Get<Option<BlockNumberFor<Self, I>>>;

- *update_due = (Self::treasury_block_number() + T::BountyUpdatePeriod::get()).max(*update_due);
+ *update_due = Self::treasury_block_number().saturating_add(
+     T::BountyUpdatePeriod::get().unwrap_or(BlockNumberFor::<T, I>::MAX)
+ ); 
```
</details>

Author: BigTava

prdoc/pr_7723.prdoc

@@ -0,0 +1,14 @@
+title: '[pallet-bounties] Allow bounties to never expire'
+doc:
+- audience: Runtime Dev
+  description: |
+    Refactored the `update_due` calculation to use `saturating_add`, allowing bounties to remain active   
+    indefinitely without requiring `extend_bounty_expiry` and preventing automatic curator slashing for  
+    inactivity. Previously, setting `BountyUpdatePeriod` to a large value, such as `BlockNumber::max_value()`,  
+    could cause an overflow. 
+
+crates:
+- name: pallet-bounties
+  bump: patch
+- name: pallet-child-bounties
+  bump: patch

substrate/frame/bounties/src/benchmarking.rs

@@ -146,15 +146,26 @@ benchmarks_instance_pallet! {
 		);
 	}
 
-	// Worst case when curator is inactive and any sender unassigns the curator.
+	// Worst case when curator is inactive and any sender unassigns the curator,
+	// or if `BountyUpdatePeriod` is large enough and `RejectOrigin` executes the call.
 	unassign_curator {
 		setup_pot_account::<T, I>();
 		let (curator_lookup, bounty_id) = create_bounty::<T, I>()?;
 		Treasury::<T, I>::on_initialize(frame_system::Pallet::<T>::block_number());
 		let bounty_id = BountyCount::<T, I>::get() - 1;
-		set_block_number::<T, I>(T::SpendPeriod::get() + T::BountyUpdatePeriod::get() + 2u32.into());
-		let caller = whitelisted_caller();
-	}: _(RawOrigin::Signed(caller), bounty_id)
+		let bounty_update_period = T::BountyUpdatePeriod::get();
+		let inactivity_timeout = T::SpendPeriod::get().saturating_add(bounty_update_period);
+		set_block_number::<T, I>(inactivity_timeout.saturating_add(2u32.into()));
+
+		// If `BountyUpdatePeriod` overflows the inactivity timeout the benchmark still executes the slash
+		let origin = if Pallet::<T, I>::treasury_block_number() <= inactivity_timeout {
+			let curator = T::Lookup::lookup(curator_lookup).map_err(<&str>::from)?;
+			T::RejectOrigin::try_successful_origin().unwrap_or_else(|_| RawOrigin::Signed(curator).into())
+		} else {
+			let caller = whitelisted_caller();
+			RawOrigin::Signed(caller).into()
+		};
+	}: _<T::RuntimeOrigin>(origin, bounty_id)
 
 	accept_curator {
 		setup_pot_account::<T, I>();

substrate/frame/bounties/src/lib.rs

@@ -221,7 +221,12 @@ pub mod pallet {
 		#[pallet::constant]
 		type BountyDepositPayoutDelay: Get<BlockNumberFor<Self, I>>;
 
-		/// Bounty duration in blocks.
+		/// The time limit for a curator to act before a bounty expires.
+		///
+		/// The period that starts when a curator is approved, during which they must execute or
+		/// update the bounty via `extend_bounty_expiry`. If missed, the bounty expires, and the
+		/// curator may be slashed. If `BlockNumberFor::MAX`, bounties stay active indefinitely,
+		/// removing the need for `extend_bounty_expiry`.
 		#[pallet::constant]
 		type BountyUpdatePeriod: Get<BlockNumberFor<Self, I>>;
 
@@ -578,8 +583,8 @@ pub mod pallet {
 						T::Currency::reserve(curator, deposit)?;
 						bounty.curator_deposit = deposit;
 
-						let update_due =
-							Self::treasury_block_number() + T::BountyUpdatePeriod::get();
+						let update_due = Self::treasury_block_number()
+							.saturating_add(T::BountyUpdatePeriod::get());
 						bounty.status =
 							BountyStatus::Active { curator: curator.clone(), update_due };
 
@@ -820,9 +825,9 @@ pub mod pallet {
 				match bounty.status {
 					BountyStatus::Active { ref curator, ref mut update_due } => {
 						ensure!(*curator == signer, Error::<T, I>::RequireCurator);
-						*update_due = (Self::treasury_block_number() +
-							T::BountyUpdatePeriod::get())
-						.max(*update_due);
+						*update_due = Self::treasury_block_number()
+							.saturating_add(T::BountyUpdatePeriod::get())
+							.max(*update_due);
 					},
 					_ => return Err(Error::<T, I>::UnexpectedStatus.into()),
 				}

substrate/frame/bounties/src/tests.rs

@@ -137,14 +137,14 @@ parameter_types! {
 	pub const CuratorDepositMultiplier: Permill = Permill::from_percent(50);
 	pub const CuratorDepositMax: Balance = 1_000;
 	pub const CuratorDepositMin: Balance = 3;
-
+	pub static BountyUpdatePeriod: u64 = 20;
 }
 
 impl Config for Test {
 	type RuntimeEvent = RuntimeEvent;
 	type BountyDepositBase = ConstU64<80>;
 	type BountyDepositPayoutDelay = ConstU64<3>;
-	type BountyUpdatePeriod = ConstU64<20>;
+	type BountyUpdatePeriod = BountyUpdatePeriod;
 	type CuratorDepositMultiplier = CuratorDepositMultiplier;
 	type CuratorDepositMax = CuratorDepositMax;
 	type CuratorDepositMin = CuratorDepositMin;
@@ -160,7 +160,7 @@ impl Config<Instance1> for Test {
 	type RuntimeEvent = RuntimeEvent;
 	type BountyDepositBase = ConstU64<80>;
 	type BountyDepositPayoutDelay = ConstU64<3>;
-	type BountyUpdatePeriod = ConstU64<20>;
+	type BountyUpdatePeriod = BountyUpdatePeriod;
 	type CuratorDepositMultiplier = CuratorDepositMultiplier;
 	type CuratorDepositMax = CuratorDepositMax;
 	type CuratorDepositMin = CuratorDepositMin;
@@ -1416,3 +1416,68 @@ fn approve_bounty_with_curator_proposed_unassign_works() {
 		assert_eq!(last_event(), BountiesEvent::CuratorUnassigned { bounty_id: 0 });
 	});
 }
+
+#[test]
+fn accept_curator_sets_update_due_correctly() {
+	ExtBuilder::default().build_and_execute(|| {
+		// Given (BountyUpdatePeriod = 20)
+		let bounty_id = 0;
+		let proposer = 0;
+		let fee = 10;
+		let curator = 4;
+		Balances::make_free_balance_be(&Treasury::account_id(), 101);
+		Balances::make_free_balance_be(&curator, 12);
+		assert_ok!(Bounties::propose_bounty(
+			RuntimeOrigin::signed(proposer),
+			50,
+			b"12345".to_vec()
+		));
+		assert_ok!(Bounties::approve_bounty(RuntimeOrigin::root(), 0));
+		go_to_block(4);
+		assert_ok!(Bounties::propose_curator(RuntimeOrigin::root(), bounty_id, curator, fee));
+
+		// When
+		assert_ok!(Bounties::accept_curator(RuntimeOrigin::signed(curator), bounty_id));
+
+		// Then
+		assert_eq!(
+			pallet_bounties::Bounties::<Test>::get(bounty_id).unwrap().status,
+			BountyStatus::Active { curator, update_due: 24 }
+		);
+
+		// Given (BountyUpdatePeriod = BlockNumber::max_value())
+		BountyUpdatePeriod::set(BlockNumberFor::<Test>::max_value());
+		Balances::make_free_balance_be(&Treasury1::account_id(), 101);
+		assert_ok!(Bounties1::propose_bounty(
+			RuntimeOrigin::signed(proposer),
+			50,
+			b"12345".to_vec()
+		));
+		assert_ok!(Bounties1::approve_bounty(RuntimeOrigin::root(), bounty_id));
+		go_to_block(6);
+		<Treasury1 as OnInitialize<u64>>::on_initialize(6);
+		assert_ok!(Bounties1::propose_curator(RuntimeOrigin::root(), bounty_id, curator, fee));
+
+		// When
+		assert_ok!(Bounties1::accept_curator(RuntimeOrigin::signed(curator), bounty_id));
+
+		// Then
+		assert_eq!(
+			pallet_bounties::Bounties::<Test, Instance1>::get(bounty_id).unwrap().status,
+			BountyStatus::Active { curator, update_due: BlockNumberFor::<Test>::max_value() }
+		);
+
+		// When
+		assert_ok!(Bounties1::extend_bounty_expiry(
+			RuntimeOrigin::signed(curator),
+			bounty_id,
+			Vec::new()
+		));
+
+		// Then
+		assert_eq!(
+			pallet_bounties::Bounties::<Test, Instance1>::get(bounty_id).unwrap().status,
+			BountyStatus::Active { curator, update_due: BlockNumberFor::<Test>::max_value() }
+		);
+	});
+}

substrate/frame/child-bounties/src/benchmarking.rs

@@ -267,17 +267,31 @@ mod benchmarks {
 		Ok(())
 	}
 
-	// Worst case when curator is inactive and any sender un-assigns the curator.
+	// Worst case when curator is inactive and any sender un-assigns the curator,
+	// or if `BountyUpdatePeriod` is large enough and `RejectOrigin` executes the call.
 	#[benchmark]
 	fn unassign_curator() -> Result<(), BenchmarkError> {
 		setup_pot_account::<T>();
 		let bounty_setup = activate_child_bounty::<T>(0, T::MaximumReasonLength::get())?;
 		Treasury::<T>::on_initialize(frame_system::Pallet::<T>::block_number());
-		set_block_number::<T>(T::SpendPeriod::get() + T::BountyUpdatePeriod::get() + 1u32.into());
-		let caller = whitelisted_caller();
+		let bounty_update_period = T::BountyUpdatePeriod::get();
+		let inactivity_timeout = T::SpendPeriod::get().saturating_add(bounty_update_period);
+		set_block_number::<T>(inactivity_timeout.saturating_add(1u32.into()));
+
+		// If `BountyUpdatePeriod` overflows the inactivity timeout the benchmark still
+		// executes the slash
+		let origin: T::RuntimeOrigin = if Pallet::<T>::treasury_block_number() <= inactivity_timeout
+		{
+			let child_curator = bounty_setup.child_curator;
+			T::RejectOrigin::try_successful_origin()
+				.unwrap_or_else(|_| RawOrigin::Signed(child_curator).into())
+		} else {
+			let caller = whitelisted_caller();
+			RawOrigin::Signed(caller).into()
+		};
 
 		#[extrinsic_call]
-		_(RawOrigin::Signed(caller), bounty_setup.bounty_id, bounty_setup.child_bounty_id);
+		_(origin as T::RuntimeOrigin, bounty_setup.bounty_id, bounty_setup.child_bounty_id);
 
 		Ok(())
 	}