fix: don't block fetches (#5632)

state-chain/pallets/cf-ingress-egress/src/lib.rs

@@ -1602,32 +1602,52 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
 									deposit_address,
 									deposit_fetch_id,
 									..
-								} =>
-									Self::should_fetch_or_transfer(
-										&mut maybe_no_of_fetches_remaining,
-									) && DepositChannelLookup::<T, I>::mutate(
-										deposit_address,
-										|details| {
-											details
-												.as_mut()
-												.map(|details| {
-													let can_fetch =
-														details.deposit_channel.state.can_fetch();
-
-													if can_fetch {
-														deposit_fetch_id.replace(
-															details.deposit_channel.fetch_id(),
-														);
-														details
+								} => {
+									// Either:
+									// 1. We always want to fetch
+									// 2. We have a restriction on fetches, in which case we need to
+									//    have fetches remaining.
+									// And we must be able to fetch the channel (it must exist and
+									// can_fetch must be true)
+									if (maybe_no_of_fetches_remaining.is_none_or(|n| n > 0)) &&
+										DepositChannelLookup::<T, I>::mutate(
+											deposit_address,
+											|details| {
+												details
+													.as_mut()
+													.map(|details| {
+														let can_fetch = details
 															.deposit_channel
 															.state
-															.on_fetch_scheduled();
-													}
-													can_fetch
-												})
-												.unwrap_or(false)
-										},
-									),
+															.can_fetch();
+
+														if can_fetch {
+															deposit_fetch_id.replace(
+																details.deposit_channel.fetch_id(),
+															);
+															details
+																.deposit_channel
+																.state
+																.on_fetch_scheduled();
+														}
+														can_fetch
+													})
+													.unwrap_or(false)
+											},
+										) {
+										if let Some(n) = maybe_no_of_fetches_remaining.as_mut() {
+											*n = n.saturating_sub(1);
+										}
+										true
+									} else {
+										// If we have a restriction on fetches, but we have no fetch
+										// slots remaining then we don't want to fetch any
+										// more. OR:
+										// If the channel is expired / `can_fetch` returns
+										// false then we can't/shouldn't fetch.
+										false
+									}
+								},
 								FetchOrTransfer::Transfer { .. } => Self::should_fetch_or_transfer(
 									&mut maybe_no_of_transfers_remaining,
 								),

state-chain/pallets/cf-ingress-egress/src/tests.rs

@@ -1695,36 +1695,48 @@ fn do_not_batch_more_transfers_than_the_limit_allows() {
 	});
 }
 
+fn trigger_n_fetches(n: usize) -> Vec<H160> {
+	let mut channel_addresses = vec![];
+
+	const ASSET: EthAsset = EthAsset::Eth;
+
+	for i in 1..=n {
+		let (_, address, ..) = IngressEgress::request_liquidity_deposit_address(
+			i.try_into().unwrap(),
+			ASSET,
+			0,
+			ForeignChainAddress::Eth(Default::default()),
+		)
+		.unwrap();
+
+		let address: <Ethereum as Chain>::ChainAccount = address.try_into().unwrap();
+
+		channel_addresses.push(address);
+
+		assert_ok!(IngressEgress::process_channel_deposit_full_witness_inner(
+			&DepositWitness {
+				deposit_address: address,
+				asset: ASSET,
+				amount: DEFAULT_DEPOSIT_AMOUNT,
+				deposit_details: Default::default(),
+			},
+			Default::default()
+		));
+	}
+
+	channel_addresses
+}
+
 #[test]
 fn do_not_batch_more_fetches_than_the_limit_allows() {
 	new_test_ext().execute_with(|| {
 		MockFetchesTransfersLimitProvider::enable_limits();
 
 		const EXCESS_FETCHES: usize = 1;
-		const ASSET: EthAsset = EthAsset::Eth;
 
 		let fetch_limits = MockFetchesTransfersLimitProvider::maybe_fetches_limit().unwrap();
 
-		for i in 1..=fetch_limits + EXCESS_FETCHES {
-			let (_, address, ..) = IngressEgress::request_liquidity_deposit_address(
-				i.try_into().unwrap(),
-				ASSET,
-				0,
-				ForeignChainAddress::Eth(Default::default()),
-			)
-			.unwrap();
-			let address: <Ethereum as Chain>::ChainAccount = address.try_into().unwrap();
-
-			assert_ok!(IngressEgress::process_channel_deposit_full_witness_inner(
-				&DepositWitness {
-					deposit_address: address,
-					asset: ASSET,
-					amount: DEFAULT_DEPOSIT_AMOUNT,
-					deposit_details: Default::default(),
-				},
-				Default::default()
-			));
-		}
+		trigger_n_fetches(fetch_limits + EXCESS_FETCHES);
 
 		let scheduled_egresses = ScheduledEgressFetchOrTransfer::<Test, ()>::get();
 
@@ -1738,6 +1750,7 @@ fn do_not_batch_more_fetches_than_the_limit_allows() {
 
 		let scheduled_egresses = ScheduledEgressFetchOrTransfer::<Test, ()>::get();
 
+		// We should have fetched all except the exceess fetch.
 		assert_eq!(scheduled_egresses.len(), EXCESS_FETCHES, "Wrong amount of left egresses!");
 
 		IngressEgress::on_finalize(2);
@@ -1748,6 +1761,53 @@ fn do_not_batch_more_fetches_than_the_limit_allows() {
 	});
 }
 
+#[test]
+fn invalid_fetches_do_not_get_scheduled_and_do_not_block_other_fetches() {
+	new_test_ext().execute_with(|| {
+		MockFetchesTransfersLimitProvider::enable_limits();
+
+		const EXCESS_FETCHES: usize = 5;
+
+		let fetch_limits = MockFetchesTransfersLimitProvider::maybe_fetches_limit().unwrap();
+
+		assert!(
+			fetch_limits > EXCESS_FETCHES,
+			"We assume excess_fetches can be processed in a single on_finalize for this test"
+		);
+
+		let channel_addresses = trigger_n_fetches(fetch_limits + EXCESS_FETCHES);
+
+		assert_eq!(
+			ScheduledEgressFetchOrTransfer::<Test, ()>::get().len(),
+			fetch_limits + EXCESS_FETCHES,
+			"All the fetches should have been scheduled!"
+		);
+
+		for address in channel_addresses.iter().take(fetch_limits) {
+			IngressEgress::recycle_channel(&mut Weight::zero(), *address);
+		}
+
+		IngressEgress::on_finalize(1);
+
+		// Check the addresses are the same as the expired ones, we can do this by comparing
+		// the scheduled egresses with the expired addresses
+		assert_eq!(
+			ScheduledEgressFetchOrTransfer::<Test, ()>::get()
+				.iter()
+				.filter_map(|f_or_t| match f_or_t {
+					FetchOrTransfer::Fetch { deposit_address, .. } => Some(*deposit_address),
+					_ => None,
+				})
+				.collect::<Vec<_>>(),
+			channel_addresses[0..fetch_limits],
+			// Note: Ideally this shouldn't be the case since we don't want to keep holding fetches
+			// that will never be scheduled. However, at least we do not block ones that can be
+			// scheduled.
+			"The channels that expired should be the same as the scheduled egresses!"
+		);
+	});
+}
+
 #[test]
 fn do_not_process_more_ccm_swaps_than_allowed_by_limit() {
 	new_test_ext().execute_with(|| {