Remove service, migrate all to service-new #1630

coriolinus · 2020-08-24T14:33:32Z

Makes progress towards #1620. Does not close that issue.

Integrates #1697.

Note: because this work involves replacing the old service with service-new, this needs some burn-in to ensure that we can still follow the existing chains and author blocks.

Note: initially, the goal was to do all the work necessary for #1620 in this PR, hence the branch name. However, recently I've been spending a lot of time just keeping up with changes to master which update service but not service-new, and that gets old fast. There are therefore a couple of changes in here not related to the service update, which would probably be more effort than they're worth to extract.

[edit] Burnin running on kusama-sentry-ue1-0 / kusama-validator-ue1-0

- Move the chain-spec files to node-service - update sufficient cargo files that polkadot-service-new builds - not everything else builds yet

There's a lot more to go, but some progress has happened.

This begins to make Polkadot usable from Cumulus.

…un-rococo

This reverts commit 7b8f9ba.

…un-rococo

This is necessary because one of the current errors when building the test service boils down to: the trait bound `polkadot_test_runtime::RuntimeApiImpl<...>`: `polkadot_primitives::v1::ParachainHost<...>` is not satisfied This is WIP because it appears to be causing some std leakage into the wasm environment, or something; the compiler is currently complaining about duplicate definitions of `panic_handler` and `oom`. Presumably I have to identify all std types (Vec etc) and replace them with sp_std equivalents.

it wasn't std leakage, after all

runtime/test-runtime/src/lib.rs

…ws up

…impl blows up" This reverts commit 68d2f38.

…he real impl blows up"" This reverts commit 04cb1cb. Might have just forgotten to disable default features

runtime/polkadot/src/lib.rs

https://github.com/paritytech/polkadot/blob/26f1fa47f7836ab4bee5d4aad127ebce748320dd/service/src/chain_spec.rs#L362

Cargo.lock

* master: Bump Nominator Reward Limits (#1668)

Note: something changed in the `sc_cli::init_logger` call relatively recently; I had to update it slightly for things to work.

ordian

LGTM

bkchr · 2020-09-18T08:51:36Z

Was this also tested on Polkadot nodes?

We can not merge a pr that touches such a sensible part of the code base without testing it on Polkadot.

This reverts commit af14ea5.

tomaka · 2020-09-18T09:04:25Z

Nothing problematic, but on the burnin I see that we're running 15 tasks/node named DummySubsystem, is that normal?

coriolinus · 2020-09-18T09:05:21Z

Yes, for now. Replacing those with real subsystems is next up.

bkchr · 2020-09-18T09:05:55Z

Nothing problematic, but on the burnin I see that we're running 15 tasks/node named DummySubsystem, is that normal?

We still need to test it at least once on Polkadot.

This reverts commit af14ea5.

i.e. Revert "Revert "Remove service, migrate all to service-new (#1630)" (#1731)" This reverts commit c68aee3. This allows us to get the changeset from #1630 into a new branch which can be merged sometime in the future after appropriate burnin tests have completed.

gavofyork · 2020-09-18T09:26:52Z

node/service/src/grandpa_support.rs

-			client.info().best_number,
-			10,
-		);
+		assert_eq!(client.info().best_number, 10,);


was this done intentionally or is an automatic formatter being used?

In this case rustfmt generated the change.

gavofyork · 2020-09-18T09:27:24Z

node/service/src/grandpa_support.rs

-				&get_header(10),
-				&get_header(10),
-			),
+			voting_rule.restrict_vote(&*client, &get_header(0), &get_header(10), &get_header(10),),


with the exception of 1-item tuples, ,) is an invalid string in our codebase.

OK: 372f4f6.

Though I suspect that it is also valid when used appropriately in macros, i.e.

polkadot/node/network/availability-distribution/src/tests.rs

Line 35 in c68aee3

