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

Rename CalculateHeadersWork to CalculateClaimedHeadersWork #29569

Merged
merged 1 commit into from Mar 9, 2024

Conversation

instagibbs
Copy link
Member

And clean up some comments. Confusion about what this is doing seems to be a running theme:

#29549 (comment)

#27278 (comment)

@DrahtBot
Copy link
Contributor

DrahtBot commented Mar 5, 2024

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type Reviewers
ACK pablomartin4btc, 0xB10C, dergoegge, BrandonOdiwuor, achow101

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Copy link
Member

@pablomartin4btc pablomartin4btc left a comment

Choose a reason for hiding this comment

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

ACK eb7cc9f

Copy link
Contributor

@0xB10C 0xB10C left a comment

Choose a reason for hiding this comment

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

ACK eb7cc9f

I misinterpreted CalculateHeadersWork - CalculateClaimedHeadersWork is a bit clearer.

// Check work on this block against our anti-dos thresholds.
if (prev_block && prev_block->nChainWork + CalculateHeadersWork({pblock->GetBlockHeader()}) >= GetAntiDoSWorkThreshold()) {
// Check claimed work on this block against our anti-dos thresholds.
if (prev_block && prev_block->nChainWork + CalculateClaimedHeadersWork({pblock->GetBlockHeader()}) >= GetAntiDoSWorkThreshold()) {
min_pow_checked = true;
Copy link
Contributor

@0xB10C 0xB10C Mar 6, 2024

Choose a reason for hiding this comment

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

nit: I also found min_pow_checked confusing. At least I read "PoW checked" at first.

Copy link
Member Author

Choose a reason for hiding this comment

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

agreed it's also confusing, but it's also called that down the call stack for a bit, so I kept scope to here

Copy link
Member

@dergoegge dergoegge left a comment

Choose a reason for hiding this comment

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

ACK eb7cc9f

Copy link
Contributor

@BrandonOdiwuor BrandonOdiwuor left a comment

Choose a reason for hiding this comment

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

ACK eb7cc9f

@achow101
Copy link
Member

achow101 commented Mar 9, 2024

ACK eb7cc9f

@achow101 achow101 merged commit 4cc99df into bitcoin:master Mar 9, 2024
16 checks passed
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.

None yet

7 participants