Parachains: Use relay chain slot for velocity measurement#6825
Parachains: Use relay chain slot for velocity measurement#6825
Conversation
|
bot fmt |
|
@skunert https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/7892036 was started for your command Comment |
|
@skunert Command |
|
bot fmt |
|
@skunert https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/7898854 was started for your command Comment |
|
@skunert Command |
|
/cmd prdoc --bump patch --audience runtime_dev,node_dev |
| let Ok(Some(api_version)) = | ||
| runtime_api.api_version::<dyn AuraUnincludedSegmentApi<Block>>(parent_hash) | ||
| else { | ||
| return Some(SlotClaim::unchecked::<P>(author_pub, para_slot, timestamp)) |
There was a problem hiding this comment.
shouldn't we return None if we couldn't get the api version?
There was a problem hiding this comment.
Yeah, if we have None, it means the api doesn't exist and we should return an error.
There was a problem hiding this comment.
I changed this to be backward compatible with the previous behaviour. If we are building on top of the included block, the previous implementation would return Some. I don't think it is likely that someone runs this with a runtime without AuraUnincludedSegmentApi, but technically its possible.
| // Fetch the authorities once to get them into the storage proof of the PoV. | ||
| Authorities::<T>::get(); | ||
|
|
||
| let new_slot = pallet_aura::CurrentSlot::<T>::get(); |
There was a problem hiding this comment.
it feels a bit weird that the pallet now has a RelaySlotInfo storage item that is never updated by the pallet (but only in an optional ConsensusHook, which parachains could in theory overwrite). Not a big deal though
| let Ok(Some(api_version)) = | ||
| runtime_api.api_version::<dyn AuraUnincludedSegmentApi<Block>>(parent_hash) | ||
| else { | ||
| return Some(SlotClaim::unchecked::<P>(author_pub, para_slot, timestamp)) |
There was a problem hiding this comment.
Yeah, if we have None, it means the api doesn't exist and we should return an error.
* master: (33 commits) Implement `pallet-asset-rewards` (#3926) [pallet-revive] Add host function `to_account_id` (#7091) [pallet-revive] Remove revive events (#7164) [pallet-revive] Remove debug buffer (#7163) litep2p: Provide partial results to speedup GetRecord queries (#7099) [pallet-revive] Bump asset-hub westend spec version (#7176) Remove 0 as a special case in gas/storage meters (#6890) [pallet-revive] Fix `caller_is_root` return value (#7086) req-resp/litep2p: Reject inbound requests from banned peers (#7158) Add "run to block" tools (#7109) Fix reversed error message in DispatchInfo (#7170) approval-voting: Make importing of duplicate assignment idempotent (#6971) Parachains: Use relay chain slot for velocity measurement (#6825) PRDOC: Document `validate: false` (#7117) xcm: convert properly assets in xcmpayment apis (#7134) CI: Only format umbrella crate during umbrella check (#7139) approval-voting: Fix sending of assignments after restart (#6973) Retry approval on availability failure if the check is still needed (#6807) [pallet-revive-eth-rpc] persist eth transaction hash (#6836) litep2p: Sufix litep2p to the identify agent version for visibility (#7133) ...
…d block (#7205) Follow-up to #6825, which introduced this bug. We use the `can_build_upon` method to ask the runtime if it is fine to build another block. The runtime checks this based on the [`ConsensusHook`](https://github.com/paritytech/polkadot-sdk/blob/c1b7c3025aa4423d4cf3e57309b60fb7602c2db6/cumulus/pallets/aura-ext/src/consensus_hook.rs#L110-L110) implementation, the most popular one being the `FixedConsensusHook`. In #6825 I removed a check that would always allow us to build when we are building on an included block. Turns out this check is still required when: 1. The [`UnincludedSegment` ](https://github.com/paritytech/polkadot-sdk/blob/c1b7c3025aa4423d4cf3e57309b60fb7602c2db6/cumulus/pallets/parachain-system/src/lib.rs#L758-L758) storage item in pallet-parachain-system is equal or larger than the unincluded segment. 2. We are calling the `can_build_upon` runtime API where the included block has progressed offchain to the current parent block (so last entry in the `UnincludedSegment` storage item). In this scenario the last entry in `UnincludedSegment` does not have a hash assigned yet (because it was not available in `on_finalize` of the previous block). So the unincluded segment will be reported at its maximum length which will forbid building another block. Ideally we would have a more elegant solution than to rely on the node-side here. But for now the check is reintroduced and a test is added to not break it again by accident. --------- Co-authored-by: command-bot <> Co-authored-by: Michal Kucharczyk <1728078+michalkucharczyk@users.noreply.github.com>
…#6825) closes paritytech#3967 ## Changes We now use relay chain slots to measure velocity on chain. Previously we were storing the current parachain slot. Then in `on_state_proof` of the `ConsensusHook` we were checking how many blocks were athored in the current parachain slot. This works well when the parachain slot time and relay chain slot time is the same. With elastic scaling, we can have parachain slot times lower than that of the relay chain. In these cases we want to measure velocity in relation to the relay chain. This PR adjusts that. ## Migration This PR includes a migration. Storage item `SlotInfo` of pallet `aura-ext` is renamed to `RelaySlotInfo` to better reflect its new content. A migration has been added that just kills the old storage item. `RelaySlotInfo` will be `None` initially but its value will be adjusted after one new relay chain slot arrives. --------- Co-authored-by: command-bot <> Co-authored-by: Bastian Köcher <git@kchr.de>
…d block (paritytech#7205) Follow-up to paritytech#6825, which introduced this bug. We use the `can_build_upon` method to ask the runtime if it is fine to build another block. The runtime checks this based on the [`ConsensusHook`](https://github.com/paritytech/polkadot-sdk/blob/c1b7c3025aa4423d4cf3e57309b60fb7602c2db6/cumulus/pallets/aura-ext/src/consensus_hook.rs#L110-L110) implementation, the most popular one being the `FixedConsensusHook`. In paritytech#6825 I removed a check that would always allow us to build when we are building on an included block. Turns out this check is still required when: 1. The [`UnincludedSegment` ](https://github.com/paritytech/polkadot-sdk/blob/c1b7c3025aa4423d4cf3e57309b60fb7602c2db6/cumulus/pallets/parachain-system/src/lib.rs#L758-L758) storage item in pallet-parachain-system is equal or larger than the unincluded segment. 2. We are calling the `can_build_upon` runtime API where the included block has progressed offchain to the current parent block (so last entry in the `UnincludedSegment` storage item). In this scenario the last entry in `UnincludedSegment` does not have a hash assigned yet (because it was not available in `on_finalize` of the previous block). So the unincluded segment will be reported at its maximum length which will forbid building another block. Ideally we would have a more elegant solution than to rely on the node-side here. But for now the check is reintroduced and a test is added to not break it again by accident. --------- Co-authored-by: command-bot <> Co-authored-by: Michal Kucharczyk <1728078+michalkucharczyk@users.noreply.github.com>
…d block (paritytech#7205) Follow-up to paritytech#6825, which introduced this bug. We use the `can_build_upon` method to ask the runtime if it is fine to build another block. The runtime checks this based on the [`ConsensusHook`](https://github.com/paritytech/polkadot-sdk/blob/c1b7c3025aa4423d4cf3e57309b60fb7602c2db6/cumulus/pallets/aura-ext/src/consensus_hook.rs#L110-L110) implementation, the most popular one being the `FixedConsensusHook`. In paritytech#6825 I removed a check that would always allow us to build when we are building on an included block. Turns out this check is still required when: 1. The [`UnincludedSegment` ](https://github.com/paritytech/polkadot-sdk/blob/c1b7c3025aa4423d4cf3e57309b60fb7602c2db6/cumulus/pallets/parachain-system/src/lib.rs#L758-L758) storage item in pallet-parachain-system is equal or larger than the unincluded segment. 2. We are calling the `can_build_upon` runtime API where the included block has progressed offchain to the current parent block (so last entry in the `UnincludedSegment` storage item). In this scenario the last entry in `UnincludedSegment` does not have a hash assigned yet (because it was not available in `on_finalize` of the previous block). So the unincluded segment will be reported at its maximum length which will forbid building another block. Ideally we would have a more elegant solution than to rely on the node-side here. But for now the check is reintroduced and a test is added to not break it again by accident. --------- Co-authored-by: command-bot <> Co-authored-by: Michal Kucharczyk <1728078+michalkucharczyk@users.noreply.github.com>
This PR will make omni-node dev-mode once again compatible with older runtimes. The changes introduced in #6825 changed constraints that are enforced in the runtime. For normal chains this should work fine, since we have real parameters there, like relay chain slots and parachain slots. For these manual seal parameters we need to respect the constraints, while faking all the parameters. This PR should fix manual seal in omni-node to work with runtime build before and after #6825 (I tested that). In the future, we should look into improving the parameterization here, possibly by introducing proper aura pre-digests so that the parachain slot moves forward. This will require quite a bit of refactoring on the manual seal node side however. Issue: #7453 Also, the dev chain spec in parachain template is updated. This makes it work with stable2412-1 and master omni-node. Once the changes here are backported and in a release, all combinations will work again. fixes #7341 --------- Co-authored-by: cmd[bot] <41898282+github-actions[bot]@users.noreply.github.com>
This PR will make omni-node dev-mode once again compatible with older runtimes. The changes introduced in paritytech#6825 changed constraints that are enforced in the runtime. For normal chains this should work fine, since we have real parameters there, like relay chain slots and parachain slots. For these manual seal parameters we need to respect the constraints, while faking all the parameters. This PR should fix manual seal in omni-node to work with runtime build before and after paritytech#6825 (I tested that). In the future, we should look into improving the parameterization here, possibly by introducing proper aura pre-digests so that the parachain slot moves forward. This will require quite a bit of refactoring on the manual seal node side however. Issue: paritytech#7453 Also, the dev chain spec in parachain template is updated. This makes it work with stable2412-1 and master omni-node. Once the changes here are backported and in a release, all combinations will work again. fixes paritytech#7341 --------- Co-authored-by: cmd[bot] <41898282+github-actions[bot]@users.noreply.github.com>
closes #3967
Changes
We now use relay chain slots to measure velocity on chain. Previously we were storing the current parachain slot. Then in
on_state_proofof theConsensusHookwe were checking how many blocks were athored in the current parachain slot. This works well when the parachain slot time and relay chain slot time is the same. With elastic scaling, we can have parachain slot times lower than that of the relay chain. In these cases we want to measure velocity in relation to the relay chain. This PR adjusts that.Migration
This PR includes a migration. Storage item
SlotInfoof palletaura-extis renamed toRelaySlotInfoto better reflect its new content. A migration has been added that just kills the old storage item.RelaySlotInfowill beNoneinitially but its value will be adjusted after one new relay chain slot arrives.