( $( $hash:expr ),* $(,)? ) => [

gavofyork · 2020-09-18T09:29:09Z

cli/src/command.rs

-					= service::new_full_nongeneric(
-						config, None, authority_discovery_enabled, grandpa_pause, false,
+					= service::build_full(
+						config, None, authority_discovery_enabled, grandpa_pause,


invalid formatting (though this PR makes it no worse).

OK: 944b745.

gavofyork · 2020-09-18T09:31:18Z

node/service/src/grandpa_support.rs

-				target_header = backend.header(BlockId::Hash(target_hash)).ok()?
-					.expect("Header known to exist due to the existence of one of its descendents; qed");
+				target_header = backend.header(BlockId::Hash(target_hash)).ok()?.expect(
+					"Header known to exist due to the existence of one of its descendents; qed",


previous formatting was better.

OK: b7b45b3.

* master: Revert "Remove service, migrate all to service-new (#1630)" (#1731) Remove service, migrate all to service-new (#1630) Updating tracing dependency (#1722)

* Restore "Remove service, migrate all to service-new (#1630)" i.e. Revert "Revert "Remove service, migrate all to service-new (#1630)" (#1731)" This reverts commit c68aee3. This allows us to get the changeset from #1630 into a new branch which can be merged sometime in the future after appropriate burnin tests have completed. * remove ',)' from codebase outside of macros * restore bdfl-preferred formatting * attempt to improve destructuring formatting * rename polkadot-service-new -> polkadot-service * Apply suggestions from code review Co-authored-by: Bastian Köcher <[email protected]> * remove unused import * Update runtime/rococo-v1/README.md Co-authored-by: Bastian Köcher <[email protected]> Co-authored-by: Andronik Ordian <[email protected]> Co-authored-by: Bastian Köcher <[email protected]>

coriolinus added 2 commits August 24, 2020 14:00

import rococo into chain-spec

03e9dde

Merge remote-tracking branch 'origin/master' into prgn-run-rococo

71bffb8

coriolinus self-assigned this Aug 24, 2020

github-actions bot added the A3-in_progress Pull request is in progress. No review needed at this stage. label Aug 24, 2020

coriolinus and others added 11 commits August 25, 2020 17:08

make a few stabs at moving forward

4b8e65c

wip: rococo readme

141a5eb

Merge remote-tracking branch 'origin/master' into prgn-run-rococo

420e67b

remove /service crate

948efd6

- Move the chain-spec files to node-service - update sufficient cargo files that polkadot-service-new builds - not everything else builds yet

wip: chase down some build errors in polkadot-cli

b32ed40

There's a lot more to go, but some progress has happened.

make more progress getting polkadot-cli to build

61b584f

don't ignore polkadot.json within the res directory

a94a4f6

don't recreate pathbufs

84de271

Prepare Polkadot to be used by Cumulus

fe36c55

This begins to make Polkadot usable from Cumulus.

Remove old test

b8b958f

Merge remote-tracking branch 'origin/bkchr-start-cumulus' into prgn-r…

bf58a14

…un-rococo

paritytech-ci added the D2-breaksapi label Sep 10, 2020

coriolinus and others added 9 commits September 10, 2020 12:47

Merge remote-tracking branch 'origin/bkchr-start-cumulus' into prgn-r…

2e8fdab

…un-rococo

migrate new_chain_ops fix from /service

24e2e29

partially remove node/test-service

7b8f9ba

Reset some changes

bb177a9

Revert "partially remove node/test-service"

a148e7d

This reverts commit 7b8f9ba.

Merge remote-tracking branch 'origin/bkchr-start-cumulus' into prgn-r…

39e140e

…un-rococo

fix test runtime build

68fccb5

it wasn't std leakage, after all

bump westend spec version

93f8d4f

rphmeier reviewed Sep 11, 2020

View reviewed changes

runtime/test-runtime/src/lib.rs Outdated Show resolved Hide resolved

coriolinus added 4 commits September 11, 2020 15:41

use service-new as service within cli

2ae8cee

to revert: demo that forwarding the test runtime to the real impl blo…

68d2f38

…ws up

Revert "to revert: demo that forwarding the test runtime to the real …

04cb1cb

…impl blows up" This reverts commit 68d2f38.

Revert "Revert "to revert: demo that forwarding the test runtime to t…

32fc3ab

…he real impl blows up"" This reverts commit 04cb1cb. Might have just forgotten to disable default features

bkchr reviewed Sep 16, 2020

View reviewed changes

runtime/polkadot/src/lib.rs Outdated Show resolved Hide resolved

coriolinus added 2 commits September 16, 2020 13:45

adjust rococo chainspec per example

34e51e5

https://github.com/paritytech/polkadot/blob/26f1fa47f7836ab4bee5d4aad127ebce748320dd/service/src/chain_spec.rs#L362

Merge remote-tracking branch 'origin/master' into prgn-run-rococo

3060842

ordian reviewed Sep 16, 2020

View reviewed changes

Cargo.lock Outdated Show resolved Hide resolved

Andronik Ordian and others added 4 commits September 16, 2020 15:06

try to fix Cargo.lock

cf2062a

Merge branch 'master' into prgn-run-rococo

563555d

* master: Bump Nominator Reward Limits (#1668)

Merge remote-tracking branch 'origin/master' into prgn-run-rococo

df7a758

Merge remote-tracking branch 'origin/master' into prgn-run-rococo

731ebf4

Note: something changed in the `sc_cli::init_logger` call relatively recently; I had to update it slightly for things to work.

ordian approved these changes Sep 17, 2020

View reviewed changes

Merge remote-tracking branch 'origin/master' into prgn-run-rococo

65c388b

coriolinus merged commit af14ea5 into master Sep 18, 2020

coriolinus deleted the prgn-run-rococo branch September 18, 2020 08:27

coriolinus added a commit that referenced this pull request Sep 18, 2020

Revert "Remove service, migrate all to service-new (#1630)"

58a5a70

This reverts commit af14ea5.

coriolinus mentioned this pull request Sep 18, 2020

Revert "Remove service, migrate all to service-new" #1731

Merged

coriolinus restored the prgn-run-rococo branch September 18, 2020 09:00

bkchr deleted the prgn-run-rococo branch September 18, 2020 09:05

coriolinus added a commit that referenced this pull request Sep 18, 2020

Revert "Remove service, migrate all to service-new (#1630)" (#1731)

c68aee3

This reverts commit af14ea5.

coriolinus mentioned this pull request Sep 18, 2020

Remove Old Service, 2nd try #1732

Merged

gavofyork reviewed Sep 18, 2020

View reviewed changes

coriolinus mentioned this pull request Oct 2, 2020

Remove old service, 3rd try #1776

Merged

Remove service, migrate all to service-new #1630

Remove service, migrate all to service-new #1630

Uh oh!

Conversation

coriolinus commented Aug 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ordian left a comment

Choose a reason for hiding this comment

Uh oh!

bkchr commented Sep 18, 2020

Uh oh!

tomaka commented Sep 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coriolinus commented Sep 18, 2020

Uh oh!

bkchr commented Sep 18, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

coriolinus commented Aug 24, 2020 •

edited

Loading

tomaka commented Sep 18, 2020 •

edited

Loading