From e4f7814cbcd67321f31ea09aa0d967335095465e Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Tiago=20Tavares=20=E2=AD=95=EF=B8=8F?=
<43147091+BigTava@users.noreply.github.com>
Date: Tue, 18 Mar 2025 10:25:09 +0000
Subject: [PATCH] pallet-bounties: allow bounties to never expire (#7723)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
# 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>
---
prdoc/pr_7723.prdoc | 14 ++++
substrate/frame/bounties/src/benchmarking.rs | 19 +++--
substrate/frame/bounties/src/lib.rs | 17 +++--
substrate/frame/bounties/src/tests.rs | 71 ++++++++++++++++++-
.../frame/child-bounties/src/benchmarking.rs | 22 ++++--
5 files changed, 126 insertions(+), 17 deletions(-)
create mode 100644 prdoc/pr_7723.prdoc
diff --git a/prdoc/pr_7723.prdoc b/prdoc/pr_7723.prdoc
new file mode 100644
index 00000000000..efd102d8a36
--- /dev/null
+++ b/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
diff --git a/substrate/frame/bounties/src/benchmarking.rs b/substrate/frame/bounties/src/benchmarking.rs
index b5155909e3c..ca678057a90 100644
--- a/substrate/frame/bounties/src/benchmarking.rs
+++ b/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>();
diff --git a/substrate/frame/bounties/src/lib.rs b/substrate/frame/bounties/src/lib.rs
index 9b6e3c06e91..b7710f9b1c0 100644
--- a/substrate/frame/bounties/src/lib.rs
+++ b/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()),
}
diff --git a/substrate/frame/bounties/src/tests.rs b/substrate/frame/bounties/src/tests.rs
index c9f6c1319ed..4b74c4162c3 100644
--- a/substrate/frame/bounties/src/tests.rs
+++ b/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() }
+ );
+ });
+}
diff --git a/substrate/frame/child-bounties/src/benchmarking.rs b/substrate/frame/child-bounties/src/benchmarking.rs
index 2864f3ab504..14d06ba4d08 100644
--- a/substrate/frame/child-bounties/src/benchmarking.rs
+++ b/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(())
}
--
GitLab