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

undo BlockRecord cache insert, when DB fails #16909

Merged
merged 3 commits into from
Jan 23, 2024

Conversation

arvidn
Copy link
Contributor

@arvidn arvidn commented Nov 21, 2023

We currently have two caches on top of the block store database.

  1. The BlockStore itself keeps an LRU of recently requested FullBlock objects.
  2. The Blockchain class keeps recent BlockRecords in a cache (recent, as in close to the peak)

When we fail to insert a block in the DB, we undo the insertion into the block cache in BlockStore, but we currently don't undo the insertion of the block record into the block-record cache in Blockchain.

This PR fixes that.

Additionally, this PR rolls back any additions and removals from ForkInfo, if they have been added while validating the block.

@arvidn arvidn requested a review from emlowe November 21, 2023 21:21
@arvidn arvidn added the Fixed Required label for PR that categorizes merge commit message as "Fixed" for changelog label Nov 21, 2023
@arvidn

This comment was marked as outdated.

@arvidn arvidn force-pushed the rollback-block-record-cache branch from d0c3615 to cddd90d Compare November 22, 2023 00:28
@arvidn arvidn marked this pull request as ready for review November 22, 2023 11:31
@arvidn arvidn requested a review from a team as a code owner November 22, 2023 11:31
wjblanke
wjblanke previously approved these changes Nov 22, 2023
Copy link
Contributor

@wjblanke wjblanke left a comment

Choose a reason for hiding this comment

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

aok

@wjblanke wjblanke self-requested a review November 22, 2023 18:32
@wjblanke
Copy link
Contributor

earle had issues so holding off for now

@cmmarslender cmmarslender dismissed wjblanke’s stale review November 27, 2023 16:21

Dismissing because he doesn't actually want this merged yet

@arvidn arvidn force-pushed the rollback-block-record-cache branch from cddd90d to 0750d4e Compare November 28, 2023 10:47
@arvidn arvidn changed the base branch from main to release/2.1.2 November 28, 2023 10:53
@github-actions github-actions bot added the merge_conflict Branch has conflicts that prevent merge to main label Nov 29, 2023

This comment was marked as outdated.

@arvidn arvidn force-pushed the rollback-block-record-cache branch from 2b18bd9 to 2349e33 Compare November 29, 2023 21:59
@github-actions github-actions bot removed the merge_conflict Branch has conflicts that prevent merge to main label Nov 29, 2023
@arvidn arvidn changed the base branch from release/2.1.2 to main November 29, 2023 22:00

This comment was marked as outdated.

Copy link

Pull Request Test Coverage Report for Build 7039010402

Warning: This coverage report may be inaccurate.

We've detected an issue with your CI configuration that might affect the accuracy of this pull request's coverage report.
To ensure accuracy in future PRs, please see these guidelines.
A quick fix for this PR: rebase it; your next report should be accurate.

  • 13 of 13 (100.0%) changed or added relevant lines in 2 files are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage increased (+0.1%) to 90.361%

Files with Coverage Reduction New Missed Lines %
tests/util/setup_nodes.py 1 98.31%
Totals Coverage Status
Change from base Build 7038638915: 0.1%
Covered Lines: 93668
Relevant Lines: 103612

💛 - Coveralls

This comment was marked as outdated.

@github-actions github-actions bot added the stale-pr Flagged as stale and in need of manual review label Jan 14, 2024
…the block cache in BlockStore, but we currently don't undo the insertion of the block record into the block-record cache in Blockchain.
@arvidn arvidn force-pushed the rollback-block-record-cache branch from 2349e33 to 36c38eb Compare January 23, 2024 11:17
Copy link
Contributor

@wjblanke wjblanke left a comment

Choose a reason for hiding this comment

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

aok

@Starttoaster Starttoaster removed the stale-pr Flagged as stale and in need of manual review label Jan 23, 2024
@Starttoaster Starttoaster merged commit 0bf842c into main Jan 23, 2024
267 checks passed
@Starttoaster Starttoaster deleted the rollback-block-record-cache branch January 23, 2024 18:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Fixed Required label for PR that categorizes merge commit message as "Fixed" for changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants