script: Remove undocumented and unused operator+ #18612

maflcko · 2020-04-12T22:27:55Z

This operator has no documented use case and is also unused outside of test code. The test code and all other (imaginary) code that might use this operator is written more clear and concise by the existing CScript push operators for opcodes and data.

Removing the operator is also going to protect against accidentally reintroducing bugs like this 6ff5f71#diff-8458adcedc17d046942185cb709ff5c3L1135 (last time it was used).

sipa · 2020-04-13T00:08:36Z

src/script/script.h

@@ -487,15 +473,6 @@ class CScript : public CScriptBase
        return *this;
    }

-    CScript& operator<<(const CScript& b)


Deleting this is dangerous, as attempts to call it will still work, except they'll use the function above instead (CScript::operator<<(const std::vector<unsigned char>&)).

~~I think it could be made into a static check too:~~

Better solution:

CScript& operator<<(const CScript& b) = delete;

They shouldn't use the above. I tested and this is the error:

error: invalid operands to binary expression ('CScript' and 'CScript') ./script/script.h:431:14: note: candidate function not viable: no known conversion from 'CScript' to 'int64_t' (aka 'long') for 1st argument CScript& operator<<(int64_t b) { return push_int64(b); } ^ ./script/script.h:433:14: note: candidate function not viable: no known conversion from 'CScript' to 'opcodetype' for 1st argument CScript& operator<<(opcodetype opcode) ^ ./script/script.h:441:14: note: candidate function not viable: no known conversion from 'CScript' to 'const CScriptNum' for 1st argument CScript& operator<<(const CScriptNum& b) ^ ./script/script.h:447:14: note: candidate function not viable: no known conversion from 'CScript' to 'const std::vector<unsigned char>' for 1st argument CScript& operator<<(const std::vector<unsigned char>& b) ^ 1 error generated.

I think you wanted to say that the non-existent operator should be explicitly deleted just as it has been done for the constructor in e1a5569?

If yes, I have done that. The error is now

error: overload resolution selected deleted operator '<<' ./script/script.h:430:14: note: candidate function has been explicitly deleted CScript& operator<<(const CScript& b) = delete; ^ ./script/script.h:432:14: note: candidate function not viable: no known conversion from 'CScript' to 'int64_t' (aka 'long') for 1st argument CScript& operator<<(int64_t b) { return push_int64(b); } ^ ./script/script.h:434:14: note: candidate function not viable: no known conversion from 'CScript' to 'opcodetype' for 1st argument CScript& operator<<(opcodetype opcode) ^ ./script/script.h:442:14: note: candidate function not viable: no known conversion from 'CScript' to 'const CScriptNum' for 1st argument CScript& operator<<(const CScriptNum& b) ^ ./script/script.h:448:14: note: candidate function not viable: no known conversion from 'CScript' to 'const std::vector<unsigned char>' for 1st argument CScript& operator<<(const std::vector<unsigned char>& b) ^

@MarcoFalke Oh, I forgot that CScript isn't a vector anymore (but a prevector), so indeed the function above isn't viable. If we'd have an operator that pushes a prevector however (which would be implicitly added by #13062 for example), then having an explicitly deleted version for CScript still prevents it from being called.

Checked that with Span, the output looks like this:

overload resolution selected deleted operator '<<' script << s; ~~~~~~ ^ ~ ./script/script.h:435:14: note: candidate function has been explicitly deleted CScript& operator<<(const CScript& b) = delete; ^ ./script/script.h:452:14: note: candidate function CScript& operator<<(const Span& b) ^ ./script/script.h:437:14: note: candidate function not viable: no known conversion from 'const CScript' to 'int64_t' (aka 'long') for 1st argument CScript& operator<<(int64_t b) { return push_int64(b); } ^ ./script/script.h:439:14: note: candidate function not viable: no known conversion from 'const CScript' to 'opcodetype' for 1st argument CScript& operator<<(opcodetype opcode) ^ ./script/script.h:447:14: note: candidate function not viable: no known conversion from 'const CScript' to 'const CScriptNum' for 1st argument CScript& operator<<(const CScriptNum& b) ^ ./script/script.h:454:14: note: candidate function not viable: no known conversion from 'const CScript' to 'const std::vector<unsigned char>' for 1st argument CScript& operator<<(const std::vector<unsigned char>& b) ^

