Skip to content

Fix wrong ref counting #1358

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.

Sign up for GitHub

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

Merged
eskimor merged 3 commits into from
Sep 1, 2023
Merged

Fix wrong ref counting #1358

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
052c14b
Fix wrong ref counting
Sep 1, 2023
0ecc208
More warn
Sep 1, 2023
2706a61
Merge branch 'master' into rk-fix-wrong-refcounting
eskimor Sep 1, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions polkadot/node/core/dispute-coordinator/src/initialized.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1271,7 +1271,7 @@ impl Initialized {
if import_result.has_fresh_byzantine_threshold_against() {
let blocks_including = self.scraper.get_blocks_including_candidate(&candidate_hash);
for (parent_block_number, parent_block_hash) in &blocks_including {
gum::trace!(
gum::warn!(
target: LOG_TARGET,
?candidate_hash,
?parent_block_number,
Expand All @@ -1282,7 +1282,7 @@ impl Initialized {
if blocks_including.len() > 0 {
ctx.send_message(ChainSelectionMessage::RevertBlocks(blocks_including)).await;
} else {
gum::debug!(
gum::warn!(
target: LOG_TARGET,
?candidate_hash,
?session,
Expand Down
11 changes: 8 additions & 3 deletions polkadot/node/core/dispute-coordinator/src/scraping/candidates.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,11 +135,14 @@ impl ScrapedCandidates {
}

pub fn insert(&mut self, block_number: BlockNumber, candidate_hash: CandidateHash) {
self.candidates.insert(candidate_hash);
self.candidates_by_block_number
if self
.candidates_by_block_number
.entry(block_number)
.or_default()
.insert(candidate_hash);
.insert(candidate_hash)
{
self.candidates.insert(candidate_hash);
}
Comment on lines +138 to +145
Copy link
Copy Markdown
Contributor

@ordian ordian Sep 1, 2023

Choose a reason for hiding this comment

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

How is this different from previous logic? I don't really understand, it should have worked for multiple blocks per height previously. This if will fail only in case of insertion of the same CandidateHash, which shouldn't happen, but even then it would be the same as previously.

Copy link
Copy Markdown
Contributor

@ordian ordian Sep 1, 2023

Choose a reason for hiding this comment

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

Oh, it's because of ref counting. nvm

Copy link
Copy Markdown
Member Author

@eskimor eskimor Sep 1, 2023

Choose a reason for hiding this comment

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

No, because the inserts in candidates is ref counted. Therefore if we insert at the same height multiple times, we would increase the refcount each time. But in cleanup we would only decrement it once, hence we would never clean it.

Demonstration: The modified test fails without that change (pruning does not happen).

In case of forks there can easily be the same candidate included twice at the same height.

Copy link
Copy Markdown
Contributor

@ordian ordian Sep 1, 2023

Choose a reason for hiding this comment

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

But this will only happen if we insert the same candidate (hash), not different candidates at the same height, no? How is this possible?

Copy link
Copy Markdown
Contributor

@ordian ordian Sep 1, 2023

Choose a reason for hiding this comment

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

I guess if there are multiple forks at the same height including the same candidate.

Copy link
Copy Markdown
Member Author

@eskimor eskimor Sep 1, 2023

Choose a reason for hiding this comment

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

Yes same hash, same height.

     - B
A<
     - B'

A candidate gets backed in A, afterwards we get a fork. Bitfields will be imported in both forks. Result: same candidate included at the same height.

Copy link
Copy Markdown
Contributor

@tdimitrov tdimitrov Sep 1, 2023

Choose a reason for hiding this comment

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

This is really nasty. Good catch!

}

// Used only for tests to verify the pruning doesn't leak data.
Expand All @@ -159,6 +162,8 @@ mod scraped_candidates_tests {
let mut candidates = ScrapedCandidates::new();
let target = CandidateHash(BlakeTwo256::hash(&vec![1, 2, 3]));
candidates.insert(1, target);
// Repeated inserts at same height should be fine:
candidates.insert(1, target);

assert!(candidates.contains(&target));

Expand Down