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

Refactor polkadot-parachain service for more code reuse #3511

Merged
merged 18 commits into from
Mar 8, 2024

Conversation

skunert
Copy link
Contributor

@skunert skunert commented Feb 28, 2024

This is a refactoring, no changes to the logic are included (if you find some, report :D).

Change Overview

In #2455, dependency on actual runtimes was removed for the system parachains. This means that the trait bounds we have on various start_node_xy do not check anything anymore, since they all point to the same runtime. Exception is asset-hub-polkadot which uses a different key type.

This PR unifies the different nodes as much as possible. start_node_impl is doing the heavy lifting and has been made a bit more flexible to support the rpc extension instantiation that was there before.

The generics for Runtime and AuraId have been removed where possible. The fake runtime is imported as FakeRuntime to make it very clear to readers that it is not the generic parameter.

Multiple nodes where using the same import queue/start_consensus closure, they have been unified.

@skunert skunert added R0-silent Changes should not be mentioned in any release notes T0-node This PR/Issue is related to the topic “node”. labels Feb 28, 2024
@skunert skunert requested review from a team and seadanda February 29, 2024 16:40
@skunert skunert marked this pull request as ready for review February 29, 2024 16:41
@skunert skunert changed the title WIP: Unify polkadot-parachain service Refactor polkadot-parachain service for more code reuse Feb 29, 2024
@michalkucharczyk michalkucharczyk requested a review from a team March 1, 2024 08:51
kianenigma and others added 9 commits March 4, 2024 18:36
This fixes an issue introduced in
paritytech/substrate#14101, in which I removed
the `Call` enum's documentation and replaced it with a link to the
`Pallet` struct, but this also removed any docs related to call from the
metadata.

I tried to add a regression test for this, but it seems to me that this
is not possible, given that using `type-info` we only assert in type-ids
for `Call`, `Event` and `Error`. I removed some doc comments from a test
setup in `frame-support-test` to demonstrate the issue there. @jsdw do
you have any comments on this?

I also fixed a small issue in the custom html/css of `polkadot-sdk-doc`
crate, making sure it does not affect the rust-doc page of all other
crates.

- [x] Investigate a regression test
- [x] prdoc
If an XCM execution fails or ends with leftover assets, these will be
trapped.
In order to claim them, a custom XCM has to be executed, with the
`ClaimAsset` instruction.
However, arbitrary XCM execution is not allowed everywhere yet and XCM
itself is still not easy enough to use for users out there with trapped
assets.
This new extrinsic in `pallet-xcm` will allow these users to easily
claim their assets, without concerning themselves with writing arbitrary
XCMs.

Part of fixing paritytech#3495

---------

Co-authored-by: command-bot <>
Co-authored-by: Adrian Catangiu <adrian@parity.io>
…doc files from 1.8.0 (paritytech#3508)

This PR backports Node version and `spec_version` bumps to `1.8.0` from
the latest release and orders prdoc files related to it.
expose para id via metadata

paritytech#2116 is blocked by this
…paritytech#3311)

### What's been done
- `subsystem-bench` has been split into two parts: a cli benchmark
runner and a library.
- The cli runner is quite simple. It just allows us to run `.yaml` based
test sequences. Now it should only be used to run benchmarks during
development.
- The library is used in the cli runner and in regression tests. Some
code is changed to make the library independent of the runner.
- Added first regression tests for availability read and write that
replicate existing test sequences.

### How we run regression tests
- Regression tests are simply rust integration tests without the
harnesses.
- They should only be compiled under the `subsystem-benchmarks` feature
to prevent them from running with other tests.
- This doesn't work when running tests with `nextest` in CI, so
additional filters have been added to the `nextest` runs.
- Each benchmark run takes a different time in the beginning, so we
"warm up" the tests until their CPU usage differs by only 1%.
- After the warm-up, we run the benchmarks a few more times and compare
the average with the exception using a precision.

### What is still wrong?
- I haven't managed to set up approval voting tests. The spread of their
results is too large and can't be narrowed down in a reasonable amount
of time in the warm-up phase.
- The tests start an unconfigurable prometheus endpoint inside, which
causes errors because they use the same 9999 port. I disable it with a
flag, but I think it's better to extract the endpoint launching outside
the test, as we already do with `valgrind` and `pyroscope`. But we still
use `prometheus` inside the tests.

### Future work
* paritytech#3528
* paritytech#3529
* paritytech#3530
* paritytech#3531

---------

Co-authored-by: Alexander Samusev <41779041+alvicsam@users.noreply.github.com>
…ch#3233)

paritytech#3130

builds on top of paritytech#3160

Processes the availability cores and builds a record of how many
candidates it should request from prospective-parachains and their
predecessors.
Tries to supply as many candidates as the runtime can back. Note that
the runtime changes to back multiple candidates per para are not yet
done, but this paves the way for it.

The following backing/inclusion policy is assumed:
1. the runtime will never back candidates of the same para which don't
form a chain with the already backed candidates. Even if the others are
still pending availability. We're optimistic that they won't time out
and we don't want to back parachain forks (as the complexity would be
huge).
2. if a candidate is timed out of the core before being included, all of
its successors occupying a core will be evicted.
3. only the candidates which are made available and form a chain
starting from the on-chain para head may be included/enacted and cleared
from the cores. In other words, if para head is at A and the cores are
occupied by B->C->D, and B and D are made available, only B will be
included and its core cleared. C and D will remain on the cores awaiting
for C to be made available or timed out. As point (2) above already
says, if C is timed out, D will also be dropped.
4. The runtime will deduplicate candidates which form a cycle. For
example if the provisioner supplies candidates A->B->A, the runtime will
only back A (as the state output will be the same)