Not sure if you're arguing in favor or against what I'm saying, but to be clear: marking it explicitly deleted is the right approach IMO (not just because it prevents reintroducing the problem, but also to make it easy to see this PR doesn't change behavior).

I posted this for myself to remind me that your comment (#18612 (comment)) is right

src/test/fuzz/script_ops.cpp

sipa · 2020-04-13T01:35:11Z

utACK 4444902

It seems like a no-brainer to remove the +/+= if this is all they're used for. As far as the operator<< is concerned, I believe that replacing any member function with a deleted version never changes semantics, if the result still compiles.

DrahtBot · 2020-04-13T16:42:21Z

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

Conflicts

No conflicts as of last run.

practicalswift · 2020-04-13T17:13:51Z

Concept ACK

Non-existing is better than unused/undocumented :)

ryanofsky

Code review ACK 4444902

src/test/fuzz/script_ops.cpp

src/test/script_tests.cpp

maflcko · 2020-04-16T23:23:51Z

Rebased

laanwj · 2020-04-22T12:04:44Z

ACK ccccd51

… CScript.__add__ faff9e4 test: Remove unused, undocumented and misleading CScript.__add__ (MarcoFalke) Pull request description: See the corresponding pull bitcoin#18612 ACKs for top commit: laanwj: ACK faff9e4 provided it passes Travis Tree-SHA512: 5d9c4d5b6453c70b24a6960d3b42834e9b31f6dbb99ac47a6abfd85f2739d5372563e7188c22aceabeee1c37eb218bf580848356f4a77268d65f178a9419b269

ccccd51 script: Remove undocumented and unused operator+ (MarcoFalke) Pull request description: This operator has no documented use case and is also unused outside of test code. The test code and all other (imaginary) code that might use this operator is written more clear and concise by the existing CScript push operators for opcodes and data. Removing the operator is also going to protect against accidentally reintroducing bugs like this bitcoin@6ff5f71#diff-8458adcedc17d046942185cb709ff5c3L1135 (last time it was used). ACKs for top commit: laanwj: ACK ccccd51 Tree-SHA512: 43898ac77e4d9643d9f8ac6f8f65497a4f0bbb1fb5dcaecc839c3719aa36181ba77befb213e59a9f33a20a29e0173a0e9c4763b1930940b32c3d1598b3e39af9

Rewrite code that used CScript's operator+ for building name operations, following bitcoin/bitcoin#18612.

Summary: This is a backport of Core [[bitcoin/bitcoin#18612 | PR18612]] Test Plan: ``` ninja all check-all ninja bitcoin-fuzzers ``` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Subscribers: Fabien Differential Revision: https://reviews.bitcoinabc.org/D8973

This was removed in "bitcoin#18612: script: Remove undocumented and unused operator+".

ccccd51 script: Remove undocumented and unused operator+ (MarcoFalke) Pull request description: This operator has no documented use case and is also unused outside of test code. The test code and all other (imaginary) code that might use this operator is written more clear and concise by the existing CScript push operators for opcodes and data. Removing the operator is also going to protect against accidentally reintroducing bugs like this bitcoin@6ff5f71#diff-8458adcedc17d046942185cb709ff5c3L1135 (last time it was used). ACKs for top commit: laanwj: ACK ccccd51 Tree-SHA512: 43898ac77e4d9643d9f8ac6f8f65497a4f0bbb1fb5dcaecc839c3719aa36181ba77befb213e59a9f33a20a29e0173a0e9c4763b1930940b32c3d1598b3e39af9

This was removed in "bitcoin#18612: script: Remove undocumented and unused operator+".

ccccd51 script: Remove undocumented and unused operator+ (MarcoFalke) Pull request description: This operator has no documented use case and is also unused outside of test code. The test code and all other (imaginary) code that might use this operator is written more clear and concise by the existing CScript push operators for opcodes and data. Removing the operator is also going to protect against accidentally reintroducing bugs like this bitcoin@6ff5f71#diff-8458adcedc17d046942185cb709ff5c3L1135 (last time it was used). ACKs for top commit: laanwj: ACK ccccd51 Tree-SHA512: 43898ac77e4d9643d9f8ac6f8f65497a4f0bbb1fb5dcaecc839c3719aa36181ba77befb213e59a9f33a20a29e0173a0e9c4763b1930940b32c3d1598b3e39af9

…oin#18612: script: Remove undocumented and unused operator+

…oin#18612: script: Remove undocumented and unused operator+ ccccd51 script: Remove undocumented and unused operator+ (MarcoFalke) Pull request description: This operator has no documented use case and is also unused outside of test code. The test code and all other (imaginary) code that might use this operator is written more clear and concise by the existing CScript push operators for opcodes and data. Removing the operator is also going to protect against accidentally reintroducing bugs like this bitcoin@6ff5f71#diff-8458adcedc17d046942185cb709ff5c3L1135 (last time it was used). ACKs for top commit: laanwj: ACK ccccd51 Tree-SHA512: 43898ac77e4d9643d9f8ac6f8f65497a4f0bbb1fb5dcaecc839c3719aa36181ba77befb213e59a9f33a20a29e0173a0e9c4763b1930940b32c3d1598b3e39af9

ccccd51 script: Remove undocumented and unused operator+ (MarcoFalke) Pull request description: This operator has no documented use case and is also unused outside of test code. The test code and all other (imaginary) code that might use this operator is written more clear and concise by the existing CScript push operators for opcodes and data. Removing the operator is also going to protect against accidentally reintroducing bugs like this bitcoin@6ff5f71#diff-8458adcedc17d046942185cb709ff5c3L1135 (last time it was used). ACKs for top commit: laanwj: ACK ccccd51 Tree-SHA512: 43898ac77e4d9643d9f8ac6f8f65497a4f0bbb1fb5dcaecc839c3719aa36181ba77befb213e59a9f33a20a29e0173a0e9c4763b1930940b32c3d1598b3e39af9

This was removed in "bitcoin#18612: script: Remove undocumented and unused operator+".

DrahtBot added Consensus Tests labels Apr 12, 2020

sipa reviewed Apr 13, 2020

View reviewed changes

maflcko force-pushed the 2004-scriptNoAdd branch from dddde1c to 4444902 Compare April 13, 2020 01:13

sipa reviewed Apr 13, 2020

View reviewed changes

src/test/fuzz/script_ops.cpp Outdated Show resolved Hide resolved

DrahtBot mentioned this pull request Apr 13, 2020

script: Disallow silent bool -> CScript conversion #18621

Merged

maflcko removed the Tests label Apr 13, 2020

ryanofsky approved these changes Apr 13, 2020

View reviewed changes

src/test/fuzz/script_ops.cpp Outdated Show resolved Hide resolved

src/test/script_tests.cpp Outdated Show resolved Hide resolved

DrahtBot added the Needs rebase label Apr 15, 2020

script: Remove undocumented and unused operator+

ccccd51

maflcko force-pushed the 2004-scriptNoAdd branch from 4444902 to ccccd51 Compare April 15, 2020 14:02

DrahtBot removed the Needs rebase label Apr 15, 2020

maflcko closed this Apr 16, 2020

maflcko reopened this Apr 16, 2020

laanwj merged commit 19032c7 into bitcoin:master Apr 22, 2020

maflcko deleted the 2004-scriptNoAdd branch April 22, 2020 12:43

maflcko mentioned this pull request Apr 22, 2020

test: Remove unused, undocumented and misleading CScript.__add__ #18732

Merged

domob1812 added a commit to domob1812/namecoin-core that referenced this pull request Apr 27, 2020

Merge branch 'auxpow'

de280a1

Rewrite code that used CScript's operator+ for building name operations, following bitcoin/bitcoin#18612.

sanket1729 added a commit to sanket1729/bitcoin that referenced this pull request Jul 21, 2021

Reintroduce + and += for Cscript

1b0bdbb

This was removed in "bitcoin#18612: script: Remove undocumented and unused operator+".

darosior pushed a commit to darosior/bitcoin that referenced this pull request Aug 24, 2021

Reintroduce + and += for Cscript

f3d7526

This was removed in "bitcoin#18612: script: Remove undocumented and unused operator+".

vijaydasmp added a commit to vijaydasmp/dash that referenced this pull request Aug 29, 2021

Merge bitcoin#17519: rpc: Remove unused COINBASE_FLAGS and Merge bitc…

aa0e130

…oin#18612: script: Remove undocumented and unused operator+

vijaydasmp mentioned this pull request Aug 29, 2021

Merge #18612: script: Remove undocumented and unused operator+ dashpay/dash#4374

Closed

vijaydasmp added a commit to vijaydasmp/dash that referenced this pull request Aug 29, 2021

Merge bitcoin#17519: rpc: Remove unused COINBASE_FLAGS and Merge bitc…

12fef2d

…oin#18612: script: Remove undocumented and unused operator+

vijaydasmp added a commit to vijaydasmp/dash that referenced this pull request Aug 29, 2021

Merge bitcoin#17519: rpc: Remove unused COINBASE_FLAGS and Merge bitc…

079e4a3

…oin#18612: script: Remove undocumented and unused operator+

darosior pushed a commit to darosior/bitcoin that referenced this pull request Sep 29, 2021

Reintroduce + and += for Cscript

b252ef4

This was removed in "bitcoin#18612: script: Remove undocumented and unused operator+".

darosior pushed a commit to darosior/bitcoin that referenced this pull request Sep 29, 2021

Reintroduce + and += for Cscript

b6042f9

This was removed in "bitcoin#18612: script: Remove undocumented and unused operator+".

JeremyRubin mentioned this pull request Nov 10, 2021

WIP: Make CScript addition concatenative #18613

Closed

darosior pushed a commit to darosior/bitcoin that referenced this pull request Dec 8, 2021

Reintroduce + and += for Cscript

aa29321

This was removed in "bitcoin#18612: script: Remove undocumented and unused operator+".

darosior pushed a commit to darosior/bitcoin that referenced this pull request Dec 18, 2021

Reintroduce + and += for Cscript

675c8cc

This was removed in "bitcoin#18612: script: Remove undocumented and unused operator+".

darosior pushed a commit to darosior/bitcoin that referenced this pull request Jan 21, 2022

Reintroduce + and += for Cscript

9c74806

This was removed in "bitcoin#18612: script: Remove undocumented and unused operator+".

darosior pushed a commit to darosior/bitcoin that referenced this pull request Jan 25, 2022

Reintroduce + and += for Cscript

e4711c2

This was removed in "bitcoin#18612: script: Remove undocumented and unused operator+".

bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

script: Remove undocumented and unused operator+ #18612

script: Remove undocumented and unused operator+ #18612

Uh oh!

maflcko commented Apr 12, 2020

Uh oh!

sipa Apr 13, 2020 •

edited

Loading

Uh oh!

maflcko Apr 13, 2020

Uh oh!

sipa Apr 13, 2020

Uh oh!

maflcko Apr 13, 2020

Uh oh!

sipa Apr 13, 2020

Uh oh!

maflcko Apr 13, 2020

Uh oh!

Uh oh!

sipa commented Apr 13, 2020

Uh oh!

DrahtBot commented Apr 13, 2020 •

edited

Loading

Uh oh!

practicalswift commented Apr 13, 2020

Uh oh!

ryanofsky left a comment

Uh oh!

Uh oh!

Uh oh!

maflcko commented Apr 16, 2020

Uh oh!

laanwj commented Apr 22, 2020

Uh oh!

Uh oh!

script: Remove undocumented and unused operator+ #18612

script: Remove undocumented and unused operator+ #18612

Uh oh!

Conversation

maflcko commented Apr 12, 2020

Uh oh!

sipa Apr 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maflcko Apr 13, 2020

Choose a reason for hiding this comment

Uh oh!

sipa Apr 13, 2020

Choose a reason for hiding this comment

Uh oh!

maflcko Apr 13, 2020

Choose a reason for hiding this comment

Uh oh!

sipa Apr 13, 2020

Choose a reason for hiding this comment

Uh oh!

maflcko Apr 13, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sipa commented Apr 13, 2020

Uh oh!

DrahtBot commented Apr 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

practicalswift commented Apr 13, 2020

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

maflcko commented Apr 16, 2020

Uh oh!

laanwj commented Apr 22, 2020

Uh oh!

Uh oh!

sipa Apr 13, 2020 •

edited

Loading

DrahtBot commented Apr 13, 2020 •

edited

Loading