Collator: Fix can_build_upon by always allowing to build on included block
#7205
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
/cmd prdoc --bump minor --audience node_dev |
bkchr
approved these changes
Jan 16, 2025
lexnv
approved these changes
Jan 17, 2025
michalkucharczyk
approved these changes
Jan 17, 2025
Co-authored-by: Michal Kucharczyk <1728078+michalkucharczyk@users.noreply.github.com>
auto-merge was automatically disabled
January 17, 2025 17:39
Pull Request is not mergeable
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
Nathy-bajo
pushed a commit
to Nathy-bajo/polkadot-sdk
that referenced
this pull request
Jan 21, 2025
…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>
Nathy-bajo
pushed a commit
to Nathy-bajo/polkadot-sdk
that referenced
this pull request
Jan 21, 2025
…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>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Follow-up to #6825, which introduced this bug.
We use the
can_build_uponmethod to ask the runtime if it is fine to build another block. The runtime checks this based on theConsensusHookimplementation, the most popular one being theFixedConsensusHook.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:
UnincludedSegmentstorage item in pallet-parachain-system is equal or larger than the unincluded segment.can_build_uponruntime API where the included block has progressed offchain to the current parent block (so last entry in theUnincludedSegmentstorage item).In this scenario the last entry in
UnincludedSegmentdoes not have a hash assigned yet (because it was not available inon_finalizeof 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.