Skip to content

Commit

Permalink
refund extra weight in receive_messages_delivery_proof call (parityte…
Browse files Browse the repository at this point in the history 
…ch#2031)
  • Loading branch information
@svyatonik
svyatonik committed Apr 12, 2023
1 parent 77f1641 commit 3a2311b
Show file tree
Hide file tree
Showing 4 changed files with 104 additions and 43 deletions.
120 changes: 84 additions & 36 deletions modules/messages/src/lib.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ use bp_messages::{
MessageKey, MessageNonce, MessagePayload, MessagesOperatingMode, OutboundLaneData,
OutboundMessageDetails, UnrewardedRelayersState,
};
use bp_runtime::{BasicOperatingMode, ChainId, OwnedBridgeModule, Size};
use bp_runtime::{BasicOperatingMode, ChainId, OwnedBridgeModule, PreComputedSize, Size};
use codec::{Decode, Encode, MaxEncodedLen};
use frame_support::{dispatch::PostDispatchInfo, ensure, fail, traits::Get};
use sp_runtime::{traits::UniqueSaturatedFrom, SaturatedConversion};
Expand Down Expand Up @@ -423,10 +423,11 @@ pub mod pallet {
pub fn receive_messages_delivery_proof(
origin: OriginFor<T>,
proof: MessagesDeliveryProofOf<T, I>,
relayers_state: UnrewardedRelayersState,
) -> DispatchResult {
mut relayers_state: UnrewardedRelayersState,
) -> DispatchResultWithPostInfo {
Self::ensure_not_halted().map_err(Error::<T, I>::BridgeModule)?;

let proof_size = proof.size();
let confirmation_relayer = ensure_signed(origin)?;
let (lane_id, lane_data) = T::TargetHeaderChain::verify_messages_delivery_proof(proof)
.map_err(|err| {
Expand Down Expand Up @@ -499,13 +500,27 @@ pub mod pallet {
});

// if some new messages have been confirmed, reward relayers
T::DeliveryConfirmationPayments::pay_reward(
let actually_rewarded_relayers = T::DeliveryConfirmationPayments::pay_reward(
lane_id,
lane_data.relayers,
&confirmation_relayer,
&received_range,
);
}

// update relayers state with actual numbers to compute actual weight below
relayers_state.unrewarded_relayer_entries = sp_std::cmp::min(
relayers_state.unrewarded_relayer_entries,
actually_rewarded_relayers,
);
relayers_state.total_messages = sp_std::cmp::min(
relayers_state.total_messages,
received_range
.end()
.checked_sub(*received_range.start())
.and_then(|x| x.checked_add(1))
.unwrap_or(MessageNonce::MAX),
);
};

log::trace!(
target: LOG_TARGET,
Expand All @@ -514,11 +529,15 @@ 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)
// because of lags, the inbound lane state (`lane_data`) may have entries for
// already rewarded relayers and messages (if all entries are duplicated, then
// this transaction must be filtered out by our signed extension)
let actual_weight = T::WeightInfo::receive_messages_delivery_proof_weight(
&PreComputedSize(proof_size as usize),
&relayers_state,
);

Ok(())
Ok(PostDispatchInfo { actual_weight: Some(actual_weight), pays_fee: Pays::Yes })
}
}

Expand Down Expand Up @@ -940,8 +959,9 @@ mod tests {
message, message_payload, run_test, unrewarded_relayer, AccountId, DbWeight,
RuntimeEvent as TestEvent, RuntimeOrigin, TestDeliveryConfirmationPayments,
TestDeliveryPayments, TestMessagesDeliveryProof, TestMessagesProof, TestRelayer,
TestRuntime, MAX_OUTBOUND_PAYLOAD_SIZE, PAYLOAD_REJECTED_BY_TARGET_CHAIN, REGULAR_PAYLOAD,
TEST_LANE_ID, TEST_LANE_ID_2, TEST_LANE_ID_3, TEST_RELAYER_A, TEST_RELAYER_B,
TestRuntime, TestWeightInfo, MAX_OUTBOUND_PAYLOAD_SIZE, PAYLOAD_REJECTED_BY_TARGET_CHAIN,
REGULAR_PAYLOAD, TEST_LANE_ID, TEST_LANE_ID_2, TEST_LANE_ID_3, TEST_RELAYER_A,
TEST_RELAYER_B,
};
use bp_messages::{BridgeMessagesCall, UnrewardedRelayer, UnrewardedRelayersState};
use bp_test_utils::generate_owned_bridge_module_tests;
Expand Down Expand Up @@ -1398,50 +1418,78 @@ mod tests {
));

