refactor: Allow CScript construction from any std::input_iterator

[maflcko]

Currently only (pre)vector iterators and raw pointers are accepted. However, this makes it harder to construct from input iterators provided by other classes, such as std::span.

Fix that.

[DrahtBot]

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

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	hernanmarino
Stale ACK	delta1, ajtowns

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

[DrahtBot]

🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the
documentation.

Possibly this is due to a silent merge conflict (the changes in this pull request being
incompatible with the current code in the target branch). If so, make sure to rebase on the latest
commit of the target branch.

Leave a comment here, if you need help tracking down a confusing failure.

_{Debug: https://github.com/bitcoin/bitcoin/runs/21148310935}

[delta1]

Concept ACK dddd56c

Looks like a nice code change to me. "previous releases" CI is turning up an issue in script/interpreter

script/interpreter.cpp: In function ‘bool EvalChecksigPreTapscript(const valtype&, const valtype&, prevector<28, unsigned char>::const_iterator, prevector<28, unsigned char>::const_iterator, unsigned int, const BaseSignatureChecker&, SigVersion, ScriptError*, bool&)’:
script/interpreter.cpp:325:44: error: no matching function for call to ‘CScript::CScript(prevector<28, unsigned char>::const_iterator&, prevector<28, unsigned char>::const_iterator&)’
  325 |     CScript scriptCode(pbegincodehash, pend);

[delta1]

CI is turning up an issue in script/interpreter

looks like a few issues in that file

[maflcko]

@delta1 This is an issue in the C++ standard library. Basically, the standard assumes that element_type and value_type types are never provided by an iterator type at the same time. This was fixed in https://cplusplus.github.io/LWG/issue3446 and gcc-mirror/gcc@186aa63

[delta1]

@maflcko interesting, thanks for the context. does that mean that the previous releases CI job needs to be changed to use a newer gcc?

[maflcko]

Yes, either use a newer GCC, or remove the value_type type, as in C++20 it is expected to be derived from element_type via std::remove_cv_t<element_type>.

[maflcko]

Removed unused value_type, which was wrong anyway, since it was cv qualified when it shouldn't.

[delta1]

ACK fa9ef95

[ajtowns]

utACK fa9ef95

LGTM. Could change template<typename InputIterator>prevector(...) to also expect std::input_iterator.

The prevector functions that take two iterators first increase the capacity by last - first then run fill(ptr, first, last) -- that's buggy if the passed in iterators specify a greater range than a size_type (or difference_type for insert()) can hold. Could perhaps be worth changing:

-    template<typename InputIterator>
-    void fill(T* dst, InputIterator first, InputIterator last) {
-        while (first != last) {
+    template<std::input_iterator InputIterator>
+    void fill(T* dst, InputIterator first, size_type n) {
+        while (n-- > 0) {

[maflcko]

LGTM. Could change template<typename InputIterator>prevector(...) to also expect std::input_iterator.

Sure, done for all InputIterators in this file.

that's buggy if the passed in iterators specify a greater range than a size_type (or difference_type for insert()) can hold.

I think it will in all cases be buggy if the range can not be held in difference_type, as difference_type (aka int32_t) is used to represent the subtraction of two pointers in the prevector iterators (as opposed to using std::ptrdiff_t). So I think a better fix may be to use std::ptrdiff_t and the corresponding type for the size. However, I am not sure if it is needed, so it may be better to do in a separate follow-up.

[hernanmarino]

utACK fa40ae2