diff --git a/polkadot/node/subsystem-util/src/inclusion_emulator/mod.rs b/polkadot/node/subsystem-util/src/inclusion_emulator/mod.rs index 8a620db4ab0c9..b6d4810a9a886 100644 --- a/polkadot/node/subsystem-util/src/inclusion_emulator/mod.rs +++ b/polkadot/node/subsystem-util/src/inclusion_emulator/mod.rs @@ -274,7 +274,7 @@ impl Constraints { ) -> Result<(), ModificationError> { if let Some(HrmpWatermarkUpdate::Trunk(hrmp_watermark)) = modifications.hrmp_watermark { // head updates are always valid. - if self.hrmp_inbound.valid_watermarks.iter().all(|w| w != &hrmp_watermark) { + if !self.hrmp_inbound.valid_watermarks.contains(&hrmp_watermark) { return Err(ModificationError::DisallowedHrmpWatermark(hrmp_watermark)) } } @@ -347,7 +347,7 @@ impl Constraints { match new.hrmp_inbound.valid_watermarks.binary_search(&hrmp_watermark.watermark()) { Ok(pos) => { // Exact match, so this is OK in all cases. - let _ = new.hrmp_inbound.valid_watermarks.drain(..pos + 1); + let _ = new.hrmp_inbound.valid_watermarks.drain(..pos); }, Err(pos) => match hrmp_watermark { HrmpWatermarkUpdate::Head(_) => { @@ -1038,30 +1038,44 @@ mod tests { } #[test] - fn constraints_disallowed_trunk_watermark() { + fn constraints_check_trunk_watermark() { let constraints = make_constraints(); let mut modifications = ConstraintModifications::identity(); - modifications.hrmp_watermark = Some(HrmpWatermarkUpdate::Trunk(7)); + // The current hrmp watermark is kept + modifications.hrmp_watermark = Some(HrmpWatermarkUpdate::Trunk(6)); + assert!(constraints.check_modifications(&modifications).is_ok()); + let new_constraints = constraints.apply_modifications(&modifications).unwrap(); + assert_eq!(new_constraints.hrmp_inbound.valid_watermarks, vec![6, 8]); + + modifications.hrmp_watermark = Some(HrmpWatermarkUpdate::Trunk(7)); assert_eq!( constraints.check_modifications(&modifications), Err(ModificationError::DisallowedHrmpWatermark(7)), ); - assert_eq!( constraints.apply_modifications(&modifications), Err(ModificationError::DisallowedHrmpWatermark(7)), ); + + modifications.hrmp_watermark = Some(HrmpWatermarkUpdate::Trunk(8)); + assert!(constraints.check_modifications(&modifications).is_ok()); + let new_constraints = constraints.apply_modifications(&modifications).unwrap(); + assert_eq!(new_constraints.hrmp_inbound.valid_watermarks, vec![8]); } #[test] - fn constraints_always_allow_head_watermark() { + fn constraints_check_head_watermark() { let constraints = make_constraints(); let mut modifications = ConstraintModifications::identity(); - modifications.hrmp_watermark = Some(HrmpWatermarkUpdate::Head(7)); + modifications.hrmp_watermark = Some(HrmpWatermarkUpdate::Head(5)); assert!(constraints.check_modifications(&modifications).is_ok()); + let new_constraints = constraints.apply_modifications(&modifications).unwrap(); + assert_eq!(new_constraints.hrmp_inbound.valid_watermarks, vec![6, 8]); + modifications.hrmp_watermark = Some(HrmpWatermarkUpdate::Head(7)); + assert!(constraints.check_modifications(&modifications).is_ok()); let new_constraints = constraints.apply_modifications(&modifications).unwrap(); assert_eq!(new_constraints.hrmp_inbound.valid_watermarks, vec![8]); } diff --git a/polkadot/runtime/parachains/src/hrmp.rs b/polkadot/runtime/parachains/src/hrmp.rs index 65345e4064f5c..efb4944de67ef 100644 --- a/polkadot/runtime/parachains/src/hrmp.rs +++ b/polkadot/runtime/parachains/src/hrmp.rs @@ -1189,12 +1189,16 @@ impl Pallet { } if let Some(last_watermark) = HrmpWatermarks::::get(&recipient) { - if new_hrmp_watermark <= last_watermark { + if new_hrmp_watermark < last_watermark { return Err(HrmpWatermarkAcceptanceErr::AdvancementRule { new_watermark: new_hrmp_watermark, last_watermark, }) } + + if new_hrmp_watermark == last_watermark { + return Ok(()) + } } // Second, check where the watermark CAN land. It's one of the following: @@ -1215,10 +1219,19 @@ impl Pallet { /// Returns HRMP watermarks of previously sent messages to a given para. pub(crate) fn valid_watermarks(recipient: ParaId) -> Vec> { - HrmpChannelDigests::::get(&recipient) + let mut valid_watermarks: Vec<_> = HrmpChannelDigests::::get(&recipient) .into_iter() .map(|(block_no, _)| block_no) - .collect() + .collect(); + + // The current watermark will remain valid until updated. + if let Some(last_watermark) = HrmpWatermarks::::get(&recipient) { + if valid_watermarks.first().map_or(false, |w| w > &last_watermark) { + valid_watermarks.insert(0, last_watermark); + } + } + + valid_watermarks } 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 52db932c7962b..9c72a64a5f4ce 100644 --- a/polkadot/runtime/parachains/src/hrmp/tests.rs +++ b/polkadot/runtime/parachains/src/hrmp/tests.rs @@ -892,6 +892,86 @@ fn cancel_pending_open_channel_request() { }); } +#[test] +fn watermark_can_remain_the_same() { + let para_a = 2032.into(); + let para_b = 2064.into(); + let para_c = 3000.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); + register_parachain(para_c); + + run_to_block(3, Some(vec![2, 3])); + Hrmp::init_open_channel(para_a, para_b, 2, 20).unwrap(); + Hrmp::accept_open_channel(para_b, para_a).unwrap(); + Hrmp::init_open_channel(para_c, para_b, 2, 20).unwrap(); + Hrmp::accept_open_channel(para_b, para_c).unwrap(); + + // Update watermark + assert!(Hrmp::check_hrmp_watermark(para_b, 3, 3).is_ok()); + let _ = Hrmp::prune_hrmp(para_b, 3); + + // On Block 6: + // A sends 1 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"HRMP message from A".to_vec() }] + .try_into() + .unwrap(); + let config = configuration::ActiveConfig::::get(); + assert!(Hrmp::check_outbound_hrmp(&config, para_a, &msgs).is_ok()); + let _ = Hrmp::queue_outbound_hrmp(para_a, msgs); + Hrmp::assert_storage_consistency_exhaustive(); + // C sends 1 message to B + assert!(channel_exists(para_c, para_b)); + let msgs: HorizontalMessages = + vec![OutboundHrmpMessage { recipient: para_b, data: b"HRMP message from C".to_vec() }] + .try_into() + .unwrap(); + let config = configuration::ActiveConfig::::get(); + assert!(Hrmp::check_outbound_hrmp(&config, para_c, &msgs).is_ok()); + let _ = Hrmp::queue_outbound_hrmp(para_c, msgs); + Hrmp::assert_storage_consistency_exhaustive(); + + // Check that a smaller HRMP watermark is not accepted + assert!(matches!( + Hrmp::check_hrmp_watermark(para_b, 6, 2), + Err(HrmpWatermarkAcceptanceErr::AdvancementRule { + new_watermark: 2, + last_watermark: 3 + }) + )); + + // Check that an HRMP watermark representing a relay chain block that doesn't contain + // any message is not accepted + assert!(matches!( + Hrmp::check_hrmp_watermark(para_b, 6, 5), + Err(HrmpWatermarkAcceptanceErr::LandsOnBlockWithNoMessages { new_watermark: 5 }) + )); + + // On block 7: + // B receives the messages, but can process only the one sent by A. + // B keeps the old watermark. + run_to_block(7, None); + assert!(Hrmp::check_hrmp_watermark(para_b, 7, 3).is_ok()); + let _ = Hrmp::prune_hrmp(para_b, 5); + Hrmp::assert_storage_consistency_exhaustive(); + + // On block 8: + // B can also process the message sent by C. B sets the watermark to 6. + run_to_block(8, None); + assert!(Hrmp::check_hrmp_watermark(para_b, 7, 6).is_ok()); + let _ = Hrmp::prune_hrmp(para_b, 6); + Hrmp::assert_storage_consistency_exhaustive(); + }); +} + #[test] fn watermark_maxed_out_at_relay_parent() { let para_a = 2032.into(); @@ -921,6 +1001,15 @@ fn watermark_maxed_out_at_relay_parent() { let _ = Hrmp::queue_outbound_hrmp(para_a, msgs); Hrmp::assert_storage_consistency_exhaustive(); + // Check that an HRMP watermark greater than the relay parent is not accepted + assert!(matches!( + Hrmp::check_hrmp_watermark(para_b, 6, 7), + Err(HrmpWatermarkAcceptanceErr::AheadRelayParent { + new_watermark: 7, + relay_chain_parent_number: 6, + }) + )); + // On block 8: // B receives the message sent by A. B sets the watermark to 7. run_to_block(8, None); diff --git a/prdoc/pr_8860.prdoc b/prdoc/pr_8860.prdoc new file mode 100644 index 0000000000000..6ffc74ec05cf4 --- /dev/null +++ b/prdoc/pr_8860.prdoc @@ -0,0 +1,10 @@ +title: '[stable2506] Backport #8860 relay chain logic (runtime + client) ' +doc: +- audience: Node Operator + description: |- + This PR relaxes the advancement rule for HRMP messages, making it possible to keep the old watermark. +crates: +- name: polkadot-node-subsystem-util + bump: patch +- name: polkadot-runtime-parachains + bump: patch