// this reports delivery of message 1 => reward is paid to TEST_RELAYER_A
assert_ok!(Pallet::<TestRuntime>::receive_messages_delivery_proof(
let single_message_delivery_proof = TestMessagesDeliveryProof(Ok((
TEST_LANE_ID,
InboundLaneData {
relayers: vec![unrewarded_relayer(1, 1, TEST_RELAYER_A)].into_iter().collect(),
..Default::default()
},
)));
let single_message_delivery_proof_size = single_message_delivery_proof.size();
let result = Pallet::<TestRuntime>::receive_messages_delivery_proof(
RuntimeOrigin::signed(1),
TestMessagesDeliveryProof(Ok((
TEST_LANE_ID,
InboundLaneData {
relayers: vec![unrewarded_relayer(1, 1, TEST_RELAYER_A)]
.into_iter()
.collect(),
..Default::default()
}
))),
single_message_delivery_proof,
UnrewardedRelayersState {
unrewarded_relayer_entries: 1,
total_messages: 1,
last_delivered_nonce: 1,
..Default::default()
},
));
);
assert_ok!(result);
assert_eq!(
result.unwrap().actual_weight.unwrap(),
TestWeightInfo::receive_messages_delivery_proof_weight(
&PreComputedSize(single_message_delivery_proof_size as _),
&UnrewardedRelayersState {
unrewarded_relayer_entries: 1,
total_messages: 1,
..Default::default()
},
)
);
assert!(TestDeliveryConfirmationPayments::is_reward_paid(TEST_RELAYER_A, 1));
assert!(!TestDeliveryConfirmationPayments::is_reward_paid(TEST_RELAYER_B, 1));

// this reports delivery of both message 1 and message 2 => reward is paid only to
// TEST_RELAYER_B
assert_ok!(Pallet::<TestRuntime>::receive_messages_delivery_proof(
let two_messages_delivery_proof = TestMessagesDeliveryProof(Ok((
TEST_LANE_ID,
InboundLaneData {
relayers: vec![
unrewarded_relayer(1, 1, TEST_RELAYER_A),
unrewarded_relayer(2, 2, TEST_RELAYER_B),
]
.into_iter()
.collect(),
..Default::default()
},
)));
let two_messages_delivery_proof_size = two_messages_delivery_proof.size();
let result = Pallet::<TestRuntime>::receive_messages_delivery_proof(
RuntimeOrigin::signed(1),
TestMessagesDeliveryProof(Ok((
TEST_LANE_ID,
InboundLaneData {
relayers: vec![
unrewarded_relayer(1, 1, TEST_RELAYER_A),
unrewarded_relayer(2, 2, TEST_RELAYER_B)
]
.into_iter()
.collect(),
..Default::default()
}
))),
two_messages_delivery_proof,
UnrewardedRelayersState {
unrewarded_relayer_entries: 2,
total_messages: 2,
last_delivered_nonce: 2,
..Default::default()
},
));
);
assert_ok!(result);
// even though the pre-dispatch weight was for two messages, the actual weight is
// for single message only
assert_eq!(
result.unwrap().actual_weight.unwrap(),
TestWeightInfo::receive_messages_delivery_proof_weight(
&PreComputedSize(two_messages_delivery_proof_size as _),
&UnrewardedRelayersState {
unrewarded_relayer_entries: 1,
total_messages: 1,
..Default::default()
},
)
);
assert!(!TestDeliveryConfirmationPayments::is_reward_paid(TEST_RELAYER_A, 1));
assert!(TestDeliveryConfirmationPayments::is_reward_paid(TEST_RELAYER_B, 1));
});
Expand Down
10 changes: 8 additions & 2 deletions modules/messages/src/mock.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -148,9 +148,12 @@ parameter_types! {
pub const ActiveOutboundLanes: &'static [LaneId] = &[TEST_LANE_ID, TEST_LANE_ID_2];
}

