backport: finish merging 25001 #6531
Conversation
Base32/base64 are mechanisms for encoding binary data. That they'd decode to a string is just bizarre. The fact that they'd do that based on the type of input arguments even more so.
WalkthroughThe provided pull request introduces a comprehensive set of changes across multiple source files in the Bitcoin codebase, focusing on improving string handling, error management, and performance. The modifications primarily revolve around three key aspects:
These modifications aim to enhance code clarity, improve performance, and strengthen error handling throughout the Bitcoin implementation, demonstrating a systematic approach to code modernization. Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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: 0
🧹 Nitpick comments (6)
src/util/string.h (1)
79-87: Consider adding noexcept specifierThe Join template functions could benefit from noexcept specification where applicable.
-template <typename T, typename T2> -T Join(const std::vector<T>& list, const T2& separator) +template <typename T, typename T2> +T Join(const std::vector<T>& list, const T2& separator) noexcept(noexcept(T{}))src/util/strencodings.h (1)
67-67: Improved error handling with std::optional return typeThe change from output parameters to
std::optionalreturn type forDecodeBase64andDecodeBase32functions improves error handling and makes the API more modern and safer.Consider adding documentation to clarify what conditions would cause the functions to return an empty optional.
Also applies to: 70-71
src/util/strencodings.cpp (1)
205-210: Comprehensive Base32 padding handlingThe Base32 padding logic handles all valid cases (1, 3, 4, or 6 padding characters) explicitly and safely.
Consider adding a comment explaining why these specific padding lengths are valid for Base32.
src/rpc/evo.cpp (2)
818-820: Improve error handling for base64 signature decodingThe code now properly handles base64 decoding errors using std::optional, providing better error handling and safety.
Consider extracting the error message to a constant to maintain consistency across the codebase:
static const char* BASE64_DECODE_ERROR = "failed to decode base64 ready signature for protx"; if (!opt_vchSig.has_value()) throw JSONRPCError(RPC_INTERNAL_ERROR, BASE64_DECODE_ERROR);
880-884: Improve error handling for base64 signature parsingThe code now properly handles base64 decoding errors using std::optional and provides a more specific error message.
Consider making the error message more descriptive to help users understand what went wrong:
if (!opt_vchSig.has_value()) { throw JSONRPCError(RPC_INVALID_PARAMETER, "The provided signature is not a valid base64 string"); }src/test/util_tests.cpp (1)
2697-2703: Add test cases for RemovePrefixView functionNew test cases added to verify the RemovePrefixView functionality with various input combinations.
Consider adding more edge cases to improve test coverage:
BOOST_CHECK_EQUAL(RemovePrefixView("foo/bar", "foo/"), "bar"); BOOST_CHECK_EQUAL(RemovePrefixView("foo/bar/", "foo/bar/"), ""); BOOST_CHECK_EQUAL(RemovePrefixView("foo/bar/baz", "foo/bar/baz/"), "foo/bar/baz");
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (26)
src/base58.cpp(2 hunks)src/bitcoin-tx.cpp(1 hunks)src/httprpc.cpp(1 hunks)src/httpserver.cpp(1 hunks)src/i2p.cpp(1 hunks)src/netaddress.cpp(3 hunks)src/netbase.cpp(5 hunks)src/psbt.cpp(1 hunks)src/psbt.h(1 hunks)src/qt/walletframe.cpp(2 hunks)src/rpc/evo.cpp(2 hunks)src/rpc/governance.cpp(3 hunks)src/stacktraces.cpp(1 hunks)src/test/base32_tests.cpp(1 hunks)src/test/base64_tests.cpp(2 hunks)src/test/fuzz/base_encode_decode.cpp(2 hunks)src/test/fuzz/http_request.cpp(1 hunks)src/test/fuzz/psbt.cpp(2 hunks)src/test/fuzz/string.cpp(2 hunks)src/test/util_tests.cpp(2 hunks)src/util/message.cpp(1 hunks)src/util/moneystr.cpp(1 hunks)src/util/strencodings.cpp(9 hunks)src/util/strencodings.h(10 hunks)src/util/string.h(3 hunks)src/util/system.cpp(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: Clang Diff Format Check
src/rpc/evo.cpp
[warning] 816-816: Code formatting issue: Conditional statement needs to be split into multiple lines
[warning] 877-877: Code formatting issue: Missing space after variable declaration
🔇 Additional comments (47)
src/test/base32_tests.cpp (2)
25-27: LGTM! Improved error handling approachThe transition from bool* to std::optional for DecodeBase32 results in cleaner and more idiomatic C++ code. The use of BOOST_REQUIRE ensures immediate test failure on decode errors.
31-34: LGTM! Comprehensive null character handling testsGood coverage of edge cases for null character handling, including:
- Middle of string
- Valid input
- After valid base32 string
- Invalid size scenarios
src/test/base64_tests.cpp (2)
22-24: LGTM! Consistent error handling improvementThe changes align with the base32 updates, using std::optional for cleaner error handling.
38-41: LGTM! Comprehensive null character validationGood set of test cases covering various null character scenarios in base64 strings.
src/test/fuzz/base_encode_decode.cpp (3)
25-25: Performance improvement using TrimStringViewGood optimization by using TrimStringView instead of TrimString to avoid unnecessary string copies.
35-39: LGTM! Consistent Base32 decoding patternThe changes align with the new error handling approach using std::optional.
42-44: LGTM! Consistent Base64 decoding patternThe changes maintain consistency with the Base32 implementation.
src/util/moneystr.cpp (1)
43-43: LGTM! Consistent string validationThe change from ValidAsCString to ContainsNoNUL provides more precise validation and maintains consistency with similar changes across the codebase.
src/util/message.cpp (2)
38-39: LGTM! Improved error handling with std::optionalThe change from boolean flag to std::optional for DecodeBase64 result improves code clarity and error handling.
44-44: Verify proper error handling for RecoverCompactWhile the dereferencing of signature_bytes is safe due to the prior check, consider adding error handling for RecoverCompact failure.
src/test/fuzz/psbt.cpp (1)
26-27: LGTM! Improved type safety in fuzzing testsThe changes enhance type safety by:
- Using an intermediate variable for the random string
- Explicitly converting to byte span using MakeByteSpan
Also applies to: 74-75
src/test/fuzz/http_request.cpp (1)
57-57: LGTM! More explicit string constructionThe change improves code clarity by using explicit std::string construction instead of relying on implicit conversion.
src/util/string.h (4)
32-39: LGTM! Optimized string trimming with string_viewThe new TrimStringView implementation properly handles empty results and avoids unnecessary string allocations.
42-45: LGTM! Clean separation of view and string operationsGood practice to provide both string_view and string versions of the trim operation.
47-58: LGTM! Consistent pattern for prefix removalFollowing the same pattern as trim operations with separate view and string versions.
102-107: LGTM! Improved null character detectionThe new ContainsNoNUL function is more explicit about its purpose and uses modern C++ features.
src/base58.cpp (2)
129-132: LGTM! The validation change improves clarity.The change from
ValidAsCStringtoContainsNoNULmakes the validation more focused and explicit about what it's checking.
163-166: LGTM! Consistent validation approach.The validation change is consistent with the
DecodeBase58function above.src/qt/walletframe.cpp (4)
252-252: LGTM! Better type for binary data.Changed from
std::stringtostd::vector<unsigned char>, which is more appropriate for handling binary PSBT data.
256-261: LGTM! Improved error handling with std::optional.The change to use
std::optionalfor base64 decoding provides clearer error handling and better type safety.
272-272: LGTM! Direct binary reading.Using
std::istream_iterator<unsigned char>provides direct binary reading without string conversion.
277-277: LGTM! Consistent byte span usage.Using
MakeByteSpanadapts the data correctly for the updatedDecodeRawPSBTinterface.src/test/fuzz/string.cpp (1)
43-43: LGTM! Consistent validation approach.The change from
ValidAsCStringtoContainsNoNULaligns with similar changes in other files, making the validation intent more explicit.src/psbt.cpp (2)
346-351: LGTM! Improved error handling with std::optional.The change to use
std::optionalfor base64 decoding provides better type safety and clearer error handling.
354-356: LGTM! Better type safety with Span.Changed function signature to use
Span<const std::byte>, providing better type safety for binary data handling.src/util/strencodings.h (3)
238-244: Good addition of IntIdentity helperThe
IntIdentityhelper class provides a clean default implementation for theinfnparameter inConvertBits. This improves code clarity and maintainability.
248-249: Verify ConvertBits template usage with new IntIdentityThe
ConvertBitstemplate function now uses the newIntIdentityhelper. We should verify all existing uses remain compatible.✅ Verification successful
ConvertBits template modification is backward compatible
All existing uses of ConvertBits are compatible with the new IntIdentity parameter as they rely on the default value, requiring no changes to the calling code.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for all uses of ConvertBits to verify compatibility rg "ConvertBits<" --type cppLength of output: 648
97-102: Verify the behavior change in LocaleIndependentAtoiThe function now uses
TrimStringViewandstd::from_charsinstead of the C locale-dependent functions. While this is a good modernization, we should verify the behavior is identical.✅ Verification successful
✓ Behavior change in LocaleIndependentAtoi is correct
The function's behavior is identical to the C locale-dependent functions, as verified by:
- Comprehensive test suite covering all edge cases
- Explicit fuzz test verification against legacy behavior
- Consistent usage patterns across the codebase
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for all calls to LocaleIndependentAtoi to verify usage patterns rg -A 2 "LocaleIndependentAtoi"Length of output: 10089
src/httprpc.cpp (1)
174-178: Improved error handling in RPCAuthorizedThe changes improve both efficiency and safety:
- Using
std::string_viewreduces unnecessary copying- Using
std::optionalfor Base64 decoding makes error handling more explicit- Early return on decoding failure simplifies the control flow
src/i2p.cpp (1)
72-76: Streamlined Base64 decoding with std::optionalThe changes improve the code by:
- Using
std::optionalfor clearer error handling- Using
std::moveto avoid unnecessary copying of the decoded data- Providing a more descriptive error message on failure
src/util/strencodings.cpp (4)
150-153: Robust padding handling in Base64 decodingThe padding removal logic is now more explicit and safer, properly handling one or two padding characters.
156-162: Efficient Base64 decoding implementationThe implementation is improved with:
- Proper memory pre-allocation
- Use of ConvertBits for the actual conversion
- Clear error handling through the return value
213-222: Efficient Base32 decoding implementationSimilar improvements to Base64 decoding:
- Proper memory pre-allocation
- Use of ConvertBits for conversion
- Clear error handling
494-494: Verify ParseByteUnits with new string_view parameterThe function now takes std::string_view, which is more efficient but we should verify all callers handle the lifetime correctly.
✅ Verification successful
String view lifetime usage is safe
All current usages of
ParseByteUnitsmaintain proper string_view lifetime:
- Test cases use string literals
- Production code in
init.cppsafely uses temporary strings🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for all calls to ParseByteUnits rg "ParseByteUnits\(" --type cppLength of output: 2251
src/httpserver.cpp (1)
275-279: LGTM! Improved error handling for Base64 decoding.The changes enhance robustness and readability by:
- Using
std::optionalfor clearer error handling- Employing string iterator for more idiomatic delimiter search
- Properly handling all error cases with early returns
src/stacktraces.cpp (1)
430-435: LGTM! Enhanced error handling for crash info deserialization.The changes improve robustness by:
- Using
std::optionalfor explicit error handling- Adding proper validation of the decoded buffer
- Using clear variable naming with
opt_bufsrc/netbase.cpp (1)
147-147: LGTM! Consistent input validation across network functions.The changes standardize string validation by using
ContainsNoNULconsistently across:
LookupInternLookupHostLookupLookupNumericLookupSubNetThis provides a more focused check for null characters in input strings.
Also applies to: 176-176, 194-194, 219-219, 671-671
src/bitcoin-tx.cpp (1)
226-226: LGTM! Performance optimization in string handling.The change from
TrimStringtoTrimStringViewpotentially improves performance by avoiding unnecessary string copies during parsing.src/netaddress.cpp (4)
214-214: LGTM! Improved string validation.The change from
ValidAsCStringtoContainsNoNULis a good improvement as it makes the validation intent clearer.
244-247: LGTM! Improved readability with descriptive variable names.The use of descriptive variable names (
input_pubkey,input_checksum,input_version) improves code readability.
283-290: LGTM! Consistent error handling pattern.The changes in
SetI2Pfollow the same pattern asSetTor, maintaining consistency in error handling.
238-244: Verify error handling in base32 decoding.The change to use
std::optionalfor base32 decoding improves error handling, but we should verify that all error cases are properly handled.✅ Verification successful
Base32 decoding error handling is properly implemented
The implementation in
DecodeBase32returnsstd::optionalfor all error cases (invalid length, invalid characters, conversion failures) and all callers properly check for nullopt before using the result. The test suite includes comprehensive validation of error cases.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other base32 decoding error handling patterns rg -A 3 "DecodeBase32\(" | grep -v "if.*has_value"Length of output: 2565
Script:
#!/bin/bash # Check the implementation of DecodeBase32 for error cases ast-grep --pattern 'DecodeBase32(std::string_view str) { $$$ }'Length of output: 67
Script:
#!/bin/bash # Get the implementation of DecodeBase32 rg -A 30 "^std::optional<std::vector<unsigned char>> DecodeBase32\(std::string_view str\)" src/util/strencodings.cppLength of output: 2053
Script:
#!/bin/bash # Get the complete implementation including the return statement rg -A 40 "^std::optional<std::vector<unsigned char>> DecodeBase32\(std::string_view str\)" src/util/strencodings.cppLength of output: 2191
src/psbt.h (1)
917-917: LGTM! Improved type safety with Span.The change from
std::stringtoSpan<const std::byte>for raw PSBT data improves type safety and makes the API more explicit about its binary data handling.src/rpc/governance.cpp (2)
425-428: LGTM! Improved error handling with optional.The change to use
std::optionalfor base64 decoding improves error handling and makes the success/failure state more explicit.
Line range hint
962-977: LGTM! Consistent error handling pattern.The changes in
voterawfollow the same pattern asSignVote, maintaining consistency in error handling across the codebase.src/util/system.cpp (1)
907-908: LGTM! Performance optimization with string_view.The change to use
std::string_viewavoids unnecessary string copies during config parsing, which is a good performance optimization.src/test/util_tests.cpp (1)
281-291: Update string trimming tests to use TrimStringViewTest cases have been updated to use TrimStringView, which is more efficient than TrimString as it avoids unnecessary string copies.
PastaPastaPasta
force-pushed
the
finish-25001
branch
from
fae670a to
e714607
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
src/rpc/evo.cpp (2)
816-820: Split the conditional statement for better readability.The conditional statement is too long and should be split into multiple lines for better readability.
Apply this diff:
- if (err == SigningResult::SIGNING_FAILED) { + if (err == SigningResult::SIGNING_FAILED) + { throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, SigningResultString(err)); } else if (err != SigningResult::OK){ throw JSONRPCError(RPC_WALLET_ERROR, SigningResultString(err)); }🧰 Tools
🪛 GitHub Actions: Clang Diff Format Check
[warning] 816-816: Code formatting issue: Conditional statement needs to be split into multiple lines
880-884: Add space after variable declaration.Add a space after the variable declaration for consistent code formatting.
Apply this diff:
- auto opt_vchSig= DecodeBase64(request.params[1].get_str()); + auto opt_vchSig = DecodeBase64(request.params[1].get_str());src/util/strencodings.h (1)
248-249: Consider adding noexcept specifierThe
ConvertBitstemplate function appears to be noexcept as it only performs arithmetic operations. Consider adding the noexcept specifier to communicate this guarantee to callers.-bool ConvertBits(O outfn, It it, It end, I infn = {}) { +bool ConvertBits(O outfn, It it, It end, I infn = {}) noexcept {src/util/string.h (2)
47-58: Consider optimizing the prefix comparison.The implementation is good, but the
substrcall in the comparison could be replaced with a more efficient solution.Consider this alternative implementation that avoids the
substrallocation:[[nodiscard]] inline std::string_view RemovePrefixView(std::string_view str, std::string_view prefix) { - if (str.substr(0, prefix.size()) == prefix) { + if (str.size() >= prefix.size() && str.compare(0, prefix.size(), prefix) == 0) { return str.substr(prefix.size()); } return str; }
102-107: Consider using std::find for better performance.While the current implementation is correct, using std::find could be more efficient.
Consider this alternative implementation:
[[nodiscard]] inline bool ContainsNoNUL(std::string_view str) noexcept { - for (auto c : str) { - if (c == 0) return false; - } - return true; + return std::find(str.begin(), str.end(), '\0') == str.end(); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
src/base58.cpp(2 hunks)src/bitcoin-tx.cpp(1 hunks)src/httprpc.cpp(1 hunks)src/httpserver.cpp(1 hunks)src/netaddress.cpp(3 hunks)src/netbase.cpp(5 hunks)src/rpc/evo.cpp(2 hunks)src/rpc/governance.cpp(3 hunks)src/stacktraces.cpp(1 hunks)src/test/fuzz/base_encode_decode.cpp(2 hunks)src/test/fuzz/http_request.cpp(1 hunks)src/test/fuzz/string.cpp(2 hunks)src/test/util_tests.cpp(2 hunks)src/util/moneystr.cpp(1 hunks)src/util/strencodings.cpp(9 hunks)src/util/strencodings.h(10 hunks)src/util/string.h(3 hunks)src/util/system.cpp(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (13)
- src/httpserver.cpp
- src/test/fuzz/string.cpp
- src/util/moneystr.cpp
- src/test/fuzz/http_request.cpp
- src/stacktraces.cpp
- src/base58.cpp
- src/bitcoin-tx.cpp
- src/netaddress.cpp
- src/netbase.cpp
- src/httprpc.cpp
- src/test/util_tests.cpp
- src/test/fuzz/base_encode_decode.cpp
- src/util/strencodings.cpp
🧰 Additional context used
🪛 GitHub Actions: Clang Diff Format Check
src/rpc/evo.cpp
[warning] 816-816: Code formatting issue: Conditional statement needs to be split into multiple lines
[warning] 877-877: Code formatting issue: Missing space after variable declaration
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build Dependencies (arm-linux, arm-linux-gnueabihf)
🔇 Additional comments (11)
src/rpc/evo.cpp (1)
818-820: LGTM! Robust error handling pattern.The error handling using
std::optionalfor base64 decoding is well-implemented. The code:
- Uses modern C++ practices with
std::optional- Provides clear error messages
- Handles failures consistently
Also applies to: 880-884
src/util/strencodings.h (3)
67-71: LGTM: Improved string handling with std::string_view and std::optionalThe changes to Base64/Base32 functions improve efficiency and error handling:
- Using
std::string_viewreduces unnecessary string copies- Returning
std::optionalprovides clearer error handling semantics
87-87: LGTM: Consistent use of std::string_view across parsing functionsThe string parsing functions have been updated consistently to use
std::string_view, improving performance by avoiding string copies while maintaining const-correctness.Also applies to: 97-97, 166-201
238-244: LGTM: Well-designed helper class for ConvertBitsThe
IntIdentityhelper class provides a clean default identity function implementation for theConvertBitstemplate.src/rpc/governance.cpp (2)
424-427: LGTM: Improved error handling with std::optionalThe changes to
SignVoteimprove error handling by:
- Using
std::optionalfor clearer success/failure indication- Using
CHECK_NONFATALfor runtime validation
Line range hint
961-976: LGTM: Consistent error handling in voterawThe changes to
voterawalign with the new error handling pattern:
- Clear error message for malformed base64 encoding
- Safe access to optional value using has_value()
src/util/system.cpp (1)
907-908: LGTM: Optimized string handling in config parsingThe changes improve performance by:
- Using
std::string_viewfor name and value to avoid string copies- Using
TrimStringViewfor efficient string trimmingsrc/util/string.h (4)
32-39: Well-implemented string view optimization!The
TrimStringViewfunction is efficiently implemented usingstd::string_view, avoiding unnecessary string copies. The empty return for all-pattern strings and proper edge case handling are particularly good.
42-45: LGTM! Clean wrapper implementation.Good separation of concerns between view and string operations.
79-81: Great template parameter flexibility!The addition of the T2 template parameter and string_view optimization improves both flexibility and performance while maintaining compatibility.
Also applies to: 86-86
Line range hint
32-107: Verify the impact of string_view changes across the codebase.The changes to use
std::string_vieware well-implemented, but we should verify the impact on calling code.Let's check for potential issues in the codebase:
✅ Verification successful
String view changes are safely implemented and used correctly across the codebase.
The verification shows:
- All string_view storage is properly managed
- Modified functions are used correctly with proper lifetime management
- Existing test coverage validates the changes
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for potential string_view lifetime issues and usage patterns # Look for cases where string_view might be stored rg "string_view.*;" --type cpp -g '!src/util/string.h' # Check for calls to the modified functions rg "TrimString|RemovePrefix|ContainsNoNUL|ValidAsCString" --type cpp -g '!src/util/string.h'Length of output: 11776
Issue being fixed or feature implemented
This was partially done earlier; do the remaining commits
What was done?
see commits
How Has This Been Tested?
built locally
Breaking Changes
none
Checklist:
Go over all the following points, and put an
xin all the boxes that apply.