refactor: move Boost Datetime usage to wallet #26198

1. 
   fanquake commented at 8:57 pm on September 28, 2022: member

   This means we don’t need Boost Datetime in a --disable-wallet build, and it isn’t included in the kernel (via time.h/cpp). Split from a larger boost removal branch/effort.
2. DrahtBot added the label Refactoring on Sep 28, 2022
3. 
   DrahtBot commented at 11:20 pm on September 28, 2022: contributor

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

   Conflicts

   Reviewers, this pull request conflicts with the following ones:

   • #26094 (rpc: Return block hash & height in getbalances, gettransaction and getwalletinfo by aureleoules)
   • #25722 (refactor: Use util::Result class for wallet loading by ryanofsky)

   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.

4. aureleoules approved
5. 
   aureleoules commented at 8:27 am on September 29, 2022: member

   ACK aec34489854198908424b6ef61142315533fd074
6. 
   hebasto commented at 10:21 am on September 29, 2022: member

   This means we don’t need Boost Datetime in a --disable-wallet build,

   Could it be enforced to avoid reintroducing by accident in the future? lint-includes.py?

7. in src/test/fuzz/util.cpp:311 in aec3448985 outdated

   306@@ -307,8 +307,8 @@ CAmount ConsumeMoney(FuzzedDataProvider& fuzzed_data_provider, const std::option
   307 int64_t ConsumeTime(FuzzedDataProvider& fuzzed_data_provider, const std::optional<int64_t>& min, const std::optional<int64_t>& max) noexcept
   308 {
   309     // Avoid t=0 (1970-01-01T00:00:00Z) since SetMockTime(0) disables mocktime.
   310-    static const int64_t time_min{ParseISO8601DateTime("2000-01-01T00:00:01Z")};
   311-    static const int64_t time_max{ParseISO8601DateTime("2100-12-31T23:59:59Z")};
   312+    static const int64_t time_min{946684801}; // 2000-01-01T00:00:01Z
   313+    static const int64_t time_max{4133980799}; // 2100-12-31T23:59:59Z
   

   ---

   ---

   

   hebasto commented at 11:21 am on September 29, 2022:

   nit: maybe add

   0    BOOST_CHECK_EQUAL(ParseISO8601DateTime("2000-01-01T00:00:01Z"), 946684801);
   1    BOOST_CHECK_EQUAL(ParseISO8601DateTime("2100-12-31T23:59:59Z"), 4133980799);
   

   to src/wallet/test/rpc_util_tests.cpp?

   ---

   

   fanquake commented at 10:45 am on October 1, 2022:

   Done
8. hebasto approved
9. 
   hebasto commented at 11:28 am on September 29, 2022: member

   ACK aec34489854198908424b6ef61142315533fd074, I have reviewed the code and it looks OK, I agree it can be merged.
10. 
    theuni commented at 3:22 pm on September 29, 2022: member

    Concept ACK.
11. 
    theuni commented at 7:16 pm on September 30, 2022: member

    ACK aec34489854198908424b6ef61142315533fd074. Agree with @hebasto’s comment to avoid losing some test coverage.

    Nice to relegate this to wallet only.

12. bitcoin deleted a comment on Oct 1, 2022
13. refactor: move Boost datetime usage to wallet

    This means we don't need datetime in a --disable-wallet build, and it
    isn't included in the kernel.

    079cf88c0d
14. fanquake force-pushed on Oct 1, 2022
15. hebasto approved
16. 
    hebasto commented at 8:44 pm on October 1, 2022: member

    re-ACK 079cf88c0df6de038b8f8933d55c0d17af007b43
17. aureleoules approved
18. 
    aureleoules commented at 9:01 pm on October 1, 2022: member

    re-ACK 079cf88c0df6de038b8f8933d55c0d17af007b43 - rebased and two additional unit tests since my last review.
19. 
    jarolrod commented at 7:12 am on October 3, 2022: member

    crACK 079cf88c0df6de038b8f8933d55c0d17af007b43
20. bitcoin deleted a comment on Oct 3, 2022
21. 
    MarcoFalke commented at 9:54 am on October 3, 2022: member

    This is reverting 9e2c623be50ee7e586a411923b9ed136acfa2b3f, but seems fine if the goal is to remove boost from kernel?
22. 
    fanquake commented at 9:59 am on October 3, 2022: member

    but seems fine if the goal is to remove boost from kernel?

    Yes, and removing it from bitcoind+utils more generally. There’s no reason for this wallet-only Boost usage to be sucked into everything else.

23. 
    MarcoFalke commented at 10:02 am on October 3, 2022: member

    Right, I can’t see a use case to parse this outside of the wallet, so lgtm
24. fanquake merged this on Oct 3, 2022
25. fanquake closed this on Oct 3, 2022

    ---

26. fanquake deleted the branch on Oct 3, 2022
27. sidhujag referenced this in commit bae93eb4d6 on Oct 4, 2022
28. bitcoin locked this on Oct 3, 2023

Contributors
fanquake DrahtBot aureleoules hebasto theuni jarolrod MarcoFalke

Labels
Refactoring