This makes use of the implicit constructions and conversions to Span introduced in #18468 to simplify the hash.h interface:
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
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.
Span<const unsigned char> as a drop-in replacement for const std::vector<unsigned char>& as parameter type is safe use of Span :)
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.
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:
0inline unsigned char* UnsignedCharCast(char* c) { return (unsigned char*)c; }
1inline unsigned char* UnsignedCharCast(unsigned char* c) { return c; }
2inline const unsigned char* UnsignedCharCast(const char* c) { return (unsigned char*)c; }
3inline const unsigned char* UnsignedCharCast(const unsigned char* c) { return c; }
4
5//! Safely convert a character array into a span of unsigned chars.
6template<typename T>
7Span<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.
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.
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.
git range-diff 14ceddd 49fc016 77c5073