From 33432fa02091cb4e2b5b93b8bb69a84e86b2e02a Mon Sep 17 00:00:00 2001 From: asynchronous rob 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 --- polkadot/runtime/parachains/src/hrmp.rs | 48 ++++++++++--------- polkadot/runtime/parachains/src/hrmp/tests.rs | 45 +++++++++++++++++ 2 files changed, 70 insertions(+), 23 deletions(-) diff --git a/polkadot/runtime/parachains/src/hrmp.rs b/polkadot/runtime/parachains/src/hrmp.rs index 034fac9ccf..f33e753f62 100644 --- a/polkadot/runtime/parachains/src/hrmp.rs +++ b/polkadot/runtime/parachains/src/hrmp.rs @@ -889,11 +889,23 @@ impl Pallet { ) -> Result<(), HrmpWatermarkAcceptanceErr> { // 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 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, + relay_chain_parent_number, + }) + } + if let Some(last_watermark) = HrmpWatermarks::::get(&recipient) { if new_hrmp_watermark <= last_watermark { return Err(HrmpWatermarkAcceptanceErr::AdvancementRule { @@ -902,31 +914,21 @@ impl Pallet { }) } } - if new_hrmp_watermark > relay_chain_parent_number { - return Err(HrmpWatermarkAcceptanceErr::AheadRelayParent { - new_watermark: new_hrmp_watermark, - relay_chain_parent_number, - }) - } // 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::::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(()) + // (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::::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/polkadot/runtime/parachains/src/hrmp/tests.rs b/polkadot/runtime/parachains/src/hrmp/tests.rs index c396260d2c..7bffdc818f 100644 --- a/polkadot/runtime/parachains/src/hrmp/tests.rs +++ b/polkadot/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(); + }); +}