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

approval-voting: populate session cache in advance #3954

Merged
11 commits merged into from
Sep 29, 2021
Merged

approval-voting: populate session cache in advance #3954

11 commits merged into from
Sep 29, 2021

Conversation

ordian
Copy link
Member

@ordian ordian commented Sep 28, 2021

No description provided.

@ordian ordian added A3-in_progress Pull request is in progress. No review needed at this stage. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. labels Sep 28, 2021
@rphmeier
Copy link
Contributor

Code changes look OK; this will just update the session window more aggressively.

@ordian ordian marked this pull request as ready for review September 29, 2021 09:05
@ordian ordian added A0-please_review Pull request needs code review. and removed A3-in_progress Pull request is in progress. No review needed at this stage. labels Sep 29, 2021
drahnr
drahnr approved these changes Sep 29, 2021
View reviewed changes
Copy link
Contributor

@drahnr drahnr left a comment

Choose a reason for hiding this comment

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

LGTM 👍 , given that the semantic change of block's session vs block's child's session is taken care of where it's used

@ordian
Copy link
Member Author

ordian commented Sep 29, 2021

bot merge

@ghost
Copy link

ghost commented Sep 29, 2021

Waiting for commit status.

rphmeier
rphmeier reviewed Sep 29, 2021
View reviewed changes
node/primitives/src/lib.rs
@@ -69,7 +69,8 @@ pub const POV_BOMB_LIMIT: usize = (MAX_POV_SIZE * 4u32) as usize;
/// On Polkadot this is 1 day, and on Kusama it's 6 hours.
///
/// Number of sessions we want to consider in disputes.
pub const DISPUTE_WINDOW: SessionIndex = 6;
/// + 1 for the child's session.
pub const DISPUTE_WINDOW: SessionIndex = 6 + 1;
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this change is necessary.

  1. BABE session changes occur at slots, so at particular points in time.
  2. The parachains runtime defers all session changes by a single block in order to have predictability of the validator-set.
  3. Therefore, if we've imported one block which has SessionIndexForChild(b) > SessionIndex(b), any blocks constructed by honest validators with accurate clocks after b will have SessionIndexForChild(b') >= SessionIndexForChild(b).
  4. What this means is that the change-set of being more aggressive with advancing the rolling session window will lead us to evict data at most 1 block-time (6 seconds) earlier than we could without the change. As this is dwarfed by the size of the window overall (hundreds or thousands of blocks), it doesn't justify a parameter tweak.

Copy link
Member Author

Choose a reason for hiding this comment

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

Agreed, but this param should be fetched from runtime anyway (#3227) and this change doesn't harm.

@ghost ghost merged commit ad40ebb into master Sep 29, 2021
@ghost ghost deleted the ao-fix-cache-session-info branch September 29, 2021 09:53
@ordian ordian mentioned this pull request Sep 29, 2021
Merged
ordian added a commit that referenced this pull request Sep 29, 2021
@ordian
Merge branch 'master' into temporary-malus-2
f320c20 
* master:
  feat: measured oneshots (#3902)
  remove `AllSubsystems` and `AllSubsystemsGen` types (#3874)
  Companion for Substrate#9867 (#3938)
  Substrate Companion for #9552 (#3834)
  CI: run disputes tests (#3962)
  Bump parity-scale-codec from 2.3.0 to 2.3.1 (#3959)
  approval-voting: populate session cache in advance (#3954)
  Bump libc from 0.2.102 to 0.2.103 (#3950)
  fix master (#3955)
  Docker files chore (#3880)
  Bump nix from 0.19.1 to 0.20.0 (#3587)
  remove connected disconnected state, 3rd attempt (#3898)
  fix flaky chain-selection tests (#3948)
  Add benchmarking for parachain runtime initializer pallet (#3913)
  minor chore changes (#3944)
  disputes: reject single-sided disputes (#3903)
ordian added a commit that referenced this pull request Sep 30, 2021
@ordian
Merge branch 'master' into ladi-logging-disputes
cac79f1 
* master: (52 commits)
  Companion for substrate PR#9890 (#3961)
  Bump version, tx_version and spec_version in prep for v0.9.11 (#3970)
  Fix master compilation (#3977)
  Make most XCM APIs accept an Into<MultiLocation> where MultiLocation is accepted (#3627)
  fix disputes tests (#3974)
  Drop availability only for candidates that lose disputes (#3973)
  revert +1 change to be on the safer side (#3972)
  paras_inherent: reject only candidates with concluded disputes (#3969)
  feat: measured oneshots (#3902)
  remove `AllSubsystems` and `AllSubsystemsGen` types (#3874)
  Companion for Substrate#9867 (#3938)
  Substrate Companion for #9552 (#3834)
  CI: run disputes tests (#3962)
  Bump parity-scale-codec from 2.3.0 to 2.3.1 (#3959)
  approval-voting: populate session cache in advance (#3954)
  Bump libc from 0.2.102 to 0.2.103 (#3950)
  fix master (#3955)
  Docker files chore (#3880)
  Bump nix from 0.19.1 to 0.20.0 (#3587)
  remove connected disconnected state, 3rd attempt (#3898)
  ...
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@rphmeier rphmeier rphmeier approved these changes

@drahnr drahnr drahnr approved these changes

Assignees
No one assigned
Labels
A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ordian @rphmeier @drahnr