Xpan auto retry limit

cumulus/parachains/runtimes/coretime/coretime-rococo/src/coretime.rs

@@ -28,7 +28,7 @@ use frame_support::{
 };
 use frame_system::Pallet as System;
 use pallet_broker::{
-	CoreAssignment, CoreIndex, CoretimeInterface, PartsOf57600, RCBlockNumberOf, TaskId,
+	CoreAssignment, CoreIndex, CoretimeInterface, PartsOf57600, RCBlockNumberOf, TaskId
 };
 use parachains_common::{AccountId, Balance};
 use rococo_runtime_constants::system_parachain::coretime;
@@ -318,4 +318,5 @@ impl pallet_broker::Config for Runtime {
 	type MaxAutoRenewals = ConstU32<100>;
 	type PriceAdapter = pallet_broker::MinimumPrice<Balance, MinimumEndPrice>;
 	type MinimumCreditPurchase = MinimumCreditPurchase;
+	type MaxAutoRenewalRetries = ConstU8<2>;
 }

cumulus/parachains/runtimes/coretime/coretime-westend/src/coretime.rs

@@ -331,4 +331,5 @@ impl pallet_broker::Config for Runtime {
 	type MaxAutoRenewals = ConstU32<20>;
 	type PriceAdapter = pallet_broker::MinimumPrice<Balance, MinimumEndPrice>;
 	type MinimumCreditPurchase = MinimumCreditPurchase;
+	type MaxAutoRenewalRetries = ConstU8<2>;
 }

prdoc/pr_10178.prdoc

@@ -0,0 +1,36 @@
+title: Xpan auto retry limit
+doc:
+- audience: Runtime Dev
+  description: |-
+    BREAKING CHANGE: runtime must define `MaxAutoRenewalRetries`
+
+    Fixes #9548
+    Problem: After auto-renewal fails due to some error, the system disables autorenewal entirely contrary to the documentation.
+    Solution: A more robust retry which is configurable in the pallet config. This is compile time setting currently.
+    Introduce `T::MaxAutoRenewalRetries` constant in `Config` trait track retry count in `AutoRenewalRetries` storage map, increment on each failed renewal attempt. When retries >= max, emit `AutoRenewalFailed`, remove auto-renew record, and reset counter to 0. Add integration tests:
+    - persistent retry when count < max
+    - record removal when count >= max
+
+    ## Integration
+    Downstream projects using pallet-broker must define `MaxAutoRenewalRetries` config. They just need to pass a retries limit. Can be set to `0` for no retry. This is similar to current logic.
+
+    ## Review Notes
+    ### Changes:
+    1.  Added `MaxAutoRenewalRetries` to config trait. Default type is `u8`.
+    2. Added a storage for renewal retries record. This increments for failed retries. It resets on success or when renewal is cancelled.
+    3. Existing code needs to add the new configuration parameter.
+    4. Added the config parameter with default `constu8<2>` as in test into `coretime-rococo` and `coretime-westend`.
+
+    # Checklist
+
+    * [x] My PR includes a detailed description as outlined in the "Description" and its two subsections above.
+    * [x] My PR follows the [labeling requirements](
+    https://github.com/paritytech/polkadot-sdk/blob/master/docs/contributor/CONTRIBUTING.md#Process
+    ) of this project (at minimum one label for `T` required)
+        * External contributors: Use `/cmd label <label-name>` to add labels
+        * Maintainers can also add labels manually
+    * [  ] I have made corresponding changes to the documentation (if applicable)
+    * [x] I have added tests that prove my fix is effective or that my feature works (if applicable)
+crates:
+- name: pallet-broker
+  bump: major

substrate/frame/broker/src/coretime_interface.rs

@@ -37,6 +37,9 @@ pub type TaskId = u32;
 /// Fraction expressed as a nominator with an assumed denominator of 57,600.
 pub type PartsOf57600 = u16;
 
+/// Count of failed renewal retries
+pub type RenewalRetriesCount = u8;
+
 /// An element to which a core can be assigned.
 #[derive(
 	Encode,

substrate/frame/broker/src/lib.rs

