Improve nStartTime value code quality #7844

In the commit logs I found

consensus.vDeployments[Consensus::DEPLOYMENT_CSV].nStartTime = 1456790400; // March 1st, 2016

As a (layman) reviewer I find it hard to ascertain how this integer constant came to be and whether it is correct. For all I know this could be a bug or a backdoor. IMHO this could be improved by calculating the constant at runtime in a more obviously correct way such as:

nStartTime = BLOCK_TIMESTAMP_FROM_DATE(2016, 3, 1);

This requires a suitable macro or preferrably a constexpr function.

I would request that this improvement to the source code be made in the interest of reviewability.

I prefer it this way... It is de-facto standard to enter time as seconds after start of an epoch...

To check the date, you can do:

Pavels-MacBook-Pro:qt pavel$ date -u -r 1456790400
Tue Mar  1 00:00:00 CET 2016
Pavels-MacBook-Pro:qt pavel$ 

There couldn't be a more obfuscated standard for doing it.

There is one in fact - go check how OOXML stores the time ;-)

nStartTime = BLOCK_TIMESTAMP_FROM_DATE(2016, 3, 1);

It is impossible to write such macro. Please take a look at https://www.youtube.com/watch?v=-5wpm-gesOY to see why.

The only valid way is to express it in something like unix time.

Timezones do not come into play in UTC time as I understand it. Let's make it a function call then or some constexpr thing if possible and supported.

The Unix time number is zero at the Unix epoch, and increases by exactly 86 400 per day since the epoch. Thus 2004-09-16T00:00:00Z, 12 677 days after the epoch, is represented by the Unix time number 12 677 × 86 400 = 1 095 292 800. This can be extended backwards from the epoch too, using negative numbers; thus 1957-10-04T00:00:00Z, 4 472 days before the epoch, is represented by the Unix time number −4 472 × 86 400 = -386 380 800.

So the only difficult problem is to calculate the number of days since start of the epoch. That should require only one mod 4 adjustment and one mod 100 adjustment. I think this can be done in a single expression if it really needs to be crammed into a macro.

But I don't see why a function could not be used. Are C compilers not capable of statically evaluating a pure function with constant arguments? Playing around with this suggests "yes": https://gcc.godbolt.org/#compilers:!((compiler:g530,options:'-O2+-march%3Dnehalem+-funsafe-math-optimizations',source:'int+F1(int+y,+int+m,+int+d)+%7B%0A++++int+adjust+%3D+y+%25+4+%3F+1+:+0%3B%0A++++return+(y+-+1970)+*+(356+%2B+adjust)+%2B+m+*+30+%2B+d%3B%0A%7D%0Avoid+F2(int*+y)+%7B%0A++++*y+%3D+F1(2016,+4,+9)%3B%0A%7D%0A')),filterAsm:(colouriseAsm:!t,commentOnly:!t,directives:!t,intel:!t,labels:!t),version:3 (That code does not work at all, just an optimizer test).

The difficulty here would not be making it compile to a constant (though that's a nice benefit, we can certainly call functions from the chainparams initializers).

We would need to be absolutely sure that it converts to the same constant on every platform. Since UTC is defined as literally days_since_1_1970 * 86400 + hour * 3600 + minute * 60 + seconds, regardless of whether leap seconds occurred in between, I think this is perfectly doable.

I would choose to have a simple function included in util.h or similar, rather than calling a system library for it, to guarantee consistency.

Timezones do not come into play in UTC time as I understand it. So the only difficult problem is to calculate the number of days since start of the epoch.

Correct. unix time uses UTC and "only cares" about the number of days. Also there seem to be algorithms out there, so we don't have to invent our own.

A unit test could call the function in a loop over all possible dates (100,000?) and validate it against one or two reference implementations. I find that comparing two independent implementations of a complicated algorithm for all inputs is a great way to drive out bugs.

This way you also can check invariants such as if (day < 28) assert(EpochTime(year, month, day + 1) == EpochTime(year, month, day) + 1); for all possible inputs. Another one: year1 < year2 => EpochTime(year1, ...) < EpochTime(year2, ...). This kind of exhaustive testing reminds of mathematical proofs.

A unit test could call the function in a loop over all possible dates

Maybe too much for a unit test. Should be fine to run this only once on every platform every time the function is modified.

In the unit test you can test all relevant corner cases and a few non corner cases.

concept ACK, this is easily doable without worrying about local time.

Maybe the guy who ends up implementing this can implement it simultaneously for C, C# and Java so that a reference implementation exists that all Bitcoin projects can just use. The code should end up being almost identical in all languages. For C#, I recommend placing it in checked { }.

The implementations can be checked for conformance by writing test output line-wise to a text file and comparing the files.

@GSPP Patches welcome.

Closing issue due to inactivity. This qualifies as nice-to-have refactoring, no need to keep an issue open.