availability-distribution: look for leaf ancestors within the same session #4596
Conversation
github-actions
bot
added
the
A3-in_progress
slumber
added
B0-silent
eskimor
left a comment
eskimor
left a comment
There was a problem hiding this comment.
Looks good overall!
But I noticed a bug, that is not related to your changes but would be nice to be fixed in the course of this PR:
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.
I'll tell @sandreim - would be good to have this fixed in the course of his PR.
|
Some tests would be nice - especially for behavior at session boundaries, other than that - good to go. |
slumber
force-pushed
the
slumber-av-distribution-ancestors-lookup
branch
from
725e8f0 to
0f54ea3
Compare
github-actions
bot
added
A0-please_review
Lldenaurois
left a comment
Lldenaurois
left a comment
There was a problem hiding this comment.
I still need to review the tests, but the code itself lgtm
| runtime: &mut RuntimeInfo, | ||
| update: ActiveLeavesUpdate, | ||
| ) -> super::Result<()> | ||
| ) -> Result<()> |
| if let Some(activated) = activated { | ||
| // Stale leaves happen after a reversion - we don't want to re-run availability there. | ||
| if let LeafStatus::Fresh = activated.status { |
There was a problem hiding this comment.
Is there really no better way to do this check??
Maybe this is more elegant than two if let's or the ugly thing that was there before:
match activated {
Some(act) if act.status == LeafStatus::Fresh => { ... },
_ => {},
}
There was a problem hiding this comment.
You can also express it with
if matches!(activated, Some(activated) if activated.status == LeafStatus::Fresh) {(provided one derives the PartialEq for status)
But to be honest nested if let is the most readable option to me, 2 more lines is not a big deal
| Vec::new() | ||
| }); | ||
| // Also spawn or bump tasks for candidates in ancestry in the same session. | ||
| for hash in std::iter::once(leaf).chain(ancestors_in_session) { |
| } | ||
|
|
||
| /// Requests up to `limit` ancestor hashes of relay parent in the same session. | ||
| async fn get_block_ancestors_in_same_session<Context>( |
There was a problem hiding this comment.
Logic here seems accurate accounting for the fact that get_session_index is actually returning the session index of the child. Nice
| "Query occupied core" | ||
| ); | ||
| // Important: | ||
| // We mark the whole ancestry as live in the **leaf** hash, so we don't need to track |
There was a problem hiding this comment.
This comment is great and seems to be correct behavior. I had noticed that add_cores might race with remove_leaf if you used hash.
It would be even more clear if you wrote "We invoke add_cores with the leaf and not with the ancestor hash to avoid a race condition and avoid requesting chunks multiple times" or something like that.
rphmeier
left a comment
rphmeier
left a comment
There was a problem hiding this comment.
Looks good. Very much up to standard of neatness, correctness, and testing. Thank you.
* 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)
…ssion (paritytech#4596) * availability-distribution: look for leaf ancestors * Re-use subsystem-util * Rework ancestry tasks scheduling * Requester tests * Improve readability for ancestors lookup
Restores the behavior that was removed in #2423
Resolves #2513