Skip to content
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.

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

[Consensus] nTimeOffset warning for time protocol V2 #1128

Merged
merged 4 commits into from Nov 25, 2019

Conversation

@random-zebra
Copy link
Collaborator

random-zebra commented Nov 22, 2019

Open for brainstorming.
Follow up to #1002

  • reject connection with a node having offset higher than 30 secs (and don't add the sample to timedata)
  • repeatedly trigger the warning message when the total offset (absolute value) is above 15 seconds (and set the offset to +15/-15 instead of 0).
@random-zebra random-zebra added this to the 4.0.0 milestone Nov 22, 2019
@random-zebra random-zebra self-assigned this Nov 22, 2019
Copy link
Collaborator

Fuzzbawls left a comment

some changes to include order semantics + a missing include dependency

src/timedata.cpp Show resolved Hide resolved
@random-zebra random-zebra force-pushed the random-zebra:2019_nTimeOffset_warning branch from 8e650e6 to 32b0e83 Nov 23, 2019
Copy link
Collaborator

Fuzzbawls left a comment

utACK 32b0e83

src/main.cpp Outdated Show resolved Hide resolved
@random-zebra random-zebra force-pushed the random-zebra:2019_nTimeOffset_warning branch from 32b0e83 to f14af4f Nov 24, 2019
@random-zebra

This comment has been minimized.

Copy link
Collaborator Author

random-zebra commented Nov 24, 2019

Fixed duplicate call to Params().TimeSlotLength() as per review.
Removing WIP label.

@random-zebra random-zebra changed the title [WIP][Consensus] nTimeOffset warning for time protocol V2 [Consensus] nTimeOffset warning for time protocol V2 Nov 24, 2019
Copy link
Collaborator

Warrows left a comment

When starting the wallet if the computer clock is out of whack each new connection will be dropped in the ProcessMessage step. So AddTimeData won't be called and the UI warning in there won't be displayed. User will have to check his logs to understand why he isn't getting any peer. I'm not sure that's the intended workflow.

@random-zebra

This comment has been minimized.

Copy link
Collaborator Author

random-zebra commented Nov 24, 2019

Indeed it is (when the clock is out of whack by more than 30 seconds with any other node on the network). User would need to check the logs in that case. Up for suggestions.

@Warrows

This comment has been minimized.

Copy link
Collaborator

Warrows commented Nov 24, 2019

Using uiInterface.ThreadSafeMessageBox(); is more "in your face" than a warning in the logs. So it doesn't really make much sense to throw it when the clock is desynchronized by 15~30 sec but not when it is by more than 30 sec. While the opposite would make sense.
We should probably add such a call in ProcessMessage when no peer is connected.

@random-zebra

This comment has been minimized.

Copy link
Collaborator Author

random-zebra commented Nov 24, 2019

The logic there is a bit more subtle.
It gets fired when it's clear that it's a node issue, because you have enough time samples.

You can't fire it in main, when disconnecting a node, because you are not sure who is "wrong" there (the node's clock might be fine... and the peer being disconnected might be the one having the clock out of whack).

@Warrows

This comment has been minimized.

Copy link
Collaborator

Warrows commented Nov 24, 2019

Maybe we should add the time sample even when we disconnect the node then.

@random-zebra

This comment has been minimized.

Copy link
Collaborator Author

random-zebra commented Nov 24, 2019

I'm not sure this would be good either. As we would be modifying a node's offset (thus its adjustedTime) for peers not connected (possibly to anyone). Also the vTimeOffsets structure has room for only 200 samples. Also check bitcoin issue 4521.

@Warrows

This comment has been minimized.

Copy link
Collaborator

Warrows commented Nov 24, 2019

True, we don't want to open up clock attacks. But having the node refuse all connection with no information on what's happening outside of logs is going to get users wondering.
What about keeping a list or count (separate from time samples) and after seeing n nodes with differing clocks and still no connected peer fire the GUI warning from main?

@random-zebra

This comment has been minimized.

Copy link
Collaborator Author

