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

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.

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.

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

DrahtBot added the label Tests on Apr 15, 2024

maflcko force-pushed on Apr 15, 2024

DrahtBot added the label CI failed on Apr 15, 2024

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.

<sub>Debug: https://github.com/bitcoin/bitcoin/runs/23822732364</sub>

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

refactor: Use chrono type for g_mock_time

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

fae0db555c

maflcko force-pushed on Apr 15, 2024

DrahtBot removed the label CI failed on Apr 15, 2024

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.

stickies-v approved

stickies-v commented at 5:30 PM on April 15, 2024: contributor

ACK fae0db555c12dca75fb09e5fa7bbabdf39b8c1df

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:

void 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.

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

maflcko commented at 10:45 AM on April 29, 2024: member

Anything left to be done here?

fanquake merged this on Apr 29, 2024

fanquake closed this on Apr 29, 2024

maflcko deleted the branch on Apr 29, 2024

PastaPastaPasta referenced this in commit c6e5dc8c56 on Oct 25, 2024

PastaPastaPasta referenced this in commit 23f25a94fa on Oct 26, 2024

PastaPastaPasta referenced this in commit f211bb9289 on Oct 27, 2024

bitcoin locked this on Apr 29, 2025