LPv2: Fix weights with batch messages #1951

lemunozm · 2024-08-07T17:36:17Z

Description

Add max_processing_weight() method to MessageProcessor that allows to know upfront the worst case for processing a message.
Check the worst case before processing.
Adapt weight to work fine with batches

TODOS

This should be merged first: LPv2: Batch message logic (r2) #1949
We need to add on_idle tests for the event pallet. Will do it in this PR later

lemunozm

I leave some comment justifiying the changes, which could be not easy to understand.

lemunozm · 2024-08-07T17:37:10Z

pallets/liquidity-pools-gateway/queue/src/lib.rs

+		#[pallet::weight(MessageQueue::<T>::get(nonce)
+            .map(|msg| T::MessageProcessor::max_processing_weight(&msg))
+            .unwrap_or(T::DbWeight::get().reads(1)))]
 		#[pallet::call_index(0)]
-		pub fn process_message(origin: OriginFor<T>, nonce: T::MessageNonce) -> DispatchResult {
+		pub fn process_message(


We compute the max required weight for the specific message upfront. Later the extrinsic could consume less using PostInfo

lemunozm · 2024-08-07T17:38:39Z

pallets/liquidity-pools-gateway/queue/src/lib.rs

+				let remaining_weight = max_weight.saturating_sub(weight_used);
+				let next_weight = T::MessageProcessor::max_processing_weight(&message);
+
+				// We ensure we have still capacity in the block to process the message
+				if remaining_weight.all_gte(next_weight) {
+					break;
+				}


We need to be sure we have capacity in the block before we start to compute the message. We can not add the weight later, it could be too late (even more risky with a batch with a lot of sub-messages)

lemunozm · 2024-08-07T17:39:11Z

pallets/liquidity-pools-gateway/queue/src/weights.rs

-use cfg_primitives::LP_DEFENSIVE_WEIGHT;
-use frame_support::weights::Weight;
-
-pub trait WeightInfo {


No longer needed, they use T::MessageProcessor::max_processing_weight() instead

lemunozm · 2024-08-07T17:40:07Z

pallets/liquidity-pools-gateway/src/lib.rs

-			// TODO: do we really need to return the weights in send() if later we use the
-			// defensive ones?
-			let (result, router_weight) = match router.send(sender, serialized) {
+			let (result, router_weight) = match router.send(sender, message.serialize()) {
 				Ok(dispatch_info) => (Ok(()), dispatch_info.actual_weight),
 				Err(e) => (Err(e.error), e.post_info.actual_weight),
 			};

-			(
-				result,
-				router_weight
-					.unwrap_or(Weight::from_parts(LP_DEFENSIVE_WEIGHT_REF_TIME, 0))
-					.saturating_add(read_weight)
-					.saturating_add(Weight::from_parts(0, serialized_len)),
-			)


After investigating this. We delegate the processing weight to the router, which should take care of the bytes sent. In case we need some extra weight, it should be added to the router.

BTW, We need to check the current XcmDomain::overall_weight used in production to be sure it's higher than our needs

lemunozm · 2024-08-07T17:42:18Z

pallets/liquidity-pools-gateway/src/lib.rs

+		fn max_processing_weight(msg: &Self::Message) -> Weight {
+			match msg {
+				GatewayMessage::Inbound { message, .. } => {
+					LP_DEFENSIVE_WEIGHT.saturating_mul(message.unpack().len() as u64)
+				}
+				GatewayMessage::Outbound { .. } => LP_DEFENSIVE_WEIGHT, /* TODO can be much lower */
+			}
+		}


This is the required available weight the block should have to start processing the message. I think the outbound case can be much less, but not sure how many. We can leave it as it is because later, the real weights coming from the router will be used.

codecov · 2024-08-08T12:47:22Z

Codecov Report

Attention: Patch coverage is 76.00000% with 6 lines in your changes missing coverage. Please review.

Project coverage is 47.97%. Comparing base (4bfeabe) to head (cac1931).

Files	Patch %	Lines
pallets/liquidity-pools-gateway/src/lib.rs	28.57%	5 Missing ⚠️
pallets/liquidity-pools-gateway/queue/src/lib.rs	92.30%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1951      +/-   ##
==========================================
+ Coverage   47.83%   47.97%   +0.14%     
==========================================
  Files         183      183              
  Lines       12904    12908       +4     
==========================================
+ Hits         6172     6192      +20     
+ Misses       6732     6716      -16

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

cdamian

Would've loved to avoid some of the conflicts here but it's too late for that now. Let's get this in asap so I can do another rebase.

lemunozm · 2024-08-09T08:42:36Z

You should not be too many, because the parts I modify you don't 🤞🏻. @wischli tell me if you have some comments regarding this post-mortem, to merge this ASAP for @cdamian

wischli

Post mortem: Would love to use defensive conversion for production code (NIT). Otherwise clean and beautiful as always!

wischli · 2024-08-09T09:54:25Z

pallets/liquidity-pools-gateway/src/lib.rs

+		fn max_processing_weight(msg: &Self::Message) -> Weight {
+			match msg {
+				GatewayMessage::Inbound { message, .. } => {
+					LP_DEFENSIVE_WEIGHT.saturating_mul(message.submessages().len() as u64)


Nit: For production code, I would like to avoid casting with as and rather use the defensive saturated_into.

Only when converting from usize to u64, I use as because our machine target is no more than u64 (are they machines longer than that?)

But it seems reasonable; I'll add it to my mental list of changes!

lemunozm requested review from cdamian and wischli August 7, 2024 17:36

lemunozm self-assigned this Aug 7, 2024

lemunozm commented Aug 7, 2024

View reviewed changes

lemunozm force-pushed the lpv2/fix-weights branch from 2bafd7b to d28c780 Compare August 7, 2024 17:46

lemunozm mentioned this pull request Aug 8, 2024

LPv2: Batch message logic (r2) #1949

Merged

lemunozm force-pushed the lpv2/fix-weights branch 2 times, most recently from 1f1356c to 3eb4bff Compare August 8, 2024 09:35

lemunozm changed the base branch from lpv2/batch-logic-r2 to main August 8, 2024 09:35

lemunozm marked this pull request as ready for review August 8, 2024 09:35

lemunozm added 3 commits August 8, 2024 13:02

fix weights with limits

14a9e21

tests on_idle with weights

45f7a7d

fix namings

1a54c40

lemunozm force-pushed the lpv2/fix-weights branch from 3eb4bff to 1a54c40 Compare August 8, 2024 11:06

fix clippy

cac1931

cdamian approved these changes Aug 9, 2024

View reviewed changes

lemunozm merged commit e0d2230 into main Aug 9, 2024
12 checks passed

wischli reviewed Aug 9, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

LPv2: Fix weights with batch messages #1951

LPv2: Fix weights with batch messages #1951

lemunozm commented Aug 7, 2024 •

edited

Loading

lemunozm left a comment

lemunozm Aug 7, 2024 •

edited

Loading

lemunozm Aug 7, 2024

lemunozm Aug 7, 2024

lemunozm Aug 7, 2024

lemunozm Aug 7, 2024

lemunozm Aug 7, 2024 •

edited

Loading

codecov bot commented Aug 8, 2024 •

edited

Loading

cdamian left a comment

lemunozm commented Aug 9, 2024

wischli left a comment

wischli Aug 9, 2024

lemunozm Aug 9, 2024

LPv2: Fix weights with batch messages #1951

LPv2: Fix weights with batch messages #1951

Conversation

lemunozm commented Aug 7, 2024 • edited Loading

Description

TODOS

lemunozm left a comment

Choose a reason for hiding this comment

lemunozm Aug 7, 2024 • edited Loading

Choose a reason for hiding this comment

lemunozm Aug 7, 2024

Choose a reason for hiding this comment

lemunozm Aug 7, 2024

Choose a reason for hiding this comment

lemunozm Aug 7, 2024

Choose a reason for hiding this comment

lemunozm Aug 7, 2024

Choose a reason for hiding this comment

lemunozm Aug 7, 2024 • edited Loading

Choose a reason for hiding this comment

codecov bot commented Aug 8, 2024 • edited Loading

Codecov Report

cdamian left a comment

Choose a reason for hiding this comment

lemunozm commented Aug 9, 2024

wischli left a comment

Choose a reason for hiding this comment

wischli Aug 9, 2024

Choose a reason for hiding this comment

lemunozm Aug 9, 2024

Choose a reason for hiding this comment

lemunozm commented Aug 7, 2024 •

edited

Loading

lemunozm Aug 7, 2024 •

edited

Loading

lemunozm Aug 7, 2024 •

edited

Loading

codecov bot commented Aug 8, 2024 •

edited

Loading