From e6472412b15022d0dfbe8f6a0efba25496b7a5bf Mon Sep 17 00:00:00 2001 From: Svyatoslav Nikolsky Date: Tue, 4 Apr 2023 14:59:18 +0300 Subject: [PATCH 1/5] reject delivery transactions with at least one obsolete message --- bin/runtime-common/src/messages_call_ext.rs | 57 +++++++++++++++---- .../src/refund_relayer_extension.rs | 18 +++--- modules/messages/src/lib.rs | 4 ++ 3 files changed, 60 insertions(+), 19 deletions(-) diff --git a/bin/runtime-common/src/messages_call_ext.rs b/bin/runtime-common/src/messages_call_ext.rs index 588afad106d..934425ccf8c 100644 --- a/bin/runtime-common/src/messages_call_ext.rs +++ b/bin/runtime-common/src/messages_call_ext.rs @@ -21,18 +21,24 @@ use bp_messages::{LaneId, MessageNonce}; use frame_support::{dispatch::CallableCallFor, traits::IsSubType, RuntimeDebug}; use pallet_bridge_messages::{Config, Pallet}; use sp_runtime::transaction_validity::TransactionValidity; +use sp_std::ops::RangeInclusive; /// Generic info about a messages delivery/confirmation proof. #[derive(PartialEq, RuntimeDebug)] pub struct BaseMessagesProofInfo { + /// Message lane, used by the call. pub lane_id: LaneId, - pub best_bundled_nonce: MessageNonce, + /// Nonces of messages, included in the call. + pub bundled_range: RangeInclusive, + /// Nonce of the best message, stored by this chain before the call is dispatched. pub best_stored_nonce: MessageNonce, } impl BaseMessagesProofInfo { + /// Returns true if `bundled_range` cannot be directly appended to the `best_stored_nonce` + /// or if the `bundled_range` is empty. fn is_obsolete(&self) -> bool { - self.best_bundled_nonce <= self.best_stored_nonce + *self.bundled_range.start() != self.best_stored_nonce + 1 || self.bundled_range.is_empty() } } @@ -126,7 +132,9 @@ impl< return Some(ReceiveMessagesProofInfo(BaseMessagesProofInfo { lane_id: proof.lane, - best_bundled_nonce: proof.nonces_end, + // we want all messages in this range to be new for us. Otherwise transaction will + // be considered obsolete. + bundled_range: proof.nonces_start..=proof.nonces_end, best_stored_nonce: inbound_lane_data.last_delivered_nonce(), })) } @@ -145,7 +153,12 @@ impl< return Some(ReceiveMessagesDeliveryProofInfo(BaseMessagesProofInfo { lane_id: proof.lane, - best_bundled_nonce: relayers_state.last_delivered_nonce, + // there's a time frame between message delivery, message confirmation and reward + // confirmation. Because of that, we can't assume that our state has been confirmed + // to the bridged chain. So we are accepting any proof that brings new + // confirmations. + bundled_range: outbound_lane_data.latest_received_nonce + 1..= + relayers_state.last_delivered_nonce, best_stored_nonce: outbound_lane_data.latest_received_nonce, })) } @@ -247,8 +260,8 @@ mod tests { #[test] fn extension_rejects_obsolete_messages() { sp_io::TestExternalities::new(Default::default()).execute_with(|| { - // when current best delivered is message#10 and we're trying to deliver message#5 => tx - // is rejected + // when current best delivered is message#10 and we're trying to deliver messages 8..=9 + // => tx is rejected deliver_message_10(); assert!(!validate_message_delivery(8, 9)); }); @@ -257,20 +270,40 @@ mod tests { #[test] fn extension_rejects_same_message() { sp_io::TestExternalities::new(Default::default()).execute_with(|| { - // when current best delivered is message#10 and we're trying to import message#10 => tx - // is rejected + // when current best delivered is message#10 and we're trying to import messages 10..=10 + // => tx is rejected deliver_message_10(); assert!(!validate_message_delivery(8, 10)); }); } #[test] - fn extension_accepts_new_message() { + fn extension_rejects_call_with_some_obsolete_messages() { sp_io::TestExternalities::new(Default::default()).execute_with(|| { - // when current best delivered is message#10 and we're trying to deliver message#15 => - // tx is accepted + // when current best delivered is message#10 and we're trying to deliver messages + // 10..=15 => tx is rejected + deliver_message_10(); + assert!(!validate_message_delivery(10, 15)); + }); + } + + #[test] + fn extension_rejects_call_with_future_messages() { + sp_io::TestExternalities::new(Default::default()).execute_with(|| { + // when current best delivered is message#10 and we're trying to deliver messages + // 13..=15 => tx is rejected + deliver_message_10(); + assert!(!validate_message_delivery(13, 15)); + }); + } + + #[test] + fn extension_accepts_new_messages() { + sp_io::TestExternalities::new(Default::default()).execute_with(|| { + // when current best delivered is message#10 and we're trying to deliver message 11..=15 + // => tx is accepted deliver_message_10(); - assert!(validate_message_delivery(10, 15)); + assert!(validate_message_delivery(11, 15)); }); } diff --git a/bin/runtime-common/src/refund_relayer_extension.rs b/bin/runtime-common/src/refund_relayer_extension.rs index 7be97f19ad2..fa06ecbce54 100644 --- a/bin/runtime-common/src/refund_relayer_extension.rs +++ b/bin/runtime-common/src/refund_relayer_extension.rs @@ -538,7 +538,11 @@ mod tests { bridged_header_hash: Default::default(), storage_proof: vec![], lane: TestLaneId::get(), - nonces_start: best_message, + nonces_start: pallet_bridge_messages::InboundLanes::::get( + &TEST_LANE_ID, + ) + .last_delivered_nonce() + + 1, nonces_end: best_message, }, messages_count: 1, @@ -629,7 +633,7 @@ mod tests { MessagesCallInfo::ReceiveMessagesProof(ReceiveMessagesProofInfo( BaseMessagesProofInfo { lane_id: TEST_LANE_ID, - best_bundled_nonce: 200, + bundled_range: 101..=200, best_stored_nonce: 100, }, )), @@ -654,7 +658,7 @@ mod tests { MessagesCallInfo::ReceiveMessagesDeliveryProof(ReceiveMessagesDeliveryProofInfo( BaseMessagesProofInfo { lane_id: TEST_LANE_ID, - best_bundled_nonce: 200, + bundled_range: 101..=200, best_stored_nonce: 100, }, )), @@ -674,7 +678,7 @@ mod tests { MessagesCallInfo::ReceiveMessagesProof(ReceiveMessagesProofInfo( BaseMessagesProofInfo { lane_id: TEST_LANE_ID, - best_bundled_nonce: 200, + bundled_range: 101..=200, best_stored_nonce: 100, }, )), @@ -694,7 +698,7 @@ mod tests { MessagesCallInfo::ReceiveMessagesDeliveryProof(ReceiveMessagesDeliveryProofInfo( BaseMessagesProofInfo { lane_id: TEST_LANE_ID, - best_bundled_nonce: 200, + bundled_range: 101..=200, best_stored_nonce: 100, }, )), @@ -708,7 +712,7 @@ mod tests { call_info: CallInfo::Msgs(MessagesCallInfo::ReceiveMessagesProof( ReceiveMessagesProofInfo(BaseMessagesProofInfo { lane_id: TEST_LANE_ID, - best_bundled_nonce: 200, + bundled_range: 101..=200, best_stored_nonce: 100, }), )), @@ -721,7 +725,7 @@ mod tests { call_info: CallInfo::Msgs(MessagesCallInfo::ReceiveMessagesDeliveryProof( ReceiveMessagesDeliveryProofInfo(BaseMessagesProofInfo { lane_id: TEST_LANE_ID, - best_bundled_nonce: 200, + bundled_range: 101..=200, best_stored_nonce: 100, }), )), diff --git a/modules/messages/src/lib.rs b/modules/messages/src/lib.rs index 3faf00b9e75..2de5866035c 100644 --- a/modules/messages/src/lib.rs +++ b/modules/messages/src/lib.rs @@ -508,6 +508,10 @@ pub mod pallet { lane_id, ); + // TODO: https://github.com/paritytech/parity-bridges-common/issues/2020 + // we need to refund unused weight (because the inbound lane state may contain + // already confirmed messages and already rewarded relayer entries) + Ok(()) } } From 615a62971d1f7fa5bc0b17ec7ca8ae9000d580a8 Mon Sep 17 00:00:00 2001 From: Svyatoslav Nikolsky Date: Tue, 4 Apr 2023 15:27:44 +0300 Subject: [PATCH 2/5] clippy --- bin/runtime-common/src/refund_relayer_extension.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/bin/runtime-common/src/refund_relayer_extension.rs b/bin/runtime-common/src/refund_relayer_extension.rs index fa06ecbce54..7df5714265f 100644 --- a/bin/runtime-common/src/refund_relayer_extension.rs +++ b/bin/runtime-common/src/refund_relayer_extension.rs @@ -539,7 +539,7 @@ mod tests { storage_proof: vec![], lane: TestLaneId::get(), nonces_start: pallet_bridge_messages::InboundLanes::::get( - &TEST_LANE_ID, + TEST_LANE_ID, ) .last_delivered_nonce() + 1, From 9ea7ee83c53661bb2dc44e6d9ffc2e2f34e054fa Mon Sep 17 00:00:00 2001 From: Svyatoslav Nikolsky Date: Fri, 7 Apr 2023 11:50:04 +0300 Subject: [PATCH 3/5] allow empty delivery transactions with rewards confirmations BUT only when there's no room left in the unrewarded relayers vector --- bin/runtime-common/src/messages_call_ext.rs | 204 ++++++++++++++++-- .../src/refund_relayer_extension.rs | 6 + 2 files changed, 197 insertions(+), 13 deletions(-) diff --git a/bin/runtime-common/src/messages_call_ext.rs b/bin/runtime-common/src/messages_call_ext.rs index 8ab62b4f069..f82029dc91f 100644 --- a/bin/runtime-common/src/messages_call_ext.rs +++ b/bin/runtime-common/src/messages_call_ext.rs @@ -17,11 +17,15 @@ use crate::messages::{ source::FromBridgedChainMessagesDeliveryProof, target::FromBridgedChainMessagesProof, }; -use bp_messages::{LaneId, MessageNonce}; -use frame_support::{dispatch::CallableCallFor, traits::IsSubType, RuntimeDebug}; +use bp_messages::{LaneId, MessageNonce, UnrewardedRelayer}; +use frame_support::{ + dispatch::CallableCallFor, + traits::{Get, IsSubType}, + RuntimeDebug, +}; use pallet_bridge_messages::{Config, Pallet}; use sp_runtime::transaction_validity::TransactionValidity; -use sp_std::ops::RangeInclusive; +use sp_std::{collections::vec_deque::VecDeque, ops::RangeInclusive}; /// Generic info about a messages delivery/confirmation proof. #[derive(PartialEq, RuntimeDebug)] @@ -29,16 +33,38 @@ pub struct BaseMessagesProofInfo { /// Message lane, used by the call. pub lane_id: LaneId, /// Nonces of messages, included in the call. + /// + /// For delivery transaction, it is nonces of bundled messages. For confirmation + /// transaction, it is nonces that are to be confirmed during the call. pub bundled_range: RangeInclusive, /// Nonce of the best message, stored by this chain before the call is dispatched. + /// + /// For delivery transaction, it is the nonce of best delivered message before the call. + /// For confirmation transaction, it is the nonce of best confirmed message before the call. pub best_stored_nonce: MessageNonce, + /// For message delivery transactions, the number of free entries in the unrewarded relayers + /// vector before call is dispatched. + pub stored_free_unrewarded_entries: Option, } impl BaseMessagesProofInfo { /// Returns true if `bundled_range` cannot be directly appended to the `best_stored_nonce` - /// or if the `bundled_range` is empty. + /// or if the `bundled_range` is empty (unless we're confirming rewards when unrewarded + /// relayers vector is full). fn is_obsolete(&self) -> bool { - *self.bundled_range.start() != self.best_stored_nonce + 1 || self.bundled_range.is_empty() + // we allow delivery transactions without messages only when no free entries + // left in unrewarded relayers vector + if self.bundled_range.is_empty() { + let empty_transactions_allowed = self.stored_free_unrewarded_entries == Some(0); + if empty_transactions_allowed { + return false + } + + return true + } + + // otherwise we require bundled messages to continue stored range + *self.bundled_range.start() != self.best_stored_nonce + 1 } } @@ -75,12 +101,21 @@ impl, I: 'static> CallHelper { CallInfo::ReceiveMessagesProof(info) => { let inbound_lane_data = pallet_bridge_messages::InboundLanes::::get(info.0.lane_id); - inbound_lane_data.last_delivered_nonce() == info.0.best_bundled_nonce + if info.0.bundled_range.is_empty() { + if let Some(stored_free_unrewarded_entries) = + info.0.stored_free_unrewarded_entries + { + return free_unrewarded_entries::(&inbound_lane_data.relayers) > + stored_free_unrewarded_entries + } + } + + inbound_lane_data.last_delivered_nonce() == *info.0.bundled_range.end() }, CallInfo::ReceiveMessagesDeliveryProof(info) => { let outbound_lane_data = pallet_bridge_messages::OutboundLanes::::get(info.0.lane_id); - outbound_lane_data.latest_received_nonce == info.0.best_bundled_nonce + outbound_lane_data.latest_received_nonce == *info.0.bundled_range.end() }, } } @@ -140,6 +175,9 @@ impl< // be considered obsolete. bundled_range: proof.nonces_start..=proof.nonces_end, best_stored_nonce: inbound_lane_data.last_delivered_nonce(), + stored_free_unrewarded_entries: Some(free_unrewarded_entries::( + &inbound_lane_data.relayers, + )), })) } @@ -164,6 +202,7 @@ impl< bundled_range: outbound_lane_data.latest_received_nonce + 1..= relayers_state.last_delivered_nonce, best_stored_nonce: outbound_lane_data.latest_received_nonce, + stored_free_unrewarded_entries: None, })) } @@ -221,8 +260,17 @@ impl< } } +/// Returns number of free entries in the unrewarded relayers vector. +fn free_unrewarded_entries, I: 'static>( + relayers: &VecDeque>, +) -> MessageNonce { + let max_entries = T::MaxUnrewardedRelayerEntriesAtInboundLane::get(); + max_entries.saturating_sub(relayers.len() as MessageNonce) +} + #[cfg(test)] mod tests { + use super::*; use crate::{ messages::{ source::FromBridgedChainMessagesDeliveryProof, target::FromBridgedChainMessagesProof, @@ -230,11 +278,27 @@ mod tests { messages_call_ext::MessagesCallSubType, mock::{TestRuntime, ThisChainRuntimeCall}, }; - use bp_messages::UnrewardedRelayersState; + use bp_messages::{DeliveredMessages, UnrewardedRelayersState}; + use sp_std::ops::RangeInclusive; + + fn fill_unrewarded_relayers() { + let mut inbound_lane_state = + pallet_bridge_messages::InboundLanes::::get(LaneId([0, 0, 0, 0])); + for _ in 0..::MaxUnrewardedRelayerEntriesAtInboundLane::get() { + inbound_lane_state.relayers.push_back(UnrewardedRelayer { + relayer: Default::default(), + messages: DeliveredMessages { begin: 0, end: 0 }, + }); + } + pallet_bridge_messages::InboundLanes::::insert( + LaneId([0, 0, 0, 0]), + inbound_lane_state, + ); + } fn deliver_message_10() { pallet_bridge_messages::InboundLanes::::insert( - bp_messages::LaneId([0, 0, 0, 0]), + LaneId([0, 0, 0, 0]), bp_messages::InboundLaneData { relayers: Default::default(), last_confirmed_nonce: 10 }, ); } @@ -246,12 +310,13 @@ mod tests { ThisChainRuntimeCall::BridgeMessages( pallet_bridge_messages::Call::::receive_messages_proof { relayer_id_at_bridged_chain: 42, - messages_count: (nonces_end - nonces_start + 1) as u32, + messages_count: nonces_end.checked_sub(nonces_start).map(|x| x + 1).unwrap_or(0) + as u32, dispatch_weight: frame_support::weights::Weight::zero(), proof: FromBridgedChainMessagesProof { bridged_header_hash: Default::default(), storage_proof: vec![], - lane: bp_messages::LaneId([0, 0, 0, 0]), + lane: LaneId([0, 0, 0, 0]), nonces_start, nonces_end, }, @@ -301,6 +366,25 @@ mod tests { }); } + #[test] + fn extension_rejects_empty_delivery_with_rewards_confirmations_if_there_are_free_unrewarded_entries( + ) { + sp_io::TestExternalities::new(Default::default()).execute_with(|| { + deliver_message_10(); + assert!(!validate_message_delivery(10, 9)); + }); + } + + #[test] + fn extension_accepts_empty_delivery_with_rewards_confirmations_if_there_are_no_free_unrewarded_entries( + ) { + sp_io::TestExternalities::new(Default::default()).execute_with(|| { + deliver_message_10(); + fill_unrewarded_relayers(); + assert!(validate_message_delivery(10, 9)); + }); + } + #[test] fn extension_accepts_new_messages() { sp_io::TestExternalities::new(Default::default()).execute_with(|| { @@ -313,7 +397,7 @@ mod tests { fn confirm_message_10() { pallet_bridge_messages::OutboundLanes::::insert( - bp_messages::LaneId([0, 0, 0, 0]), + LaneId([0, 0, 0, 0]), bp_messages::OutboundLaneData { oldest_unpruned_nonce: 0, latest_received_nonce: 10, @@ -328,7 +412,7 @@ mod tests { proof: FromBridgedChainMessagesDeliveryProof { bridged_header_hash: Default::default(), storage_proof: Vec::new(), - lane: bp_messages::LaneId([0, 0, 0, 0]), + lane: LaneId([0, 0, 0, 0]), }, relayers_state: UnrewardedRelayersState { last_delivered_nonce, @@ -360,6 +444,15 @@ mod tests { }); } + #[test] + fn extension_rejects_empty_confirmation_even_if_there_are_no_free_unrewarded_entries() { + sp_io::TestExternalities::new(Default::default()).execute_with(|| { + confirm_message_10(); + fill_unrewarded_relayers(); + assert!(!validate_message_confirmation(10)); + }); + } + #[test] fn extension_accepts_new_confirmation() { sp_io::TestExternalities::new(Default::default()).execute_with(|| { @@ -369,4 +462,89 @@ mod tests { assert!(validate_message_confirmation(15)); }); } + + fn was_message_delivery_successful(bundled_range: RangeInclusive) -> bool { + CallHelper::::was_successful(&CallInfo::ReceiveMessagesProof( + ReceiveMessagesProofInfo(BaseMessagesProofInfo { + lane_id: LaneId([0, 0, 0, 0]), + bundled_range, + best_stored_nonce: 0, // doesn't matter for `was_successful` + stored_free_unrewarded_entries: Some(0), + }), + )) + } + + #[test] + fn was_successful_returns_false_for_failed_reward_confirmation_transaction() { + sp_io::TestExternalities::new(Default::default()).execute_with(|| { + fill_unrewarded_relayers(); + assert!(!was_message_delivery_successful(10..=9)); + }); + } + + #[test] + fn was_successful_returns_true_for_successful_reward_confirmation_transaction() { + sp_io::TestExternalities::new(Default::default()).execute_with(|| { + assert!(was_message_delivery_successful(10..=9)); + }); + } + + #[test] + fn was_successful_returns_false_for_failed_delivery() { + sp_io::TestExternalities::new(Default::default()).execute_with(|| { + deliver_message_10(); + assert!(!was_message_delivery_successful(10..=12)); + }); + } + + #[test] + fn was_successful_returns_false_for_partially_successful_delivery() { + sp_io::TestExternalities::new(Default::default()).execute_with(|| { + deliver_message_10(); + assert!(!was_message_delivery_successful(9..=12)); + }); + } + + #[test] + fn was_successful_returns_true_for_successful_delivery() { + sp_io::TestExternalities::new(Default::default()).execute_with(|| { + deliver_message_10(); + assert!(was_message_delivery_successful(9..=10)); + }); + } + + fn was_message_confirmation_successful(bundled_range: RangeInclusive) -> bool { + CallHelper::::was_successful(&CallInfo::ReceiveMessagesDeliveryProof( + ReceiveMessagesDeliveryProofInfo(BaseMessagesProofInfo { + lane_id: LaneId([0, 0, 0, 0]), + bundled_range, + best_stored_nonce: 0, // doesn't matter for `was_successful` + stored_free_unrewarded_entries: None, + }), + )) + } + + #[test] + fn was_successful_returns_false_for_failed_confirmation() { + sp_io::TestExternalities::new(Default::default()).execute_with(|| { + confirm_message_10(); + assert!(!was_message_confirmation_successful(10..=12)); + }); + } + + #[test] + fn was_successful_returns_false_for_partially_successful_confirmation() { + sp_io::TestExternalities::new(Default::default()).execute_with(|| { + confirm_message_10(); + assert!(!was_message_confirmation_successful(9..=12)); + }); + } + + #[test] + fn was_successful_returns_true_for_successful_confirmation() { + sp_io::TestExternalities::new(Default::default()).execute_with(|| { + confirm_message_10(); + assert!(was_message_confirmation_successful(9..=10)); + }); + } } diff --git a/bin/runtime-common/src/refund_relayer_extension.rs b/bin/runtime-common/src/refund_relayer_extension.rs index 0408530a5b6..0c4b0d3dd7c 100644 --- a/bin/runtime-common/src/refund_relayer_extension.rs +++ b/bin/runtime-common/src/refund_relayer_extension.rs @@ -664,6 +664,7 @@ mod tests { lane_id: TEST_LANE_ID, bundled_range: 101..=200, best_stored_nonce: 100, + stored_free_unrewarded_entries: Some(16), }, )), ), @@ -689,6 +690,7 @@ mod tests { lane_id: TEST_LANE_ID, bundled_range: 101..=200, best_stored_nonce: 100, + stored_free_unrewarded_entries: None, }, )), ), @@ -709,6 +711,7 @@ mod tests { lane_id: TEST_LANE_ID, bundled_range: 101..=200, best_stored_nonce: 100, + stored_free_unrewarded_entries: Some(16), }, )), ), @@ -729,6 +732,7 @@ mod tests { lane_id: TEST_LANE_ID, bundled_range: 101..=200, best_stored_nonce: 100, + stored_free_unrewarded_entries: None, }, )), ), @@ -743,6 +747,7 @@ mod tests { lane_id: TEST_LANE_ID, bundled_range: 101..=200, best_stored_nonce: 100, + stored_free_unrewarded_entries: Some(16), }), )), } @@ -756,6 +761,7 @@ mod tests { lane_id: TEST_LANE_ID, bundled_range: 101..=200, best_stored_nonce: 100, + stored_free_unrewarded_entries: None, }), )), } From ebad370aad134d7d2bb6260ac16d56444675dea9 Mon Sep 17 00:00:00 2001 From: Svyatoslav Nikolsky Date: Fri, 7 Apr 2023 12:52:55 +0300 Subject: [PATCH 4/5] clippy --- bin/runtime-common/src/messages_call_ext.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/bin/runtime-common/src/messages_call_ext.rs b/bin/runtime-common/src/messages_call_ext.rs index f82029dc91f..d81786482c4 100644 --- a/bin/runtime-common/src/messages_call_ext.rs +++ b/bin/runtime-common/src/messages_call_ext.rs @@ -475,6 +475,7 @@ mod tests { } #[test] + #[allow(clippy::reversed_empty_ranges)] fn was_successful_returns_false_for_failed_reward_confirmation_transaction() { sp_io::TestExternalities::new(Default::default()).execute_with(|| { fill_unrewarded_relayers(); @@ -483,6 +484,7 @@ mod tests { } #[test] + #[allow(clippy::reversed_empty_ranges)] fn was_successful_returns_true_for_successful_reward_confirmation_transaction() { sp_io::TestExternalities::new(Default::default()).execute_with(|| { assert!(was_message_delivery_successful(10..=9)); From abfc9a67947705d266c53f99b3e2e333d72fa8cb Mon Sep 17 00:00:00 2001 From: Svyatoslav Nikolsky Date: Mon, 10 Apr 2023 11:22:06 +0300 Subject: [PATCH 5/5] allow empty delivery transactions if no message slots in unrewarded relayers vector --- bin/runtime-common/src/messages_call_ext.rs | 155 +++++++++++++----- bin/runtime-common/src/mock.rs | 8 +- .../src/refund_relayer_extension.rs | 22 ++- 3 files changed, 139 insertions(+), 46 deletions(-) diff --git a/bin/runtime-common/src/messages_call_ext.rs b/bin/runtime-common/src/messages_call_ext.rs index d81786482c4..b299d758a32 100644 --- a/bin/runtime-common/src/messages_call_ext.rs +++ b/bin/runtime-common/src/messages_call_ext.rs @@ -17,7 +17,7 @@ use crate::messages::{ source::FromBridgedChainMessagesDeliveryProof, target::FromBridgedChainMessagesProof, }; -use bp_messages::{LaneId, MessageNonce, UnrewardedRelayer}; +use bp_messages::{InboundLaneData, LaneId, MessageNonce}; use frame_support::{ dispatch::CallableCallFor, traits::{Get, IsSubType}, @@ -25,7 +25,7 @@ use frame_support::{ }; use pallet_bridge_messages::{Config, Pallet}; use sp_runtime::transaction_validity::TransactionValidity; -use sp_std::{collections::vec_deque::VecDeque, ops::RangeInclusive}; +use sp_std::ops::RangeInclusive; /// Generic info about a messages delivery/confirmation proof. #[derive(PartialEq, RuntimeDebug)] @@ -42,9 +42,19 @@ pub struct BaseMessagesProofInfo { /// For delivery transaction, it is the nonce of best delivered message before the call. /// For confirmation transaction, it is the nonce of best confirmed message before the call. pub best_stored_nonce: MessageNonce, - /// For message delivery transactions, the number of free entries in the unrewarded relayers - /// vector before call is dispatched. - pub stored_free_unrewarded_entries: Option, + /// For message delivery transactions, the state of unrewarded relayers vector before the + /// call is dispatched. + pub unrewarded_relayers: Option, +} + +/// Occupation state of the unrewarded relayers vector. +#[derive(PartialEq, RuntimeDebug)] +#[cfg_attr(test, derive(Default))] +pub struct UnrewardedRelayerOccupation { + /// The number of remaining unoccupied entries for new relayers. + pub free_relayer_slots: MessageNonce, + /// The number of messages that we are ready to accept. + pub free_message_slots: MessageNonce, } impl BaseMessagesProofInfo { @@ -52,10 +62,20 @@ impl BaseMessagesProofInfo { /// or if the `bundled_range` is empty (unless we're confirming rewards when unrewarded /// relayers vector is full). fn is_obsolete(&self) -> bool { - // we allow delivery transactions without messages only when no free entries - // left in unrewarded relayers vector + // transactions with zero bundled nonces are not allowed, unless they're message + // delivery transactions, which brings reward confirmations required to unblock + // the lane if self.bundled_range.is_empty() { - let empty_transactions_allowed = self.stored_free_unrewarded_entries == Some(0); + let (free_relayer_slots, free_message_slots) = self + .unrewarded_relayers + .as_ref() + .map(|s| (s.free_relayer_slots, s.free_message_slots)) + .unwrap_or((MessageNonce::MAX, MessageNonce::MAX)); + let empty_transactions_allowed = + // we allow empty transactions when we can't accept delivery from new relayers + free_relayer_slots == 0 || + // or if we can't accept new messages at all + free_message_slots == 0; if empty_transactions_allowed { return false } @@ -102,11 +122,21 @@ impl, I: 'static> CallHelper { let inbound_lane_data = pallet_bridge_messages::InboundLanes::::get(info.0.lane_id); if info.0.bundled_range.is_empty() { - if let Some(stored_free_unrewarded_entries) = - info.0.stored_free_unrewarded_entries - { - return free_unrewarded_entries::(&inbound_lane_data.relayers) > - stored_free_unrewarded_entries + match info.0.unrewarded_relayers { + Some(ref pre_occupation) => { + let post_occupation = + unrewarded_relayers_occupation::(&inbound_lane_data); + // we don't care about `free_relayer_slots` here - it is checked in + // `is_obsolete` and every relayer has delivered at least one message, + // so if relayer slots are released, then message slots are also + // released + return post_occupation.free_message_slots > + pre_occupation.free_message_slots + }, + None => { + // shouldn't happen in practice, given our code + return false + }, } } @@ -175,8 +205,8 @@ impl< // be considered obsolete. bundled_range: proof.nonces_start..=proof.nonces_end, best_stored_nonce: inbound_lane_data.last_delivered_nonce(), - stored_free_unrewarded_entries: Some(free_unrewarded_entries::( - &inbound_lane_data.relayers, + unrewarded_relayers: Some(unrewarded_relayers_occupation::( + &inbound_lane_data, )), })) } @@ -202,7 +232,7 @@ impl< bundled_range: outbound_lane_data.latest_received_nonce + 1..= relayers_state.last_delivered_nonce, best_stored_nonce: outbound_lane_data.latest_received_nonce, - stored_free_unrewarded_entries: None, + unrewarded_relayers: None, })) } @@ -260,12 +290,22 @@ impl< } } -/// Returns number of free entries in the unrewarded relayers vector. -fn free_unrewarded_entries, I: 'static>( - relayers: &VecDeque>, -) -> MessageNonce { - let max_entries = T::MaxUnrewardedRelayerEntriesAtInboundLane::get(); - max_entries.saturating_sub(relayers.len() as MessageNonce) +/// Returns occupation state of unrewarded relayers vector. +fn unrewarded_relayers_occupation, I: 'static>( + inbound_lane_data: &InboundLaneData, +) -> UnrewardedRelayerOccupation { + UnrewardedRelayerOccupation { + free_relayer_slots: T::MaxUnrewardedRelayerEntriesAtInboundLane::get() + .saturating_sub(inbound_lane_data.relayers.len() as MessageNonce), + free_message_slots: { + // 5 - 0 = 5 ==> 1,2,3,4,5 + // 5 - 3 = 2 ==> 4,5 + let unconfirmed_messages = inbound_lane_data + .last_delivered_nonce() + .saturating_sub(inbound_lane_data.last_confirmed_nonce); + T::MaxUnconfirmedMessagesAtInboundLane::get().saturating_sub(unconfirmed_messages) + }, + } } #[cfg(test)] @@ -276,18 +316,21 @@ mod tests { source::FromBridgedChainMessagesDeliveryProof, target::FromBridgedChainMessagesProof, }, messages_call_ext::MessagesCallSubType, - mock::{TestRuntime, ThisChainRuntimeCall}, + mock::{ + MaxUnconfirmedMessagesAtInboundLane, MaxUnrewardedRelayerEntriesAtInboundLane, + TestRuntime, ThisChainRuntimeCall, + }, }; - use bp_messages::{DeliveredMessages, UnrewardedRelayersState}; + use bp_messages::{DeliveredMessages, UnrewardedRelayer, UnrewardedRelayersState}; use sp_std::ops::RangeInclusive; fn fill_unrewarded_relayers() { let mut inbound_lane_state = pallet_bridge_messages::InboundLanes::::get(LaneId([0, 0, 0, 0])); - for _ in 0..::MaxUnrewardedRelayerEntriesAtInboundLane::get() { + for n in 0..MaxUnrewardedRelayerEntriesAtInboundLane::get() { inbound_lane_state.relayers.push_back(UnrewardedRelayer { relayer: Default::default(), - messages: DeliveredMessages { begin: 0, end: 0 }, + messages: DeliveredMessages { begin: n + 1, end: n + 1 }, }); } pallet_bridge_messages::InboundLanes::::insert( @@ -296,6 +339,22 @@ mod tests { ); } + fn fill_unrewarded_messages() { + let mut inbound_lane_state = + pallet_bridge_messages::InboundLanes::::get(LaneId([0, 0, 0, 0])); + inbound_lane_state.relayers.push_back(UnrewardedRelayer { + relayer: Default::default(), + messages: DeliveredMessages { + begin: 1, + end: MaxUnconfirmedMessagesAtInboundLane::get(), + }, + }); + pallet_bridge_messages::InboundLanes::::insert( + LaneId([0, 0, 0, 0]), + inbound_lane_state, + ); + } + fn deliver_message_10() { pallet_bridge_messages::InboundLanes::::insert( LaneId([0, 0, 0, 0]), @@ -367,7 +426,7 @@ mod tests { } #[test] - fn extension_rejects_empty_delivery_with_rewards_confirmations_if_there_are_free_unrewarded_entries( + fn extension_rejects_empty_delivery_with_rewards_confirmations_if_there_are_free_relayer_and_message_slots( ) { sp_io::TestExternalities::new(Default::default()).execute_with(|| { deliver_message_10(); @@ -376,7 +435,7 @@ mod tests { } #[test] - fn extension_accepts_empty_delivery_with_rewards_confirmations_if_there_are_no_free_unrewarded_entries( + fn extension_accepts_empty_delivery_with_rewards_confirmations_if_there_are_no_free_relayer_slots( ) { sp_io::TestExternalities::new(Default::default()).execute_with(|| { deliver_message_10(); @@ -385,6 +444,18 @@ mod tests { }); } + #[test] + fn extension_accepts_empty_delivery_with_rewards_confirmations_if_there_are_no_free_message_slots( + ) { + sp_io::TestExternalities::new(Default::default()).execute_with(|| { + fill_unrewarded_messages(); + assert!(validate_message_delivery( + MaxUnconfirmedMessagesAtInboundLane::get(), + MaxUnconfirmedMessagesAtInboundLane::get() - 1 + )); + }); + } + #[test] fn extension_accepts_new_messages() { sp_io::TestExternalities::new(Default::default()).execute_with(|| { @@ -463,13 +534,23 @@ mod tests { }); } - fn was_message_delivery_successful(bundled_range: RangeInclusive) -> bool { + fn was_message_delivery_successful( + bundled_range: RangeInclusive, + is_empty: bool, + ) -> bool { CallHelper::::was_successful(&CallInfo::ReceiveMessagesProof( ReceiveMessagesProofInfo(BaseMessagesProofInfo { lane_id: LaneId([0, 0, 0, 0]), bundled_range, best_stored_nonce: 0, // doesn't matter for `was_successful` - stored_free_unrewarded_entries: Some(0), + unrewarded_relayers: Some(UnrewardedRelayerOccupation { + free_relayer_slots: 0, // doesn't matter for `was_successful` + free_message_slots: if is_empty { + 0 + } else { + MaxUnconfirmedMessagesAtInboundLane::get() + }, + }), }), )) } @@ -478,8 +559,8 @@ mod tests { #[allow(clippy::reversed_empty_ranges)] fn was_successful_returns_false_for_failed_reward_confirmation_transaction() { sp_io::TestExternalities::new(Default::default()).execute_with(|| { - fill_unrewarded_relayers(); - assert!(!was_message_delivery_successful(10..=9)); + fill_unrewarded_messages(); + assert!(!was_message_delivery_successful(10..=9, true)); }); } @@ -487,7 +568,7 @@ mod tests { #[allow(clippy::reversed_empty_ranges)] fn was_successful_returns_true_for_successful_reward_confirmation_transaction() { sp_io::TestExternalities::new(Default::default()).execute_with(|| { - assert!(was_message_delivery_successful(10..=9)); + assert!(was_message_delivery_successful(10..=9, true)); }); } @@ -495,7 +576,7 @@ mod tests { fn was_successful_returns_false_for_failed_delivery() { sp_io::TestExternalities::new(Default::default()).execute_with(|| { deliver_message_10(); - assert!(!was_message_delivery_successful(10..=12)); + assert!(!was_message_delivery_successful(10..=12, false)); }); } @@ -503,7 +584,7 @@ mod tests { fn was_successful_returns_false_for_partially_successful_delivery() { sp_io::TestExternalities::new(Default::default()).execute_with(|| { deliver_message_10(); - assert!(!was_message_delivery_successful(9..=12)); + assert!(!was_message_delivery_successful(9..=12, false)); }); } @@ -511,7 +592,7 @@ mod tests { fn was_successful_returns_true_for_successful_delivery() { sp_io::TestExternalities::new(Default::default()).execute_with(|| { deliver_message_10(); - assert!(was_message_delivery_successful(9..=10)); + assert!(was_message_delivery_successful(9..=10, false)); }); } @@ -521,7 +602,7 @@ mod tests { lane_id: LaneId([0, 0, 0, 0]), bundled_range, best_stored_nonce: 0, // doesn't matter for `was_successful` - stored_free_unrewarded_entries: None, + unrewarded_relayers: None, }), )) } diff --git a/bin/runtime-common/src/mock.rs b/bin/runtime-common/src/mock.rs index c4c7c2fa8ac..967682ccbc5 100644 --- a/bin/runtime-common/src/mock.rs +++ b/bin/runtime-common/src/mock.rs @@ -33,7 +33,7 @@ use crate::messages::{ }; use bp_header_chain::{ChainWithGrandpa, HeaderChain}; -use bp_messages::{target_chain::ForbidInboundMessages, LaneId}; +use bp_messages::{target_chain::ForbidInboundMessages, LaneId, MessageNonce}; use bp_parachains::SingleParaStoredHeaderDataBuilder; use bp_runtime::{Chain, ChainId, Parachain, UnderlyingChainProvider}; use codec::{Decode, Encode}; @@ -126,6 +126,8 @@ parameter_types! { pub AdjustmentVariable: Multiplier = Multiplier::saturating_from_rational(3, 100_000); pub MinimumMultiplier: Multiplier = Multiplier::saturating_from_rational(1, 1_000_000u128); pub MaximumMultiplier: Multiplier = sp_runtime::traits::Bounded::max_value(); + pub const MaxUnrewardedRelayerEntriesAtInboundLane: MessageNonce = 16; + pub const MaxUnconfirmedMessagesAtInboundLane: MessageNonce = 32; } impl frame_system::Config for TestRuntime { @@ -216,8 +218,8 @@ impl pallet_bridge_messages::Config for TestRuntime { type RuntimeEvent = RuntimeEvent; type WeightInfo = pallet_bridge_messages::weights::BridgeWeight; type ActiveOutboundLanes = ActiveOutboundLanes; - type MaxUnrewardedRelayerEntriesAtInboundLane = ConstU64<16>; - type MaxUnconfirmedMessagesAtInboundLane = ConstU64<16>; + type MaxUnrewardedRelayerEntriesAtInboundLane = MaxUnrewardedRelayerEntriesAtInboundLane; + type MaxUnconfirmedMessagesAtInboundLane = MaxUnconfirmedMessagesAtInboundLane; type MaximalOutboundPayloadSize = FromThisChainMaximalOutboundPayloadSize; type OutboundPayload = FromThisChainMessagePayload; diff --git a/bin/runtime-common/src/refund_relayer_extension.rs b/bin/runtime-common/src/refund_relayer_extension.rs index 0c4b0d3dd7c..ebfc0b33466 100644 --- a/bin/runtime-common/src/refund_relayer_extension.rs +++ b/bin/runtime-common/src/refund_relayer_extension.rs @@ -461,6 +461,7 @@ mod tests { }, messages_call_ext::{ BaseMessagesProofInfo, ReceiveMessagesDeliveryProofInfo, ReceiveMessagesProofInfo, + UnrewardedRelayerOccupation, }, mock::*, }; @@ -664,7 +665,10 @@ mod tests { lane_id: TEST_LANE_ID, bundled_range: 101..=200, best_stored_nonce: 100, - stored_free_unrewarded_entries: Some(16), + unrewarded_relayers: Some(UnrewardedRelayerOccupation { + free_relayer_slots: MaxUnrewardedRelayerEntriesAtInboundLane::get(), + free_message_slots: MaxUnconfirmedMessagesAtInboundLane::get(), + }), }, )), ), @@ -690,7 +694,7 @@ mod tests { lane_id: TEST_LANE_ID, bundled_range: 101..=200, best_stored_nonce: 100, - stored_free_unrewarded_entries: None, + unrewarded_relayers: None, }, )), ), @@ -711,7 +715,10 @@ mod tests { lane_id: TEST_LANE_ID, bundled_range: 101..=200, best_stored_nonce: 100, - stored_free_unrewarded_entries: Some(16), + unrewarded_relayers: Some(UnrewardedRelayerOccupation { + free_relayer_slots: MaxUnrewardedRelayerEntriesAtInboundLane::get(), + free_message_slots: MaxUnconfirmedMessagesAtInboundLane::get(), + }), }, )), ), @@ -732,7 +739,7 @@ mod tests { lane_id: TEST_LANE_ID, bundled_range: 101..=200, best_stored_nonce: 100, - stored_free_unrewarded_entries: None, + unrewarded_relayers: None, }, )), ), @@ -747,7 +754,10 @@ mod tests { lane_id: TEST_LANE_ID, bundled_range: 101..=200, best_stored_nonce: 100, - stored_free_unrewarded_entries: Some(16), + unrewarded_relayers: Some(UnrewardedRelayerOccupation { + free_relayer_slots: MaxUnrewardedRelayerEntriesAtInboundLane::get(), + free_message_slots: MaxUnconfirmedMessagesAtInboundLane::get(), + }), }), )), } @@ -761,7 +771,7 @@ mod tests { lane_id: TEST_LANE_ID, bundled_range: 101..=200, best_stored_nonce: 100, - stored_free_unrewarded_entries: None, + unrewarded_relayers: None, }), )), }