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

pull maflcko wants to merge 2 commits into bitcoin:master from maflcko:2403-div-zero- changing 1 files +6 −4
  1. maflcko commented at 2:06 pm on March 13, 2024: member

    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.

  2. DrahtBot commented at 2:06 pm on March 13, 2024: contributor

    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.

  3. maflcko commented at 2:07 pm on March 13, 2024: member
    This should only happen on regtest, or if the clock is off by years, so no backport is needed
  4. maflcko renamed this:
    Avoid divide-by-zero in ProcessNewBlockHeaders when NodeClock is behind
    Avoid divide-by-zero in ProcessNewBlockHeaders log when NodeClock is behind
    on Mar 13, 2024
  5. maflcko force-pushed on Mar 13, 2024
  6. maflcko renamed this:
    Avoid divide-by-zero in ProcessNewBlockHeaders log when NodeClock is behind
    Avoid divide-by-zero in header sync logs when NodeClock is behind
    on Mar 13, 2024
  7. maflcko force-pushed on Mar 13, 2024
  8. refactor: Modernize header sync logs 
    No change in behavior, only the modern aliases and types are used.
    fa58550317
  9. Avoid divide-by-zero in header sync logs when NodeClock is behind fa4d98b3c8
  10. maflcko force-pushed on Mar 13, 2024
  11. maflcko commented at 9:20 am on March 14, 2024: member

    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%)
  12. sr-gi commented at 1:47 am on March 16, 2024: member
    Concept ACK, will review it soon
  13. tdb3 commented at 2:35 am on March 20, 2024: contributor

    ACK fa4d98b3c8e63f20c6f366fc9382039ba52614a4 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).

  14. DrahtBot requested review from sr-gi on Mar 20, 2024
  15. maflcko commented at 6:44 am on March 20, 2024: member

    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 (review)?

  16. tdb3 commented at 12:23 pm on March 20, 2024: contributor

    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:

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

    PR 29647:

    02019-12-31T06:02:36Z Pre-synchronizing blockheaders, height: 6000 (~100.00%)
12019-12-31T06:02:36Z Pre-synchronizing blockheaders, height: 10000 (~100.00%)
2...
32019-12-31T06:02:52Z Pre-synchronizing blockheaders, height: 174000 (~100.00%)
42019-12-31T06:02:52Z Pre-synchronizing blockheaders, height: 178000 (~100.00%)
52019-12-31T06:02:52Z Cannot connect to socket for 119.123.205.192:38333
62019-12-31T06:02:53Z Cannot connect to socket for 100.26.155.44:38333
72019-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.
8Warning: Please check that your computer's date and time are correct! If your clock is wrong, Bitcoin Core will not work properly.
  17. sipa commented at 12:43 pm on March 20, 2024: member
    utACK fa4d98b3c8e63f20c6f366fc9382039ba52614a4
  18. sr-gi commented at 3:20 pm on March 22, 2024: member

    tACK fa4d98b

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

    Output after dropping 813961f057255835285266b90fcd6351ff4a3d1e:

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

    Output after cherry-picking:

    0[validation.cpp:4194] [ProcessNewBlockHeaders] Synchronizing blockheaders, height: 1 (~100.00%)
  19. DrahtBot added the label CI failed on Mar 22, 2024
  20. achow101 commented at 5:08 pm on March 22, 2024: member
    ACK fa4d98b3c8e63f20c6f366fc9382039ba52614a4
  21. achow101 merged this on Mar 22, 2024
  22. achow101 closed this on Mar 22, 2024
  23. maflcko deleted the branch on Mar 24, 2024


maflcko DrahtBot sr-gi tdb3 sipa achow101


sr-gi

Labels
CI failed

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-12-22 06:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me