random-zebra commented Nov 24, 2019

Yes, this might be doable.

Copy link
Collaborator

Fuzzbawls left a comment

utACK f14af4f

@Fuzzbawls

This comment has been minimized.

Copy link
Collaborator

Fuzzbawls commented Nov 25, 2019

We can address @Warrows concern in a separate PR

@Mrs-X
Mrs-X approved these changes Nov 25, 2019
Copy link
Collaborator

Mrs-X left a comment

NIT that the last LogPrintf() does not have a time-unit anymore, but utACK f14af4f

Copy link
Collaborator

Warrows left a comment

Yeah, let's improve on the current in an other PR. That's good to go.
ACK f14af4f

Fuzzbawls added a commit that referenced this pull request Nov 25, 2019
f14af4f [Trivial] Fix timedata.cpp includes (random-zebra)
b6992ac [Tests] Add p2p_time_offset functional test to test_runner (random-zebra)
faf03a6 [Tests] Define p2p_time_offset functional test (random-zebra)
d9e9284 [Consensus] reduce possible nTimeOffset to 15 seconds (random-zebra)

Pull request description:

  Open for brainstorming.
  Follow up to #1002

  - reject connection with a node having offset higher than 30 secs (and don't add the sample to timedata)
  - repeatedly trigger the warning message when the total offset (absolute value) is above 15 seconds (and set the offset to +15/-15 instead of 0).

ACKs for top commit:
  Fuzzbawls:
    utACK f14af4f
  Mrs-X:
    NIT that the last LogPrintf() does not have a time-unit anymore, but utACK f14af4f
  Warrows:
    ACK f14af4f

Tree-SHA512: e2a8b7ee7718814ce49fcb536af48afcf1b2829644699abbe5279b064efe1ff066c3100e65229abf06a7787b9f6e813b971300404841a0e6fe41375780a5e318
@Fuzzbawls Fuzzbawls merged commit f14af4f into PIVX-Project:master Nov 25, 2019
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
furszy added a commit that referenced this pull request Nov 25, 2019
44fc641 [P2P] Define CheckOffsetDisconnectedPeers (random-zebra)
21eb7dc [Bug] Use abs value of nTimeOffset to check whether to call AddTimeData (random-zebra)

Pull request description:

  Follow up to #1128 .
  Fixes a bug where negative offsets would always lead to connection.

  Also addresses the concern raised about throwing a `uiInterface.ThreadSafeMessageBox` warning when the node isn't able to add any sample (due to excessive time drift).

  Logic:
  Node mantains a set `setOffsetDisconnectedPeers` of peer's IP addresses.
  When there is a disconnection due to excessive nTimeOffset, the peer's IP is added to the set, unless the node has already `ENOUGH_CONNECTIONS` (which is set to `2` intead of `1` to offer a bit of leeway).
  When the set contains `MAX_TIMEOFFSET_DISCONNECTIONS` (set to `16`) different IPs, the warning is triggered (and the set is cleared).

ACKs for top commit:
  furszy:
    utACK 44fc641
  Warrows:
    ACK 44fc641

Tree-SHA512: c49e4a62578820e1d125dd2b6a253f05a39b110d533b2083516c5d84410d665ad20f0415dffa2343db2ce4092bb253583a5cb12076f5c90180815cf961350096
furszy added a commit that referenced this pull request Dec 5, 2019
…ithin range

c24d2b1 [Trivial] Clean time offset warning when it gets back within range (random-zebra)

Pull request description:

  Simple update (related to #1128 and #1138).
  Clear `strMiscWarning` if the median gets back within the offset limit (after being off, for example during startup when the node has very few time samples).

ACKs for top commit:
  Fuzzbawls:
    utACK c24d2b1
  furszy:
    utACK c24d2b1

Tree-SHA512: c0d4b206eb85cd6d6ba580a3e2b74d46b7518225733c88412cbf810f863910455b3daa35c72c9442d02a80593d79916f83eed26a2ca26903596f2a0182558906
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.