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

Fix currently-checking-cache #4410

Merged
merged 9 commits into from
Dec 21, 2021
Merged

Fix currently-checking-cache #4410

merged 9 commits into from
Dec 21, 2021

Conversation

rphmeier
Copy link
Contributor

@rphmeier rphmeier commented Nov 29, 2021

We were spawning validation work in approval-voting for every (candidate, relay-parent) combination. Since candidate validation is the same regardless of relay-parent of inclusion, the intent of this code was to spawn work only once per candidate. I have fixed the code to do exactly that.

@rphmeier rphmeier 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. A1-needsburnin D5-nicetohaveaudit ⚠️ PR contains trivial changes to logic that should be properly reviewed. labels Nov 29, 2021
@github-actions github-actions bot added the A0-please_review Pull request needs code review. label Nov 29, 2021
Copy link
Member

@bkchr bkchr left a comment

Choose a reason for hiding this comment

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

A test would be nice, but otherwise looks good.

Comment on lines 535 to 537
if !entry.get().contains(&relay_block) {
entry.get_mut().push(relay_block);
}
Copy link
Member

Choose a reason for hiding this comment

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

If we would directly use a HashSet, we would only require a insert.

},
Entry::Vacant(mut entry) => {
// validation not ongoing. launch work and time out the remote handle.
let _ = entry.insert(vec![relay_block]);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
let _ = entry.insert(vec![relay_block]);
entry.insert(vec![relay_block]);

Is not required.

@rphmeier
Copy link
Contributor Author

rphmeier commented Nov 29, 2021

@Lldenaurois can you add tests?

  1. Importing same candidate on 2 forks doesn't issue 2 validation requests
  2. approvals are issued on both forks after the candidate is validated

@bkchr bkchr added this to the v0.9.14 milestone Dec 6, 2021
node/core/approval-voting/src/lib.rs Outdated Show resolved Hide resolved
node/core/approval-voting/src/tests.rs Show resolved Hide resolved
node/core/approval-voting/src/tests.rs Outdated Show resolved Hide resolved
node/core/approval-voting/src/tests.rs Show resolved Hide resolved
@ordian ordian modified the milestones: v0.9.14, v0.9.15 Dec 15, 2021
@s3krit s3krit modified the milestones: v0.9.15, v0.9.16 Dec 17, 2021
@rphmeier rphmeier merged commit 42c6665 into master Dec 21, 2021
@rphmeier rphmeier deleted the rh-currently-checking-cache branch December 21, 2021 22:21
ordian added a commit that referenced this pull request Dec 22, 2021
* master:
  Fix currently-checking-cache (#4410)
  Companion for Substrate#10445 (#4506)
  pvf-precheck: update rustdoc for paras module (#4459)
ordian added a commit that referenced this pull request Dec 22, 2021
* master:
  Fix currently-checking-cache (#4410)
  Companion for Substrate#10445 (#4506)
  pvf-precheck: update rustdoc for paras module (#4459)
@ordian ordian modified the milestones: v0.9.16, v0.9.15 Dec 23, 2021
drahnr pushed a commit that referenced this pull request Jan 4, 2022
* alter currently-checking-set to launch work only on new candidates

* fmt

* fix compilation

* address review

* Introduce approvals cache test that ensures approval work is only triggered once for each Candidate Hash

* Fix formatting

* Address Feedback

* Move final message await into handle function

Co-authored-by: Chris Sosnin <[email protected]>
Co-authored-by: Lldenaurois <[email protected]>
@chevdor chevdor modified the milestones: v0.9.15, v0.9.16 Jan 11, 2022
Wizdave97 pushed a commit to ComposableFi/polkadot that referenced this pull request Feb 3, 2022
* alter currently-checking-set to launch work only on new candidates

* fmt

* fix compilation

* address review

* Introduce approvals cache test that ensures approval work is only triggered once for each Candidate Hash

* Fix formatting

* Address Feedback

* Move final message await into handle function

Co-authored-by: Chris Sosnin <[email protected]>
Co-authored-by: Lldenaurois <[email protected]>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
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. D5-nicetohaveaudit ⚠️ PR contains trivial changes to logic that should be properly reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants