Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Security fix 4 - Simplify node time and network-adjusted time warning logic #315

Merged
merged 9 commits into from
Nov 5, 2024

Conversation

Naviabheeman
Copy link
Contributor

@Naviabheeman Naviabheeman commented Sep 24, 2024

Port fix of vulnerability fix of Disclosure of netsplit due to timestamp adjustment which was changed later by PR 29623(Simplify network-adjusted time warning logic)

As this PR removes the timeoffset previously defined in each node, the notion of 'adjusted time' is also removed. So a unified std::chrono::system_clock is used to get the time in GetTime, GetAdjustedTime, GetSystemTimeInSeconds, GetTimeMillis and GetTimeMicros.

  • GetAdjustedTime and GetTime are now the same. They return the mockable system time.
  • GetSystemTimeInSeconds does not allow mocktime. This is essential as the p2p layer depends heavily on the connection and message time. Any change to this time due to mocking results in unpredictable behaviour in functional tests.

removes the timeoffset defined in each node, all time functions defined in tapyrus are affected
use std::chrono::system_clock is used to get the time in GetTime, GetAdjustedTime, GetSystemTimeInSeconds, GetTimeMillis and GetTimeMicros. This system time takes into account the mocktime used testing also.
…Time, GetAdjustedTime, GetSystemTimeInSeconds, GetTimeMillis and GetTimeMicros. This system time takes into account the mocktime used testing also.
Copy link
Contributor

@azuchi azuchi left a comment

Choose a reason for hiding this comment

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

Isn't the nTimeOffset field in classes CNode and CNodeStats also unnecessary?

@Naviabheeman
Copy link
Contributor Author

Isn't the nTimeOffset field in classes CNode and CNodeStats also unnecessary?

nTimeOffset field in CNode class has the time offset for each peer. I was not able to remove it.

@azuchi
Copy link
Contributor

azuchi commented Oct 30, 2024

Isn't the nTimeOffset field in classes CNode and CNodeStats also unnecessary?

nTimeOffset field in CNode class has the time offset for each peer. I was not able to remove it.

but, nTimeOffset in CNodeStats seems not referenced anywhere.

@Naviabheeman
Copy link
Contributor Author

Naviabheeman commented Nov 2, 2024

but, nTimeOffset in CNodeStats seems not referenced anywhere.

I tried to remove it, but this is used in the GUI after the previous commit (
#315 (comment)). I'm removing both.

@azuchi azuchi merged commit a5423d8 into chaintope:master Nov 5, 2024
10 checks passed
@Naviabheeman Naviabheeman deleted the Securityfix4_node_time branch February 20, 2025 05:52
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants