backport: merge bitcoin#24469, #24470, #29040, #29181, #29085, #29484, #29577, #29081, #29815, #28687, #31502, partial bitcoin#26707, #28894 (c++20 backports: part 2) #6657
Conversation
…lowed by path append operators
WalkthroughThis set of changes modernizes and refines several aspects of the codebase, focusing on filesystem path handling, time formatting, and iteration utilities. The custom reverse iteration utility is removed and replaced throughout with standard C++20 ranges ( 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (39)
💤 Files with no reviewable changes (7)
✅ Files skipped from review due to trivial changes (4)
🚧 Files skipped from review as they are similar to previous changes (28)
⏰ Context from checks skipped due to timeout of 90000ms (7)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
src/test/util_tests.cpp (1)
1100-1135: 💡 Verification agent🧩 Analysis chain
Lock helpers now take
fs::path– good move, but verify remaining call-sitesUpdating
TestOtherThread/TestOtherProcessto passlocknameasfs::pathkeeps the tests in sync with the production API. Please run a quick grep to ensure there are no lingeringLockDirectory(..., ".lock")calls still usingstd::string.
🏁 Script executed:
#!/bin/bash # Find remaining string-literal lockname usages. rg --line-number --fixed-strings 'LockDirectory(' | grep '".*\.lock"' -nLength of output: 150
Wrap remaining string-literal lockname in an fs::path
There’s one lingering call-site still passing
".lock"as a raw string. Please update it to use anfs::path:• src/init.cpp:1407
- if (!LockDirectory(datadir, ".lock", probeOnly)) { + if (!LockDirectory(datadir, fs::path(".lock"), probeOnly)) {
🧹 Nitpick comments (7)
src/test/util_tests.cpp (2)
269-279: Great to see upper-bound date/time coverage, but consider adding lower-bound casesThe new assertions for the maximum representable date (
32767-12-31) ensure the chrono-based formatter works at the upper edge – nice!
For completeness (and to keep parity with the existing parse tests that already exercise 1960), you may want to add a couple of negative/epoch-minus-one tests (e.g. ‑1 →1969-12-31T23:59:59Z). This would guarantee the floor/remainder logic inFormatISO8601*behaves for pre-epoch values as well.
1141-1141: Minor nit: preferconstexprfor static path
locknameis a compile-time constant; declaring itstatic constexpr fs::path lockname{".lock"};would avoid repeated constructions when this block is executed in multiple tests.src/util/system.h (3)
85-87: Path-typed locking API – 👍, but document probe-only semanticsGood switch from
std::stringtofs::path. While you are touching the signature, consider updating the header comment to clarify thatprobe_only=truenever holds the lock (mirrors behaviour insystem.cpp).
299-316: Return-by-value is safer, yet add[[nodiscard]]Returning
fs::pathby value avoids dangling references, nice. To prevent accidental ignore of the path (especially now that copies are cheap), tagging these methods with[[nodiscard]]would enforce usage.- fs::path GetBlocksDirPath() const; + [[nodiscard]] fs::path GetBlocksDirPath() const;(Apply similarly to
GetDataDirBase,GetDataDirNet, and the privateGetDataDir.)
506-506: Return-value copy of cached path incurs extra allocation
GetDataDir(bool)now copies the cachedfs::pathon every call. If this becomes hot, consider returningconst fs::path&and documenting lifetime guarantees, or usingstd::shared_ptr<const fs::path>.src/util/time.cpp (2)
73-79: Duplicate day-flooring logic – refactor into a small helper
FormatISO8601DateTime,FormatISO8601Date, andFormatISO8601Timeall repeatsys_seconds → days → ymd. Extracting a tiny internal lambda would deduplicate the code and make future fixes easier.
81-87: Hour/min/sec formatting: use unsigned to silence potential sign warnings
hms.hours().count()etc. return unsigned values;%02iexpects signed. Change to%02u(or cast toint) to avoid compiler warnings on some platforms.- return strprintf("%02i:%02i:%02iZ", hms.hours().count(), hms.minutes().count(), hms.seconds().count()); + return strprintf("%02u:%02u:%02uZ", + static_cast<unsigned>(hms.hours().count()), + static_cast<unsigned>(hms.minutes().count()), + static_cast<unsigned>(hms.seconds().count()));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (51)
configure.ac(1 hunks)contrib/devtools/copyright_header.py(0 hunks)depends/hosts/netbsd.mk(0 hunks)depends/packages/systemtap.mk(1 hunks)depends/patches/systemtap/fix_variadic_warning.patch(0 hunks)doc/developer-notes.md(1 hunks)src/Makefile.am(1 hunks)src/Makefile.bench.include(1 hunks)src/addrdb.cpp(1 hunks)src/bench/wallet_create.cpp(1 hunks)src/crypto/chacha20.cpp(1 hunks)src/crypto/chacha20poly1305.cpp(2 hunks)src/crypto/sha3.cpp(2 hunks)src/crypto/siphash.cpp(1 hunks)src/flat-database.h(1 hunks)src/flatfile.cpp(1 hunks)src/fs.cpp(2 hunks)src/fs.h(6 hunks)src/hash.cpp(3 hunks)src/index/blockfilterindex.cpp(1 hunks)src/init/common.cpp(0 hunks)src/net.cpp(1 hunks)src/net_processing.cpp(3 hunks)src/node/blockstorage.cpp(2 hunks)src/qt/guiutil.cpp(4 hunks)src/reverse_iterator.h(0 hunks)src/rpc/blockchain.cpp(3 hunks)src/rpc/mempool.cpp(1 hunks)src/rpc/server.cpp(1 hunks)src/serialize.h(3 hunks)src/test/fs_tests.cpp(3 hunks)src/test/fuzz/prevector.cpp(2 hunks)src/test/prevector_tests.cpp(2 hunks)src/test/sanity_tests.cpp(0 hunks)src/test/util/chainstate.h(1 hunks)src/test/util_tests.cpp(5 hunks)src/txmempool.cpp(2 hunks)src/util/getuniquepath.cpp(1 hunks)src/util/system.cpp(4 hunks)src/util/system.h(3 hunks)src/util/time.cpp(2 hunks)src/util/time.h(0 hunks)src/validation.cpp(4 hunks)src/validation.h(2 hunks)src/wallet/bdb.cpp(13 hunks)src/wallet/bdb.h(5 hunks)src/wallet/rpcdump.cpp(2 hunks)src/wallet/rpcwallet.cpp(1 hunks)src/wallet/test/db_tests.cpp(3 hunks)src/wallet/test/init_test_fixture.cpp(2 hunks)src/wallet/wallet.cpp(4 hunks)
💤 Files with no reviewable changes (7)
- src/util/time.h
- src/test/sanity_tests.cpp
- depends/hosts/netbsd.mk
- src/init/common.cpp
- depends/patches/systemtap/fix_variadic_warning.patch
- contrib/devtools/copyright_header.py
- src/reverse_iterator.h
🧰 Additional context used
🧬 Code Graph Analysis (14)
src/index/blockfilterindex.cpp (1)
src/fs.h (9)
path(37-37)path(38-38)path(38-38)path(39-39)path(39-39)path(42-42)path(48-48)u8path(74-77)u8path(74-74)
src/util/getuniquepath.cpp (1)
src/fs.h (2)
u8path(74-77)u8path(74-74)
src/test/util/chainstate.h (1)
src/fs.h (2)
u8path(74-77)u8path(74-74)
src/flat-database.h (1)
src/fs.h (2)
u8path(74-77)u8path(74-74)
src/test/prevector_tests.cpp (1)
src/test/fuzz/prevector.cpp (11)
pre_vector(152-155)pre_vector(157-160)real_vector(129-133)real_vector(135-139)real_vector(147-150)real_vector(162-166)real_vector(168-174)real_vector(176-180)pos(117-121)pos(117-117)const_pre_vector(29-68)
src/flatfile.cpp (1)
src/fs.h (2)
u8path(74-77)u8path(74-74)
src/qt/guiutil.cpp (2)
src/util/system.cpp (2)
GetSpecialFolderPath(1299-1310)GetSpecialFolderPath(1299-1299)src/fs.h (9)
u8path(74-77)u8path(74-74)path(37-37)path(38-38)path(38-38)path(39-39)path(39-39)path(42-42)path(48-48)
src/rpc/blockchain.cpp (1)
src/fs.h (7)
path(37-37)path(38-38)path(38-38)path(39-39)path(39-39)path(42-42)path(48-48)
src/net.cpp (1)
src/fs.h (2)
u8path(74-77)u8path(74-74)
src/wallet/bdb.h (1)
src/fs.h (3)
filename(71-71)PathToString(150-168)PathToString(150-150)
src/test/util_tests.cpp (2)
src/util/time.cpp (4)
FormatISO8601DateTime(64-71)FormatISO8601DateTime(64-64)FormatISO8601Date(73-79)FormatISO8601Date(73-73)src/util/time.h (2)
FormatISO8601DateTime(110-110)FormatISO8601Date(111-111)
src/util/system.h (1)
src/util/system.cpp (2)
GetDataDir(446-478)GetDataDir(446-446)
src/test/fs_tests.cpp (2)
src/fs.h (14)
PathToString(150-168)PathToString(150-150)PathFromString(173-180)PathFromString(173-173)u8path(74-77)u8path(74-74)path(37-37)path(38-38)path(38-38)path(39-39)path(39-39)path(42-42)path(48-48)AbsPathJoin(218-218)src/fs.cpp (2)
AbsPathJoin(36-40)AbsPathJoin(36-36)
src/util/system.cpp (1)
src/util/system.h (3)
GetDataDir(307-307)GetDataDir(315-315)net_specific(506-506)
🔇 Additional comments (100)
depends/packages/systemtap.mk (1)
6-6: Patch simplification looks good.This change removes the
fix_variadic_warning.patchfrom the systemtap package build process, which aligns with the PR's C++20 modernization efforts. The patch was likely used to suppress variadic macro warnings that are now handled correctly with C++20 compiler support.src/fs.cpp (2)
21-21: Include missing<cerrno>header forerrnousage.
This addition ensures thaterrnois properly declared when callingSysErrorString(errno)in the non-Windows implementation.
139-139: Correct namespace closing comment.
Updating the comment to// namespace fsbridgeimproves clarity and accurately reflects the enclosing namespace.src/Makefile.bench.include (1)
86-86: Include new wallet creation benchmark source.
Addingbench/wallet_create.cppunder theENABLE_WALLETguard correctly integrates the new benchmark into the build.src/Makefile.am (1)
15-15: DefineAM_OBJCXXFLAGSfor Objective-C++ compilation.
MirroringAM_CXXFLAGSintoAM_OBJCXXFLAGSensures Objective-C++ sources inherit the same warning and optimization flags (e.g., to suppress deprecated-declarations warnings on macOS).src/rpc/server.cpp (1)
261-263: Useutf8string()instead of deprecatedu8string().
This aligns with the updated filesystem API, ensuring the RPCgetrpcinforesponse uses the new UTF-8 conversion method for the log file path.src/wallet/rpcwallet.cpp (1)
2606-2608: Useutf8string()for correct UTF-8 path conversion
The change replaces the deprecatedu8string()call with the newutf8string()API fromfs::path, aligning with the recent filesystem backports and ensuring wallet names are properly encoded as UTF-8.src/util/getuniquepath.cpp (1)
11-12: Improved UTF-8 path handling implementation.The change properly ensures that the random hex string is explicitly treated as a UTF-8 encoded path component by using
fs::u8path(). This is part of a broader effort to standardize and improve UTF-8 path handling across the codebase.src/crypto/chacha20.cpp (2)
14-14: Added the C++20<bit>header for standard bitwise operations.This header provides access to standard bitwise operations like
std::rotlwhich will be used in the QUARTERROUND macro.
18-21: Modernized code by replacing custom rotation withstd::rotl.The update uses the standard library's
std::rotlfunction from C++20 for bit rotation operations instead of custom implementations. This is a good modernization that leverages standard library features which are likely to be more optimized and better tested.src/test/util/chainstate.h (1)
32-32: Improved UTF-8 path handling.The change ensures that the formatted filename string is properly treated as a UTF-8 encoded path component by wrapping it with
fs::u8path(). This is consistent with the broader codebase modernization effort for UTF-8 path handling.src/addrdb.cpp (1)
57-57: Enhanced UTF-8 path handling.The change correctly wraps the temporary filename string with
fs::u8path()before appending it to the data directory path, ensuring proper UTF-8 encoding. This follows the same pattern applied consistently throughout the codebase in this PR.src/txmempool.cpp (2)
32-32: Include<ranges>for C++20 view adaptors
The addition of<ranges>ensures thatstd::views::reverseis available for reverse iteration, replacing the custom reverse iterator header.
198-198: Replace custom reverse iteration withstd::views::reverse
UsingvHashesToUpdate | std::views::reversepreserves the original semantics while adopting the standard C++20 ranges API for clarity and maintainability.src/crypto/sha3.cpp (3)
13-13: Include<bit>header forstd::rotl
#include <bit>is required to bringstd::rotlinto scope. Good addition to support the replacements below.
38-42: Usestd::rotlin Theta step
Replacing the customRotlcalls withstd::rotlmaintains identical functionality and leverages the standard library’s optimized rotate operation.
46-69: Usestd::rotlin Rho–Pi step
All bit-rotation operations in the Rho–Pi section now callstd::rotl, simplifying the code and ensuring efficient, portable behavior.src/index/blockfilterindex.cpp (1)
107-107: Convert filter name to UTF-8 path withfs::u8path
Usingfs::u8path(filter_name)guarantees correct UTF-8 encoding for the filter directory on all platforms, aligning with the updated filesystem utilities.src/net_processing.cpp (3)
49-49: Modern C++20 feature adoption: Adding ranges headerThis adds support for the C++20 ranges library, which provides powerful abstractions for sequence operations including the reverse view adaptor used elsewhere in this file.
2127-2129: Good modernization to C++20 rangesReplacing the custom
reverse_iterateutility with standard C++20std::views::reverseis a positive change that improves code clarity and maintenance.
3038-3047: Well-applied standard ranges functionalityAnother good use of C++20's
std::views::reverseto simplify reverse iteration. This modernization removes dependency on custom utilities while maintaining the same behavior.src/wallet/rpcdump.cpp (1)
961-961: UTF-8 path handling method updated correctly.The change from
u8string()toutf8string()aligns with the codebase-wide transition to use the newerutf8string()method for UTF-8 path string conversions as defined insrc/fs.h.src/crypto/chacha20poly1305.cpp (2)
29-29: Function renamed to avoid platform-specific conflicts.Renaming
timingsafe_bcmptotimingsafe_bcmp_internalhelps clarify this is an internal implementation and prevents conflicts with system implementations oftimingsafe_bcmp. This aligns with the removal of platform-specific feature checks inconfigure.ac.
91-91: Function call updated to match the renamed function.Call site correctly updated to use the renamed
timingsafe_bcmp_internalfunction.src/hash.cpp (4)
10-10: Added C++20 bit header for standard rotation functions.Added the
<bit>header which provides standard bit manipulation functions includingstd::rotl.
30-31: Replaced custom rotation with standard library function.The custom
ROTL32implementation has been replaced with the standard C++20std::rotlfunction, which improves code clarity and maintainability.
34-35: Replaced custom rotation with standard library function.Another instance of the custom
ROTL32implementation replaced withstd::rotl.
54-55: Replaced custom rotation with standard library function.Third instance of the custom
ROTL32implementation replaced withstd::rotl.src/flatfile.cpp (1)
30-30: Improved UTF-8 path handling.The formatted filename string is now explicitly converted to a UTF-8 encoded filesystem path using
fs::u8path(). This enhances cross-platform compatibility and ensures consistent UTF-8 handling throughout the codebase.src/rpc/blockchain.cpp (3)
2634-2635: Updated to use the new utf8string methodThe code now uses
utf8string()instead ofu8string()for path to UTF-8 string conversion. This is consistent with the systemic update to UTF-8 path string conversions throughout the codebase.
2645-2645: Updated to use the new utf8string methodThe code now uses
utf8string()instead ofu8string()for path to UTF-8 string conversion in the JSON result.
2716-2716: Updated to use the new utf8string methodThe code now uses
utf8string()instead ofu8string()for path to UTF-8 string conversion in the JSON result. This follows the same pattern as the earlier changes for consistent UTF-8 path handling.src/node/blockstorage.cpp (2)
26-26: C++20 ranges header addedAdded inclusion of the
<ranges>standard header to support the updated reverse iteration syntax.
413-413: Replaced custom reverse_iterate with standard rangesThe code now uses C++20's
std::views::reverserange adaptor instead of a custom reverse iteration utility. This is a good modernization that leverages standard library features for reverse iteration.doc/developer-notes.md (1)
1411-1415: Updated RPC interface guidelines for filesystem path conversion.The documentation now includes
fs::path::utf8string()as an alternative tofs::path::u8string()for converting paths to JSON strings, maintaining the guideline to avoid usingfs::PathToStringandfs::PathFromString. This update aligns with broader codebase changes whereutf8string()is replacingu8string().src/test/prevector_tests.cpp (3)
12-13: Updated includes to use C++20 ranges for reverse iteration.The code now includes the standard
<ranges>header instead of what was likely a custom reverse iterator implementation. This modernizes the code to use C++20 standard library features.
59-61: Replaced custom reverse iteration with C++20 range-based approach.The code now uses
std::views::reversefor reverse iteration onpre_vector, which is a cleaner, more modern approach following C++20 standards.
65-67: Replaced custom reverse iteration with C++20 range-based approach.Similar to the previous change, the code now uses
std::views::reversefor reverse iteration onconst_pre_vector, following modern C++20 idioms.src/rpc/mempool.cpp (1)
462-462: Updated path-to-string conversion.Changed from
u8string()toutf8string()for path-to-UTF8-string conversion, maintaining consistency with the updated guidelines for filesystem path handling in RPC responses.src/crypto/siphash.cpp (2)
7-7: Added standard bit manipulation header.The code now includes the C++20
<bit>header which provides standardized bit manipulation utilities, specifically for thestd::rotlfunction.
10-15: Replaced custom bit rotation with standard library functions.The SIPROUND macro now uses
std::rotlfrom the standard library instead of custom bit rotation implementations. This improves code clarity, maintainability, and potentially performance through compiler optimizations.src/net.cpp (1)
5125-5129: Consistent UTF-8 path handling applied
Wrappingclean_addrwithfs::u8path()aligns with the project's C++20 backports for proper UTF-8 encoding when constructing filesystem paths. This change is correct and integrates cleanly with the surrounding code.src/flat-database.h (1)
178-178: Improved UTF-8 path handling.This change ensures proper UTF-8 handling for filesystem paths by explicitly converting the filename string to a UTF-8 encoded path using
fs::u8path()before appending it to the data directory path.src/test/fuzz/prevector.cpp (3)
11-12: Updated to use C++20 ranges for reverse iteration.Replaced custom reverse iteration utilities with standard C++20
<ranges>header, modernizing the code and reducing dependencies.
48-48: Modernized reverse iteration with ranges.Using
std::views::reverserange adaptor provides a cleaner, more standardized way to perform reverse iteration compared to the previous custom solution.
56-56: Modernized reverse iteration with ranges.Using
std::views::reverserange adaptor provides a cleaner, more standardized way to perform reverse iteration compared to the previous custom solution.src/wallet/test/init_test_fixture.cpp (2)
20-20: Simplified separator variable declaration.Changed from constructing a string with separator appended to directly using the preferred separator character, making the code cleaner and more efficient.
29-30: Improved path construction with trailing separators.Changed the order of operations when constructing paths with trailing separators - now first joins paths with the
/operator and then appends separators. This matches the pattern used in other filesystem path handling throughout the codebase.configure.ac (1)
855-857: Added warning suppression for deprecated macOS APIs.Suppresses warnings about deprecated declarations in ObjectiveC++ code, specifically handling
NSUserNotificationCenterdeprecation in macOS 11.0, which allows for cleaner builds without unnecessary warning noise.src/validation.cpp (4)
68-68: Add C++20 ranges header
Including<ranges>is required for usingstd::views::reverse. This aligns with the modern C++20 backports and removes the dependency on the customreverse_iterator.h.
1505-1511: Usefs::pathfor database name parameter
Changing the constructor ofCoinsViewsto acceptfs::pathimproves type safety and ensures proper filesystem handling (including UTF-8 and platform-specific path semantics). Ensure all call sites pass a validfs::pathand remove any obsolete string-to-path conversions.
1533-1538: ChangeInitCoinsDBto acceptfs::path
UpdatingInitCoinsDBto takefs::pathinstead ofstd::stringforleveldb_nameenhances type correctness. Confirm that callers provide the network-specific data directory joined with the path, and update any related documentation or overloads if necessary.
3231-3234: Adoptstd::views::reversefor reverse iteration
Replacing the custom reverse iterator withvpindexToConnect | std::views::reverseleverages standard C++20 ranges for cleaner and more idiomatic code. Ensure<ranges>is imported (as added) and consider similar updates elsewhere in the codebase.src/validation.h (2)
418-418: Parameter type updated fromstd::stringtofs::pathfor better path handling.The constructor parameter type for
CoinsViewsis nowfs::pathinstead ofstd::string, improving type safety and making filesystem path handling more explicit.
517-517: Updated parameter type fromstd::stringtofs::pathfor consistency.This change aligns the
leveldb_nameparameter with the constructor change above, ensuring consistent use offs::pathfor database paths throughout the codebase.src/wallet/test/db_tests.cpp (3)
17-17: Function signature updated to usefs::pathinstead ofstd::string.The
GetWalletEnvfunction now takes and returnsfs::pathtypes instead of strings, improving type safety when dealing with filesystem paths.
20-20: Updated to use path-specific methods for filename extraction.Now using
filename()method on thefs::pathobject instead of string manipulation to extract the filename component.
26-27: Test variable types changed fromstd::stringtofs::path.All filename and path-related variables have been updated from
std::stringtofs::paththroughout the test cases for consistency with the updated function signature, enhancing type safety for path operations.Also applies to: 32-32, 40-41, 43-44, 53-53, 66-67
src/qt/guiutil.cpp (4)
736-736: Improved UTF-8 path handling withfs::u8path.Now explicitly using
fs::u8pathto construct paths from UTF-8 encoded strings, ensuring proper handling of non-ASCII characters in filenames.
817-817: Enhanced UTF-8 support for path construction.Similar to the previous change, this update ensures proper handling of UTF-8 encoded strings when constructing paths for the autostart file.
1669-1669: Updated to useutf8string()instead of deprecatedu8string().This change updates the code to use the new
utf8string()method for path-to-string conversion, which is the preferred approach for UTF-8 handling in the updated codebase.
1717-1729: Modernized code with C++20 chrono facilities.The
formatDurationStrfunction now uses standardstd::chrono::daysinstead of a custom implementation, leveraging C++20 calendar features for better time formatting.src/test/fs_tests.cpp (4)
21-21: Added explicitstd::u8stringvariable for UTF-8 testing.Introduced a UTF-8 string literal (
u8"...") to test the proper handling of UTF-8 encoded paths, enhancing the test coverage for Unicode path support.
23-26: Updated path conversion test to useutf8string().The test now verifies UTF-8 path handling using the new
utf8string()method instead of the deprecatedu8string(), while also testing bothfs::u8pathand directfs::pathconstruction from UTF-8 strings.
52-53: Enhanced path construction tests for UTF-8 compatibility.Added test coverage for two equivalent methods of creating paths with UTF-8 characters:
- Using
fs::u8pathwith a standard string- Using
fs::pathwith a UTF-8 string literalThis ensures both methods work correctly and produce consistent results.
107-107: Updated path joining to use UTF-8-aware path construction.The test for
AbsPathJoinnow explicitly usesfs::u8pathwhen constructing the path with non-ASCII characters, ensuring proper UTF-8 handling in path operations.src/wallet/wallet.cpp (5)
54-55: Added modern C++20 includes for improved functionality.These additions are necessary to support the C++20 features used in the updated code, with
<ranges>supporting the new iteration patterns and<cassert>for assertions.
3265-3271: Great modernization of timestamp generation using C++20 chrono utilities.This change replaces legacy C-style time handling with modern C++20 calendar utilities, eliminating platform-dependent code (
gmtime_r/gmtime_s) and improving safety and maintainability. The conversion to proper year/month/day types with explicit signedness handling is well-implemented.
3276-3276: Improved UTF-8 support for backup filenames.Using
fs::u8path()ensures proper handling of UTF-8 encoded filenames, which is important for international compatibility when wallet names contain non-ASCII characters.
3299-3299: Consistent UTF-8 path handling for wallet backups.Similar to the earlier change, this ensures UTF-8 encoded filenames are properly handled for backup files created from an existing wallet file.
3338-3338: Simplified code using C++20 ranges for reverse iteration.The change from a custom reverse iterator to standard C++20
std::views::reversemakes the code more readable and maintainable while following modern C++ idioms. This pattern is consistent with other similar changes across the codebase.src/wallet/bdb.cpp (8)
274-274: This is a good use of fs::path operator/The code now uses the
/operator to concatenate directory and filename paths, which is more idiomatic forfs::pathobjects than string concatenation.
288-289: Good local conversion for Berkeley DB APIThe code correctly converts the
fs::pathto a string only when needed for interaction with the Berkeley DB API that expects C-style strings.
311-316: Consistent type safety improvements for filesystem pathsThe code now consistently uses
fs::pathfor file path handling including database operations, map indexing, and interactions with the Berkeley DB environment. The conversion to string is only done when necessary for API compatibility.
416-428: Updated method signature to use fs::pathThe
CloseDbmethod signature has been updated to usefs::pathinstead ofstd::string, improving type safety for filesystem operations. The implementation has been updated to handle path objects correctly.
443-450: Improved type safety in filesystem iterationThe code now uses
fs::pathtypes for the file names vector and iteration, ensuring consistent type safety throughout the filesystem operations.
554-558: Properly typed path handling in Flush methodThe filesystem paths in the database flush method now use the correct
fs::pathtype with appropriate conversions when interfacing with Berkeley DB API functions.
629-633: Improved path composition in Backup methodPath composition now uses the proper
fs::pathoperators (/) and types, leading to more robust path handling while maintaining compatibility with Berkeley DB.
845-853: Consistent fs::path usage in database initializationThe
MakeBerkeleyDatabasefunction now correctly handles the filesystem path components when initializing the database, using proper path composition and filename extraction.src/util/system.cpp (4)
110-132: Improved filesystem path handling in LockDirectoryThe function signature has been updated to use
fs::pathfor thelockfile_nameparameter instead ofstd::string, which is more type-safe for filesystem operations. The implementation properly composes paths using the path concatenation operator.
134-138: Consistent type usage in UnlockDirectoryThe
UnlockDirectoryfunction now acceptsfs::pathparameters for consistency withLockDirectory, maintaining proper typing throughout the directory locking system.
421-444: Removed const qualifier from GetBlocksDirPath return valueThe function now returns a non-const
fs::path, allowing modification of the returned path object if needed. This is consistent with the declaration in the header file and modern C++ practices.
446-478: Removed const qualifier from GetDataDir return valueThe function now returns a non-const
fs::path, which is consistent with the declaration in the header file. This allows for more flexibility in using the returned path object.src/serialize.h (3)
13-13: Added C++20 concepts supportThe include for C++ concepts header has been added to support the updated type constraints.
198-204: Modernized type constraint with C++20 conceptThe code replaces the preprocessor-based exclusion with a modern C++20 concept
CharNotInt8that more clearly expresses the intent: preventing serialization ofchartype unless it's the same asint8_t. This is more type-safe and maintainable than the previous preprocessor approach.
218-218: Consistent use of concept for deserializationThe same concept constraint has been correctly applied to the deserialization function, maintaining consistency between serialization and deserialization operations.
src/test/util_tests.cpp (1)
281-287: Complementary date-only tests look goodThe additional
FormatISO8601Datechecks mirror the date-time tests and improve confidence. No issues spotted.src/bench/wallet_create.cpp (3)
38-38: Correct UTF-8 string conversion usageThe code correctly uses the
utf8string()method to convert the filesystem path to a UTF-8 encoded string, which aligns with the updated UTF-8 handling in the codebase.
51-54: Appropriate conditional compilation guardThe benchmarks are correctly guarded with
#ifdef USE_SQLITEto ensure they're only compiled and registered when SQLite support is enabled.
36-36: 🛠️ Refactor suggestionPotential safety issue with temporary C-string pointer usage
The path construction uses
.c_str()which produces a temporary C-string pointer that is then passed to the path concatenation operator. This could lead to undefined behavior if the temporary string is deallocated before the path is constructed.Use this safer alternative:
-fs::path wallet_path = test_setup->m_path_root / strprintf("test_wallet_%d", random.rand32()).c_str(); +std::string wallet_name = strprintf("test_wallet_%d", random.rand32()); +fs::path wallet_path = test_setup->m_path_root / wallet_name.c_str();Likely an incorrect or invalid review comment.
src/wallet/bdb.h (5)
54-54: Good filesystem path type safety improvementChanging the map key type from
std::stringtofs::pathimproves type safety and consistency for filesystem path handling.
72-72: Consistent parameter type changeChanging the parameter type from
std::stringtofs::pathfor theCloseDbmethod maintains consistency with the path handling approach throughout the codebase.
99-102: Proper move semantics with path parametersThe constructor now correctly uses
fs::pathfor the filename parameter and member variable, with appropriate move semantics in the constructor initializer list. Theemplacecall is also updated correctly to use the path as the map key.
143-143: Improved path construction and string conversionThe
Filename()implementation now correctly usesfs::pathoperations followed byfs::PathToStringfor conversion, which is safer and more consistent than direct string concatenation.
160-160: Type-safe member variable declarationThe member variable type is appropriately changed from
std::stringtofs::pathfor improved type safety and consistency.src/fs.h (5)
39-39: Improved parameter passingChanged from potentially moving a path to taking a const reference, which is more efficient and prevents unnecessary copies.
56-66: Enhanced UTF-8 string conversion methodThe renamed
utf8string()method provides better clarity about its purpose and correctly handles the conversion fromstd::u8stringtostd::string. The documentation clearly explains the relationship with the underlyingstd::filesystem::path::u8string()method.
100-119: Safe path operator overloadsThese explicit operator overloads for path concatenation and addition with
pathandconst char*improve type safety while maintaining usability.
122-123: Strong type safety with deleted templatesThe deleted template overloads prevent unsafe implicit conversions that could lead to errors or undefined behavior, enforcing proper path handling throughout the codebase.
143-143: Consistent UTF-8 terminology in documentation and implementationThe documentation and implementation are updated to consistently refer to the renamed
utf8string()method alongsideu8string(), and the Windows implementation now correctly usesutf8string()for path-to-string conversion.Also applies to: 157-159, 163-163
| std::string FormatISO8601DateTime(int64_t nTime) | ||
| { | ||
| const std::chrono::sys_seconds secs{std::chrono::seconds{nTime}}; | ||
| const auto days{std::chrono::floor<std::chrono::days>(secs)}; | ||
| const std::chrono::year_month_day ymd{days}; | ||
| const std::chrono::hh_mm_ss hms{secs - days}; | ||
| return strprintf("%04i-%02u-%02uT%02i:%02i:%02iZ", signed{ymd.year()}, unsigned{ymd.month()}, unsigned{ymd.day()}, hms.hours().count(), hms.minutes().count(), hms.seconds().count()); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Edge-case safety: guard against out-of-range chrono conversions
std::chrono::year_month_day is only guaranteed to support years ‑32767…32767. If nTime exceeds that (e.g. user supplies 40000-01-01), construction invokes undefined behaviour. Consider detecting the overflow and falling back to "9999-12-31T23:59:59Z" or throwing.
- const std::chrono::year_month_day ymd{days};
+ const auto ymd = std::chrono::year_month_day{days};
+ if (!ymd.ok()) return "9999-12-31T23:59:59Z"; // or handle appropriately📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| std::string FormatISO8601DateTime(int64_t nTime) | |
| { | |
| const std::chrono::sys_seconds secs{std::chrono::seconds{nTime}}; | |
| const auto days{std::chrono::floor<std::chrono::days>(secs)}; | |
| const std::chrono::year_month_day ymd{days}; | |
| const std::chrono::hh_mm_ss hms{secs - days}; | |
| return strprintf("%04i-%02u-%02uT%02i:%02i:%02iZ", signed{ymd.year()}, unsigned{ymd.month()}, unsigned{ymd.day()}, hms.hours().count(), hms.minutes().count(), hms.seconds().count()); | |
| } | |
| std::string FormatISO8601DateTime(int64_t nTime) | |
| { | |
| const std::chrono::sys_seconds secs{std::chrono::seconds{nTime}}; | |
| const auto days{std::chrono::floor<std::chrono::days>(secs)}; | |
| const auto ymd = std::chrono::year_month_day{days}; | |
| if (!ymd.ok()) return "9999-12-31T23:59:59Z"; // or handle appropriately | |
| const std::chrono::hh_mm_ss hms{secs - days}; | |
| return strprintf( | |
| "%04i-%02u-%02uT%02i:%02i:%02iZ", | |
| signed{ymd.year()}, | |
| unsigned{ymd.month()}, | |
| unsigned{ymd.day()}, | |
| hms.hours().count(), | |
| hms.minutes().count(), | |
| hms.seconds().count() | |
| ); | |
| } |
There was a problem hiding this comment.
generally LGTM 4dd79b0; one commit references incorrect included commit
…a single db txn includes: - bb4554c
The next commit moves inherited code away from `gmtime` and removes it altogether.
, bitcoin#24739, bitcoin#25011, bitcoin#25045, bitcoin#25106, bitcoin#25213, bitcoin#25003, bitcoin#24629, bitcoin#24640, bitcoin#24913, bitcoin#24924, bitcoin#25023, bitcoin#25887, bitcoin#25810, bitcoin#25792, bitcoin#25227, bitcoin#27218 (auxiliary backports: part 26) cc0e2d1 merge bitcoin#27218: Work around ParseHex gcc cross compiler bug (Kittywhiskers Van Gogh) b8c3535 merge bitcoin#25227: Handle invalid hex encoding in ParseHex (Kittywhiskers Van Gogh) 0f51413 fix: use properly formed hex encoding in `evo_assetlock` (Kittywhiskers Van Gogh) cccd71f merge bitcoin#25792: add tests for `datacarrier` and `datacarriersize` options (Kittywhiskers Van Gogh) f55742b merge bitcoin#25810: rename MAX_{ANCESTORS,DESCENDANTS} to DEFAULT_{ANCESTOR,DESCENDANT}_LIMIT (Kittywhiskers Van Gogh) 83acd1d merge bitcoin#25887: avoid unsetting service bits from `nLocalServices` (Kittywhiskers Van Gogh) 528795d merge bitcoin#25023: Remove unused SetTip(nullptr) code (Kittywhiskers Van Gogh) 6c8a8f7 merge bitcoin#24924: Make WalletLoading benchmark run faster (Kittywhiskers Van Gogh) 7b09514 merge bitcoin#24913: Add a benchmark for wallet loading (Kittywhiskers Van Gogh) f0975dc merge bitcoin#24640: Correct description of getblockchaininfo's pruneheight result (Kittywhiskers Van Gogh) b6b94b2 merge bitcoin#24629: Return the height of the actual last pruned block (Kittywhiskers Van Gogh) fdba53e merge bitcoin#25003: fix `coin_selection:aps_create_tx_internal` calling logic (Kittywhiskers Van Gogh) 0b915e7 merge bitcoin#25213: fix crash at coinselection, add missing fee rate (Kittywhiskers Van Gogh) e39b9d8 merge bitcoin#25106: check `fopen` return code (Kittywhiskers Van Gogh) 6f09738 merge bitcoin#25045: add coverage for invalid requests for `blockfilterheaders` (REST) (Kittywhiskers Van Gogh) 68e4f98 merge bitcoin#25011: Do not always create a descriptor wallet in wallet_createwallet (Kittywhiskers Van Gogh) be86792 merge bitcoin#24739: Fix intermittent test failure in wallet_listreceivedby.py (Kittywhiskers Van Gogh) 2c6d522 merge bitcoin#24576: remove redundant base58 implementation (Kittywhiskers Van Gogh) 53bfca8 merge bitcoin#26143: wait for the expected basic block filter index in `interface_rest` (Kittywhiskers Van Gogh) 624727f merge bitcoin#24098: Use query parameters to control resource loading (Kittywhiskers Van Gogh) Pull request description: ## Additional information * The documentation introduced in [bitcoin#24098](bitcoin#24098) has been updated to reflect the affected version as v23. * In [bitcoin#24576](bitcoin#24576), while the definitions `OP_{0,1,2,16}` are removed, they were not subsequently imported from `test_framework.script` as they are unused in `gen_key_io_test_vectors.py` * In [bitcoin#25106](bitcoin#25106), `utf8string()` is called instead of `u8string()` due to [bitcoin#29040](bitcoin#29040) (backported as 209488d in [dash#6657](#6657)) updating the function name. * Due to stricter validation introduced in [bitcoin#25227](bitcoin#25227), the unit test `evo_assetlock` fails (see below) due to `ParseHex` refusing to treat malformed hex encoding as representing valid data and the unit test used a dummy message of improper length. <details> <summary>Test failure:</summary> ``` $ ./src/test/test_dash Running 644 test cases... test/evo_assetlocks_tests.cpp(294): error: in "evo_assetlocks_tests/evo_assetlock": check !CheckAssetLockTx(CTransaction(txReturnData), tx_state) has failed test/evo_assetlocks_tests.cpp(295): error: in "evo_assetlocks_tests/evo_assetlock": check tx_state.GetRejectReason() == "bad-assetlocktx-non-empty-return" has failed *** 2 failures are detected in the test module "Dash Core Test Suite" ``` </details> This has been resolved in a separate commit. ## Breaking Changes * The return value of the `pruneblockchain` method had an off-by-one bug, returning the height of the block *after* the most recent pruned. This has been corrected, and it now returns the height of the last pruned block as documented. ## Checklist - [x] I have performed a self-review of my own code - [x] I have commented my code, particularly in hard-to-understand areas **(note: N/A)** - [x] I have added or updated relevant unit/integration/functional/e2e tests - [x] I have made corresponding changes to the documentation - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ ACKs for top commit: knst: utACK cc0e2d1 UdjinM6: utACK cc0e2d1 Tree-SHA512: bc90217977a75a6554a16fd9e93664abaa25fedd8e9e75051f8dca1dff45cf2667a1072b0440f565aae75d644a1813166a2689a6d290705e9134bbf83fb34b71
3 participants
Additional Information
Breaking Changes
None expected.
Checklist