fuzz: Return chrono point from ConsumeTime(), Add ConsumeDuration() #34337

Returning a raw i64 is a bit confusing when it comes to chrono types. For example, in the addrman fuzz tests, the time_penalty is not a time point, but a duration.

Also, all call-sites assume second resolution right now, so document that better by returning NodeSeconds from ConsumeTime(...) and std::chrono::seconds from ConsumeDuration(...).

<!--e57a25ab6845829454e8d69fc972939a-->

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

<!--006a51241073e994b41acfe9ec718e94-->

Code Coverage & Benchmarks

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

<!--021abf342d371248e50ceaed478a90ca-->

Reviews

See the guideline for information on the review process.

If your review is incorrectly listed, please copy-paste <code>&lt;!--meta-tag:bot-skip--&gt;</code> into the comment that the bot should ignore.

<!--174a7506f384e20aa4161008e828411d-->

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #32430 (test: Add and use ElapseTime helper by maflcko)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

<!--5faf32d7da4f0f540f40219e4f7537a3-->

Concept ACK, fixes primitive obsession with the new std::chrono helpers for better type safety. I know that we have to stop refactoring somewhere - and I don't want to endlessly extend it either -, but I think we can take another step and make the old method more type safe not just in its return value but the args as well - which would clean up a few remaining call sites as a bonus.

<details><summary>changes done during code review</summary>

diff --git a/src/test/fuzz/addrman.cpp b/src/test/fuzz/addrman.cpp
index 88260b272f..57799ba9ec 100644
--- a/src/test/fuzz/addrman.cpp
+++ b/src/test/fuzz/addrman.cpp
@@ -144,7 +144,7 @@ FUZZ_TARGET(addrman, .init = initialize_addrman)
                     addresses.push_back(ConsumeAddress(fuzzed_data_provider));
                 }
                 auto net_addr = ConsumeNetAddr(fuzzed_data_provider);
-                auto time_penalty = ConsumeDuration(fuzzed_data_provider, /*min=*/0s, /*max=*/100000000s);
+                auto time_penalty = ConsumeDuration(fuzzed_data_provider, /*min=*/0s, /*max=*/1000 * 24h);
                 addr_man.Add(addresses, net_addr, time_penalty);
             },
             [&] {
diff --git a/src/test/fuzz/headerssync.cpp b/src/test/fuzz/headerssync.cpp
index f6e574f404..201a94cbeb 100644
--- a/src/test/fuzz/headerssync.cpp
+++ b/src/test/fuzz/headerssync.cpp
@@ -59,7 +59,7 @@ FUZZ_TARGET(headers_sync_state, .init = initialize_headers_sync_state_fuzz)
     CBlockHeader genesis_header{Params().GenesisBlock()};
     CBlockIndex start_index(genesis_header);
 
-    SetMockTime(ConsumeTime(fuzzed_data_provider, /*min=*/start_index.GetMedianTimePast()));
+    SetMockTime(ConsumeTime(fuzzed_data_provider, /*min=*/start_index.GetMedianTimePast() * 1s));
 
     const uint256 genesis_hash = genesis_header.GetHash();
     start_index.phashBlock = &genesis_hash;
diff --git a/src/test/fuzz/package_eval.cpp b/src/test/fuzz/package_eval.cpp
index 1cc2caa396..7ed729365e 100644
--- a/src/test/fuzz/package_eval.cpp
+++ b/src/test/fuzz/package_eval.cpp
@@ -106,14 +106,6 @@ struct TransactionsDelta final : public CValidationInterface {
     }
 };
 
