Decouple the session validators from im-online #7127

liuchengxu · 2020-09-17T08:55:11Z

This PR decouples the session validators from im-online pallet, so that the user of im-online pallet can pass any specific validator list that are ought to be online, particularly useful for the projects having their own staking logic like ChainX.

Fixes #7124

polkadot companion: Conpanion for Substrate#7127 polkadot#1865

… make-im-online-validator-set-flexiable

liuchengxu · 2020-09-22T02:59:09Z

Ping @andresilva @bkchr @tomusdrw

tomusdrw

IMHO the decoupling is incomplete and may cause more harm than good in it's current form.

frame/im-online/src/lib.rs

Add ValidatorId in im-online Trait Make im-online compile Make substrate binary compile

…ub.com/liuchengxu/substrate into make-im-online-validator-set-flexiable

… make-im-online-validator-set-flexiable

tomusdrw

I like the direction it's going to.
Couple of suggestions:

Merge ValidatorSet and SessionInterface. I think the session_index and ValidatorSet must stay tightly coupled.
Add extra implementation docs to the join ValidatorSet trait. AFAIU the validators() can only return different values in case the session_index() changes as well. This stills also have to be coupled with on_before_session_ending callbacks. I.e. session_index() must change after each on_before_session_ending and therefor validators() may change as well.
Implement the joint ValidatorSet trait in pallet_session, so that you can do type ValidatorSet = Session instead of implementing these extra traits for the entire runtime.
Remove pallet-session from im-online/Cargo.toml (or move it to dev-dependencies).

Wrap a few too long lines Add some docs

liuchengxu · 2020-09-25T04:07:21Z

@tomusdrw I have merged ValidatorSet into SessionInterface trait. Since it's not allowed to use frame dep in primitives, I moved ValidatorIdentification trait into a new pallet-sesion-common pallet, but not sure it's a good choice, I can also put it into session pallet. Please have another review before I continue this PR.

… make-im-online-validator-set-flexiable

bkchr · 2021-02-01T11:34:22Z

@liuchengxu could you please merge master to this pr and your companion?

… make-im-online-validator-set-flexiable

…ub.com/liuchengxu/substrate into make-im-online-validator-set-flexiable

liuchengxu · 2021-02-01T12:13:25Z

@bkchr Done.

bkchr · 2021-02-01T12:56:13Z

@liuchengxu you did not update your companion pr :D Sorry for the complex process 🙈

liuchengxu · 2021-02-01T13:13:11Z

@bkchr I'm pretty sure I have updated the companion paritytech/polkadot@08c14e7 :), the companion just needs approval IMO.

bkchr · 2021-02-01T14:27:13Z

Ahh had overseen this! However the pr is not building, I started the build another time to maybe catch some misalignment between the updates. Otherwise please fix the pr. Ty :) You can see the status here in gitlab-check-polkadot-companion-build

… make-im-online-validator-set-flexiable

liuchengxu · 2021-02-02T01:26:50Z

Hi @bkchr gitlab-check-polkadot-companion-build is green now, and gitlab-check-polkadot-companion-status says polkadot pr #1865 not APPROVED.

shawntabrizi

Looks good to me :)

… make-im-online-validator-set-flexiable

bkchr · 2021-02-02T14:21:43Z

bot merge

ghost · 2021-02-02T14:21:44Z

The PR is currently unmergeable.

… make-im-online-validator-set-flexiable

bkchr

Sorry for never have looked over this, but here are really some changes that need to be done. Most of the stuff is also relative straightforward.

Some general comments. You should not bring FRAME concepts to the primitive crates and comments above traits should also be as much as possible generic. This mean, do not mention any particular pallet or whatever.

And again, sorry! If you have done these changes, I will merge this right away!

frame/im-online/src/lib.rs

primitives/session/src/lib.rs

bkchr · 2021-02-02T14:45:13Z

primitives/session/src/lib.rs

+	/// Returns all the validators ought to be online in a session.
+	///
+	/// The returned validators are all expected to be running an authority node.


Suggested change

/// Returns all the validators ought to be online in a session.

///

/// The returned validators are all expected to be running an authority node.

/// Returns the active set of validators.

primitives/session/src/lib.rs

bkchr · 2021-02-02T14:46:40Z

primitives/session/src/lib.rs

