script: Remove undocumented and unused operator+ #18612

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 e1a55690e66ca962179bc8170695b92af8a3caa8?

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 (https://github.com/bitcoin/bitcoin/pull/18612#issuecomment-612710105) is right

I think if you'd merge these two into one call for both case 0 & case 1, the fuzzer might work slightly more efficiently by not making 0 and 1 take a different code path.

I think if you'd merge these two into one call for both case 0 & case 1, the fuzzer might work slightly more efficiently by not making 0 and 1 take a different code path.

Tend to agree. If not going to collapse now could at least add a comment to avoid confusion here

Isn't the point here not to invalidate the existing seed corpus?

I mean keep case 0 and case 1, but merge them into a single branch.

case 0:
case 1:
    (void)ConsumeScript(fuzzed_data_provider);

I am planning to replace them by operator=(const& and operator=(&& respectively

Replaced by operator=(const& and operator=(&& respectively

Maybe this method should be called Opcode() instead of Add() to be consistent with the Num() method below and since it's no longer doing + operation

Will do when I rebase

Renamed to Opcode