Note that if a candidate is timed out, we don't guarantee that in the
next relay chain block the block author will be able to fill all of the
timed out cores of the para. That increases complexity by a lot.
Instead, the provisioner will supply N candidates where N is the number
of candidates timed out, but doesn't include their successors which will
be also deleted by the runtime. This'll be backfilled in the next relay
chain block.

Adjacent changes:
- Also fixes: paritytech#3141
- For non prospective-parachains, don't supply multiple candidates per
para (we can't have elastic scaling without prospective parachains
enabled). paras_inherent should already sanitise this input but it's
more efficient this way.

Note: all of these changes are backwards-compatible with the
non-elastic-scaling scenario (one core per para).
…derive_impl` (paritytech#3505)

Step in paritytech#171

This PR removes the need to specify `as [disambiguation_path]` for cases
where the trait definition resides within the same scope as default impl
path.

For example, in the following macro invocation
```rust
#[derive_impl(frame_system::config_preludes::TestDefaultConfig as frame_system::DefaultConfig)]
impl frame_system::Config for Runtime {
   ...
}
```
the trait `DefaultConfig` lies within the `frame_system` scope and
`TestDefaultConfig` impls the `DefaultConfig` trait. Using this
information, we can compute the disambiguation path internally, thus
removing the need of an explicit specification.

In cases where the trait lies outside this scope, we would still need to
specify it explicitly, but this should take care of most (if not all)
uses of `derive_impl` within FRAME's context.
Closes paritytech/polkadot-sdk-docs#35

- Moves pallet proc macro docs to `frame_support`
- Adds missing docs
- Revise revise existing docs, adding compiling doctests where
appropriate

---------

Co-authored-by: command-bot <>
Co-authored-by: Kian Paimani <5588131+kianenigma@users.noreply.github.com>
@skunert skunert requested review from a team as code owners March 4, 2024 17:36
@michalkucharczyk
Copy link
Contributor

I guess you wanted to merge origin/master?

@seadanda
Copy link
Contributor

seadanda commented Mar 6, 2024

@skunert if you can merge master I can finish my review, currently there are lots of unrelated changes in the diff

@skunert
Copy link
Contributor Author

skunert commented Mar 6, 2024

@skunert if you can merge master I can finish my review, currently there are lots of unrelated changes in the diff

@seadanda Yes, sorry! Messed this somehow, fill fix asap.

Copy link
Contributor

@seadanda seadanda left a comment

Choose a reason for hiding this comment

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

LGTM, nice refactor

cumulus/polkadot-parachain/src/service.rs Outdated Show resolved Hide resolved
cumulus/polkadot-parachain/src/service.rs Outdated Show resolved Hide resolved
cumulus/polkadot-parachain/src/service.rs Outdated Show resolved Hide resolved
cumulus/polkadot-parachain/src/service.rs Show resolved Hide resolved
@skunert skunert enabled auto-merge March 7, 2024 22:56
@skunert skunert added this pull request to the merge queue Mar 7, 2024
Merged via the queue into paritytech:master with commit 2aa006e Mar 8, 2024
129 of 130 checks passed
@skunert skunert deleted the skunert/clean-up-pp-service branch March 8, 2024 00:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
R0-silent Changes should not be mentioned in any release notes T0-node This PR/Issue is related to the topic “node”.
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.