+}
+
+/// A session handler for specific key type.
+pub trait OneSessionHandler<ValidatorId>: BoundToRuntimeAppPublic {


All these 3 traits here are frame specific. Please move them to frame-support. They do not belong here.

@bkchr Please have another look, hopefully, I have applied all the suggestions, let me know if I miss anything.

bkchr

Ty for the fast integration!

bkchr · 2021-02-02T15:51:11Z

bot merge

ghost · 2021-02-02T15:51:13Z

Waiting for commit status.

ghost · 2021-02-02T16:03:45Z

Head SHA changed; merge aborted.

… make-im-online-validator-set-flexiable

bkchr · 2021-02-02T16:19:14Z

bot merge

ghost · 2021-02-02T16:19:17Z

Waiting for commit status.

Decouple the session validators from im-online

33b105b

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

liuchengxu added 3 commits September 18, 2020 10:01

Merge branch 'master' of https://github.com/paritytech/substrate into…

d5ce27f

… make-im-online-validator-set-flexiable

Merge branch 'master' of https://github.com/paritytech/substrate into…

1070e51

… make-im-online-validator-set-flexiable

.

526dbd7

tomusdrw suggested changes Sep 22, 2020

View reviewed changes

frame/im-online/src/lib.rs Outdated Show resolved Hide resolved

liuchengxu added 2 commits September 24, 2020 13:32

Add SessionInterface trait in im-online

5f1ab56

Add ValidatorId in im-online Trait Make im-online compile Make substrate binary compile

Merge branch 'make-im-online-validator-set-flexiable' of https://gith…

451179d

…ub.com/liuchengxu/substrate into make-im-online-validator-set-flexiable

liuchengxu requested a review from kianenigma as a code owner September 24, 2020 05:33

liuchengxu added 4 commits September 24, 2020 13:35

Merge branch 'master' of https://github.com/paritytech/substrate into…

725ca92

… make-im-online-validator-set-flexiable

Fix merging issue

141b27b

Make all compile

9bed321

Fix tests

1582b7b

liuchengxu requested review from andresilva and mxinden as code owners September 24, 2020 08:07

tomusdrw reviewed Sep 24, 2020

View reviewed changes

Avoid using frame dep in primitives via pallet-session-common

8fded12

liuchengxu force-pushed the make-im-online-validator-set-flexiable branch 9 times, most recently from baf5e4e to 2f87b4a Compare September 25, 2020 03:53

Merge ValidatorSet into SessionInterface trait

db2d1a4

Wrap a few too long lines Add some docs

liuchengxu force-pushed the make-im-online-validator-set-flexiable branch from 2f87b4a to db2d1a4 Compare September 25, 2020 03:57

liuchengxu added 3 commits January 24, 2021 17:35

Merge branch 'master' of https://github.com/paritytech/substrate into…

7a265af

… make-im-online-validator-set-flexiable

Merge branch 'master' of https://github.com/paritytech/substrate into…

d4787ad

… make-im-online-validator-set-flexiable

Merge branch 'master' of https://github.com/paritytech/substrate into…

2c007d0

… make-im-online-validator-set-flexiable

liuchengxu added 2 commits February 1, 2021 20:08

Merge branch 'master' of https://github.com/paritytech/substrate into…

a34f75c

… make-im-online-validator-set-flexiable

Merge branch 'make-im-online-validator-set-flexiable' of https://gith…

04f247c

…ub.com/liuchengxu/substrate into make-im-online-validator-set-flexiable

Merge branch 'master' of https://github.com/paritytech/substrate into…

865bfd8

… make-im-online-validator-set-flexiable

shawntabrizi approved these changes Feb 2, 2021

View reviewed changes

Merge branch 'master' of https://github.com/paritytech/substrate into…

0a39d79

… make-im-online-validator-set-flexiable

Merge branch 'master' of https://github.com/paritytech/substrate into…

e8c8ce5

… make-im-online-validator-set-flexiable

bkchr suggested changes Feb 2, 2021

View reviewed changes

Apply suggestions from code review

12ff709

bkchr approved these changes Feb 2, 2021

View reviewed changes

Cleaup Cargo.toml

12324f3

liuchengxu added 2 commits February 3, 2021 00:06

Aura has migrated to Pallet now

3551036

Merge branch 'master' of https://github.com/paritytech/substrate into…

8e392fa

… make-im-online-validator-set-flexiable

ghost merged commit 075796f into paritytech:master Feb 2, 2021

liuchengxu mentioned this pull request Apr 26, 2021

Tracking issue for Substrate 3.0 upgrade chainx-org/ChainX#522

Closed

21 tasks

This pull request was closed.

Decouple the session validators from im-online #7127

Decouple the session validators from im-online #7127

Uh oh!

Conversation

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

Uh oh!

Uh oh!

liuchengxu commented Sep 22, 2020

Uh oh!

tomusdrw left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tomusdrw left a comment

Choose a reason for hiding this comment

Uh oh!

liuchengxu commented Sep 25, 2020

Uh oh!

bkchr commented Feb 1, 2021

Uh oh!

liuchengxu commented Feb 1, 2021

Uh oh!

bkchr commented Feb 1, 2021

Uh oh!

liuchengxu commented Feb 1, 2021

Uh oh!

bkchr commented Feb 1, 2021

Uh oh!

liuchengxu commented Feb 2, 2021

Uh oh!

shawntabrizi left a comment

Choose a reason for hiding this comment

Uh oh!

bkchr commented Feb 2, 2021

Uh oh!

ghost commented Feb 2, 2021

Uh oh!

bkchr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bkchr Feb 2, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bkchr Feb 2, 2021

Choose a reason for hiding this comment

Uh oh!

liuchengxu Feb 2, 2021

Choose a reason for hiding this comment

Uh oh!

bkchr left a comment

Choose a reason for hiding this comment

Uh oh!

bkchr commented Feb 2, 2021

Uh oh!

ghost commented Feb 2, 2021

Uh oh!

ghost commented Feb 2, 2021

Uh oh!

bkchr commented Feb 2, 2021

Uh oh!

ghost commented Feb 2, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

liuchengxu commented Sep 17, 2020 •

edited

Loading