/// weights of messages pallet calls we use in tests.
pub type TestWeightInfo = ();

impl Config for TestRuntime {
type RuntimeEvent = RuntimeEvent;
type WeightInfo = ();
type WeightInfo = TestWeightInfo;
type ActiveOutboundLanes = ActiveOutboundLanes;
type MaxUnrewardedRelayerEntriesAtInboundLane = MaxUnrewardedRelayerEntriesAtInboundLane;
type MaxUnconfirmedMessagesAtInboundLane = MaxUnconfirmedMessagesAtInboundLane;
Expand Down Expand Up @@ -381,12 +384,15 @@ impl DeliveryConfirmationPayments<AccountId> for TestDeliveryConfirmationPayment
messages_relayers: VecDeque<UnrewardedRelayer<AccountId>>,
_confirmation_relayer: &AccountId,
received_range: &RangeInclusive<MessageNonce>,
) {
) -> MessageNonce {
let relayers_rewards = calc_relayers_rewards(messages_relayers, received_range);
let rewarded_relayers = relayers_rewards.len();
for (relayer, reward) in &relayers_rewards {
let key = (b":relayer-reward:", relayer, reward).encode();
frame_support::storage::unhashed::put(&key, &true);
}

rewarded_relayers as _
}
}

Expand Down
7 changes: 5 additions & 2 deletions modules/relayers/src/payment_adapter.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ use crate::{Config, Pallet};

use bp_messages::{
source_chain::{DeliveryConfirmationPayments, RelayersRewards},
LaneId,
LaneId, MessageNonce,
};
use bp_relayers::{RewardsAccountOwner, RewardsAccountParams};
use frame_support::{sp_runtime::SaturatedConversion, traits::Get};
Expand All @@ -47,9 +47,10 @@ where
messages_relayers: VecDeque<bp_messages::UnrewardedRelayer<T::AccountId>>,
confirmation_relayer: &T::AccountId,
received_range: &RangeInclusive<bp_messages::MessageNonce>,
) {
) -> MessageNonce {
let relayers_rewards =
bp_messages::calc_relayers_rewards::<T::AccountId>(messages_relayers, received_range);
let rewarded_relayers = relayers_rewards.len();

register_relayers_rewards::<T>(
confirmation_relayer,
Expand All @@ -61,6 +62,8 @@ where
),
DeliveryReward::get(),
);

rewarded_relayers as _
}
}

Expand Down
10 changes: 7 additions & 3 deletions primitives/messages/src/source_chain.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -98,12 +98,14 @@ pub trait DeliveryConfirmationPayments<AccountId> {
///
/// The implementation may also choose to pay reward to the `confirmation_relayer`, which is
/// a relayer that has submitted delivery confirmation transaction.
///
/// Returns number of actually rewarded relayers.
fn pay_reward(
lane_id: LaneId,
messages_relayers: VecDeque<UnrewardedRelayer<AccountId>>,
confirmation_relayer: &AccountId,
received_range: &RangeInclusive<MessageNonce>,
);
) -> MessageNonce;
}

impl<AccountId> DeliveryConfirmationPayments<AccountId> for () {
Expand All @@ -114,8 +116,9 @@ impl<AccountId> DeliveryConfirmationPayments<AccountId> for () {
_messages_relayers: VecDeque<UnrewardedRelayer<AccountId>>,
_confirmation_relayer: &AccountId,
_received_range: &RangeInclusive<MessageNonce>,
) {
) -> MessageNonce {
// this implementation is not rewarding relayers at all
0
}
}

Expand Down Expand Up @@ -202,6 +205,7 @@ impl<AccountId> DeliveryConfirmationPayments<AccountId> for ForbidOutboundMessag
_messages_relayers: VecDeque<UnrewardedRelayer<AccountId>>,
_confirmation_relayer: &AccountId,
_received_range: &RangeInclusive<MessageNonce>,
) {
) -> MessageNonce {
0
}
}

0 comments on commit 3a2311b

Please sign in to comment.