Skip to content

Reduce unnecessary default logging #23235

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 4 commits into from
Oct 14, 2021
Merged

Reduce unnecessary default logging #23235

merged 4 commits into from
Oct 14, 2021

Conversation

ajtowns
Copy link
Contributor

@ajtowns ajtowns commented Oct 9, 2021

Moves the following log messages into debug log categories:

  • "AcceptBlockHeader: ..." to validation
  • "Prune: deleted blk/rev" to new blockstorage log category
  • "Leaving block file" moves from validation to blockstorage
  • "write coins cache to disk" to bench

Also adds the hash of the block to the log message when AcceptBlockHeader is rejecting because of problems with the prev block.

@ajtowns
Copy link
Contributor Author

ajtowns commented Oct 9, 2021

Inspired by an irc complaint the other day about a ContextualCheckBlockHeader log message about a bad-diffbits failure for block 000000000000000000639be19a0123a1c99d9fef89f0b8ac055a77f4ef86ae3b which is BCH block at height 478577 from Aug 2017.

This might resolve issue #17421

@practicalswift
Copy link
Contributor

Very Strong Concept ACK

We still do too much unconditional logging IMO: it is spammy (low signal-to-noise) and potentially dangerous (see disk fill attack scenarios described in #21559).

Thanks a lot for improving the situation! :)

I'm willing to review any PR that improves the logging situation by thoughtfully moving unconditional logging (LogPrintf(…)) to conditional logging (LogPrint(CATEGORY, …): feel free to ping me in to such PRs for speedy review! :)

FWIW:

$ git grep 'LogPrintf(' -- "*.cpp" "*.h" | wc -l
493

@DrahtBot
Copy link
Contributor

DrahtBot commented Oct 9, 2021

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #22956 (validation: log CChainState::CheckBlockIndex() consistency checks by jonatack)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

@DrahtBot DrahtBot mentioned this pull request Oct 10, 2021
Closed
@maflcko
Copy link
Member

maflcko commented Oct 11, 2021 

                                     File "/tmp/cirrus-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/rpc_misc.py", line 60, in run_test
                                       assert_equal(len(node.logging()), 26)
                                     File "/tmp/cirrus-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/test_framework/util.py", line 50, in assert_equal
                                       raise AssertionError("not(%s)" % " == ".join(str(arg) for arg in (thing1, thing2) + args))
                                   AssertionError: not(27 == 26)

@ajtowns ajtowns force-pushed the 202110-acceptblockheader-silence branch from 8af3c18 to b5950dd Compare October 11, 2021 12:08
promag
promag reviewed Oct 12, 2021
View reviewed changes
Copy link
Contributor

@promag promag left a comment

Choose a reason for hiding this comment

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

Code review ACK b5950dd.

src/validation.cpp
@@ -3205,7 +3205,7 @@ bool BlockManager::AcceptBlockHeader(const CBlockHeader& block, BlockValidationS
if (ppindex)
*ppindex = pindex;
if (pindex->nStatus & BLOCK_FAILED_MASK) {
LogPrintf("ERROR: %s: block %s is marked invalid\n", __func__, hash.ToString());
Copy link
Contributor

Choose a reason for hiding this comment

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

da94ebc

nit, not sure if this change (commit) merits a release note.

@practicalswift
Copy link
Contributor

cr ACK b5950dd

@Empact
Copy link
Contributor

Empact commented Oct 13, 2021

Code review ACK b5950dd

@laanwj
Copy link
Member

laanwj commented Oct 13, 2021

Code review ACK b5950dd

meshcollider
meshcollider reviewed Oct 14, 2021
View reviewed changes
Copy link
Contributor

@meshcollider meshcollider left a comment

Choose a reason for hiding this comment

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

Code review ACK b5950dd

@meshcollider meshcollider merged commit ec4e43c into bitcoin:master Oct 14, 2021
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Oct 14, 2021
@meshcollider
Merge bitcoin#23235: Reduce unnecessary default logging
2eecdc5
Fabcien pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Sep 23, 2022
@ajtowns @PiRK
validation: include block hash when reporting prev block not found er…
baa8f6f 
…rors

Summary:
This is a backport of [[bitcoin/bitcoin#23235 | core#23235]] [1/4]
bitcoin/bitcoin@1d7d835

Test Plan: `ninja all check-all`

Reviewers: #bitcoin_abc, Fabien

Reviewed By: #bitcoin_abc, Fabien

Differential Revision: https://reviews.bitcoinabc.org/D12038
Fabcien pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Sep 23, 2022
@ajtowns @PiRK
validation: move header validation error logging to VALIDATION debug …
5e24b19 
…category

Summary:
This is a backport of [[bitcoin/bitcoin#23235 | core#23235]] [2/4]
bitcoin/bitcoin@da94ebc

Depends on D12038

Test Plan: `ninja all check-all`

Reviewers: #bitcoin_abc, Fabien

Reviewed By: #bitcoin_abc, Fabien

Subscribers: Fabien

Differential Revision: https://reviews.bitcoinabc.org/D12039
Fabcien pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Sep 23, 2022
@ajtowns @PiRK
blockstorage: use debug log category
a589c68 
Summary:
This is a backport of [[bitcoin/bitcoin#23235 | core#23235]] [3/4]
bitcoin/bitcoin@31b2b80

Depends on D12039, D12040 and D12041

Test Plan: `ninja all check-all`

Reviewers: #bitcoin_abc, Fabien

Reviewed By: #bitcoin_abc, Fabien

Differential Revision: https://reviews.bitcoinabc.org/D12042
Fabcien pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Sep 23, 2022
@ajtowns @PiRK
validation: put coins cache write log into bench debug log
85f4c47 
Summary:
This is a backport of [[bitcoin/bitcoin#23235 | core#23235]] [4/4]
bitcoin/bitcoin@b5950dd

Depends on D12042

Test Plan: `ninja all check-all`

Reviewers: #bitcoin_abc, Fabien

Reviewed By: #bitcoin_abc, Fabien

Differential Revision: https://reviews.bitcoinabc.org/D12043
@Sjors Sjors mentioned this pull request Oct 3, 2022
Merged
@bitcoin bitcoin locked and limited conversation to collaborators Oct 30, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
3 more reviewers

@meshcollider meshcollider meshcollider left review comments

@promag promag promag left review comments

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
Utils/log/libs
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@ajtowns @practicalswift @DrahtBot @maflcko @Empact @laanwj @meshcollider @promag