polkadot-fellows / RFCs

Proposals for change to standards administered by the Fellowship.
https://polkadot-fellows.github.io/RFCs/
Creative Commons Zero v1.0 Universal
111 stars 51 forks source link

Prepare `Core` runtime API for MBMs #13

Closed ggwpez closed 4 months ago

ggwpez commented 1 year ago

This RFC aims to ratify the proposed changes to the Core runtime API.

Implementation is being conducted in https://github.com/paritytech/polkadot-sdk/pull/1781.
Status: RFC has been slimmed down and the controversial change to BlockBuilder removed by finding an alternative.

tomaka commented 1 year ago

The way the client authors a block is:

The Substrate client does not support inherents that don't come first.

As I've mentioned above, the runtime is in control of the list of inherents that will be applied, as it decides of the list returned by BlockBuilder_inherent_extrinsics.

kianenigma commented 1 year ago

The Substrate client does not support inherents that don't come first.

You are mentioning the new procedure, and of course, in the new format the Substrate client only accepts inherents that come first.

But this is a new assumption, as per this RFC, and I am asking why that is the case.

tomaka commented 1 year ago

This is not a new assumption. The block building procedure that the Substrate client follows always puts inherents first and has always done so.

If you want to add the possibility for the inherents to not come first, you have to add a way for the runtime to somehow tell the client when to insert the inherents, because no such thing exists right now. The client cannot magically know the order in which it can insert things, all it does is follow the instructions from the runtime.

JoshOrndorff commented 1 year ago

I also want to chime in about the idea of inherents always being first. I believe @tomaka's point is that the block builder has always put inherents at the beginning and this is not new. He is correct about that. I believe @kianenigma's point is that this was never specified as the the only way to do it, but rather just the way that the block builder happened to be implemented.

I share @kianenigma's concern that this removes flexibility, and I have a concrete use case. Tuxedo is a runtime framework for building utxo-based chains (it is vaguely analogous to frame).

Constraints:

  1. In the UTXO model there is one very important invariant which is that every piece of state (utxo) is created explicitly as the output of some transaction.
  2. It is also common for the difference in input and output values in the transactions to go to the block author as a reward.

In order for the block author's reward to be explicitly created by a transaction as required by 1, we need an inherent that creates the reward. But in order for it to reflect the correct tip amount which is only known after the other transactions are applied, the inherent must go at the end of the block.

My Request: Please don't bake the assumption that inherents must be first into the Substrate protocol level (I don't care if you do it in FRAME though).

ggwpez commented 1 year ago

My Request: Please don't bake the assumption that inherents must be first into the Substrate protocol level (I don't care if you do it in FRAME though).

Yes I see... The proposed design is quite strict with its assumptions. I will try the alternative approach with the transaction ordering that is mentioned in the "Future Directions" section.

kianenigma commented 1 year ago

This is not a new assumption. The block building procedure that the Substrate client follows always puts inherents first and has always done so.

The default substrate block_builder is also an implementation and not the specification.

As it stands now, both sides of a contract (block_builder and FRAME) adhere to an arbitrary property (inherents come first), but the contract in between them does not enforce this.

I don't necessarily want to disagree with this new API, but I want us to be clear about what we are doing:

tomaka commented 1 year ago

The default substrate block_builder is also an implementation and not the specification.

The fact that inherents always come first is very much the specification. It is how it's designed. It's part of the protocol between the runtime and the client.

Even if it was not the specification, as I've said earlier there's technically no way for the client to know when to inject the inherents if it's not first. The ordering of transactions follows strict rules, and the ordering of inherents is the same as what the runtime returns, but the ordering between inherents and transactions would be completely undefined if it wasn't for "inherents always come first".

If you want inherents to not come first, you have to design a new feature. You have to modify the specification. Again, it's simply not technically possible without designing a new feature, even if you wanted.

I can understand the argument that you want to leave the room later for the possibility to modify the specification so that inherents don't come first, and it is a valid argument against this RFC, but it is wrong to say that inherents do not necessarily come first now.

pepyakin commented 1 year ago

I think the concept of inherents is limited. Right now we treat inherents as transactions, i.e. each inherent triggers a state transition, but I would argue that they are not transactions, or rather treating them as such is limiting. What they actually are trying to achieve is passing some data from the block builder into the runtime, and as we learned the hard way it's very useful to separate the data submission and data handling. The main motiviation to create the original issue (https://github.com/paritytech/polkadot-sdk/issues/312) was that some of that data is not available in on_initialize. As the solution to this we thought about adding on_after_inherents, which kind of solves some issues, but doesn't solve the others. There was an idea to submit all the inherent data, we would not check/process the data and instead just stash that data somewhere, and finally in on_after_inherents we would finally unstash it and process.

But, the question is, why don't we treat inherents as what it is: just some data that block producer included in the block body?

Here is a straw-man proposal. Imagine there is a special place in a block that is dedicated for inherent data. This data is represented as a (btree)map between the inherent ID and the inherent data. The runtime interface gets a function get_inherent_data(inherent_id) → Option<Vec<u8>>. It's behavior is defined as follows:

  1. During normal block execution, this function should be able to read the inherents from the block directly.
  2. During candidate validation (in polkadot PVF), cumulus would read this data from the block similar to the normal block execution.
  3. During block building, upon the first call this function would look up the sp_inherents::InherentDataProvider for the specified ID and get the corresponding data. If it's not the first call, then the data generated on the first call is returned.

