Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Block import and verification refactoring #4844

Merged

Conversation

nazar-pc
Copy link
Contributor

A few refactorings to block import and block verification that should not be controversial.

Block verification before block import is stateless by design as described in https://substrate.stackexchange.com/a/1322/25 and the fact that it wasn't yet I consider to be a bug. Some code that requires it had to use Mutex, but I do not expect it to have a measurable performance impact.

Similarly with block import checking whether block preconditions should not be an exclusive operation, there is nothing fundamentally wrong with checking a few competing blocks whose parent blocks exist at the same time (and even import them concurrently later, though IIRC this is not yet implemented either).

They were originally a part of #4842 and upstreaming will help us to reduce the size of the patch we need to apply on top of upstream code at Subspace every time we upgrade. There are no new features introduced here, just refactoring to get rid of unnecessary requirements.

Copy link
Contributor

@skunert skunert left a comment

Choose a reason for hiding this comment

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

Overall the changes look good!

cumulus/polkadot-parachain/src/service.rs Outdated Show resolved Hide resolved
Copy link
Contributor Author

@nazar-pc nazar-pc left a comment

Choose a reason for hiding this comment

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

Applied review suggestions and added prdoc

cumulus/polkadot-parachain/src/service.rs Outdated Show resolved Hide resolved
@nazar-pc nazar-pc requested review from skunert and bkchr June 25, 2024 12:43
Copy link
Contributor

@skunert skunert left a comment

Choose a reason for hiding this comment

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

Nice, thanks a lot!

@skunert skunert added the T0-node This PR/Issue is related to the topic “node”. label Jun 25, 2024
@skunert
Copy link
Contributor

skunert commented Jun 25, 2024

@nazar-pc I think you need to include more crates into the PRDoc to make the CI semver check pass. Can you see the semver job output?

@nazar-pc
Copy link
Contributor Author

Hm... I think sc-consensus change is correct, it is definitely a breaking change what has happened there.

For others I agree that it needs to change after reading docs, but not sure if major or minor. I'm leaning towards major, any objections?

@skunert
Copy link
Contributor

skunert commented Jun 25, 2024

Hm... I think sc-consensus change is correct, it is definitely a breaking change what has happened there.

For others I agree that it needs to change after reading docs, but not sure if major or minor. I'm leaning towards major, any objections?

Yes, breaking for sure, we should make the decision, CI is just a suggestion. You can put validate: false on items where you disagree with CI.
As for the others, I also think major is correct, except maybe stuff like polkadot-parachain binary. There user should not notice any change.

@skunert skunert added this pull request to the merge queue Jun 26, 2024
Merged via the queue into paritytech:master with commit 0ed3f04 Jun 26, 2024
156 of 159 checks passed
@nazar-pc nazar-pc deleted the block-import-verification-refactoring branch June 26, 2024 12:41
@nazar-pc
Copy link
Contributor Author

Appreciate quick review, folks 👍

TarekkMA pushed a commit to moonbeam-foundation/polkadot-sdk that referenced this pull request Aug 2, 2024
A few refactorings to block import and block verification that should
not be controversial.

Block verification before block import is stateless by design as
described in https://substrate.stackexchange.com/a/1322/25 and the fact
that it wasn't yet I consider to be a bug. Some code that requires it
had to use `Mutex`, but I do not expect it to have a measurable
performance impact.

Similarly with block import checking whether block preconditions should
not be an exclusive operation, there is nothing fundamentally wrong with
checking a few competing blocks whose parent blocks exist at the same
time (and even import them concurrently later, though IIRC this is not
yet implemented either).

They were originally a part of
paritytech#4842 and upstreaming
will help us to reduce the size of the patch we need to apply on top of
upstream code at Subspace every time we upgrade. There are no new
features introduced here, just refactoring to get rid of unnecessary
requirements.
@nazar-pc nazar-pc mentioned this pull request Aug 12, 2024
github-merge-queue bot pushed a commit that referenced this pull request Aug 12, 2024
Trivial leftover from
#4844

Co-authored-by: Adrian Catangiu <adrian@parity.io>
Co-authored-by: Bastian Köcher <git@kchr.de>
github-merge-queue bot pushed a commit that referenced this pull request Aug 18, 2024
…block()` (#5339)

There was no need for it to be `&mut self` since block import can happen
concurrently for different blocks and in many cases it was `&mut Arc<dyn
BlockImport>` anyway 🤷‍♂️

Similar in nature to
#4844
programskillforverification pushed a commit to programskillforverification/polkadot-sdk that referenced this pull request Aug 19, 2024
…block()` (paritytech#5339)

There was no need for it to be `&mut self` since block import can happen
concurrently for different blocks and in many cases it was `&mut Arc<dyn
BlockImport>` anyway 🤷‍♂️

Similar in nature to
paritytech#4844
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T0-node This PR/Issue is related to the topic “node”.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants