Relay basic single-bit message dispatch results back to the source chain #935
Conversation
svyatonik
added
B-Substrate <> Substrate
P-Message Delivery
P-Message Dispatch
PR-audit-needed
| /// Called when we receive confirmation that our messages have been delivered to the | ||
| /// target chain. The confirmation aso has single bit dispatch result for every | ||
| /// confirmed message (see `DeliveredMessages` for details). | ||
| fn on_messages_delivered(_lane: &LaneId, _messages: &DeliveredMessages) {} |
There was a problem hiding this comment.
This'll need to do something with weights - I'm not yet sure how to deal with this. Change benchmarks to worst-case result and then refund? But what is worst case when there are e.g. multiple apps using different lanes for their purposes. And because of refund policy we'll need to decrease max number of messages in the confirmation tx. Probably just reserve single read and single write for the every registered callback? I'm not yet sure - will leave it for future PRs as well
There was a problem hiding this comment.
Yeah, I think the general sentiment about these kind of handlers is that they should be as simple as possible. Having one read + one write seems reasonable. In future we can prepare some tooling to kind of "subscribe" to interesting messages and for instance have them processed in on_initialize of the pallet or some manual trigger (extrinsic).
Might be good to add this assumption to the docs though, so that implementers are aware of that.
svyatonik
force-pushed
the
relay-basic-dispatch-results
branch
from
86d92dc to
2110b9a
Compare
| /// Called when we receive confirmation that our messages have been delivered to the | ||
| /// target chain. The confirmation aso has single bit dispatch result for every | ||
| /// confirmed message (see `DeliveredMessages` for details). | ||
| fn on_messages_delivered(_lane: &LaneId, _messages: &DeliveredMessages) {} |
There was a problem hiding this comment.
Yeah, I think the general sentiment about these kind of handlers is that they should be as simple as possible. Having one read + one write seems reasonable. In future we can prepare some tooling to kind of "subscribe" to interesting messages and for instance have them processed in on_initialize of the pallet or some manual trigger (extrinsic).
Might be good to add this assumption to the docs though, so that implementers are aware of that.
| /// the target chain to the message submitter at the source chain. If you're using immediate | ||
| /// call dispatcher, then it'll be result of the dispatch - `true` if dispatch has succeeded | ||
| /// and `false` otherwise. | ||
| pub dispatch_result: bool, |
There was a problem hiding this comment.
| pub dispatch_result: bool, | |
| pub dispatched: bool, |
I find it a bit confusing to have dispatch_result and bool - it kind of signifies that there is some extra result embedded in case that's true.
I'd call this simply dispatched: true/false cause it's part of MessageDispatchResult struct already. It's just a personal preference, there is nothing wrong really with dispatch_result, so if you don't find my arguments convincing feel free to leave as-is.
| .saturating_add(RocksDbWeight::get().reads(5 as Weight)) | ||
| .saturating_add(RocksDbWeight::get().writes(3 as Weight)) | ||
| } | ||
| fn receive_message_proofs_with_large_leaf(i: u32) -> Weight { | ||
| (175_300_000 as Weight) | ||
| .saturating_add((6_000 as Weight).saturating_mul(i as Weight)) | ||
| (178_139_000 as Weight) |
There was a problem hiding this comment.
Which machine did you use to generate the weights? They are surprisingly similar to the previous ones :)
There was a problem hiding this comment.
My laptop. Previous weights were also generated here, so it's expected
Co-authored-by: Tomasz Drwięga <tomusdrw@users.noreply.github.com>
…itytech/parity-bridges-common into relay-basic-dispatch-results
…ain (paritytech#935) * relay dispatch result flags back to the source chain * OnMessagesDelivered callback * add lane id to OnDeliveredMessages callback * fix benchmarks && upate weights * clippy * clippy * clipy another try * OnMessagesDelivered -> OnDeliveryConfirmed * Update primitives/messages/src/source_chain.rs Co-authored-by: Tomasz Drwięga <tomusdrw@users.noreply.github.com> Co-authored-by: Tomasz Drwięga <tomusdrw@users.noreply.github.com>
…ain (paritytech#935) * relay dispatch result flags back to the source chain * OnMessagesDelivered callback * add lane id to OnDeliveredMessages callback * fix benchmarks && upate weights * clippy * clippy * clipy another try * OnMessagesDelivered -> OnDeliveryConfirmed * Update primitives/messages/src/source_chain.rs Co-authored-by: Tomasz Drwięga <tomusdrw@users.noreply.github.com> Co-authored-by: Tomasz Drwięga <tomusdrw@users.noreply.github.com>
closes #547
on top of #911 => draft
This PR introduces basic mechanism to solve #547 that Tomek has recently suggested. So for every delivered message confirmation would contain single-bit which will mean (in our configuration) whether message (in ur config: call) has been successfully dispatched or not. At the source chain the
MessagesDeliveredevent would contain these dispatch results. So the message sender may check events (also see TODOs below) to check result of their messages.More sophisticated applications may introduce their own relays to relay extended dispatch results, but for simplest bridge apps single bit should be enough && I agree that it isn't super-heavy to maintain.
TODOs left for doing in this PR:
TODOs left for future PRs:
instead of examining events (=> additional overload indone;on_block_finalize()) we need to introducetrait OnMessagesDelivered { fn on_messages_delivered(messages: &DeliveredMessages) {} }. The method will be called only on blocks with confirmation transactions, so it must be easier to use