Call To*InboundLaneApi::message_details() with batched messages#1545
Conversation
- avoid using a HashMap for `messages_to_refine`. It seems that a vec is enough - minimize the number of conversions between `OutboundMessageDetails` and `MessageDetails` - use references where possible in order to minimize the number of intermediary Vecs - simplify `make_message_details_map()` logic, reduce its scope and rename it to `validate_out_msgs_details()` Signed-off-by: Serban Iorga <serban@parity.io>
Signed-off-by: Serban Iorga <serban@parity.io>
Signed-off-by: Serban Iorga <serban@parity.io>
Signed-off-by: Serban Iorga <serban@parity.io>
svyatonik
left a comment
svyatonik
left a comment
There was a problem hiding this comment.
Just couple of questions, then it is good to merge
| out_msg_details.dispatch_weight = in_msg_details.dispatch_weight; | ||
| } | ||
|
|
||
| current_msgs_batch = next_msgs_batch; |
There was a problem hiding this comment.
I may be missing something, but aren't you dropping remaining current_msgs_batch values here? I mean if e.g. current_msgs_batch has 5 entries initially and let's say only first entry is below P::TargetChain::max_extrinsic_size() limit, then you're pushing 2rd entry to the next_msgs_batch and then just drop remaining 3 entries here? Sorry - I may be missing something here, just checking :)
There was a problem hiding this comment.
Actually imo would be cool if we could have a function that can be easily tested (i.e. without RPC calls). The function shall just split single vector (msgs_to_refine) to a vector of vectors. t is optional, but WDYT?
There was a problem hiding this comment.
I may be missing something, but aren't you dropping remaining current_msgs_batch values here? I mean if e.g. current_msgs_batch has 5 entries initially and let's say only first entry is below P::TargetChain::max_extrinsic_size() limit, then you're pushing 2rd entry to the next_msgs_batch and then just drop remaining 3 entries here? Sorry - I may be missing something here, just checking :)
In this case all the 4 entries that don't fit in P::TargetChain::max_extrinsic_size() should be moved to next_msgs_batch in the while above:
while (self.lane_id, ¤t_msgs_batch).encoded_size() >
P::TargetChain::max_extrinsic_size() as usize
{
...
}
I hope I didn't miss anything there.
Actually imo would be cool if we could have a function that can be easily tested (i.e. without RPC calls). The function shall just split single vector (
msgs_to_refine) to a vector of vectors. t is optional, but WDYT?
Yes, that's a very good idea. I'll try to move this logic to a separate function.
| weights.last().map(|details| details.nonce != *nonces.end()).unwrap_or(true); | ||
| if last_nonce_is_missing { | ||
| // Check if last nonce is missing. The loop above is not checking this. | ||
| if !nonces.is_empty() && nonces_iter.peek() == Some(nonces.end()) { |
There was a problem hiding this comment.
Please verify me here: nonces_iter is noces.rev(), so we're moving from end to begin and here we are comparing with the end() here. Is that intended?
There was a problem hiding this comment.
Yes, if nonces_iter.peek() == Some(nonces.end()) at this point, it means that the last nonce was not found in the messages list (and that the messages list is empty actually), and we need to return an error.
There was a problem hiding this comment.
Ok, can we then check "that the messages list is empty"? :) Imo it should be simpler :)
There was a problem hiding this comment.
Yes, I think it should work.
There was a problem hiding this comment.
Done. Also added another check for the case where there are more messages than nonces.
Signed-off-by: Serban Iorga <serban@parity.io>
Bumps [clap](https://github.com/clap-rs/clap) from 3.2.16 to 3.2.17. - [Release notes](https://github.com/clap-rs/clap/releases) - [Changelog](https://github.com/clap-rs/clap/blob/v3.2.17/CHANGELOG.md) - [Commits](clap-rs/clap@v3.2.16...v3.2.17) --- updated-dependencies: - dependency-name: clap dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <support@github.com> Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
…tytech#1545) * generated_message_details() -> Simplifications - avoid using a HashMap for `messages_to_refine`. It seems that a vec is enough - minimize the number of conversions between `OutboundMessageDetails` and `MessageDetails` - use references where possible in order to minimize the number of intermediary Vecs - simplify `make_message_details_map()` logic, reduce its scope and rename it to `validate_out_msgs_details()` Signed-off-by: Serban Iorga <serban@parity.io> * Define typed_state_call() Signed-off-by: Serban Iorga <serban@parity.io> * Call To*InboundLaneApi::message_details() with single messages Signed-off-by: Serban Iorga <serban@parity.io> * Call To*InboundLaneApi::message_details() with batched messages Signed-off-by: Serban Iorga <serban@parity.io> * validate_out_msgs_details() -> change check * Define split_msgs_to_refine() Signed-off-by: Serban Iorga <serban@parity.io> Signed-off-by: Serban Iorga <serban@parity.io>
…tytech#1545) * generated_message_details() -> Simplifications - avoid using a HashMap for `messages_to_refine`. It seems that a vec is enough - minimize the number of conversions between `OutboundMessageDetails` and `MessageDetails` - use references where possible in order to minimize the number of intermediary Vecs - simplify `make_message_details_map()` logic, reduce its scope and rename it to `validate_out_msgs_details()` Signed-off-by: Serban Iorga <serban@parity.io> * Define typed_state_call() Signed-off-by: Serban Iorga <serban@parity.io> * Call To*InboundLaneApi::message_details() with single messages Signed-off-by: Serban Iorga <serban@parity.io> * Call To*InboundLaneApi::message_details() with batched messages Signed-off-by: Serban Iorga <serban@parity.io> * validate_out_msgs_details() -> change check * Define split_msgs_to_refine() Signed-off-by: Serban Iorga <serban@parity.io> Signed-off-by: Serban Iorga <serban@parity.io>
2 participants
Closes #1538