8000 Avoid divide-by-zero in header sync logs when NodeClock is behind by maflcko · Pull Request #29647 · bitcoin/bitcoin · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Avoid divide-by-zero in header sync logs when NodeClock is behind #29647

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 2 commits into from
Mar 22, 2024
Merged

Avoid divide-by-zero in header sync logs when NodeClock is behind #29647

merged 2 commits into from
Mar 22, 2024

Conversation

maflcko
Copy link
Member
@maflcko maflcko commented Mar 13, 2024

The log may be confusing, when the NodeClock is behind the current header tip.

Fix it, by assuming the NodeClock is never behind the current header tip.

@DrahtBot
Copy link
Contributor
DrahtBot commented Mar 13, 2024 •
edited
Loading

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 tdb3, sipa, sr-gi, achow101

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #29640 (Fix tiebreak when loading blocks from disk (and add tests for comparing chain ties) by sr-gi)

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.

@maflcko maflcko mentioned this pull request Mar 13, 2024
Open
@maflcko
Copy link
Member Author
maflcko commented Mar 13, 2024 •
edited
Loading

This should only happen on regtest, or if the clock is off by years, so no backport is needed

@maflcko maflcko changed the title Avoid divide-by-zero in ProcessNewBlockHeaders when NodeClock is behind Avoid divide-by-zero in ProcessNewBlockHeaders log when NodeClock is behind Mar 13, 2024
@maflcko maflcko changed the title Avoid divide-by-zero in ProcessNewBlockHeaders log when NodeClock is behind Avoid divide-by-zero in header sync logs when NodeClock is behind Mar 13, 2024
MarcoFalke added 2 commits March 13, 2024 16:16
@maflcko
Copy link
Member Author
maflcko commented Mar 14, 2024

This can be tested on regtest (for exmaple) by connecting a node with one block to a node with no blocks, whose NodeClock is behind the block time.

Example outputs (before):

  • When dividing by zero: [msghand] [validation.cpp:4193] [ProcessNewBlockHeaders] Synchronizing blockheaders, height: 1 (~inf%)
  • When dividing by a negative number: [msghand] [validation.cpp:4193] [ProcessNewBlockHeaders] Synchronizing blockheaders, height: 1 (~-100.00%)

Afterward, they will, more consistently, be assumed to be 100%:

  • [msghand] [validation.cpp:4194] [ProcessNewBlockHeaders] Synchronizing blockheaders, height: 1 (~100.00%)

@sr-gi
Copy link
Member
sr-gi commented Mar 16, 2024

Concept ACK, will review it soon

tdb3
tdb3 reviewed Mar 20, 2024
View reviewed changes
Copy link
Contributor
@tdb3 tdb3 left a comment

Choose a reason for hiding this comment

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

ACK fa4d98b
crACK.
Ran some basic sanity checks: built, and ran all unit and functional tests (all passed).

Started IBD on signet without stale clock.
"Pre-synchronizing" and "Synchronizing" messages looked ok (no negatives or inf observed).

Started IBD on signet with stale clock (about 19 hours behind tip).
"Pre-synchronizing" and "Synchronizing" messages looked ok (no negatives or inf observed).

@DrahtBot DrahtBot requested a review from sr-gi March 20, 2024 02:35
@maflcko
Copy link
Member Author
maflcko commented Mar 20, 2024

Started IBD on signet with stale clock (about 19 hours behind tip).
"Pre-synchronizing" and "Synchronizing" messages looked ok (no negatives or inf observed).

Yes, this is expected, because the signet chain has a total age of over 19 hours. You could try again, but set the time to be before the first header in the signet chain, and then submit the first header? Or you could try my modified functional test: #29640 (comment)?

@tdb3
Copy link
Contributor
tdb3 commented Mar 20, 2024

Started IBD on signet with stale clock (about 19 hours behind tip).
"Pre-synchronizing" and "Synchronizing" messages looked ok (no negatives or inf observed).

Yes, this is expected, because the signet chain has a total age of over 19 hours. You could try again, but set the time to be before the first header in the signet chain, and then submit the first header? Or you could try my modified functional test: #29640 (comment)?

v26.0:

2019-12-31T06:00:36Z New outbound-full-relay v1 peer connected: version: 70013, blocks=187585, peer=0
2019-12-31T06:00:36Z Pre-synchronizing blockheaders, height: 2000 (~-5.89%)
2019-12-31T06:00:37Z Pre-synchronizing blockheaders, height: 4000 (~-11.76%)
...
2019-12-31T06:01:01Z Pre-synchronizing blockheaders, height: 142000 (~-414.61%)
2019-12-31T06:01:02Z Pre-synchronizing blockheaders, height: 146000 (~-424.84%)
2019-12-31T06:01:05Z Warning: Please check that your computer's date and time are correct! If your clock is wrong, Bitcoin Core will not work properly.
Warning: Please check that your computer's date and time are correct! If your clock is wrong, Bitcoin Core will not work properly.
2019-12-31T06:01:05Z New outbound-full-relay v1 peer connected: version: 70016, blocks=187585, peer=3

PR 29647:

2019-12-31T06:02:36Z Pre-synchronizing blockheaders, height: 6000 (~100.00%)
2019-12-31T06:02:36Z Pre-synchronizing blockheaders, height: 10000 (~100.00%)
...
2019-12-31T06:02:52Z Pre-synchronizing blockheaders, height: 174000 (~100.00%)
2019-12-31T06:02:52Z Pre-synchronizing blockheaders, height: 178000 (~100.00%)
2019-12-31T06:02:52Z Cannot connect to socket for 119.123.205.192:38333
2019-12-31T06:02:53Z Cannot connect to socket for 100.26.155.44:38333
2019-12-31T06:02:54Z Warning: Please check that your computer's date and time are correct! If your clock is wrong, Bitcoin Core will not work properly.
Warning: Please check that your computer's date and time are correct! If your clock is wrong, Bitcoin Core will not work properly.

@sipa
Copy link
Member
sipa commented Mar 20, 2024

utACK fa4d98b

@sr-gi
Copy link
Member
sr-gi commented Mar 22, 2024

tACK fa4d98b

I tested this against #29640 by dropping 813961f and cherry-picking both commits included in the PR.

Output after dropping 813961f:

[msghand] [validation.cpp:4193] [ProcessNewBlockHeaders] Synchronizing blockheaders, height: 1 (~inf%)

Output after cherry-picking:

[validation.cpp:4194] [ProcessNewBlockHeaders] Synchronizing blockheaders, height: 1 (~100.00%)

@achow101
Copy link
Member

ACK fa4d98b

@achow101 achow101 merged commit 85c8a5e into bitcoin:master Mar 22, 2024
@maflcko maflcko deleted the 2403-div-zero- branch March 24, 2024 14:46
@bitcoin bitcoin locked and limited conversation to collaborators Mar 24, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@tdb3 tdb3 tdb3 left review comments

@sr-gi sr-gi Awaiting requested review from sr-gi

Assignees
No one assigned
Labels
CI failed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@maflcko @DrahtBot @sr-gi @tdb3 @sipa @achow101
0