Skip to content
This repository was archived by the owner on Nov 15, 2023. It is now read-only.

Fix tracking validator set in ImOnline - #3596

Merged
gavofyork merged 23 commits into
masterfrom
td-imonline-session
Sep 13, 2019
Merged

Fix tracking validator set in ImOnline#3596
gavofyork merged 23 commits into
masterfrom
td-imonline-session

Conversation

@tomusdrw

@tomusdrwtomusdrw commented Sep 11, 2019•
edited
Loading

Copy link
Copy Markdown
Contributor

Resolves#3553

Currently im-online is using (indirectly) staking::current_elected_set to get the validators and report those who didn't send a Heartbeat.
That's incorrect, since session is buffering the keys (and validator set changes). In this PR we use Session::validators instead, which allows us to get rid of the extra associated type.

  • Add some simple tests to im-online module.
  • Remove dependency on staking
  • Relax crypto requirements (don't require AppKey).
  • Make UintAuthorityId more usable in tests.

Polkadot PR: #429

@tomusdrwtomusdrw added A0-please_review Pull request needs code review. M4-core labels Sep 11, 2019
bkchr
bkchr reviewed Sep 11, 2019
View reviewed changes
Comment threadcore/sr-primitives/src/testing.rs Outdated
}
}

impl PartialOrd for UintAuthorityId {

@bkchrbkchrSep 11, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

WHy not just auto derive it?

Comment threadcore/sr-primitives/src/testing.rs Outdated
}
}

impl Ord for UintAuthorityId {

@bkchrbkchrSep 11, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Same

Comment threadcore/sr-primitives/src/testing.rs
Comment threadcore/sr-primitives/src/testing.rs
Comment threadsrml/im-online/src/tests.rs Outdated
tomusdrwand others added 3 commits September 11, 2019 15:40
@tomusdrw@bkchr
Update srml/im-online/src/tests.rs
862522f
Co-Authored-By: Bastian Köcher <bkchr@users.noreply.github.com>
@tomusdrw
Derive Ord
7093688
@tomusdrw
Merge branch 'td-imonline-session' of github.com:paritytech/substrate…
4c574eb
… into td-imonline-session
kianenigma
kianenigma reviewed Sep 11, 2019
View reviewed changes
Comment threadsrml/im-online/src/tests.rs Outdated
};
let signature = id.sign(&heartbeat.encode()).unwrap();

// when

@kianenigmakianenigmaSep 11, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
// when

kianenigma
kianenigma reviewed Sep 11, 2019
View reviewed changes
Comment threadsrml/im-online/src/tests.rs Outdated
assert!(!ImOnline::is_online_in_current_session(0));

// when
heartbeat(block, 2, 0, 1.into());

@kianenigmakianenigmaSep 11, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I don't get how in block 1 session_index = 2

@kianenigmakianenigmaSep 11, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

(I changed the code a bit but my question still holds)
(I am aware of the buffering and that we need one extra rotate_session to write the buffered validators)

@tomusdrwtomusdrwSep 11, 2019

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Session rotations are independent of blocks. In regular node-runtime that would never happen though.

kianenigma
kianenigma reviewed Sep 11, 2019
View reviewed changes
Comment threadsrml/im-online/src/tests.rs

assert_eq!(Session::current_index(), 2);
// TODO: hmmm...
assert_eq!(Session::validators(), vec![1, 2, 3]);

@kianenigmakianenigmaSep 11, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

why? I was expecting all 6 of them to get buffered.

@tomusdrwtomusdrwSep 11, 2019

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

The first time you call advance_session we switch to:
current(vec![]), queued(vec![1, 2, 3])
second time you call advance_session we switch to:
current(vec![1, 2, 3]), queued(vec![1,...6]))

Initially both current and queued are empty.

kianenigma
kianenigma approved these changes Sep 11, 2019
View reviewed changes

@kianenigmakianenigma left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Added another small test, one should probably add a LOT more in a separate PR; looks good as-is to be merged and fix a potentially critical issue.

@gavofyorkgavofyork added A8-looksgood and removed A0-please_review Pull request needs code review. labels Sep 11, 2019
cmichi
cmichi reviewed Sep 11, 2019
View reviewed changes
Comment threadsrml/im-online/src/tests.rs Outdated
advance_session();

assert_eq!(Session::current_index(), 2);
// TODO: hmmm...

@cmichicmichiSep 11, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Was this ToDo forgotten here? If not a follow-up ticket or a better explanation would be nice.

@tomusdrwtomusdrwSep 12, 2019•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Removed, it was addressed by this comment: #3596 (comment)

@kianenigmakianenigmaSep 12, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yes, my bad. I should have just asked the question. Writing to TODO was maybe too much :D

@tomusdrw

tomusdrw commented Sep 13, 2019

Copy link
Copy Markdown
ContributorAuthor

Everything fixed now, should be good to go.

@gavofyork
gavofyork merged commit 1a59e8c into masterSep 13, 2019
@gavofyork
gavofyork deleted the td-imonline-session branch September 13, 2019 12:55
@kianenigmakianenigma mentioned this pull request Sep 15, 2019
Closed
svyatonik pushed a commit that referenced this pull request Sep 17, 2019
@tomusdrw@svyatonik
Fix tracking validator set in ImOnline (#3596)
a9cae37
* Use session::validators instead of staking::current_elected
* Basic test framework.
* Initialize validators, attempt to heartbeat.
* Use dummy crypto for im-online testing.
* Remove printlns.
* Finish test, make it invalid.
* Add reporting test.
* Finalize the test.
* Remove dumbness.
* Updates.
* Update AuRa
* Update srml/im-online/src/tests.rs
Co-Authored-By: Bastian Köcher <bkchr@users.noreply.github.com>
* Derive Ord
* Add some more tests.
* Remove stray todo.
* Bump runtime version.
* Bump impl-trait-for-tuples.
* Enforce new version of trait-for-tuples.
Demi-Marie pushed a commit to Demi-Marie/substrate that referenced this pull request Sep 17, 2019
@tomusdrw@Demi-Marie
Fix tracking validator set in ImOnline (paritytech#3596)
806c4f9
* Use session::validators instead of staking::current_elected
* Basic test framework.
* Initialize validators, attempt to heartbeat.
* Use dummy crypto for im-online testing.
* Remove printlns.
* Finish test, make it invalid.
* Add reporting test.
* Finalize the test.
* Remove dumbness.
* Updates.
* Update AuRa
* Update srml/im-online/src/tests.rs
Co-Authored-By: Bastian Köcher <bkchr@users.noreply.github.com>
* Derive Ord
* Add some more tests.
* Remove stray todo.
* Bump runtime version.
* Bump impl-trait-for-tuples.
* Enforce new version of trait-for-tuples.
en pushed a commit to en/substrate that referenced this pull request Sep 24, 2019
@tomusdrw@en
Fix tracking validator set in ImOnline (paritytech#3596)
50bd25b
* Use session::validators instead of staking::current_elected
* Basic test framework.
* Initialize validators, attempt to heartbeat.
* Use dummy crypto for im-online testing.
* Remove printlns.
* Finish test, make it invalid.
* Add reporting test.
* Finalize the test.
* Remove dumbness.
* Updates.
* Update AuRa
* Update srml/im-online/src/tests.rs
Co-Authored-By: Bastian Köcher <bkchr@users.noreply.github.com>
* Derive Ord
* Add some more tests.
* Remove stray todo.
* Bump runtime version.
* Bump impl-trait-for-tuples.
* Enforce new version of trait-for-tuples.
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@gui1117gui1117Awaiting requested review from gui1117
3 more reviewers
@cmichicmichicmichi left review comments
@kianenigmakianenigmakianenigma approved these changes
@bkchrbkchrbkchr approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Time gap for validators between srml/staking and srml/session will result in direct slash for new validators.

5 participants

@tomusdrw@cmichi@kianenigma@bkchr@gavofyork