Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Inclusion pruning tweaks #1550

Merged
merged 5 commits into from
Sep 14, 2023
Merged

Inclusion pruning tweaks #1550

merged 5 commits into from
Sep 14, 2023

Conversation

Overkillus
Copy link
Contributor

@Overkillus Overkillus commented Sep 14, 2023

In follow-up to #1518

Adding extra tests for inclusion pruning. Primarily focusing on various cases surrounding candidates included in different forks (with different relay parents).

All cases fall into a few buckets based on 3 degrees of freedom - number of candidates, number of blocks (height), number of forks + extra case for pruning multiple heights at once.

The last case of pruning multiple at once has a case that panics in debug due to a potential oversight (@eskimor) but should not cause any inconsistencies or stalls when deployed. Although debug incorrectly labels it as an inconsistency.

It can happen that when the same candidate is added into multiple forks but not the same height and we prune both of those heights at the same time we can erroneously fail the debug assert and trigger the debug inconsistency log. It happens because we prune the candidate yet we still have another copy of it in the stale iterator. This case while rare is fully expectable and natural.

Added small tweak to the original pruning function to disregard stale candidate duplicates which should keep the same behaviour overall but handles this edge-case more gracefully.

@Overkillus Overkillus added T8-parachains_engineering T10-tests This PR/Issue is related to tests. labels Sep 14, 2023
@Overkillus Overkillus self-assigned this Sep 14, 2023
@Overkillus Overkillus marked this pull request as ready for review September 14, 2023 00:46
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.

Ok, this is some test coverage now. You do your name justice ;-)

let stale: HashSet<_> = std::mem::take(&mut self.candidates_by_block_number)
.into_values()
.flatten()
.collect();
Copy link
Member

Choose a reason for hiding this comment

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

Very good catch! Given that this should indeed be pretty rare, I would avoid the inefficiency of collecting into a HashMap and instead remove the debug assert and the debug log.

Really not a strong concern though, also with asynchronous backing this can happen more often.

@tdimitrov
Copy link
Contributor

Good work @Overkillus!

i've pushed a change fixing the comment visualisation in github (the tab gets messed up for some reason and the first row looks off). Feel free to revert if you don't like it.

@Overkillus Overkillus merged commit 756347a into master Sep 14, 2023
6 checks passed
@Overkillus Overkillus deleted the mz-inclusion-pruning-tweaks branch September 14, 2023 19:40
serban300 pushed a commit to serban300/polkadot-sdk that referenced this pull request Apr 8, 2024
* Bump log version to 0.4.17

* Removed unnecesseray dependency + rename BridgeGrandpaMillauCall to BridgeGrandpaCall
serban300 pushed a commit to serban300/polkadot-sdk that referenced this pull request Apr 8, 2024
* Bump log version to 0.4.17

* Removed unnecesseray dependency + rename BridgeGrandpaMillauCall to BridgeGrandpaCall
serban300 pushed a commit to serban300/polkadot-sdk that referenced this pull request Apr 8, 2024
* Bump log version to 0.4.17

* Removed unnecesseray dependency + rename BridgeGrandpaMillauCall to BridgeGrandpaCall
serban300 pushed a commit to serban300/polkadot-sdk that referenced this pull request Apr 8, 2024
* Bump log version to 0.4.17

* Removed unnecesseray dependency + rename BridgeGrandpaMillauCall to BridgeGrandpaCall
serban300 pushed a commit to serban300/polkadot-sdk that referenced this pull request Apr 9, 2024
* Bump log version to 0.4.17

* Removed unnecesseray dependency + rename BridgeGrandpaMillauCall to BridgeGrandpaCall
serban300 pushed a commit to serban300/polkadot-sdk that referenced this pull request Apr 9, 2024
* Bump log version to 0.4.17

* Removed unnecesseray dependency + rename BridgeGrandpaMillauCall to BridgeGrandpaCall
serban300 pushed a commit to serban300/polkadot-sdk that referenced this pull request Apr 9, 2024
* Bump log version to 0.4.17

* Removed unnecesseray dependency + rename BridgeGrandpaMillauCall to BridgeGrandpaCall
serban300 pushed a commit to serban300/polkadot-sdk that referenced this pull request Apr 9, 2024
* Bump log version to 0.4.17

* Removed unnecesseray dependency + rename BridgeGrandpaMillauCall to BridgeGrandpaCall
serban300 pushed a commit to serban300/polkadot-sdk that referenced this pull request Apr 9, 2024
* Bump log version to 0.4.17

* Removed unnecesseray dependency + rename BridgeGrandpaMillauCall to BridgeGrandpaCall
serban300 pushed a commit to serban300/polkadot-sdk that referenced this pull request Apr 9, 2024
* Bump log version to 0.4.17

* Removed unnecesseray dependency + rename BridgeGrandpaMillauCall to BridgeGrandpaCall
serban300 pushed a commit to serban300/polkadot-sdk that referenced this pull request Apr 10, 2024
* Bump log version to 0.4.17

* Removed unnecesseray dependency + rename BridgeGrandpaMillauCall to BridgeGrandpaCall
serban300 pushed a commit to serban300/polkadot-sdk that referenced this pull request Apr 10, 2024
* Bump log version to 0.4.17

* Removed unnecesseray dependency + rename BridgeGrandpaMillauCall to BridgeGrandpaCall
bkchr pushed a commit that referenced this pull request Apr 10, 2024
* Bump log version to 0.4.17

* Removed unnecesseray dependency + rename BridgeGrandpaMillauCall to BridgeGrandpaCall
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T10-tests This PR/Issue is related to tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants