test: Remove stale gettime test #31846

1. 
   maflcko commented at 11:18 am on February 12, 2025: member

   The gettime test is stale:

   • It was added to sanity check the time implementation in the mingw toolchain to catch a 32-bit vs 64-bit mismatch in commit eaafa23cbd83b7bda4b28779138c62446bbdea2a. However, since commit 0000a63689036dc4368d04c0648a55fdf507932f, std::chrono::system_clock is used.
   • Even though system_clock may also return incorrect values, such an error should affect all GetTime<> calls (not only the second-precision ones). (I expect such an error to lead to a signed integer overflow in the normal nanosecond precision, so it should be caught by ubsan or by the assert(ret > 0s). If not, the error should be apparent on startup in the debug log.)

   So remove it for now. An alternative would be to extend the test to cover time again, and adjust the comment to say that the test should be fixed along with the block header timestamp. Since that timestamp can’t grow beyond 2106 anyway, see the _test_y2106 functional test.

2. test: Remove stale gettime test fa3a4eafa1
3. 
   DrahtBot commented at 11:18 am on February 12, 2025: contributor

   The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

   Code Coverage & Benchmarks

   For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31846.

   Reviews

   See the guideline for information on the review process.

   Type	Reviewers
   ACK	laanwj, l0rinc

   If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

4. DrahtBot added the label Tests on Feb 12, 2025
5. laanwj approved
6. 
   laanwj commented at 11:50 am on February 12, 2025: member

   ACK fa3a4eafa11ee03fccea1c2a16df5bf2ab164159 This test has ceased to be relevant long ago.
7. 
   l0rinc commented at 1:39 pm on February 12, 2025: contributor

   ACK fa3a4eafa11ee03fccea1c2a16df5bf2ab164159

   Your explanations make sense to me here.

8. fanquake merged this on Feb 12, 2025
9. fanquake closed this on Feb 12, 2025

   ---

10. maflcko deleted the branch on Feb 12, 2025
11. 
    hodlinator commented at 5:17 pm on February 13, 2025: contributor

    Would probably have leaned towards just updating the comment and leaving the test just out of paranoia, even though so much is different with the std::chrono implementation that anything similar is unlikely to re-occur. Getting rid of lines to maintain has value too.
12. 
    laanwj commented at 7:11 pm on February 13, 2025: member

    Sure. It was good to have this test at the time to prove the issue was fixed. But it’s been a decade, it’s super unlikely that this specific toolchain ABI compatibility issue will ever pop up again in our builds, we can’t keep the tests for every possible eventuality.

    Generally we should have tests that test our code, we can’t assure the sanity of the entire OS and system libraries.

13. 
    maflcko commented at 7:43 am on February 14, 2025: member

    so much is different with the std::chrono implementation that anything similar is unlikely to re-occur.

    To give some more context, if this exact issue were to re-appear, it would now be caught by other tests (maybe).

    For testing, I took the constant offset from #3494 (comment): std::chrono::seconds{0x0000ffc4'00000000} and applied it to the system clock as offset. Given that the system clock is normally in nanosecond precision, this will lead to a signed integer overflow, so it will be caught by ubsan. Though, on the windows cross build, likely no one tests with ubsan.

    Still, there is some chance and not a guarantee (signed integer overflow is UB) that the following assert will catch it:

    https://github.com/bitcoin/bitcoin/blob/2549fc6fd1cc958a0e2d59838907c8808fd129b3/src/util/time.cpp#L36

    On Linux, it did locally, but I haven’t tested on Windows.

14. 
    hodlinator commented at 10:40 am on February 14, 2025: contributor

    Yeah, was experimenting with something like this as an extra sanity check.

     0diff --git a/src/util/time.cpp b/src/util/time.cpp
     1index cafc27e0d0..419ddb3702 100644
     2--- a/src/util/time.cpp
     3+++ b/src/util/time.cpp
     4@@ -34,6 +34,7 @@ NodeClock::time_point NodeClock::now() noexcept
     5             mocktime :
     6             std::chrono::system_clock::now().time_since_epoch()};
     7     assert(ret > 0s);
     8+    assert(ret < std::chrono::years{200});
     9     return time_point{ret};
    10 };
    

    But on the way there I realized the underlying representation of ret was closer to nanoseconds than seconds, and also managed to trigger the overflow/wrap issue, which made me see the old test as slightly less useful.

Contributors
maflcko DrahtBot laanwj l0rinc hodlinator

Labels
Tests