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

feat: failure of min difficulty should not add block to list of bad blocks #5805

Merged

Conversation

SWvheerden
Copy link
Collaborator

@SWvheerden SWvheerden commented Sep 26, 2023

Description

Blocks that fail min difficulty should not be added to the list of bad blocks.

Motivation and Context

By adding the blocks to the list of bad blocks we open up the local node to be spammed to write bad block hashes into the database. These waste space and they require a write lock on the database. Checking the difficulty is a very quick and inexpensive check.

@github-actions
Copy link

github-actions bot commented Sep 26, 2023

Test Results (CI)

1 219 tests   1 219 ✔️  9m 31s ⏱️
     39 suites         0 💤
       1 files           0

Results for commit 385dc66.

♻️ This comment has been updated with latest results.

@ghpbot-tari-project ghpbot-tari-project added P-acks_required Process - Requires more ACKs or utACKs P-reviews_required Process - Requires a review from a lead maintainer to be merged labels Sep 26, 2023
@github-actions
Copy link

github-actions bot commented Sep 26, 2023

Test Results (Integration tests)

  2 files  +  2  11 suites  +11   20m 59s ⏱️ + 20m 59s
33 tests +33  29 ✔️ +29  0 💤 ±0  4 +4 
39 runs  +39  32 ✔️ +32  0 💤 ±0  7 +7 

For more details on these failures, see this check.

Results for commit 385dc66. ± Comparison against base commit 05fc2e9.

♻️ This comment has been updated with latest results.

Copy link
Contributor

@brianp brianp left a comment

Choose a reason for hiding this comment

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

The code change itself is fine. But the PR is really easy to misinterpret:

Blocks that fail min difficulty are not added to the list of bad blocks.

Is this a problem statement that the PR addresses Or the solution the PR proposes. Should I expect to see a codechange that adds the bad blocks to the list or doesn't?

Reading the motivation and context offers more information, but again it's not immediately apparent what direction the code change will go, or whether it will be added to the list or not.

@SWvheerden SWvheerden changed the title feat: min difficulty should not list bad blocks feat: failure of min difficulty should not list bad blocks Sep 29, 2023
@SWvheerden SWvheerden changed the title feat: failure of min difficulty should not list bad blocks feat: failure of min difficulty should not add block to list of bad blocks Sep 29, 2023
@SWvheerden SWvheerden merged commit 38dc014 into tari-project:development Oct 9, 2023
12 of 13 checks passed
@SWvheerden SWvheerden deleted the sw_min_diff_bad_blocks branch October 25, 2023 10:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P-acks_required Process - Requires more ACKs or utACKs P-reviews_required Process - Requires a review from a lead maintainer to be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants