This makes use of the implicit constructions and conversions to Span introduced in #18468 to simplify the hash.h interface:
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--174a7506f384e20aa4161008e828411d-->
Reviewers, this pull request conflicts with the following ones:
insert and contains [ZAP3] by jb55)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.
yup this will clean up some of my PRs. Concept ACK (I would almost say utACK but I just woke up).
Concept ACK. Read the code, built/tested/running bitcoind, need to do more detailed code review stepping through the commits.
Concept ACK: Span<const unsigned char> as a drop-in replacement for const std::vector<unsigned char>& as parameter type is safe use of Span :)
ACK 974381f1c53ff0674e22a40d9553f962fa0df108 a useful clear abstraction and nice cleanup. Code review, compared the implementation of as_(writable_)bytes to that shown in https://en.cppreference.com/w/cpp/container/span/as_bytes (if you retouch, s/to_/as_/ in the 45316c20 commit message). Built in various ways, ran tests and benches locally, ran bitcoind without issues for a day.
re-ACK fc7c518
Code review re-ACK 675d6fd per git range-diff abdfd2d fc7c518 675d6fd
207 | @@ -206,4 +208,20 @@ T& SpanPopBack(Span<T>& span) 208 | return back; 209 | } 210 | 211 | +//! Mimick C++20's std::as_bytes (with unsigned char instead of std::byte) 212 | +template<typename T> 213 | +Span<const unsigned char> AsUnsignedChars(Span<T> in) { return {reinterpret_cast<const unsigned char*>(in.data()), in.size_bytes()}; }
In commit "Make uint256 Span-convertible by adding ::data()" (5d1f1d93154f3cee384e121963940141603e01f7)
I think it'd be better to avoid doing potentially unsafe reinterpret_casts when really we only need to cast char pointers to unsigned char pointers.
My concern is that providing this function might make it too easy to write code that looks like it is hashing vector<uint256> or Optional<uint256> or uint256* data, but actually hashing the outer pointer or container objects instead.
I think we could provide a more limited convenience function, similar to our current CharCast function that's just restricted to chars and won't reinterpret arbitrary spans of objects.
Experimenting a little, the following seems to work well:
inline unsigned char* UnsignedCharCast(char* c) { return (unsigned char*)c; }
inline unsigned char* UnsignedCharCast(unsigned char* c) { return c; }
inline const unsigned char* UnsignedCharCast(const char* c) { return (unsigned char*)c; }
inline const unsigned char* UnsignedCharCast(const unsigned char* c) { return c; }
//! Safely convert a character array into a span of unsigned chars.
template<typename T>
Span<const unsigned char> UnsignedCharSpan(const T& in) { auto span = MakeSpan(in); return {UnsignedCharCast(span.data()), span.size_bytes()}; }
Another advantage of this is allows replacing AsUnsignedChars(MakeSpan(in)) with simpler UnsignedCharSpan(in) everywhere.
Agree, that seems like both a more convenient and slightly safer construction. I've implemented it slightly differently so that it remains constexpr, and supports everything MakeSpan does (including passing temporaries, and non-const return when appropriate).
In commit "Make uint256 Span-convertible by adding ::data()" (5d1f1d9)
I think you mean a different commit?
My concern is that providing this function might make it too easy to write code that looks like it is hashing
vector<uint256>orOptional<uint256>oruint256*data, but actually hashing the outer pointer or container objects instead.
I don't think that would be a large concern. Those data types don't have data() and size() methods, so no implicit conversion to Span can happen. You can of course explicitly construct one, but you'd need to use something like AsUnsignedChars(Span<std::vector<uint256>(&vec, 1)), which is hopefully obviously accessing the vector's byte representation instead.
215 | @@ -216,7 +216,7 @@ BOOST_AUTO_TEST_CASE(script_standard_ExtractDestination) 216 | s << OP_0 << ToByteVector(pubkey.GetID()); 217 | BOOST_CHECK(ExtractDestination(s, address)); 218 | WitnessV0KeyHash keyhash; 219 | - CHash160().Write(pubkey).Finalize(keyhash.begin()); 220 | + CHash160().Write(pubkey).Finalize(keyhash);
In commit "Make CHash256/CHash160 output to Span" (b92907e0b89210ecf25baa70702550f643d53525)
This commit doesn't seem to compile. Also not sure reason scriptHash.begin() is changing to scriptHash.data() above on line 102.
Fixed.
Code review ACK 675d6fd044be08cd5dc7386b5574d845a74e9aae. I think intermediate commits might not compile, but comments below aren't critical.
ACK 675d6fd044be08cd5dc7386b5574d845a74e9aae
Rebased, and addressed @ryanofsky's comments.
Code review ACK 49fc016 per git range-diff caf1766 675d6fd 49fc016. Good improvements; the main change if I'm seeing things clearly is 4679ef6 As* Spans replaced by b0cf67f9 UChar* helper functions and the refactorings they enable, and moving commit "Make script/standard's BaseHash Span-convertible" earlier in the commit order. Verified that each commit debug-builds cleanly.
Code review ACK 49fc016c78a476e687bc73adb5c32350c2e115e5. Since last review rebased added MakeUCharSpan function, rearranged commits to fix compile error, and dropped an unneeded test change
This is in preparation for exposing a ::data member function.
-BEGIN VERIFY SCRIPT-
sed -i "s/\([^.]\|other.\)data/\1m_data/g" src/uint256.h src/uint256.cpp
-END VERIFY SCRIPT-
Based on a suggestion by Russell Yanofsky.
Rebased.
re-ACK 77c507358bda9bd6c496f33e0f4418c0603bb08d
Code review re-ACK 77c5073 per git range-diff 14ceddd 49fc016 77c5073