Now, what does this construction give us?

Note that the generation of inherent data now becomes lazy if needed but it doesn't have to be that way and the inherent data could be fixed before the block creation and just returned as is in from the InherentDataProvider. Because the data is not fixed, this allow us to provide the inherent data at the end of the block.

Another consequence, is that there is no such concept as a mandatory/optional inherents. If the inherent data returned is None but the runtime is not content with that, it can immediatelly signal that by e.g. panicking.

If sp_inherents::InherentDataProvider was changed in a way that it can inspect the storage of the system, then it would make it possible to support Tuxedo (as described by @JoshOrndorff here). Specifically, the inherent data provider would access the UTXO module that would provide the difference. Although, an alternative implementation is possible where tips are tallied up through events, this is just a demonstration of flexibility of this approach.

We could extend this proposal even more. The straw-man proposal above uses the static InherentId just because it is not too disimilar to what we have right now. However, it's easy to imagine how this mechanism could benefit from dynamic requests. To give you motivation, right now when we produce a parachain block, the collator supplies data from the relay-chain as inherent. Specifically, that data includes some storage proofs from the relay-chain required for channels to work. This is now implemented by tightly coupling the block production with the parachain-system pallet. I.e. block production knows what storage parachain-system will want to read and reads all the required data from the relay-chain along with the merkle proofs. But turns out people actually want to read the data from the relay-chain or potentially any other parachainchain (e.g. https://github.com/paritytech/polkadot-sdk/issues/82). So if we had two functions provided as inherents: 1. read storage from another chain 2. provide the multiproof of the read storage then we could support this use-case.

I must note however that the tight coupling between inherents checking and block building is desired for efficiency. In the above example, when we fetch the relay-chain storage proofs it won't be too good of an idea to wait until the runtime requests a key that we already know will be requested so we could as well request it in advance. However, note that this inherent mechanism doesn't preclude that: we could still initiate the requests for the storage items we know will be read in advance and then block in the InherentDataProvider only if the data didn't come in time.

Paging in @xlc @bkchr

ggwpez commented 11 months ago

Retracted for being too rigid with the after_inherents hook.

I will factor out the initialize_block changes to another RFC since I think that is enough to get MBMs going. Downside is that we wont be able to use it for the poll hook.

ggwpez commented 10 months ago

Reopening after further discussion at the Fellowship meeting today.
Outcomes:

ggwpez commented 5 months ago

The RFC has been slimmed down and the controversial change to BlockBuilder removed by finding an alternative.

kianenigma commented 5 months ago

/rfc propose

paritytech-rfc-bot[bot] commented 5 months ago

Hey @kianenigma, here is a link you can use to create the referendum aiming to approve this RFC number 0013.

Instructions 1. Open the [link](https://polkadot.js.org/apps/?rpc=wss%3A%2F%2Fpolkadot-collectives-rpc.polkadot.io#/extrinsics/decode/0x3d003e02015901000049015246435f415050524f564528303031332c33666438386531333433633937346635666239333237356161373536393162303561393334366531613533356637633663653638666662653337623563663936290100000000). 2. Switch to the `Submission` tab. 3. Adjust the transaction if needed (for example, the proposal Origin). 4. Submit the Transaction

It is based on commit hash d4329c544de0ef635c2c51ce2421e16e18e22858.

The proposed remark text is: RFC_APPROVE(0013,3fd88e1343c974f5fb93275aa75691b05a9346e1a535f7c6ce68ffbe37b5cf96).

ggwpez commented 5 months ago

/rfc propose

paritytech-rfc-bot[bot] commented 5 months ago

Hey @ggwpez, here is a link you can use to create the referendum aiming to approve this RFC number 0013.

Instructions 1. Open the [link](https://polkadot.js.org/apps/?rpc=wss%3A%2F%2Fpolkadot-collectives-rpc.polkadot.io#/extrinsics/decode/0x3d003e02015901000049015246435f415050524f564528303031332c33396436653739633761306164393962356664373331373131626562353164306233313836646261656238396430356436633930386430353735666663623439290100000000). 2. Switch to the `Submission` tab. 3. Adjust the transaction if needed (for example, the proposal Origin). 4. Submit the Transaction

It is based on commit hash e2769ade76f2622ccd9d46ff6c59938f51677447.

The proposed remark text is: RFC_APPROVE(0013,39d6e79c7a0ad99b5fd731711beb51d0b3186dbaeb89d05d6c908d0575ffcb49).

kianenigma commented 5 months ago

submitted: https://collectives.subsquare.io/fellowship/referenda/79

github-actions[bot] commented 5 months ago

Voting for this referenda is ongoing.

Vote for it here

ggwpez commented 4 months ago

/rfc process 0xbb5a58e7aef7153e78d37d7248c7114739e0da19040356045d1b56de86813cc2

paritytech-rfc-bot[bot] commented 4 months ago

The on-chain referendum has approved the RFC.