Avoid unnecessary "Synchronizing blockheaders" log messages #16774
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
fanquake
approved these changes
Sep 3, 2019
Member
There was a problem hiding this comment.
ACK dcc448e
The extra messages appear when we receive a headers message:
2019-09-03T05:57:30Z ProcessNewBlock
2019-09-03T05:57:30Z NotifyHeaderTip: pindexHeader->height: 593016
2019-09-03T05:57:30Z ProcessMessage: NetMsgType::HEADERS
2019-09-03T05:57:30Z ProcessHeadersMessage
2019-09-03T05:57:30Z ProcessNewBlockHeaders
2019-09-03T05:57:30Z NotifyHeaderTip: pindexHeader->height: 593016
2019-09-03T05:57:30Z Synchronizing blockheaders, height: 593016 (~100.00%)
2019-09-03T05:57:30Z ProcessNewBlock
2019-09-03T05:57:30Z NotifyHeaderTip: pindexHeader->height: 593016Adjusting NotifyHeaderTip to return a bool, and using that to determine the Synchronizing blockheaders logging seems ok, and aligns this logging with other header tip notifications (although this only logs during IBD).
The other callers of NotifyHeaderTip (ProcessNewBlock and LoadExternalBlockFile) do not use the return value.
Contributor
|
ACK dcc448e ; code review only, haven't compiled or tested.
I think it's that you only choose a couple of peers for compact blocks, and then the rest will send you a headers message instead when a new block is found, which will then trigger ProcessNewBlockHeaders; so if your node is down for a while, when you start back up you'll do a header sync and get a message about the new current tip, then you'll set yourself into IBD to catch back up, and if more new blocks come in while you're doing that, you'll get a log message for each new block for each non-compact-block peer you're connected to. After this patch I think you should still get a single log message for each new block discovered by the network while you're in IBD. If IBD takes 24 hours, that might be an additional 144 log messages, so that doesn't seem too crazy. |
Contributor
|
ACK dcc448e. |
This was referenced Sep 3, 2019
Contributor
maflcko
pushed a commit
that referenced
this pull request
Sep 3, 2019
…ages dcc448e Avoid unnecessary "Synchronizing blockheaders" log messages (Jonas Schnelli) Pull request description: Fixes #16773 I'm not entirely sure why 16773 happend, but probably due to headers fallback in a compact block. However, this PR should fix it and should have been included in #15615. ACKs for top commit: ajtowns: ACK dcc448e ; code review only, haven't compiled or tested. promag: ACK dcc448e. TheBlueMatt: utACK dcc448e. Went and read how pindexBestHeader is handled and this code looks correct (worst case it breaks a LogPrint, so whatever). I also ran into this on #16762. fanquake: ACK dcc448e Tree-SHA512: f8cac3b6eb9d4e8fab53a535b55f9ea9b058e3ab6ade64801ebc56439ede4f54b5fee36d5d2b316966ab987b65b13ab9dc18849f345d08b81ecdf2722a3f5f5a
sidhujag
pushed a commit
to syscoin/syscoin
that referenced
this pull request
Sep 3, 2019
…og messages dcc448e Avoid unnecessary "Synchronizing blockheaders" log messages (Jonas Schnelli) Pull request description: Fixes bitcoin#16773 I'm not entirely sure why 16773 happend, but probably due to headers fallback in a compact block. However, this PR should fix it and should have been included in bitcoin#15615. ACKs for top commit: ajtowns: ACK dcc448e ; code review only, haven't compiled or tested. promag: ACK dcc448e. TheBlueMatt: utACK dcc448e. Went and read how pindexBestHeader is handled and this code looks correct (worst case it breaks a LogPrint, so whatever). I also ran into this on bitcoin#16762. fanquake: ACK dcc448e Tree-SHA512: f8cac3b6eb9d4e8fab53a535b55f9ea9b058e3ab6ade64801ebc56439ede4f54b5fee36d5d2b316966ab987b65b13ab9dc18849f345d08b81ecdf2722a3f5f5a
Member
|
Thanks! |
maflcko
pushed a commit
that referenced
this pull request
Sep 6, 2019
3109a1f refactor: Avoid locking cs_main in ProcessNewBlockHeaders (João Barbosa) Pull request description: Builds on #16774, this change avoids locking `cs_main` in `ProcessNewBlockHeaders` when the tip has changed - in this case the removed lock was necessary to just log a message. Top commit has no ACKs. Tree-SHA512: 31be6d319fa122804f72fa813cec5ed041dd7e4aef3c1921124a1f03016925c43cd4d9a272d80093e77fa7600e3506ef47b7bb821afcbffe01e6be9bceb6dc00
decryp2kanon
added a commit
to sugarchain-project/sugarchain
that referenced
this pull request
Sep 16, 2020
decryp2kanon
added a commit
to sugarchain-project/sugarchain
that referenced
this pull request
Sep 16, 2020
* Add log output during initial header sync * Avoid unnecessary "Synchronizing blockheaders" log messages bitcoin/bitcoin#16774 * remove: "::ChainstateActive()" * log message Co-authored-by: Jonas Schnelli <[email protected]>
jasonbcox
pushed a commit
to Bitcoin-ABC/bitcoin-abc
that referenced
this pull request
Sep 29, 2020
…ssages Summary: This prevents "Synchronizing blockeaders" from being displayed multiple times for the same block. This is a backport of Core [[bitcoin/bitcoin#16774 | PR16774]] Test Plan: `ninja && ninja check`` Reviewers: O1 Bitcoin ABC, #bitcoin_abc, deadalnix Reviewed By: O1 Bitcoin ABC, #bitcoin_abc, deadalnix Differential Revision: https://reviews.bitcoinabc.org/D7634
PastaPastaPasta
added a commit
to dashpay/dash
that referenced
this pull request
Nov 12, 2021
merge bitcoin#15474...bitcoin#16774: miscellaneous backports
humbleDasher
pushed a commit
to humbleDasher/dash
that referenced
this pull request
Nov 13, 2021
…og messages dcc448e Avoid unnecessary "Synchronizing blockheaders" log messages (Jonas Schnelli) Pull request description: Fixes bitcoin#16773 I'm not entirely sure why 16773 happend, but probably due to headers fallback in a compact block. However, this PR should fix it and should have been included in bitcoin#15615. ACKs for top commit: ajtowns: ACK dcc448e ; code review only, haven't compiled or tested. promag: ACK dcc448e. TheBlueMatt: utACK dcc448e. Went and read how pindexBestHeader is handled and this code looks correct (worst case it breaks a LogPrint, so whatever). I also ran into this on bitcoin#16762. fanquake: ACK dcc448e Tree-SHA512: f8cac3b6eb9d4e8fab53a535b55f9ea9b058e3ab6ade64801ebc56439ede4f54b5fee36d5d2b316966ab987b65b13ab9dc18849f345d08b81ecdf2722a3f5f5a
humbleDasher
pushed a commit
to humbleDasher/dash
that referenced
this pull request
Nov 13, 2021
…ckHeaders 3109a1f refactor: Avoid locking cs_main in ProcessNewBlockHeaders (João Barbosa) Pull request description: Builds on bitcoin#16774, this change avoids locking `cs_main` in `ProcessNewBlockHeaders` when the tip has changed - in this case the removed lock was necessary to just log a message. Top commit has no ACKs. Tree-SHA512: 31be6d319fa122804f72fa813cec5ed041dd7e4aef3c1921124a1f03016925c43cd4d9a272d80093e77fa7600e3506ef47b7bb821afcbffe01e6be9bceb6dc00
pravblockc
pushed a commit
to pravblockc/dash
that referenced
this pull request
Nov 18, 2021
…og messages dcc448e Avoid unnecessary "Synchronizing blockheaders" log messages (Jonas Schnelli) Pull request description: Fixes bitcoin#16773 I'm not entirely sure why 16773 happend, but probably due to headers fallback in a compact block. However, this PR should fix it and should have been included in bitcoin#15615. ACKs for top commit: ajtowns: ACK dcc448e ; code review only, haven't compiled or tested. promag: ACK dcc448e. TheBlueMatt: utACK dcc448e. Went and read how pindexBestHeader is handled and this code looks correct (worst case it breaks a LogPrint, so whatever). I also ran into this on bitcoin#16762. fanquake: ACK dcc448e Tree-SHA512: f8cac3b6eb9d4e8fab53a535b55f9ea9b058e3ab6ade64801ebc56439ede4f54b5fee36d5d2b316966ab987b65b13ab9dc18849f345d08b81ecdf2722a3f5f5a
pravblockc
pushed a commit
to pravblockc/dash
that referenced
this pull request
Nov 18, 2021
…ckHeaders 3109a1f refactor: Avoid locking cs_main in ProcessNewBlockHeaders (João Barbosa) Pull request description: Builds on bitcoin#16774, this change avoids locking `cs_main` in `ProcessNewBlockHeaders` when the tip has changed - in this case the removed lock was necessary to just log a message. Top commit has no ACKs. Tree-SHA512: 31be6d319fa122804f72fa813cec5ed041dd7e4aef3c1921124a1f03016925c43cd4d9a272d80093e77fa7600e3506ef47b7bb821afcbffe01e6be9bceb6dc00
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
7 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #16773
I'm not entirely sure why 16773 happend, but probably due to headers fallback in a compact block.
However, this PR should fix it and should have been included in #15615.