-void MockTime(FuzzedDataProvider& fuzzed_data_provider, const Chainstate& chainstate)
-{
-    const auto time = ConsumeTime(fuzzed_data_provider,
-                                  chainstate.m_chain.Tip()->GetMedianTimePast() + 1,
-                                  std::numeric_limits<decltype(chainstate.m_chain.Tip()->nTime)>::max());
-    SetMockTime(time);
-}
-
 std::unique_ptr<CTxMemPool> MakeMempool(FuzzedDataProvider& fuzzed_data_provider, const NodeContext& node)
 {
     // Take the default options for tests...
diff --git a/src/test/fuzz/tx_pool.cpp b/src/test/fuzz/tx_pool.cpp
index f70dd710b3..82a77461a7 100644
--- a/src/test/fuzz/tx_pool.cpp
+++ b/src/test/fuzz/tx_pool.cpp
@@ -138,14 +138,6 @@ void Finish(FuzzedDataProvider& fuzzed_data_provider, MockedTxPool& tx_pool, Cha
     g_setup->m_node.validation_signals->SyncWithValidationInterfaceQueue();
 }
 
-void MockTime(FuzzedDataProvider& fuzzed_data_provider, const Chainstate& chainstate)
-{
-    const auto time = ConsumeTime(fuzzed_data_provider,
-                                  chainstate.m_chain.Tip()->GetMedianTimePast() + 1,
-                                  std::numeric_limits<decltype(chainstate.m_chain.Tip()->nTime)>::max());
-    SetMockTime(time);
-}
-
 std::unique_ptr<CTxMemPool> MakeMempool(FuzzedDataProvider& fuzzed_data_provider, const NodeContext& node)
 {
     // Take the default options for tests...
diff --git a/src/test/fuzz/util.cpp b/src/test/fuzz/util.cpp
index 3b3af16115..fe8f37bbf7 100644
--- a/src/test/fuzz/util.cpp
+++ b/src/test/fuzz/util.cpp
@@ -31,17 +31,17 @@ CAmount ConsumeMoney(FuzzedDataProvider& fuzzed_data_provider, const std::option
     return fuzzed_data_provider.ConsumeIntegralInRange<CAmount>(0, max.value_or(MAX_MONEY));
 }
 
-NodeSeconds ConsumeTime(FuzzedDataProvider& fuzzed_data_provider, const std::optional<int64_t>& min, const std::optional<int64_t>& max) noexcept
+NodeSeconds ConsumeTime(FuzzedDataProvider& fuzzed_data_provider, const std::optional<std::chrono::seconds>& min, const std::optional<std::chrono::seconds>& max) noexcept
 {
     // Avoid t=0 (1970-01-01T00:00:00Z) since SetMockTime(0) disables mocktime.
-    static const int64_t time_min{ParseISO8601DateTime("2000-01-01T00:00:01Z").value()};
-    static const int64_t time_max{ParseISO8601DateTime("2100-12-31T23:59:59Z").value()};
-    return NodeSeconds{std::chrono::seconds{fuzzed_data_provider.ConsumeIntegralInRange<int64_t>(min.value_or(time_min), max.value_or(time_max))}};
+    static constexpr auto time_min{(std::chrono::sys_days{2000y / 1 / 1} + 1s).time_since_epoch()};
+    static constexpr auto time_max{(std::chrono::sys_days{2101y / 1 / 1} - 1s).time_since_epoch()};
+    return NodeSeconds{ConsumeDuration(fuzzed_data_provider, min.value_or(time_min), max.value_or(time_max))};
 }
 
 std::chrono::seconds ConsumeDuration(FuzzedDataProvider& fuzzed_data_provider, std::chrono::seconds min, std::chrono::seconds max) noexcept
 {
-    return std::chrono::seconds{fuzzed_data_provider.ConsumeIntegralInRange<int64_t>(min.count(), max.count())};
+    return fuzzed_data_provider.ConsumeIntegralInRange<int64_t>(min.count(), max.count()) * 1s;
 }
 
 CMutableTransaction ConsumeTransaction(FuzzedDataProvider& fuzzed_data_provider, const std::optional<std::vector<Txid>>& prevout_txids, const int max_num_in, const int max_num_out) noexcept
diff --git a/src/test/fuzz/util.h b/src/test/fuzz/util.h
index 3947ee12a6..0f87d1d57d 100644
--- a/src/test/fuzz/util.h
+++ b/src/test/fuzz/util.h
@@ -20,6 +20,7 @@
 #include <test/fuzz/FuzzedDataProvider.h>
 #include <test/fuzz/fuzz.h>
 #include <uint256.h>
+#include <util/time.h>
 
 #include <algorithm>
 #include <array>
@@ -144,7 +145,7 @@ template <typename WeakEnumType, size_t size>
 
 [[nodiscard]] CAmount ConsumeMoney(FuzzedDataProvider& fuzzed_data_provider, const std::optional<CAmount>& max = std::nullopt) noexcept;
 
-[[nodiscard]] NodeSeconds ConsumeTime(FuzzedDataProvider& fuzzed_data_provider, const std::optional<int64_t>& min = std::nullopt, const std::optional<int64_t>& max = std::nullopt) noexcept;
+[[nodiscard]] NodeSeconds ConsumeTime(FuzzedDataProvider& fuzzed_data_provider, const std::optional<std::chrono::seconds>& min = std::nullopt, const std::optional<std::chrono::seconds>& max = std::nullopt) noexcept;
 [[nodiscard]] std::chrono::seconds ConsumeDuration(FuzzedDataProvider& fuzzed_data_provider, std::chrono::seconds min, std::chrono::seconds max) noexcept;
 
 [[nodiscard]] CMutableTransaction ConsumeTransaction(FuzzedDataProvider& fuzzed_data_provider, const std::optional<std::vector<Txid>>& prevout_txids, int max_num_in = 10, int max_num_out = 10) noexcept;
diff --git a/src/test/fuzz/util/mempool.cpp b/src/test/fuzz/util/mempool.cpp
index 241a4d559c..3ab4314615 100644
--- a/src/test/fuzz/util/mempool.cpp
+++ b/src/test/fuzz/util/mempool.cpp
@@ -14,6 +14,11 @@
 #include <cstdint>
 #include <limits>
 
+void MockTime(FuzzedDataProvider& fuzzed_data_provider, const Chainstate& chainstate) noexcept
+{
+    SetMockTime(ConsumeTime(fuzzed_data_provider, /*min=*/(chainstate.m_chain.Tip()->GetMedianTimePast() + 1) * 1s));
+}
+
 CTxMemPoolEntry ConsumeTxMemPoolEntry(FuzzedDataProvider& fuzzed_data_provider, const CTransaction& tx, uint32_t max_height) noexcept
 {
     // Avoid:
diff --git a/src/test/fuzz/util/mempool.h b/src/test/fuzz/util/mempool.h
index 948e936c75..308a38251d 100644
--- a/src/test/fuzz/util/mempool.h
+++ b/src/test/fuzz/util/mempool.h
@@ -21,6 +21,8 @@ public:
     }
 };
 
+void MockTime(FuzzedDataProvider& fuzzed_data_provider, const Chainstate& chainstate) noexcept;
+
 [[nodiscard]] CTxMemPoolEntry ConsumeTxMemPoolEntry(FuzzedDataProvider& fuzzed_data_provider, const CTransaction& tx, uint32_t max_height=std::numeric_limits<uint32_t>::max()) noexcept;
 
 #endif // BITCOIN_TEST_FUZZ_UTIL_MEMPOOL_H
diff --git a/src/test/fuzz/utxo_snapshot.cpp b/src/test/fuzz/utxo_snapshot.cpp
index dddc67a41b..3a87a45f4f 100644
--- a/src/test/fuzz/utxo_snapshot.cpp
+++ b/src/test/fuzz/utxo_snapshot.cpp
@@ -103,7 +103,7 @@ void utxo_snapshot_fuzz(FuzzBufferType buffer)
 {
     SeedRandomStateForTest(SeedRand::ZEROS);
     FuzzedDataProvider fuzzed_data_provider(buffer.data(), buffer.size());
-    SetMockTime(ConsumeTime(fuzzed_data_provider, /*min=*/1296688602)); // regtest genesis block timestamp
+    SetMockTime(ConsumeTime(fuzzed_data_provider, /*min=*/1'296'688'602s)); // regtest genesis block timestamp
     auto& setup{*g_setup};
     bool dirty_chainman{false}; // Reuse the global chainman, but reset it when it is dirty
     auto& chainman{*setup.m_node.chainman};
diff --git a/src/test/fuzz/utxo_total_supply.cpp b/src/test/fuzz/utxo_total_supply.cpp
index d27ca3470b..e7929f4571 100644
--- a/src/test/fuzz/utxo_total_supply.cpp
+++ b/src/test/fuzz/utxo_total_supply.cpp
@@ -24,7 +24,7 @@ FUZZ_TARGET(utxo_total_supply)
 {
     SeedRandomStateForTest(SeedRand::ZEROS);
     FuzzedDataProvider fuzzed_data_provider(buffer.data(), buffer.size());
-    SetMockTime(ConsumeTime(fuzzed_data_provider, /*min=*/1296688602)); // regtest genesis block timestamp
+    SetMockTime(ConsumeTime(fuzzed_data_provider, /*min=*/1'296'688'602s)); // regtest genesis block timestamp
     /** The testing setup that creates a chainman only (no chainstate) */
     ChainTestingSetup test_setup{
         ChainType::REGTEST,

</details

ACK eeee3755f8c415b227820479b5492261f3a8aa08

crACK eeee3755f8c415b227820479b5492261f3a8aa08

simple fuzz-only change with two acks, rfm?