test: Add missing Assert(mock_time_in >= 0s) to SetMockTime #29872

1. 
   maflcko commented at 11:05 am on April 15, 2024: member

   Seems odd to have the assert in the deprecated function, but not in the other.

   Fix this by adding it to the other, and by inlining the deprecated one.

   Also, use chrono type for the global mocktime variable.

2. 
   DrahtBot commented at 11:05 am on April 15, 2024: contributor

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

   Code Coverage

   For detailed information about the code coverage, see the test coverage report.

   Reviews

   See the guideline for information on the review process.

   Type	Reviewers
   ACK	stickies-v, davidgumberg

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

3. DrahtBot renamed this:
   test: Add missing Assert(mock_time_in >= 0s) to SetMockTime
   test: Add missing Assert(mock_time_in >= 0s) to SetMockTime
   on Apr 15, 2024
4. DrahtBot added the label Tests on Apr 15, 2024
5. maflcko force-pushed on Apr 15, 2024
6. DrahtBot added the label CI failed on Apr 15, 2024
7. 
   DrahtBot commented at 11:20 am on April 15, 2024: contributor

   🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the documentation.

   Possibly this is due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.

   Leave a comment here, if you need help tracking down a confusing failure.

   Debug: https://github.com/bitcoin/bitcoin/runs/23822732364

8. test: Add missing Assert(mock_time_in >= 0s) to SetMockTime

   Also, inline the deprecated alias to avoid having the two go out of sync
   again in the future.

   fa382d3dd0
9. refactor: Use chrono type for g_mock_time

   This avoids verbose casts in the lines where the symbol is used.

   fae0db555c
10. maflcko force-pushed on Apr 15, 2024
11. DrahtBot removed the label CI failed on Apr 15, 2024
12. in src/util/time.cpp:35 in fae0db555c

    37-
    38+void SetMockTime(int64_t nMockTimeIn) { SetMockTime(std::chrono::seconds{nMockTimeIn}); }
    39 void SetMockTime(std::chrono::seconds mock_time_in)
    40 {
    41-    nMockTime.store(mock_time_in.count(), std::memory_order_relaxed);
    42+    Assert(mock_time_in >= 0s);
    

    ---

    ---

    

    stickies-v commented at 5:21 pm on April 15, 2024:

    nit: any reason to not just use assert?

    ---

    

    maflcko commented at 8:29 am on April 16, 2024:

    Both can be used. In this context they are exactly the same.
13. stickies-v approved
14. 
    stickies-v commented at 5:30 pm on April 15, 2024: contributor

    ACK fae0db555c12dca75fb09e5fa7bbabdf39b8c1df
15. in src/util/time.cpp:32 in fae0db555c

    33-{
    34-    Assert(nMockTimeIn >= 0);
    35-    nMockTime.store(nMockTimeIn, std::memory_order_relaxed);
    36-}
    37-
    38+void SetMockTime(int64_t nMockTimeIn) { SetMockTime(std::chrono::seconds{nMockTimeIn}); }
    

    ---

    ---

    

    davidgumberg commented at 9:56 pm on April 27, 2024:

    nit:

    0void SetMockTime(int64_t mock_time_in) { SetMockTime(std::chrono::seconds{mock_time_in}); }
    

    ---

    

    maflcko commented at 10:43 am on April 29, 2024:

    I think I’d also have to touch the header to address this, so I’ll leave it as-is for now. Also, once could consider to remove this legacy interface function completely.
16. 
    davidgumberg commented at 9:59 pm on April 27, 2024: contributor

    crACK https://github.com/bitcoin/bitcoin/pull/29872/commits/fae0db555c12dca75fb09e5fa7bbabdf39b8c1df

    SetMockTime(std::chrono::seconds mock_time_in should have the Assert added here, and inlining the deprecated function is an improvement.

    nMockTime->g_mock_time more closely conforms to the project’s naming conventions

17. 
    maflcko commented at 10:45 am on April 29, 2024: member

    Anything left to be done here?
18. fanquake merged this on Apr 29, 2024
19. fanquake closed this on Apr 29, 2024

    ---

20. maflcko deleted the branch on Apr 29, 2024

Contributors
maflcko DrahtBot stickies-v davidgumberg

Labels
Tests