@@ -126,6 +126,10 @@ pub mod pallet {
 		#[pallet::constant]
 		type MaxAutoRenewals: Get<u32>;
 
+		/// Max number of retries for failed renewals
+		#[pallet::constant]
+		type MaxAutoRenewalRetries: Get<RenewalRetriesCount>;
+
 		/// The smallest amount of credits a user can purchase.
 		///
 		/// Needed to prevent spam attacks.
@@ -198,6 +202,11 @@ pub mod pallet {
 	pub type AutoRenewals<T: Config> =
 		StorageValue<_, BoundedVec<AutoRenewalRecord, T::MaxAutoRenewals>, ValueQuery>;
 
+	/// The amount of renewal retries for this payer
+	#[pallet::storage]
+	pub type AutoRenewalRetries<T: Config> =
+		StorageMap<_, Blake2_128Concat, (CoreIndex, T::AccountId), RenewalRetriesCount, ValueQuery>;
+
 	/// Received revenue info from the relay chain.
 	#[pallet::storage]
 	pub type RevenueInbox<T> = StorageValue<_, OnDemandRevenueRecordOf<T>, OptionQuery>;

substrate/frame/broker/src/mock.rs

@@ -31,7 +31,7 @@ use frame_support::{
 };
 use frame_system::{EnsureRoot, EnsureSignedBy};
 use sp_arithmetic::Perbill;
-use sp_core::{ConstU32, ConstU64, Get};
+use sp_core::{ConstU32, ConstU64, ConstU8, Get};
 use sp_runtime::{
 	traits::{BlockNumberProvider, Identity, MaybeConvert},
 	BuildStorage, Saturating,
@@ -217,6 +217,7 @@ impl crate::Config for Test {
 	type MaxAutoRenewals = ConstU32<3>;
 	type PriceAdapter = CenterTargetPrice<BalanceOf<Self>>;
 	type MinimumCreditPurchase = MinimumCreditPurchase;
+	type MaxAutoRenewalRetries = ConstU8<2>;
 }
 
 pub fn advance_to(b: u64) {

substrate/frame/broker/src/tests.rs

@@ -25,10 +25,7 @@ use frame_support::{
 };
 use frame_system::RawOrigin::Root;
 use pretty_assertions::assert_eq;
-use sp_runtime::{
-	traits::{BadOrigin, Get},
-	Perbill, TokenError,
-};
+use sp_runtime::{traits::{BadOrigin, Get}, Perbill, TokenError};
 use CoreAssignment::*;
 use CoretimeTraceItem::*;
 use Finality::*;
@@ -2247,18 +2244,100 @@ fn auto_renewal_works() {
 			.into(),
 		);
 
-		// Given that core #1 didn't get renewed due to the account not being sufficiently funded,
 		// Task (1003) will now be assigned to that core instead of core #2.
+		// The renewal persists however due to retry logic
 		assert_eq!(
 			AutoRenewals::<Test>::get().to_vec(),
 			vec![
 				AutoRenewalRecord { core: 0, task: 1001, next_renewal: 10 },
+				AutoRenewalRecord { core: 1, task: 1002, next_renewal: 10 },
+				// the renewal now retries since our default retry is 2 for test
 				AutoRenewalRecord { core: 1, task: 1003, next_renewal: 10 },
 			]
 		);
 	});
 }
 
+#[test]
+fn auto_renewal_retries_persist_when_below_max() {
+	TestExt::new().endow(1, 1000).execute_with(|| {
+		assert_ok!(Broker::do_start_sales(100, 2));
+		advance_to(2);
+
+		// Purchase 2 cores
+		let region_1 = Broker::do_purchase(1, u64::max_value()).unwrap();
+		let region_2 = Broker::do_purchase(1, u64::max_value()).unwrap();
+
+		// Assign tasks and enable auto renewals
+		assert_ok!(Broker::do_assign(region_1, Some(1), 1001, Final));
+		assert_ok!(Broker::do_assign(region_2, Some(1), 1002, Final));
+		assert_ok!(Broker::do_enable_auto_renew(1001, region_1.core, 1001, Some(7)));
+		assert_ok!(Broker::do_enable_auto_renew(1002, region_2.core, 1002, Some(7)));
+
+		// Fund only task 1001's sovereign account (1002 will fail)
+		endow(1001, 1000);
+		let initial_retry_count = AutoRenewalRetries::<Test>::get((region_2.core, 1002));
+
+		// Trigger renewal time
+		advance_to(7);
+
+		// Renewal #1 succeeds for task 1001, fails for 1002
+		System::assert_has_event(Event::<Test>::AutoRenewalFailed {
+			core: region_2.core,
+			payer: Some(1002),
+		}.into());
+
+		// Retry count should be +1 now
+		let retry_count = AutoRenewalRetries::<Test>::get((region_2.core, 1002));
+		assert_eq!(retry_count, initial_retry_count + 1);
+
+		// AutoRenewals should still contain record for core 2
+		assert!(
+			AutoRenewals::<Test>::get().iter().any(|r| r.core == region_2.core)
+		);
+	});
+}
+
+
+#[test]
+fn auto_renewal_record_removed_when_max_retries_reached() {
+	TestExt::new().endow(1, 1000).execute_with(|| {
+		assert_ok!(Broker::do_start_sales(100, 2));
+		advance_to(2);
+
+		// Purchase and assign
+		let region = Broker::do_purchase(1, u64::max_value()).unwrap();
+		assert_ok!(Broker::do_assign(region, Some(1), 1002, Final));
+		assert_ok!(Broker::do_enable_auto_renew(1002, region.core, 1002, Some(7)));
+
+		// Will just simulate we're at max
+		let max_retries: u8 = <Test as Config>::MaxAutoRenewalRetries::get();
+		AutoRenewalRetries::<Test>::insert((region.core, 1002), max_retries);
+
+		// Move to renewal block
+		advance_to(7);
+
+		// The renewal should fail again, but since we're at max retry, record dropped
+		System::assert_has_event(Event::<Test>::AutoRenewalFailed {
+			core: region.core,
+			payer: Some(1002),
+		}.into());
+
+		// Ensure it no longer exists in AutoRenewals
+		assert!(
+			!AutoRenewals::<Test>::get().iter().any(|r| r.core == region.core),
+			"Auto renewal record should be removed once max retries reached"
+		);
+
+		// Retry counter reset
+		assert_eq!(
+			AutoRenewalRetries::<Test>::get((region.core, 1002)),
+			0
+		);
+	});
+}
+
+
 #[test]
 fn disable_auto_renew_works() {
 	TestExt::new().endow(1, 1000).limit_cores_offered(Some(10)).execute_with(|| {

substrate/frame/broker/src/tick_impls.rs

@@ -360,18 +360,32 @@ impl<T: Config> Pallet<T> {
 				};
 
 				if let Ok(new_core_index) = Self::do_renew(payer.clone(), record.core) {
+					AutoRenewalRetries::<T>::insert((record.core, payer), 0);
 					Some(AutoRenewalRecord {
 						core: new_core_index,
 						task: record.task,
 						next_renewal: sale.region_end,
 					})
 				} else {
+					// check if renewal count has been exceeded and reset counter, else increment retry counter
+					let max_renewal_retries = T::MaxAutoRenewalRetries::get();
+					let mut retries = AutoRenewalRetries::<T>::get((record.core, payer.clone()));
 					Self::deposit_event(Event::<T>::AutoRenewalFailed {
 						core: record.core,
-						payer: Some(payer),
+						payer: Some(payer.clone()),
 					});
-
-					None
+					if retries >= max_renewal_retries {
+						AutoRenewalRetries::<T>::insert((record.core, payer), 0);
+						None
+					} else {
+						retries = retries.saturating_add(1);
+						AutoRenewalRetries::<T>::insert((record.core, payer), retries);
+						Some(AutoRenewalRecord {
+							core: record.core,
+							task: record.task,
+							next_renewal: sale.region_end,
+						})
+					}
 				}
 			})
 			.collect::<Vec<AutoRenewalRecord>>()

substrate/frame/broker/src/types.rs

@@ -15,10 +15,7 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
-use crate::{
-	Config, CoreAssignment, CoreIndex, CoreMask, CoretimeInterface, RCBlockNumberOf, TaskId,
-	CORE_MASK_BITS,
-};
+use crate::{Config, CoreAssignment, CoreIndex, CoreMask, CoretimeInterface, RCBlockNumberOf, RenewalRetriesCount, TaskId, CORE_MASK_BITS};
 use codec::{Decode, DecodeWithMemTracking, Encode, MaxEncodedLen};
 use frame_support::traits::fungible::Inspect;
 use frame_system::Config as SConfig;