Skip to content

Log successful AcceptBlockHeader() #27276

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
Closed
wants to merge 1 commit into from
Closed

Log successful AcceptBlockHeader() #27276

wants to merge 1 commit into from

Conversation

Sjors
Copy link
Member

@Sjors Sjors commented Mar 19, 2023

Knowing when a header was first seen may help distinguish between a regular reorg and a selfish mining or eclipse attack.

@DrahtBot
Copy link
Contributor

DrahtBot commented Mar 19, 2023 •
edited
Loading

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

Reviews

See the guideline for information on the review process.

Type Reviewers
Concept ACK dergoegge

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

@Sjors Sjors mentioned this pull request Mar 19, 2023
Closed
dergoegge
dergoegge reviewed Mar 19, 2023
View reviewed changes
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.

Concept ACK

src/validation.cpp Outdated
@@ -3828,6 +3828,7 @@ bool ChainstateManager::AcceptBlockHeader(const CBlockHeader& block, BlockValida
return state.Invalid(BlockValidationResult::BLOCK_HEADER_LOW_WORK, "too-little-chainwork");
}
CBlockIndex* pindex{m_blockman.AddToBlockIndex(block, m_best_header)};
LogPrint(BCLog::VALIDATION, "%s: added new block header %s\n", __func__, hash.ToString());
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
LogPrint(BCLog::VALIDATION, "%s: added new block header %s\n", __func__, hash.ToString());
LogPrint(BCLog::VALIDATION, "added new block header %s\n", hash.ToString());

We have an option for logging source locations -logsourcelocations.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done!

@Sjors Sjors force-pushed the 2022/03/log-accept-header branch from a6f9809 to c8d044e Compare March 19, 2023 13:24
@jamesob
Copy link
Contributor

jamesob commented Mar 19, 2023

Beat me to it! I was just about to open this PR :).

For what it's worth, I think that the right approach here is to make this message LogPrintf() and omit it when in initial block download, the rationale being that if something weird happens on the network (like the inspiration for this PR last night), having a lot of data available to investigate would be nice, and most people don't run with verbose logging. Since the additional log volume here is quite minimal (one extra line per block, basically), I don't think there's much cost.

@Sjors
Copy link
Member Author

Sjors commented Mar 19, 2023 •
edited
Loading

I opened #27277 to make -debug=validation less noisy, making it a more attractive setting. At least after IBD, since it produces ~4 lines per block: AcceptBlockHeader, NewPoWValidBlock, Pre-allocating and BlockChecked.

(just pushed another commit there to move Pre-allocating to the blockstorage category)

@fanquake
Copy link
Member

cc @0xB10C

@jamesob jamesob mentioned this pull request Mar 19, 2023
Merged
jonatack
jonatack reviewed Mar 19, 2023
View reviewed changes
src/validation.cpp Outdated
@@ -3828,6 +3828,7 @@ bool ChainstateManager::AcceptBlockHeader(const CBlockHeader& block, BlockValida
return state.Invalid(BlockValidationResult::BLOCK_HEADER_LOW_WORK, "too-little-chainwork");
}
CBlockIndex* pindex{m_blockman.AddToBlockIndex(block, m_best_header)};
LogPrint(BCLog::VALIDATION, "added new block header %s\n", hash.ToString());
Copy link
Member

@jonatack jonatack Mar 19, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

LogPrint is being migrated to LogPrintLevel -- you can use that when adding logging. Or LogPrintfCategory if you decide to make this logged unconditionally, but the former with a level of, say, info, may suffice; example below.

Suggested change
LogPrint(BCLog::VALIDATION, "added new block header %s\n", hash.ToString());
LogPrintLevel(BCLog::VALIDATION, BCLog::Level::Info, "added new block header %s\n", hash.ToString());

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

@Sjors
Log successful AcceptBlockHeader()
3d32b7d 
Knowing when a header was first seen may help distinguish between a regular reorg and a selfish mining or eclipse attack.
@Sjors Sjors force-pushed the 2022/03/log-accept-header branch from c8d044e to 3d32b7d Compare March 19, 2023 15:25
@Sjors
Copy link
Member Author

Sjors commented Mar 19, 2023

At this point I prefer #27278, but I'll leave this open in case that PR doesn't make it. This one is (slightly) easier to review.

@fanquake
Copy link
Member

Closing in favour of #27278.

@fanquake fanquake closed this Mar 20, 2023
achow101 added a commit that referenced this pull request Mar 21, 2023
@achow101
Merge #27278: Log new headers
664500f 
2c3a90f log: on new valid header (James O'Beirne)
e5ce857 log: net: new header over cmpctblock (James O'Beirne)

Pull request description:

  Alternate to #27276.

  Devs were [suprised to realize](https://twitter.com/jamesob/status/1637237917201383425) last night that we don't have definitive logging for when a given header was first received.

  This logs to the main stream when new headers are received outside of IBD, as well as when headers come in over cmpctblocks. The rationale of not hiding these under log categories is that they may be useful to have widely available when debugging strange network activity, and the marginal volume is modest.

ACKs for top commit:
  dergoegge:
    Code review ACK 2c3a90f
  achow101:
    ACK 2c3a90f
  Sjors:
    tACK 2c3a90f
  josibake:
    ACK 2c3a90f

Tree-SHA512: 49fdcbe07799c8adc24143d7e5054a0c93fef120d2e9d5fddbd3b119550d895e2985be6ac10dd1825ea23a6fa5479c1b76d5518c136fbd983fa76c0d39dc354f
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Mar 21, 2023
@achow101
Merge bitcoin#27278: Log new headers
4b54cd3 
2c3a90f log: on new valid header (James O'Beirne)
e5ce857 log: net: new header over cmpctblock (James O'Beirne)

Pull request description:

  Alternate to bitcoin#27276.

  Devs were [suprised to realize](https://twitter.com/jamesob/status/1637237917201383425) last night that we don't have definitive logging for when a given header was first received.

  This logs to the main stream when new headers are received outside of IBD, as well as when headers come in over cmpctblocks. The rationale of not hiding these under log categories is that they may be useful to have widely available when debugging strange network activity, and the marginal volume is modest.

ACKs for top commit:
  dergoegge:
    Code review ACK 2c3a90f
  achow101:
    ACK 2c3a90f
  Sjors:
    tACK 2c3a90f
  josibake:
    ACK bitcoin@2c3a90f

Tree-SHA512: 49fdcbe07799c8adc24143d7e5054a0c93fef120d2e9d5fddbd3b119550d895e2985be6ac10dd1825ea23a6fa5479c1b76d5518c136fbd983fa76c0d39dc354f
@bitcoin bitcoin locked and limited conversation to collaborators Mar 19, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
2 more reviewers

@jonatack jonatack jonatack left review comments

@dergoegge dergoegge dergoegge left review comments

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

Successfully merging this pull request may close these issues.
6 participants
@Sjors @DrahtBot @jamesob @fanquake @jonatack @dergoegge