XDM: ensure max outgoing messages per channel is atleast one and ensure to return open channel that can still accept messages

[vedhavyas]

This PR makes the following changes

• Ensures max_outgoing_messages are alteast 1 for each channel
• Ensure get_open_channel_for_chain returns a channel that is latest and has the capacity to accept new messages
• Remove CountedStorageMap for Outbox since we want to get the count of the messages in flight for given dst_chain_id and channel but not the total number of messages for all the chains and channel combined.
• Remove CountedInboxResponses map type since the count is not used anywhere and will reduce one extra read and write.

Code contributor checklist:

• I have read, understood and followed contributing guide

[vanhauser-thc]

            // ensure max outgoing messages is atleast 1
            ensure!(
                init_params.max_outgoing_messages >= 1u32,
                Error::<T>::InvalidMaxOutgoingMessages
            );

beside the missing space in the comment - do you think that 1 is a good minimum value?

[vedhavyas]

do you think that 1 is a good minimum value?

I do not have a strong reason why it should be more as long as its not going to block XDM messages. This PR also ensures latest channel which has enough capacity is picked for sending messages.

So even if someone decided to pick 1, unless all the other channels are full, XDM will always be accepted.

Let me know if you think we should pick a higher number @vanhauser-thc

[teor2345]

do you think that 1 is a good minimum value?

I can imagine some scenarios where we’d want to have a channel limit of 1:

• limit the number of large in-flight messages
• limit the number of messages to/from a busy domain

Is there a limit on the number of open channels, or do the channel open and storage fees implicitly limit the number of channels?

[teor2345]

Looks good, but I just want to check the order we’re doing storage changes - it might matter in specific edge cases.

[vedhavyas]

Is there a limit on the number of open channels, or do the channel open and storage fees implicitly limit the number of channels?

there is not limit on number of channels and anyone can initiate a channel between chain as along as the chains themselves can allow the channel creation and the channel owner can put a deposit.

Previous channel owners had an option to det their own fees but we removed that due some issues. Max outgoing messages is the only input channel owners can set.

I'm not opposed to remove that and set a default value for all the channels as well.
Thoughts @vanhauser-thc ?

but I just want to check the order we’re doing storage changes - it might matter in specific edge cases

Every extrinsic is executed in a transaction. So it does not matter the order of update since its atmoic from top view

[NingLin-P]

The change LGTM, but 2 more questions:

• Do we also need a MAX limit of max_outgoing_messages? otherwise, if someone set the max_outgoing_messages to u32::MAX that means no limit of inflight message at all
• Do we really need multiple channels between chain? since the max_outgoing_messages is the only difference between channels and the user can't select which channel to use

[vedhavyas]

When we first started the XDM, we have multiple channel per domain since there we different fee models per channel. This would ideally enable a good market from external devs in providing faster and slower token transfers for example. Unfortunately, the fee model is set to default for all the channels but I would still like to revisit this once XDM is in the mainnet.

Now, there is only one param that is configurable for the channel onwer.

Do we also need a MAX limit of max_outgoing_messages? otherwise, if someone set the max_outgoing_messages to u32::MAX that means no limit of inflight message at all

Good point but what is stopping them to set it (U32::MAX - 1).

Based on the discussion so far, seems like taking the Fee model approach to max_outgoing_messages would be one safe approach. If we decide to go that route, then we can remove that parameter and set a default value for all the channels
What is the best default value be?

Thoughts on this @dariolina ?

[dariolina]

Does the number of messages inflight affect other things, like runtime storage for keeping all their data for instance? If so, there should be a max value based on limiting that.
I'm not opposed to selecting a default ourselves based on sensible limitations like the one in previous sentence.

[vanhauser-thc]

Maybe the easiest solution is to have a fixed value (e.g. 25) and if more are needed more channels can be opened.
And if the future shows this needs to be variable then there is more real world data available to see what the sensible values are.

[NingLin-P]

This would ideally enable a good market from external devs in providing faster and slower token transfers for example.

To enable a market for relaying, the user should first be able to select which channel (fast or slow) to use.

Good point but what is stopping them to set it (U32::MAX - 1).

I mean to have a max limit for max_outgoing_messages not setting the max limit to u32::MAX.

Having a default and/or fixed value makes sense to me since the channel creator would probably not know what value to use.

[vedhavyas]

To enable a market for relaying, the user should first be able to select which channel (fast or slow) to use.

I never said this is possible right now. But it is straight forward to accept a channel ID for a given dst_chain. So this is still possible and potentially useful

I mean to have a max limit for max_outgoing_messages not setting the max limit to u32::MAX.

Its the same thing actually. I use u32::MAX as a max limit but it could be any value and some can chose one less value and create a channel. So this will always be a problem

Maybe the easiest solution is to have a fixed value (e.g. 25) and if more are needed more channels can be opened.

@vanhauser-thc fair enough. I'll pick that value and we can change when we need to change it

[teor2345]

I mean to have a max limit for max_outgoing_messages not setting the max limit to u32::MAX.

Its the same thing actually. I use u32::MAX as a max limit but it could be any value and some can chose one less value and create a channel. So this will always be a problem

I think it might help for you both to spell out this argument a bit more. What’s the threat here, and why does choosing a value slightly less than the maximum cause that threat?

I think choosing a fixed channel in-flight message limit of 25 is fine for now - it means the channel fee pays for the capacity to have 25 in-flight messages. (And then each actual message pays for itself.)

The number of channels and messages is effectively unlimited (> 2^256), but in practice it is limited by channel and storage fees. And open channels currently just use storage, which is a fee-limited resource.

[teor2345]

This looks fine to me as a security fix.

Happy for us to apply other fixes or changes to the interface or default message capacity in this PR, or in another PR.

[NingLin-P]

What’s the threat here, and why does choosing a value slightly less than the maximum cause that threat?

Actually, I don't see a threat here.

25 may be small as the message will "in-flight" for 1 day of the challenge period before it can free the slot for the new message, but we can change it anytime if needed.

[vedhavyas]

25 may be small as the message will "in-flight" for 1 day of the challenge period before it can free the slot for the new message, but we can change it anytime if needed.

Something we need think on before phase2.

A quick math i did with an avg of 4 transactions per block, with just one channel would be around 58K
Since with multiple channels, it will load balance accordingly.
Maybe we can start with 10k then
cc: @dariolina

[teor2345]

Looks like it will work!

In general, I’m not a fan of copying the same constant 8 times. But it’s not a blocker for this security fix, just something to consider as we make larger changes to the design.