Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Implementer's Guide: Flesh out more details for upward messages #1556

Merged
14 commits merged into from
Aug 14, 2020
Merged

Conversation

pepyakin
Copy link
Contributor

@pepyakin pepyakin commented Aug 7, 2020

Closes #1534

This PR changes the way how we process the upward messages.

Upward Message Kind

Upward messages are split to different kinds, with Dispatchable being only one of them. The upside is that the unlike plain opaque dispatchables, explicit message kinds allows us to impose some validity criteria upon the upward messages. E.g. we can reject candidates that we know will fail upfront.

However, we only introduce the scaffolding for different upward message kinds without actually introducing any new.

Weight

Then, in the current construction the weight consumed by messages is not limited. In the proposed changes, each upward message of dispatchable kind consumes some predefined weight and the total weight consumed during dispatching messages is limited, leaving a guaranteed amount of weight to other extrinsics.

This proposal doesn't yet address charging fees for the weight.

Round-robin dispatching

Since it can happen that some messages won't fit in the weight budget devoted for a block, we need to fairly distribute the weight among the pending dispatchables.

To address this, we go over all queues in a round robin fashion dequeuing a message each time. When we reach the weight budget for executing dispatchables or there are none left we finish processing. There can be multiple iterations. If any unused weight left it can be leveraged by signed extrinsics.

Dispatch order

The current construction dispatches messages during finalization. The proposed construction moves this processing to be just right at the end of inclusion inherent giving the dispatchables an advantage over the regular signed extrinsics. That alleviates the front-running the parachain upward messages by the validators to some degree.

Optionally, perhaps we can add another round of dispatching during the finalization stage to leverage unused weight, but that would worsen the property against front-running

@pepyakin pepyakin added I7-documentation Documentation needs fixing, improving or augmenting. A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. labels Aug 7, 2020
@pepyakin
Copy link
Contributor Author

I pushed a commit that gives up a strict enforcement that dispatchables should be decodable and that they are not exceeding some critical weight per dispatchable.

Instead, I introduced a downward message that is sent in response for each dispatchable upward message that reports the status of execution, basically Executed { success: bool } | DecodeFailed | CriticalWeightExceeded

That allows not only to perform handling of failure on the parachain side, e.g. by retrying or releasing the assets that could be potentially locked, but it also mitigates a more broad problem. It's not only the runtime code can invalidate a dispatchable but some dispatchable fails just because of some racy but nonetheless legit condition.

@rphmeier rphmeier requested a review from drahnr August 13, 2020 16:00
> NOTE: It is conceivable that upward messages will be divided to more fine-grained kinds with a dispatchable upward message
being only one kind of multiple. That would make upward messages inspectable and therefore would allow imposing additional
validity criteria for the candidates that contain these messages.
One kind of messages is `Dispatchable`. They could be thought of similar to extrinsics sent to a relay chain: they also
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this is the only kind right now, phrasing as "One kind, and the only current kind, of message..."

},
// Examples:
// HrmpOpenChannel { .. },
// HrmpCloseChannel { .. },
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What are these examples of?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Of other potential kinds of messages, I can remove that if you think that's unnecessary given that those will become real messages

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would be in favour of removing them, I find it unclear what usage they show.

Copy link
Contributor

@rphmeier rphmeier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM except for grumbles!

Copy link
Contributor

@drahnr drahnr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple of nitpicks, but LGTM 👍

@pepyakin
Copy link
Contributor Author

bot merge

@ghost
Copy link

ghost commented Aug 14, 2020

Trying merge.

@ghost ghost merged commit 73de27e into master Aug 14, 2020
@ghost ghost deleted the ser-ump-v2 branch August 14, 2020 14:03
ordian added a commit that referenced this pull request Aug 17, 2020
* master:
  Companion PR for #6862 (#1564)
  implement collation generation subsystem (#1557)
  Add spawn_blocking to SubsystemContext (#1570)
  Companion PR for #6846 (#1568)
  overseer: add a test to ensure all subsystem receive msgs (#1590)
  Implementer's Guide: Flesh out more details for upward messages (#1556)
ordian added a commit that referenced this pull request Aug 17, 2020
…n-race-condition

* master:
  Companion PR for #6862 (#1564)
  implement collation generation subsystem (#1557)
  Add spawn_blocking to SubsystemContext (#1570)
  Companion PR for #6846 (#1568)
  overseer: add a test to ensure all subsystem receive msgs (#1590)
  Implementer's Guide: Flesh out more details for upward messages (#1556)
  Make parachain validation wasm executor functional (#1574)
  Use async test helper to simplify node testing (#1578)
  guide: validation data refactoring (#1576)
  Remove v0 parachains runtime (#1501)
  [CI] Add github token to generate-release-text (#1581)
  Allow using any polkadot client instead of enum Client (#1575)
  service/src/lib: Update authority discovery construction (#1563)
ordian added a commit that referenced this pull request Aug 17, 2020
* master:
  overseer: fix build (#1596)
  Companion PR for #6862 (#1564)
  implement collation generation subsystem (#1557)
  Add spawn_blocking to SubsystemContext (#1570)
  Companion PR for #6846 (#1568)
  overseer: add a test to ensure all subsystem receive msgs (#1590)
  Implementer's Guide: Flesh out more details for upward messages (#1556)
  Make parachain validation wasm executor functional (#1574)
  Use async test helper to simplify node testing (#1578)
  guide: validation data refactoring (#1576)
  Remove v0 parachains runtime (#1501)
  [CI] Add github token to generate-release-text (#1581)
  Allow using any polkadot client instead of enum Client (#1575)
  service/src/lib: Update authority discovery construction (#1563)
  Update .editorconfig to what we have in practice (#1545)
  Companion PR for substrate #6672 (#1560)
  pre-redenomination tockenSymbol change (#1561)
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. I7-documentation Documentation needs fixing, improving or augmenting.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Guide: Impose limits on the number of messages that a candidate can send
3 participants