[bridges] Prune messages from confirmation tx body, not from the on_idle

[bkontur]

Original PR with more context: paritytech/parity-bridges-common#2211
Relates to: paritytech/parity-bridges-common#2210

TODO

• fresh weighs for pallet_bridge_messages
• add try_state for pallet_bridge_messages which checks for unpruned messages - relates to the comment
• prepare migration, that prunes leftovers, which would be pruned eventually from on_idle the comment can be done also by set_storage / kill_storage or with OnRuntimeUpgrade implementatino when do_try_state_for_outbound_lanes detects problem.

Open question

• Do we really need oldest_unpruned_nonce afterwards?
  • after the runtime upgrade and when do_try_state_for_outbound_lanes pass, we won't need any migrations here
    • we won't even need do_try_state_for_outbound_lanes

[bkontur]

bot bench cumulus-bridge-hubs --runtime=bridge-hub-rococo --pallet=pallet_bridge_messages
bot bench cumulus-bridge-hubs --runtime=bridge-hub-westend --pallet=pallet_bridge_messages

[bkontur]

bot clean

[serban300]

I'm not familiar with try_state(). Why do we use MAX_MESSAGES_ITERATION and not iterate all the way ?

Otherwise replacing the loop wit ha for looks good.

[bkontur]

I'm not familiar with try_state(). Why do we use MAX_MESSAGES_ITERATION and not iterate all the way ?

Because, on every runtime upgrade it would be more and more waste of time, e.g.

• first time it would run from e.g. message with nonce 1000 to 0
• another runtime upgrade (when more messages happen) it would run e.g. 3000 to 0 (we previously checked interval 1000 to 0)
• another runtime upgrade (when more messages happen) it would run e.g. 6000 to 0 (we previously checked interval 3000 to 0)
• and so on

Yes, that's why I updated description that maybe oldest_unpruned_nonce and do_try_state_for_outbound_lanes are not really needed.

Slava wanted to create migration, that remove all messages < oldest_unpruned_nonce, but I think his concert was valid only if we have already more than one lane, because on_idle he picked different/random lanes to prune depending on ActiveOutboundLanes::count, which is for fellows 1 :). I think MBM migration is too much for this and also for standard migration, we would need to handle consumed weights and store last pruned block somewhere else, because oldest_unpruned_nonce is incremented on delivery now.

But as I am thinking about it again now, I think we should really remove attribute lane.oldest_unpruned_nonce, because we prune on delivery and this attribute does not make sense. So, the removal of this attribute could cause storage version change and migration, which would clean everything :)

[bkontur]

actually, MBM migration and removed oldest_unpruned_nonce, would do the job :)

[bkontur]

we don't use oldest_unpruned_nonce for relayer, anyway we can leave also without these migration, the worst can happen that we will have just a few messages in the storage, which we don't need. We add monitoring or do some sanity checks with seaching storage manually after upgrade, and when few messages left, we could remove them with set_prefix / kill_storage.