LPv2: Batch message logic #1923
Conversation
|
@mustermeiszer I had a lot of issues implementing the batch(calls) method regarding filtering the accounts (that some calls can be root), different destinations, nested calls to batch, weights… I think this hybrid solution is simpler if you’re happy with the fulfilling requirements (I think everything we’ve talked about is fulfilled here). It gives enough control and never leaves the batch decision to the The user will use this with |
| for msg in incoming_msg.unpack() { | ||
| T::InboundQueue::submit(domain_address.clone(), msg)?; | ||
| } |
There was a problem hiding this comment.
Weight of this function must take into account the maximum batch size possible. Defined by the deserialization implemenation.
There was a problem hiding this comment.
This is really difficult to measure because, right now, it's "not measured". It's using hardcoded weights. The real weight of this does not depend on how many bytes we read from the message, it mostly depends of what messages are in the batch that will dispatch certain actions. i.e., if this message is an increase it will call to foreign investments and would imply swaps having a weight increase.
I'm not sure what kind of solution we want to follow here.
There was a problem hiding this comment.
I've got one which multiplies the current weight used by the number of messages in the batch
| let message_weight = | ||
| read_weight.saturating_add(Weight::from_parts(0, serialized.len() as u64)); |
There was a problem hiding this comment.
Message PoV size evaluated here
| #[pallet::weight(T::WeightInfo::receive_message().saturating_mul( | ||
| Pallet::<T>::deserialize_message(expected_gateway_origin.clone(), msg.clone()) | ||
| .map(|(_, msg)| msg.unpack().len() as u64) | ||
| .unwrap_or(T::Message::MAX_PACKED_MESSAGES as u64) | ||
| ) | ||
| )] | ||
| #[pallet::call_index(5)] | ||
| pub fn process_msg( | ||
| pub fn receive_message( | ||
| origin: OriginFor<T>, | ||
| expected_gateway_origin: GatewayOrigin, | ||
| msg: BoundedVec<u8, T::MaxIncomingMessageSize>, |
There was a problem hiding this comment.
Even though origin is the same as the GatewayOrigin, we need to pass this extra parameter to be able to read it from the pallet::weight() macro (inside that macro, we do not have access to origin). We need to read this to know how to deserialize the message and then, knows how many message in a batch we have, adjusting upfront the weight consequently
| fn weight() -> Weight { | ||
| T::DbWeight::get() | ||
| .reads_writes(3, 4) | ||
| .saturating_add(Weight::from_parts(0, T::Message::max_encoded_len() as u64)) | ||
| } |
There was a problem hiding this comment.
Here we compute the weight needed to enqueue a message the could be a batch. This size is different to the used when sending the message, which use the serialized sized.
| fn outbound_weight(reads: u64, writes: u64) -> Weight { | ||
| Weight::from_parts(1_000_000, 256) //defensive base weights | ||
| .saturating_add(T::DbWeight::get().reads_writes(reads, writes)) | ||
| .saturating_add(T::OutboundQueue::weight()) | ||
| } |
There was a problem hiding this comment.
This takes into account the correct OutboundQueue::submit() weights even when the message is a batch. We no longer need defensive_weights.rs
|
Maybe we should wait to merge @cosmin PRs first if they have more priority to avoid wasting time with conflict (that will be a lot 😄). Anyways this is ready to be reviewed in the meantime |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## feat/lp-v2 #1923 +/- ##
=============================================
Coverage ? 47.89%
=============================================
Files ? 180
Lines ? 12895
Branches ? 0
=============================================
Hits ? 6176
Misses ? 6719
Partials ? 0 ☔ View full report in Codecov by Sentry. |
|
Closed in favor of #1949 |
Description
Responsibilities
pallet-liquidity-pools-gatewayknows to batch/unbatch but doesn't know what a Batch message is. To handle this, theLPMessagetrait now allows pack/unpack methods.pallet-liquidity-poolswill never send or receive a batch. The gateway is the responsible for the batching process.Details
Batching is handled by two extrinsic:
Gateway::start_pack_messages(sender, destination): When calling this, any submitted messages from LP, that match thesender&destinationwill 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_pack_messages(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 betweenstart_packandend_packwill be bundled in the batch.This PR depends on: #1920
Changes
Gateway::start_pack_messages()andGateway::end_pack_messages()to allow creating batches.process_msg()toreceive_message(). Added a new parameter to be able to measure the weight correctly.process_message()tosend_message(). Measure the weight there based on the serialized message len.try_rangefor header message deserialization and use aBufferReader.defensive_weights.rsfile and use more adjusted weights that take into account batch messages.weight()method toOutboundQueuetrait that allows to compute correct weights from liquidity_pools taking into account batch sizes.