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

maflcko · 2024-02-02T10:27:13Z

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 · 2024-02-02T10:27:16Z

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 · 2024-02-02T11:40:08Z

🚧 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 · 2024-02-02T12:17:17Z

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 · 2024-02-02T12:21:34Z

CI is turning up an issue in script/interpreter

looks like a few issues in that file

maflcko · 2024-02-02T13:16:35Z

@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 · 2024-02-02T13:23:40Z

@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 · 2024-02-02T16:36:58Z

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 · 2024-02-05T10:55:41Z

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

delta1 · 2024-02-05T11:44:11Z

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 · 2024-02-21T11:52:33Z

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

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

delta1 · 2024-07-12T09:50:27Z

reACK fa7b9b9

hodlinator

ACK fa7b9b9

Verified that it resolved the issue I was having on other PR (#30377 (comment)) through cherry-picking the commits in this PR, undoing the local workaround, and pushing to a personal GitHub branch 239709e 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 · 2024-08-16T13:40:11Z

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

Code review ACK fa7b9b9

maflcko · 2024-08-20T06:09:00Z

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

ryanofsky · 2024-08-20T14:04:31Z

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 #29891

achow101 · 2024-08-27T17:58:23Z

ACK fa7b9b9

theuni

Post-merge utACK fa7b9b9. I had a similar commit while hacking on allocators a while back: theuni@3d34f21 . This is even more elegant :)

8756ccd scripted-diff: Replace ParseHex[<std::byte>]("str") -> "str"_hex[_u8] (Hodlinator) 9cb6873 refactor: Prepare for ParseHex -> ""_hex scripted-diff (Hodlinator) 50bc017 refactor: Hand-replace some ParseHex -> ""_hex (Hodlinator) 5b74a84 util: Add consteval ""_hex[_v][_u8] literals (l0rinc) dc5f6f6 test refactor: util_tests - parse_hex clean up (Hodlinator) 2b5e6ef refactor: Make XOnlyPubKey tolerate constexpr std::arrays (Hodlinator) 403d86f refactor: vector -> span in CCrypter (Hodlinator) bd0830b refactor: de-Hungarianize CCrypter (Hodlinator) d99c816 refactor: Improve CCrypter related lines (Hodlinator) 7e1d9a8 refactor: Enforce lowercase hex digits for consteval uint256 (Hodlinator) Pull request description: Motivation: * Validates and converts the hex string into bytes at compile time instead of at runtime like `ParseHex()`. * Eliminates runtime dependencies: #30377 (comment), #30048 (comment) * Has stricter requirements than `ParseHex()` (disallows whitespace and uppercase hex digits) and replaces it in a bunch of places. * Makes it possible to derive other compile time constants. * Minor: should shave off a few runtime CPU cycles. `""_hex` produces `std::array<std::byte>` as the momentum in the codebase is to use `std::byte` over `uint8_t`. Also makes `uint256` hex string constructor disallow uppercase hex digits. Discussed: #30560 (comment) Surprisingly does not change the size of the Guix **bitcoind** binary (on x86_64-linux-gnu) by 1 single byte. Spawned already merged PRs: #30436, #30482, #30532, #30560. ACKs for top commit: l0rinc: ACK 8756ccd stickies-v: Rebase re-ACK 8756ccd, no changes since a096215 ryanofsky: Code review ACK 8756ccd, just rebasing since last review and taking advantage of CScript constructors in #29369, also tweaking a code comment Tree-SHA512: 9b2011b7c37e0ef004c669f8601270a214b388916316458370f5902c79c2856790b1b2c7c123efa65decad04886ab5eff95644301e0d84358bb265cf1f8ec195

DrahtBot added Refactoring CI failed labels Feb 2, 2024

maflcko marked this pull request as draft February 2, 2024 16:53

maflcko force-pushed the 2402-script-input-iterator- branch from dddd56c to fa9ef95 Compare February 5, 2024 10:13

maflcko marked this pull request as ready for review February 5, 2024 10:54

DrahtBot removed the CI failed label Feb 5, 2024

maflcko requested a review from ajtowns February 9, 2024 11:46

ajtowns reviewed Feb 21, 2024

View reviewed changes

maflcko requested a review from ajtowns April 1, 2024 14:58

hernanmarino reviewed Apr 12, 2024

View reviewed changes

DrahtBot mentioned this pull request Jul 8, 2024

refactor: modernize-use-equals-default #30406

Merged

DrahtBot added the Needs rebase label Jul 11, 2024

MarcoFalke added 2 commits July 12, 2024 11:18

refactor: Allow CScript construction from any std::input_iterator

d444441

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

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

fa7b9b9

maflcko force-pushed the 2402-script-input-iterator- branch from fa40ae2 to fa7b9b9 Compare July 12, 2024 09:33

DrahtBot requested a review from hernanmarino July 12, 2024 09:50

DrahtBot removed the Needs rebase label Jul 12, 2024

maflcko mentioned this pull request Aug 16, 2024

refactor: Replace ParseHex with consteval ""_hex literals #30377

Merged

hodlinator approved these changes Aug 16, 2024

View reviewed changes

ryanofsky approved these changes Aug 16, 2024

View reviewed changes

achow101 merged commit 99b06b7 into bitcoin:master Aug 27, 2024

theuni reviewed Aug 27, 2024

View reviewed changes

maflcko deleted the 2402-script-input-iterator- branch August 29, 2024 06:30

bitcoin locked and limited conversation to collaborators Aug 29, 2025

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

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

Uh oh!

Conversation

maflcko commented Feb 2, 2024

Uh oh!

DrahtBot commented Feb 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage

Reviews

Conflicts

Uh oh!

DrahtBot commented Feb 2, 2024

Uh oh!

delta1 commented Feb 2, 2024

Uh oh!

delta1 commented Feb 2, 2024

Uh oh!

maflcko commented Feb 2, 2024

Uh oh!

delta1 commented Feb 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maflcko commented Feb 2, 2024

Uh oh!

maflcko commented Feb 5, 2024

Uh oh!

delta1 commented Feb 5, 2024

Uh oh!

ajtowns left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko commented Feb 21, 2024

Uh oh!

hernanmarino left a comment

Choose a reason for hiding this comment

Uh oh!

delta1 commented Jul 12, 2024

Uh oh!

hodlinator left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko commented Aug 16, 2024

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko commented Aug 20, 2024

Uh oh!

ryanofsky commented Aug 20, 2024

Uh oh!

achow101 commented Aug 27, 2024

Uh oh!

theuni left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

DrahtBot commented Feb 2, 2024 •

edited

Loading

delta1 commented Feb 2, 2024 •

edited

Loading