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

Conversation

@gavofyork
Copy link
Member

@gavofyork gavofyork commented Jul 29, 2018

Runtime had incorrect logic when early-exiting from a session: it relies on block_number - last_length_change % length to determine its phase within the current session. If a session early exited, nothing changed. This makes it reset last_length_change when the session early-exits in order to ensure the phase calculation works as expected.

Also, change spec version.

@gavofyork gavofyork added the A0-please_review Pull request needs code review. label Jul 29, 2018
Copy link
Contributor

@svyatonik svyatonik left a comment

Choose a reason for hiding this comment

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

Seems good. Probably worth test.
Also maybe alter documentation for LastLengthChange - now it is Block at which the session length last changed && it looks like when <SessionLength<T>>::put(); has been called last time. Actually it is something like the block at which last session of length != SessionLength has been completed.

@gavofyork gavofyork changed the title Gav fix session phase in early-exit Fix session phase in early-exit Jul 31, 2018
if is_final_block || broken_validation {
Self::rotate_session(!broken_validation);
if let Some(normal_rotation) = Self::forcing_new_session() {
Self::rotate_session(normal_rotation, is_final_block);
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure about how force_new_session() will be used, but probably Self::rotate_session(normal_rotation && !broken_validation, is_final_block) would be better (to make sure we slash validators)?

Copy link
Member Author

Choose a reason for hiding this comment

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

the idea is that if you use force_new_session(), then it is able to override everything else in terms of the slashing.

if force_new_session() isn't the reason that this session is ending, then slashing is done if this session has been prematurely ended.

pub BrokenPercentLate get(broken_percent_late): b"ses:broken_percent_late" => required T::Moment;

// New session is being forced.
pub ForcingNewSession get(forcing_new_session): b"ses:forcing_new_session" => bool;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would be nice if it was documented what actually bool means here and that it actually a tri-state (like empty, true and false)

@gavofyork gavofyork merged commit 959208c into master Jul 31, 2018
dvdplm added a commit that referenced this pull request Aug 1, 2018
* master:
  Wasm execution optimizations (#466)
  Fix the --key generation (#475)
  Fix typo in service.rs (#472)
  Fix session phase in early-exit (#453)
dvdplm added a commit that referenced this pull request Aug 1, 2018
* master:
  Collator for the "adder" (formerly basic-add) parachain and various small fixes (#438)
  Storage changes subscription (#464)
  Wasm execution optimizations (#466)
  Fix the --key generation (#475)
  Fix typo in service.rs (#472)
  Fix session phase in early-exit (#453)
  Make ping unidirectional (#458)
  Update README.adoc
@arkpar arkpar deleted the gav-fix-session-ee branch April 23, 2020 18:08
lamafab pushed a commit to lamafab/substrate that referenced this pull request Jun 16, 2020
liuchengxu pushed a commit to chainx-org/substrate that referenced this pull request Aug 23, 2021
liuchengxu pushed a commit to autonomys/substrate that referenced this pull request Jun 3, 2022
helin6 pushed a commit to boolnetwork/substrate that referenced this pull request Jul 25, 2023
* Add error information back into metadata to roll back removal in paritytech#394

* Go back to obtaining runtime error info

* re-do codegen too to check that it's all gravy

* Convert DispatchError module errors into a module variant to make them easier to work with

* Fix broken doc link
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

A0-please_review Pull request needs code review.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants