From 57ec016a97b0e67127bcdf24965ff6bfc16d4cd7 Mon Sep 17 00:00:00 2001
From: asynchronous rob <rphmeier@gmail.com>
Date: Sat, 13 May 2023 18:24:03 -0500
Subject: [PATCH] Relax the watermark rule in the runtime (#7188)

* Relax the watermark rule in the runtime

* make comment more clear

* add hrmp test

* remove TODO now comment
---
 runtime/parachains/src/hrmp.rs       | 48 +++++++++++++++-------------
 runtime/parachains/src/hrmp/tests.rs | 45 ++++++++++++++++++++++++++
 2 files changed, 70 insertions(+), 23 deletions(-)

diff --git a/runtime/parachains/src/hrmp.rs b/runtime/parachains/src/hrmp.rs
index 034fac9ccf85..f33e753f62e5 100644
--- a/runtime/parachains/src/hrmp.rs
+++ b/runtime/parachains/src/hrmp.rs
@@ -889,19 +889,16 @@ impl<T: Config> Pallet<T> {
 	) -> Result<(), HrmpWatermarkAcceptanceErr<T::BlockNumber>> {
 		// First, check where the watermark CANNOT legally land.
 		//
-		// (a) For ensuring that messages are eventually, a rule requires each parablock new
-		//     watermark should be greater than the last one.
+		// (a) For ensuring that messages are eventually processed, we require each parablock's
+		//     watermark to be greater than the last one. The exception to this is if the previous
+		//     watermark was already equal to the current relay-parent number.
 		//
 		// (b) However, a parachain cannot read into "the future", therefore the watermark should
 		//     not be greater than the relay-chain context block which the parablock refers to.
-		if let Some(last_watermark) = HrmpWatermarks::<T>::get(&recipient) {
-			if new_hrmp_watermark <= last_watermark {
-				return Err(HrmpWatermarkAcceptanceErr::AdvancementRule {
-					new_watermark: new_hrmp_watermark,
-					last_watermark,
-				})
-			}
+		if new_hrmp_watermark == relay_chain_parent_number {
+			return Ok(())
 		}
+
 		if new_hrmp_watermark > relay_chain_parent_number {
 			return Err(HrmpWatermarkAcceptanceErr::AheadRelayParent {
 				new_watermark: new_hrmp_watermark,
@@ -909,24 +906,29 @@ impl<T: Config> Pallet<T> {
 			})
 		}
 
-		// Second, check where the watermark CAN land. It's one of the following:
-		//
-		// (a) The relay parent block number.
-		// (b) A relay-chain block in which this para received at least one message.
-		if new_hrmp_watermark == relay_chain_parent_number {
-			Ok(())
-		} else {
-			let digest = HrmpChannelDigests::<T>::get(&recipient);
-			if !digest
-				.binary_search_by_key(&new_hrmp_watermark, |(block_no, _)| *block_no)
-				.is_ok()
-			{
-				return Err(HrmpWatermarkAcceptanceErr::LandsOnBlockWithNoMessages {
+		if let Some(last_watermark) = HrmpWatermarks::<T>::get(&recipient) {
+			if new_hrmp_watermark <= last_watermark {
+				return Err(HrmpWatermarkAcceptanceErr::AdvancementRule {
 					new_watermark: new_hrmp_watermark,
+					last_watermark,
 				})
 			}
-			Ok(())
 		}
+
+		// Second, check where the watermark CAN land. It's one of the following:
+		//
+		// (a) The relay parent block number (checked above).
+		// (b) A relay-chain block in which this para received at least one message (checked here)
+		let digest = HrmpChannelDigests::<T>::get(&recipient);
+		if !digest
+			.binary_search_by_key(&new_hrmp_watermark, |(block_no, _)| *block_no)
+			.is_ok()
+		{
+			return Err(HrmpWatermarkAcceptanceErr::LandsOnBlockWithNoMessages {
+				new_watermark: new_hrmp_watermark,
+			})
+		}
+		Ok(())
 	}
 
 	pub(crate) fn check_outbound_hrmp(
diff --git a/runtime/parachains/src/hrmp/tests.rs b/runtime/parachains/src/hrmp/tests.rs
index c396260d2c30..7bffdc818f7c 100644
--- a/runtime/parachains/src/hrmp/tests.rs
+++ b/runtime/parachains/src/hrmp/tests.rs
@@ -660,3 +660,48 @@ fn cancel_pending_open_channel_request() {
 		Hrmp::assert_storage_consistency_exhaustive();
 	});
 }
+
+#[test]
+fn watermark_maxed_out_at_relay_parent() {
+	let para_a = 32.into();
+	let para_b = 64.into();
+
+	let mut genesis = GenesisConfigBuilder::default();
+	genesis.hrmp_channel_max_message_size = 20;
+	genesis.hrmp_channel_max_total_size = 20;
+	new_test_ext(genesis.build()).execute_with(|| {
+		register_parachain(para_a);
+		register_parachain(para_b);
+
+		run_to_block(5, Some(vec![4, 5]));
+		Hrmp::init_open_channel(para_a, para_b, 2, 20).unwrap();
+		Hrmp::accept_open_channel(para_b, para_a).unwrap();
+
+		// On Block 6:
+		// A sends a message to B
+		run_to_block(6, Some(vec![6]));
+		assert!(channel_exists(para_a, para_b));
+		let msgs: HorizontalMessages =
+			vec![OutboundHrmpMessage { recipient: para_b, data: b"this is an emergency".to_vec() }]
+				.try_into()
+				.unwrap();
+		let config = Configuration::config();
+		assert!(Hrmp::check_outbound_hrmp(&config, para_a, &msgs).is_ok());
+		let _ = Hrmp::queue_outbound_hrmp(para_a, msgs);
+		Hrmp::assert_storage_consistency_exhaustive();
+
+		// On block 8:
+		// B receives the message sent by A. B sets the watermark to 7.
+		run_to_block(8, None);
+		assert!(Hrmp::check_hrmp_watermark(para_b, 7, 7).is_ok());
+		let _ = Hrmp::prune_hrmp(para_b, 7);
+		Hrmp::assert_storage_consistency_exhaustive();
+
+		// On block 9:
+		// B includes a candidate with the same relay parent as before.
+		run_to_block(9, None);
+		assert!(Hrmp::check_hrmp_watermark(para_b, 7, 7).is_ok());
+		let _ = Hrmp::prune_hrmp(para_b, 7);
+		Hrmp::assert_storage_consistency_exhaustive();
+	});
+}