refactor: Allow CScript construction from any std::input

maflcko commented at 10:27 am on February 2, 2024: member

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 commented at 10:27 am on February 2, 2024: contributor

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	delta1, hodlinator, ryanofsky, achow101
Stale ACK	ajtowns, hernanmarino

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

Conflicts

No conflicts as of last run.

DrahtBot added the label Refactoring on Feb 2, 2024

DrahtBot added the label CI failed on Feb 2, 2024

DrahtBot commented at 11:40 am on February 2, 2024: contributor

🚧 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 commented at 12:17 pm on February 2, 2024: none

Concept ACK dddd56c6eecfb0e0f53874355ee821f5bd6af556

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

0script/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&)’:
1script/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&)’
2  325 |     CScript scriptCode(pbegincodehash, pend);

delta1 commented at 12:21 pm on February 2, 2024: none

CI is turning up an issue in script/interpreter

looks like a few issues in that file

maflcko commented at 1:16 pm on February 2, 2024: member

@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 https://github.com/gcc-mirror/gcc/commit/186aa6304570e15065f31482e9c27326a3a6679f

delta1 commented at 1:23 pm on February 2, 2024: none

@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 commented at 4:36 pm on February 2, 2024: member

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 marked this as a draft on Feb 2, 2024

maflcko force-pushed on Feb 5, 2024

maflcko marked this as ready for review on Feb 5, 2024

maflcko commented at 10:55 am on February 5, 2024: member

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

DrahtBot removed the label CI failed on Feb 5, 2024

delta1 commented at 11:44 am on February 5, 2024: none

ACK fa9ef95

maflcko requested review from ajtowns on Feb 9, 2024

ajtowns commented at 10:50 am on February 21, 2024: contributor

utACK fa9ef95b4c1114e2d8f7c3307fdf01446efee6fd

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:

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

maflcko commented at 11:52 am on February 21, 2024: member

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.

maflcko requested review from ajtowns on Apr 1, 2024

hernanmarino commented at 11:03 pm on April 12, 2024: contributor

utACK fa40ae2dbd181b27371b91636652c9b408585384

DrahtBot added the label Needs rebase on Jul 11, 2024

refactor: Allow CScript construction from any std::input_iterator

Also, remove the value_type alias, which is not needed when element_type
is present.

d444441900

refactor: Require std::input_iterator for all InputIterator in prevector fa7b9b99a2

maflcko force-pushed on Jul 12, 2024

delta1 commented at 9:50 am on July 12, 2024: none

reACK fa7b9b9

DrahtBot requested review from hernanmarino on Jul 12, 2024

DrahtBot removed the label Needs rebase on Jul 12, 2024

hodlinator approved

hodlinator commented at 1:33 pm on August 16, 2024: contributor

ACK fa7b9b99a2ed466d1765d748f4a59c2f581b974f

Verified that it resolved the issue I was having on other PR (https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1719107583) through cherry-picking the commits in this PR, undoing the local workaround, and pushing to a personal GitHub branch 239709e194203e06bd1f0610059e00998f645d4c to see if Windows CI compiled it successfully, which it did (https://github.com/hodlinator/bitcoin/actions/runs/10419824929/job/28858657327).

nit: I’d prefer the removal of value_type from prevector was its own commit and the remaining changes were squashed into a second commit as they appear to belong closer together.

maflcko commented at 1:40 pm on August 16, 2024: member

changes were squashed into a second commit as they appear to belong closer together.

Yeah, they are related, but the prevector one adds requirements on the code, and the cscript one removes requirements on the code, which is why they are in two commits.

ryanofsky approved

ryanofsky commented at 10:21 pm on August 16, 2024: contributor

Code review ACK fa7b9b99a2ed466d1765d748f4a59c2f581b974f

maflcko commented at 6:09 am on August 20, 2024: member

rfm with 5 acks on the first commit (2 stale and 3 current), and 3 acks on the second commit?

ryanofsky commented at 2:04 pm on August 20, 2024: contributor

rfm with 5 acks on the first commit (2 stale and 3 current), and 3 acks on the second commit?

I haven’t been merging anything just because I think we are in feature freeze https://github.com/bitcoin/bitcoin/issues/29891

achow101 commented at 5:58 pm on August 27, 2024: member

ACK fa7b9b99a2ed466d1765d748f4a59c2f581b974f

achow101 merged this on Aug 27, 2024

achow101 closed this on Aug 27, 2024

theuni commented at 6:18 pm on August 27, 2024: member

Post-merge utACK fa7b9b99a2ed466d1765d748f4a59c2f581b974f. I had a similar commit while hacking on allocators a while back: https://github.com/theuni/bitcoin/commit/3d34f21931226e7c39fb0cfcfe3581c3dfa3cc0f . This is even more elegant :)

maflcko deleted the branch on Aug 29, 2024

refactor: Allow CScript construction from any std::input_iterator #29369

Code Coverage

Reviews

Conflicts