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

Collator overseer builder unification #3335

Merged

Conversation

maksimryndin
Copy link
Contributor

@maksimryndin maksimryndin commented Feb 15, 2024

resolve #3116

a follow-up on #3061 (review):

  • reuse collator overseer builder for polkadot-node and collator
  • run zombienet test (0001-parachains-smoke-test.toml)
  • make wasm build errors more user-friendly for an easier problem detection when using different toolchains in Rust

@maksimryndin
Copy link
Contributor Author

@skunert @s0me0ne-unkn0wn
Proposed labels: T0-node, T9-cumulus, I4-refactor, R0-silent

@maksimryndin maksimryndin marked this pull request as ready for review February 15, 2024 13:20
@skunert skunert self-requested a review February 16, 2024 08:34
@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”. I4-refactor Code needs refactoring. T9-cumulus This PR/Issue is related to cumulus. labels Feb 19, 2024
@skunert
Copy link
Contributor

skunert commented Feb 19, 2024

Thank you for looking into this!

IMO we could go even further and remove special treatment of the ChainApiSubsystem.
So basically what I am suggesting:

  1. Change the requirements for RuntimeClient in overseer.rs to RuntimeClient: RuntimeApiSubsystemClient + ChainApiBackend + AuxStore + 'static
  2. Implement AuxStore and HeaderBackend for DefaultSubsystemClient
  3. Instantiate DefaultSubsystemClient here
    let (overseer, overseer_handle) = overseer_gen

We would get rid of the offfchain_transaction_pool_factory arg here and can remove generic_collator_overseer_builder. Does that make sense?

@maksimryndin
Copy link
Contributor Author

maksimryndin commented Feb 19, 2024

Thank you for looking into this!

IMO we could go even further and remove special treatment of the ChainApiSubsystem. So basically what I am suggesting:

1. Change the requirements for `RuntimeClient` in `overseer.rs` to `RuntimeClient: RuntimeApiSubsystemClient + ChainApiBackend + AuxStore + 'static`

2. Implement `AuxStore` and `HeaderBackend` for `DefaultSubsystemClient`

3. Instantiate `DefaultSubsystemClient` here https://github.com/paritytech/polkadot-sdk/blob/2a32b5c9379e6daefd2061a8fb5b4d972d3460a1/polkadot/node/service/src/lib.rs#L1085

We would get rid of the offfchain_transaction_pool_factory arg here and can remove generic_collator_overseer_builder. Does that make sense?

@skunert An excellent idea! Thank you! I believe I've got it but have some compilation errors, will try to resolve tomorrow

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.

Really nice! Thanks a lot!

I left some last suggestions, otherwise looks good.

polkadot/node/subsystem-types/src/runtime_client.rs Outdated Show resolved Hide resolved
polkadot/node/subsystem-types/src/runtime_client.rs Outdated Show resolved Hide resolved
polkadot/node/service/src/overseer.rs Outdated Show resolved Hide resolved
polkadot/node/service/src/overseer.rs Outdated Show resolved Hide resolved
@skunert skunert requested review from a team and s0me0ne-unkn0wn February 21, 2024 16:51
Copy link
Member

@ordian ordian left a comment

Choose a reason for hiding this comment

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

Ty! LGTM

Copy link
Member

Choose a reason for hiding this comment

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

these changes seem unrelated, but looking welcome and good
for the future, i'd prefer this to be submitted via a different PR, but let's keep this one here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, sorry, I encountered some issues during the setup on arm64 Linux so I decided to make the messages more user-friendly :)

Copy link
Contributor

@AndreiEres AndreiEres left a comment

Choose a reason for hiding this comment

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

LGTM, thank you!

@s0me0ne-unkn0wn s0me0ne-unkn0wn added this pull request to the merge queue Feb 27, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Feb 27, 2024
@ordian ordian added this pull request to the merge queue Feb 28, 2024
Merged via the queue into paritytech:master with commit 7ec0b87 Feb 28, 2024
129 of 130 checks passed
ordian added a commit that referenced this pull request Feb 28, 2024
…head-data

* origin/master: (51 commits)
  Runtime Upgrade ref docs and Single Block Migration example pallet  (#1554)
  Collator overseer builder unification (#3335)
  Introduce storage attr macro `#[disable_try_decode_storage]` and set it on `System::Events` and `ParachainSystem::HostConfiguration` (#3454)
  Add Polkadotters bootnoders per IBP application (#3423)
  Add documentation around FRAME Origin (#3362)
  Bridge zombienet tests: Check amount received at destination (#3490)
  Snowbridge benchmark tests fix (#3424)
  fix(zombienet): increase timeout in download artifacts (#3376)
  Cleanup String::from_utf8 (#3446)
  [prdoc] Validate crate names (#3467)
  Limit max execution time for `test-linux-stable` CI jobs (#3483)
  Introduce Notification block pinning limit (#2935)
  frame-support: Improve error reporting when having too many pallets (#3478)
  add Encointer as trusted teleporter for Westend (#3411)
  [pallet-xcm] Adjust benchmarks (teleport_assets/reserve_transfer_assets) not relying on ED (#3464)
  Add more debug logs to understand if statement-distribution misbehaving (#3419)
  Remove redundant parachains assigner pallet. (#3457)
  Use generic hash for runtime wasm in resolve_state_version_from_wasm (#3447)
  Runtime: allow backing multiple candidates of same parachain on different cores  (#3231)
  Bridge zombienet tests: move all "framework" files under one folder (#3462)
  ...
bgallois pushed a commit to duniter/duniter-polkadot-sdk that referenced this pull request Mar 25, 2024
resolve paritytech#3116

a follow-up on
paritytech#3061 (review):

- [x] reuse collator overseer builder for polkadot-node and collator
- [x] run zombienet test (0001-parachains-smoke-test.toml)
- [x] make wasm build errors more user-friendly for an easier problem
detection when using different toolchains in Rust

---------

Co-authored-by: ordian <[email protected]>
Co-authored-by: s0me0ne-unkn0wn <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
I4-refactor Code needs refactoring. R0-silent Changes should not be mentioned in any release notes T0-node This PR/Issue is related to the topic “node”. T9-cumulus This PR/Issue is related to cumulus.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use common overseer construction codepath for polkadot node + collator
5 participants