Assert now > 0 in GetTime GetTimeMillis GetTimeMicros #7094

Merged
merged 1 commit into from Nov 30, 2015

Conversation

Projects
None yet
6 participants
@pstratem
Contributor

pstratem commented Nov 25, 2015

Previously all of these functions could return negative values (for different
readons). Large portions of the codebase currently assume that these
functions return positive values.

Assert now > 0 in GetTime GetTimeMillis GetTimeMicros
Previously all of these functions could return negative values (for different
readons).  Large portions of the codebase currently assume that these
functions return positive values.
@pstratem

This comment has been minimized.

Show comment
Hide comment
@pstratem

pstratem Nov 25, 2015

Contributor

(Besides if your time is before 1970-1-1 you will reject every single block as being too far in the future)

Contributor

pstratem commented Nov 25, 2015

(Besides if your time is before 1970-1-1 you will reject every single block as being too far in the future)

@dcousens

This comment has been minimized.

Show comment
Hide comment
@dcousens

dcousens Nov 25, 2015

Contributor

Sure? Concept ACK.
Any motivation for this @pstratem ?

Contributor

dcousens commented Nov 25, 2015

Sure? Concept ACK.
Any motivation for this @pstratem ?

@pstratem

This comment has been minimized.

Show comment
Hide comment
@pstratem

pstratem Nov 25, 2015

Contributor

@dcousens I was reviewing another PR and noticed that it (sort of) relied on this behavior.

Went and looked a bit and noticed basically every caller does.

Contributor

pstratem commented Nov 25, 2015

@dcousens I was reviewing another PR and noticed that it (sort of) relied on this behavior.

Went and looked a bit and noticed basically every caller does.

@dcousens

This comment has been minimized.

Show comment
Hide comment
@dcousens

dcousens Nov 25, 2015

Contributor

utACK

Contributor

dcousens commented Nov 25, 2015

utACK

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Nov 25, 2015

Member

Anything concerning time deltas is a different issue. I certainly think a monotonic clock would be useful for some measurements (such as pings).

However this code change would just make sure that the absolute time is positive. More of a sanity check of the OS and boost (eg time() returns -1 on error).

Member

laanwj commented Nov 25, 2015

Anything concerning time deltas is a different issue. I certainly think a monotonic clock would be useful for some measurements (such as pings).

However this code change would just make sure that the absolute time is positive. More of a sanity check of the OS and boost (eg time() returns -1 on error).

@jtimon

This comment has been minimized.

Show comment
Hide comment
@jtimon

jtimon Nov 27, 2015

Member

utACK

Member

jtimon commented Nov 27, 2015

utACK

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Nov 27, 2015

Member

In what cases do we expect these assertions to fail?

Member

sipa commented Nov 27, 2015

In what cases do we expect these assertions to fail?

@paveljanik

This comment has been minimized.

Show comment
Hide comment
@paveljanik

paveljanik Nov 28, 2015

Contributor

IIUIC, it is only for "just to be sure".

E.g. on Linux kernel, you can't make time to fail, especially in the case when NULL is used as an argument. If non-NULL is passed, it can fail when kernel's put_user fails (and in such case, more bad things can happen ;-). The other question is boost...

ACK

Contributor

paveljanik commented Nov 28, 2015

IIUIC, it is only for "just to be sure".

E.g. on Linux kernel, you can't make time to fail, especially in the case when NULL is used as an argument. If non-NULL is passed, it can fail when kernel's put_user fails (and in such case, more bad things can happen ;-). The other question is boost...

ACK

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Nov 30, 2015

Member

@paveljanik I guess time() could fail if the relevant syscall is somehow blocked off, say, through seccomp_bpf. And yes boost... who knows
All cases in which it's better to fail fast.

Member

laanwj commented Nov 30, 2015

@paveljanik I guess time() could fail if the relevant syscall is somehow blocked off, say, through seccomp_bpf. And yes boost... who knows
All cases in which it's better to fail fast.

@laanwj laanwj merged commit 1bb289f into bitcoin:master Nov 30, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

laanwj added a commit that referenced this pull request Nov 30, 2015

Merge pull request #7094
1bb289f Assert now > 0 in GetTime GetTimeMillis GetTimeMicros (Patick Strateman)

luke-jr added a commit to luke-jr/bitcoin that referenced this pull request Jan 10, 2016

Assert now > 0 in GetTime GetTimeMillis GetTimeMicros
Previously all of these functions could return negative values (for different
readons).  Large portions of the codebase currently assume that these
functions return positive values.

Github-Pull: #7094
Rebased-From: 1bb289f

luke-jr added a commit to luke-jr/bitcoin that referenced this pull request Jan 10, 2016

Assert now > 0 in GetTime GetTimeMillis GetTimeMicros
Previously all of these functions could return negative values (for different
readons).  Large portions of the codebase currently assume that these
functions return positive values.

Github-Pull: #7094
Rebased-From: 1bb289f
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment