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

fix(validator): session signatures storage #428

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

drmick
Copy link
Member

@drmick drmick commented Nov 7, 2024

No description provided.

@drmick drmick force-pushed the ref/validator-session-signature-storage branch 2 times, most recently from 24eeec1 to 9d624fb Compare November 8, 2024 11:49
Copy link

codecov bot commented Nov 8, 2024

Codecov Report

Attention: Patch coverage is 92.85714% with 1 line in your changes missing coverage. Please review.

Project coverage is 43.00%. Comparing base (84556e8) to head (62f748a).

Files with missing lines Patch % Lines
collator/src/validator/impls/std_impl/session.rs 92.85% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #428      +/-   ##
==========================================
- Coverage   43.03%   43.00%   -0.03%     
==========================================
  Files         262      262              
  Lines       45556    45554       -2     
  Branches    45556    45554       -2     
==========================================
- Hits        19603    19591      -12     
- Misses      24898    24905       +7     
- Partials     1055     1058       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.


🚨 Try these New Features:

@drmick drmick force-pushed the ref/validator-session-signature-storage branch from 9d624fb to c2051a4 Compare November 11, 2024 16:06
@drmick drmick marked this pull request as ready for review November 11, 2024 16:07
@drmick drmick force-pushed the ref/validator-session-signature-storage branch 2 times, most recently from d15519c to 161a241 Compare November 11, 2024 16:30
@drmick drmick changed the title ref(validator): session signatures storage fix(validator): session signatures storage Nov 11, 2024
@drmick drmick force-pushed the ref/validator-session-signature-storage branch from 161a241 to e0f45af Compare November 12, 2024 11:00
@Rexagon Rexagon linked an issue Nov 12, 2024 that may be closed by this pull request
.build(block_id, state.weight_threshold);
let block_signatures = {
match &cached {
Some(cached) => self.reuse_signatures(block_id, cached.clone()).await,
Copy link
Member

Choose a reason for hiding this comment

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

entry lives across the await here, it can deadlock

Copy link
Member Author

Choose a reason for hiding this comment

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

refactored

let cached = state
.cached_signatures
.peek(&block_id.seqno, &scc::ebr::Guard::new())
.map(Arc::clone);
.remove(&block_id.seqno)
Copy link
Member

Choose a reason for hiding this comment

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

We were intentionally overlapping cached_signatures item lifetime with block signatures lifetime. You can't delete a value here since it can miss some signatures

Copy link
Member Author

Choose a reason for hiding this comment

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

refactored

@drmick drmick requested a review from Rexagon November 13, 2024 09:20
@drmick drmick force-pushed the ref/validator-session-signature-storage branch from 1a6b32e to e5c8645 Compare November 13, 2024 09:48
@drmick drmick force-pushed the ref/validator-session-signature-storage branch from e5c8645 to 34cc696 Compare November 14, 2024 11:08
@drmick drmick requested a review from SmaGMan November 14, 2024 11:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Collected block signatures are not removed properly
3 participants