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

Refactor and fix usage of get_session_index() and get_session_info_by_index() #4735

Merged
merged 14 commits into from
Jan 26, 2022
Merged

Refactor and fix usage of get_session_index() and get_session_info_by_index() #4735

merged 14 commits into from
Jan 26, 2022

Conversation

sandreim
Copy link
Contributor

@sandreim sandreim commented Jan 18, 2022 •
edited
Loading

Fixes #4733

WIP

@sandreim
Rename/refactor around get_session_index
a1440b6 
Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>
@sandreim
choose proper head for fetching session
b0e94d1 
Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>
@github-actions github-actions bot added the A3-in_progress Pull request is in progress. No review needed at this stage. label Jan 18, 2022
@sandreim
revert rename
c557268 
Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>
@sandreim
fix comments
a814a26 
Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>
@sandreim sandreim added 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 Jan 18, 2022
@sandreim
renaming and more comments
20363b2 
Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>
drahnr
drahnr approved these changes Jan 19, 2022
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.

A few nits, one item that requires explaining, other than that LGTM

node/network/availability-distribution/src/requester/session_cache.rs Show resolved Hide resolved
node/network/dispute-distribution/src/sender/mod.rs Outdated Show resolved Hide resolved
node/network/dispute-distribution/src/sender/mod.rs Outdated Show resolved Hide resolved
node/network/statement-distribution/src/lib.rs Outdated Show resolved Hide resolved
node/subsystem-util/src/rolling_session_window.rs Show resolved Hide resolved
@sandreim
review feedback
3494d26 
Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>
@sandreim sandreim marked this pull request as ready for review January 19, 2022 17:02
@github-actions github-actions bot 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 Jan 19, 2022
@sandreim sandreim requested a review from eskimor January 19, 2022 17:20
eskimor
eskimor reviewed Jan 20, 2022
View reviewed changes
Copy link
Member

@eskimor eskimor left a comment

Choose a reason for hiding this comment

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

Looks good so far.

@eskimor
Copy link
Member

eskimor commented Jan 20, 2022

Here we are not actually using the SessionInfo for the leaf, but the SessionInfo for the leaf's child. Therefore, at session boundaries we will look up the backing group in the wrong session. 😬

So the comment is correct, but it does not matter as we don't lookup the leaf's session, but its child session. I am not 100% sure how to fix this properly. We could use the relay_parent as it is stored in the occupied core data, but this could break if relay_parent got already finalized and the state trie got cleared, then the SessionIndex lookup would fail for that reason.

What should work more reliably even with @slumber 's and asynchronous backing would be to just lookup the session for the leaf for real. An API in RuntimeInfo for fetching the block's session, instead of its child would make sense. The implementation could just use the chain api for looking up the parent and then fetch the session index for it's child. The chances of the direct parent of an active leaf to be already finalized should be negligible.

@ordian
Copy link
Member

ordian commented Jan 20, 2022

but this could break if relay_parent got already finalized and the state trie got cleared, then the SessionIndex lookup would fail for that reason.

since we cache session info per index, this shouldn't happen in practice

@eskimor
Copy link
Member

eskimor commented Jan 20, 2022

since we cache session info per index, this shouldn't happen in practice

but the index lookup might fail.

@ordian
Copy link
Member

ordian commented Jan 20, 2022

since we cache session info per index, this shouldn't happen in practice

but the index lookup might fail.

right, but it's also cached per relay_parent

ordian
ordian approved these changes Jan 26, 2022
View reviewed changes
Copy link
Member

@ordian ordian left a comment

Choose a reason for hiding this comment

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

I haven't checked all the places where we fetch session index, but the ones modified in this PR look good.

node/network/availability-distribution/src/requester/mod.rs Outdated Show resolved Hide resolved
@sandreim
Log warning when ancestors unavailable
f9e81e6 
Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>
@sandreim
Fixes
320d84f 
Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>
@sandreim
Merge remote-tracking branch 'origin' into sandreim/session_index
7470140 
Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>
@sandreim
fix
2046a43 
Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>
@sandreim
Copy link
Contributor Author

bot merge

@paritytech-processbot paritytech-processbot bot merged commit f06e87e into master Jan 26, 2022
@paritytech-processbot paritytech-processbot bot deleted the sandreim/session_index branch January 26, 2022 15:17
@slumber slumber mentioned this pull request Jan 27, 2022
Closed
ordian added a commit that referenced this pull request Jan 27, 2022
@ordian
Merge branch 'master' into refactor_check_validation_outputs
675dde3 
* master:
  Fix incomplete sorting. (#4795)
  Companion for better way to resolve `Phase::Emergency` via governance #10663 (#4757)
  Refactor and fix usage of `get_session_index()` and `get_session_info_by_index()` (#4735)
  `relay chain selection` and `dispute-coordinator` fixes and improvements (#4752)
  Fix tests (#4787)
  log concluded disputes (#4785)
  availability-distribution: look for leaf ancestors within the same session (#4596)
  Companion for Use proper bounded vector type for nominations #10601 (#4709)
  Fix release profile (#4778)
  [ci] remove publish-s3-release (#4779)
  Companion for substrate#10632 (#4689)
  [ci] pipeline chores (#4775)
  New changelog scripts (#4491)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@drahnr drahnr drahnr approved these changes

@eskimor eskimor eskimor left review comments

@ordian ordian ordian approved these changes

@slumber slumber slumber 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.

Check usage of get_session_index() and request_session_index_for_child()
5 participants
@sandreim @eskimor @ordian @drahnr @slumber