LPv2: Batch message logic (r2) #1949

lemunozm · 2024-08-07T06:33:15Z

Description

This PR overrides the previous #1923, in a simplified version

Responsibilities

pallet-liquidity-pools-gateway knows to batch/unbatch but doesn't know what a Batch message is. To handle this, the LPMessage trait allows pack/unpack methods.
pallet-liquidity-pools will never send or receive a batch. The gateway is responsible for the batching process.

Details

Batching is handled by two extrinsic:

Gateway::start_batch_message(sender, destination): When calling this, any submitted messages from LP, that match the sender & destination will be packed. NOTE: We need to index also by destination domain because the destination information is not inside the message. So, we can never bundle two messages with two different destinations.
Gateway::end_batch_message(sender, destination): When calling this, the batch is finally submitted. If this is not called, no message is sent. If the batch reaches the limit size, it will dispatch an error.

Later, the user can call pallet_utility::batch(calls), and any message sent between start_pack and end_pack will be bundled in the batch.

Changes

Added Gateway::start_batch_message() and Gateway::end_batch_message() to allow creating batches.
Renamed process_msg() to receive_message().
Removed try_range for header message deserialization and use a BufferReader.
Test message moved to the gateway mock.

Docs

See the updated diagrams: https://centrifuge.hackmd.io/i_wfI19XTgSkfWeN7IxXiQ?view

lemunozm · 2024-08-07T06:37:40Z

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) {
+				Ok(dispatch_info) => (Ok(()), dispatch_info.actual_weight),
+				Err(e) => (Err(e.error), e.post_info.actual_weight),
+			};


@cdamian, what do you think about this? Could we just convert router.send() to return just a simple DispatchResult?

cc @wischli

Below, we're adding the defensive weights

I would only return defensive weights when there's no way of finding the weight for an operation. In this case, the router does have some weight and I think we should use that, maybe alongside the defensive ones if we really wanna be extra-defensive?

Mmmm... I see. The defensive one seems to already shadow anything returned by the router. So, I would either only use the router one or only use the defensive one.

But not super strong opinion

I would also use either the "real" or the defensive weight. If the weights of send can be assumed to be representative, then let's go with that. Else, ignore it and fallback to the defensive one for now.

lemunozm · 2024-08-07T06:37:47Z

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

+	fn start_pack_messages() -> Weight {
+		// TODO: BENCHMARK CORRECTLY
+		//
+		// NOTE: Reasonable weight taken from `PoolSystem::set_max_reserve`
+		//       This one has one read and one write for sure and possible one
+		//       read for `AdminOrigin`
+		Weight::from_parts(30_117_000, 5991)
+			.saturating_add(RocksDbWeight::get().reads(1))
+			.saturating_add(RocksDbWeight::get().writes(1))
+	}


TechDebt: We should factorize the weights of this file and use some auxiliary methods instead (similar to what William has done in its PR about freezing)

Yup, we have a todo to use real weights for most of the gateway stuff.

lemunozm · 2024-08-07T06:38:36Z

Still missings UTs, will do later, but the PR is ready to be reviewed

lemunozm · 2024-08-07T06:42:50Z

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

 		#[pallet::call_index(5)]
-		pub fn process_msg(
+		pub fn receive_message(


NOTE. It's true that now we can receive batches, but from @cdamian changes in the queue, this method will no longer process messages, just queuing, which means that it will only take some more bytes as PoV. The current defensive weights are enough for that slight increment

lemunozm · 2024-08-07T06:44:01Z

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

+				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)),


NOTE we need here the serialized len instead of max_decoded_len() because the size used internally by the router is the size of the vector. The router doesn't know about Message type

cdamian · 2024-08-07T07:35:25Z

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

@@ -1188,31 +1094,27 @@ mod message_processor_impl {
 					assert_eq!(mock_sender, expected_sender);
 					assert_eq!(mock_message, expected_message.serialize());

-					Err(DispatchErrorWithPostInfo {
+					Err(dbg!(DispatchErrorWithPostInfo {


Was this a one-time change that you used when debugging or should we sprinkle it around some more? :D

Good catch! It was a one-time change 😅

cdamian · 2024-08-07T07:48:03Z

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

+		#[pallet::weight(T::WeightInfo::start_pack_messages())]
+		#[pallet::call_index(9)]
+		pub fn start_pack_messages(origin: OriginFor<T>, destination: Domain) -> DispatchResult {
+			let sender = ensure_signed(origin)?;


I have a couple of questions to make sure I understand the expected flow.

Who is expected to call these packing extrinsics?

From what I can tell, in the OutboundMessageHandler impl below, it would be a sender coming from LP. An LP sender can be some address that comes from EVM, right? If so, how does that address manage to call these extrinsics?

How do we handle cases where the supposed caller errors out between start and end?

Should we maybe consider adding a way to manually end a batch for a particular (sender, destination) pair via root?

Very good questions!

Who is expected to call these packing extrinsics?

The same as who call the extrinsics from pallet-liquidity-pools. It does not need to be an address from EVM. Just a normal account.

How do we handle cases where the supposed caller errors out between start and end?

If some call between start and end fails, the user notice that. Note that each call in between is not yet processed by the on_idle(), so it fails just in that moment.

As a resume of how all of this work:

Gateway::start_pack_messages(ALICE) LiquidityPools::add_pool(ALICE) LiquidityPools::add_tranche(ALICE) // -> If it fails, ALICE will know it and can choose to try again with the correct params before calling end Gateway::end_pack_messages(ALICE) // -> Now, the batch message is on the queue to be sent in the `on_idle()` function.

The above can be done in several blocks, but you can also batch all of them. If one fails, nothing is applied.

OK, thank you for the explanation.

Awesome explanation, thanks for that @lemunozm !

IMO, in general we can assume that {start, end}_packing_messages will only be called locally from a Substrate account because EVM accounts would call it on their EVM chain of choice such that we receive the LP batch message.

lemunozm · 2024-08-07T12:42:07Z

There are still pending batching tests (UTs and ITs). But maybe we can start merging this to avoid more conflicts with the incoming Cosmin PRs, WDYT?

We can fix some clean-ups in a next PR along with the tests.

cc @wischli @cdamian

cdamian · 2024-08-07T12:44:38Z

There are still pending batching tests (UTs and ITs). But maybe we can start merging this to avoid more conflicts with the incoming Cosmin PRs, WDYT?

We can fix some clean-ups in a next PR along with the tests.

cc @wischli @cdamian

I'm OK with this. If we merge this, I will rebase my multi-router branch, then you can continue with the rest of the stuff here.

lemunozm · 2024-08-07T12:46:17Z

@cdamian you will have quite conflicts but wanted to merge this ASAP to avoid even more 😅

Tell me if I can help you in the rebase 🙏🏻, or with some tricky union of some part

lemunozm · 2024-08-07T12:42:46Z

libs/traits/src/liquidity_pools.rs


-#[cfg(any(test, feature = "std"))]
-pub mod test_util {


Moved to the gateway/mock.rs

lemunozm · 2024-08-07T12:43:34Z

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

+pub enum Message {
+	#[default]
+	Simple,
+	Pack(Vec<Message>),
+}


We should extend this with the Proof variant from your PR @cdamian

lemunozm · 2024-08-07T12:52:24Z

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

+			let mut count = 0;
+			for submessage in message.unpack() {
+				count += 1;
+
+				if let Err(e) = T::InboundMessageHandler::handle(domain_address.clone(), submessage)
+				{
+					return (Err(e), weight.saturating_mul(count));
+				}
 			}
+
+			(Ok(()), weight.saturating_mul(count))


TODO. Debt to myself for the next PR. I think this could be an issue if we exceed the maximum during the batch processing. We should check that we can use all message weight before processing anything.

Fixed in #1951

codecov · 2024-08-07T18:08:41Z

Codecov Report

Attention: Patch coverage is 57.30337% with 38 lines in your changes missing coverage. Please review.

Project coverage is 47.69%. Comparing base (8245ce2) to head (7a9868d).

Files	Patch %	Lines
pallets/liquidity-pools-gateway/src/lib.rs	64.61%	23 Missing ⚠️
pallets/liquidity-pools/src/message.rs	0.00%	12 Missing ⚠️
libs/utils/src/lib.rs	90.00%	1 Missing ⚠️
...pools-gateway/axelar-gateway-precompile/src/lib.rs	0.00%	1 Missing ⚠️
pallets/liquidity-pools/src/lib.rs	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1949      +/-   ##
==========================================
- Coverage   47.72%   47.69%   -0.04%     
==========================================
  Files         184      183       -1     
  Lines       12969    12953      -16     
==========================================
- Hits         6190     6178      -12     
+ Misses       6779     6775       -4

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

lemunozm · 2024-08-08T08:59:13Z

@wischli @cdamian can we merge this to avoid conflicts with the cosmin part? 🙏🏻😃

wischli

LGTM!

wischli · 2024-08-08T09:10:36Z

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

+		#[pallet::weight(T::WeightInfo::start_pack_messages())]
+		#[pallet::call_index(9)]
+		pub fn start_pack_messages(origin: OriginFor<T>, destination: Domain) -> DispatchResult {
+			let sender = ensure_signed(origin)?;


Awesome explanation, thanks for that @lemunozm !

IMO, in general we can assume that {start, end}_packing_messages will only be called locally from a Substrate account because EVM accounts would call it on their EVM chain of choice such that we receive the LP batch message.

wischli · 2024-08-08T09:14:01Z

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) {
+				Ok(dispatch_info) => (Ok(()), dispatch_info.actual_weight),
+				Err(e) => (Err(e.error), e.post_info.actual_weight),
+			};


I would also use either the "real" or the defensive weight. If the weights of send can be assumed to be representative, then let's go with that. Else, ignore it and fallback to the defensive one for now.

wischli · 2024-08-08T09:18:22Z

@lemunozm Feel free to press the button. We can merge #1946 afterwards.

add batch logic

2414a5a

lemunozm self-assigned this Aug 7, 2024

lemunozm mentioned this pull request Aug 7, 2024

LPv2: Batch message logic #1923

Closed

lemunozm requested review from cdamian, wischli and mustermeiszer and removed request for cdamian, wischli and mustermeiszer August 7, 2024 06:34

lemunozm commented Aug 7, 2024

View reviewed changes

cdamian reviewed Aug 7, 2024

View reviewed changes

added infrastructure to tests batches

9c6e607

lemunozm marked this pull request as ready for review August 7, 2024 12:40

lemunozm commented Aug 7, 2024

View reviewed changes

cdamian previously approved these changes Aug 7, 2024

View reviewed changes

fix common tests

99eb01c

lemunozm dismissed cdamian’s stale review via 99eb01c August 7, 2024 16:16

lemunozm mentioned this pull request Aug 7, 2024

LPv2: Fix weights with batch messages #1951

Merged

2 tasks

fix tests and some renamings

7a9868d

wischli approved these changes Aug 8, 2024

View reviewed changes

lemunozm merged commit b5178c2 into main Aug 8, 2024
12 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

LPv2: Batch message logic (r2) #1949

LPv2: Batch message logic (r2) #1949

lemunozm commented Aug 7, 2024 •

edited

Loading

lemunozm Aug 7, 2024

lemunozm Aug 7, 2024

cdamian Aug 7, 2024

lemunozm Aug 7, 2024

wischli Aug 8, 2024

lemunozm Aug 7, 2024

cdamian Aug 7, 2024

lemunozm commented Aug 7, 2024

lemunozm Aug 7, 2024

lemunozm Aug 7, 2024 •

edited

Loading

cdamian Aug 7, 2024

lemunozm Aug 7, 2024

cdamian Aug 7, 2024

lemunozm Aug 7, 2024

cdamian Aug 7, 2024

wischli Aug 8, 2024

lemunozm commented Aug 7, 2024

cdamian commented Aug 7, 2024

lemunozm commented Aug 7, 2024

lemunozm Aug 7, 2024

lemunozm Aug 7, 2024

lemunozm Aug 7, 2024

lemunozm Aug 8, 2024

codecov bot commented Aug 7, 2024 •

edited

Loading

lemunozm commented Aug 8, 2024

wischli left a comment

wischli Aug 8, 2024

wischli Aug 8, 2024

wischli commented Aug 8, 2024

LPv2: Batch message logic (r2) #1949

LPv2: Batch message logic (r2) #1949

Conversation

lemunozm commented Aug 7, 2024 • edited Loading

Description

Responsibilities

Details

Changes

Docs

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lemunozm commented Aug 7, 2024

Choose a reason for hiding this comment

lemunozm Aug 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lemunozm commented Aug 7, 2024

cdamian commented Aug 7, 2024

lemunozm commented Aug 7, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Aug 7, 2024 • edited Loading

Codecov Report

lemunozm commented Aug 8, 2024

wischli left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wischli commented Aug 8, 2024

lemunozm commented Aug 7, 2024 •

edited

Loading

lemunozm Aug 7, 2024 •

edited

Loading

codecov bot commented Aug 7, 2024 •

edited

Loading