Delete error-prone CScript constructor only used with FindAndDelete #16128

The behavior of this constructor is not the expected behavior compared to the other constructors which directly interpret the vector as a CScript, rather than serialize it into a new CScript. It has only four uses in the entire codebase. Delete this constructor and replace its four uses with the more clear serialization construction.

There is some related IRC discussion here (line 582).

<instagibbs> CScript(byte_vector) vs CScript(byte_vector.begin(), byte_vector.end()) whyyyy
<gwillen> I suspect because certain overloads didn't used to exist or something
<instagibbs> I don't think anyone actually tries using the former?
<instagibbs> intentionally*
<kanzure> also, i'm still seeking topics for amsterdam meeting
<instagibbs> just my nth time running into it, in case my annoyance is justified
<kanzure> and also, does anyone want to take over luke-jr things and be a substitute luke-jr?
<gwillen> instagibbs: wait I'm confused now, which of these do you consider preferable
<gwillen> I can't tell why you'd ever want to use the longer form
<instagibbs> gwillen, the former gives you a script with the first byte being the size of the vector, IIRC
<instagibbs> the latter gives you what you'd expect :D
<gwillen> oh, that seems like a horrifying bug that should be fixed
<instagibbs> CScript is a prevectoryadayada
<gwillen> it's not even clear to me how that would happen
<sipa> gwillen: CScript(vector) is the same as CScript() << vector
<sipa> it's the script that consists of pushing vector onto the stack
<sipa> the confusion is because CScript (used to be) a subclass of vector
<sipa> so CScript(script) would be the script that pushes script onto the stack
<gwillen> mmm, that seems really confusing and errorprone
<sipa> gwillen: yes
<sipa> i've tried to touch the CScript API once and almost introduced a consensus bug
<gwillen> ahhhhhhh, heh. I see.
<gwillen> are those constructors ever used intentionally, do you know?
<gwillen> (specifically the ones I would describe as ambiguous, and used directly and not via << or the like)
<sipa> if they're actually unused we should just get rid of them
<sipa> feel free to try and remove them, but i suspect we'd have caught them already
<gwillen> yeah, understood, I am going to go poke around but you've made me afraid to touch it much now ;-)
<sipa> i also don't want to imply we're forever stuck with this confusing convention; just that yoi really need to be careful about it
<gwillen> yeah, that makes total sense and I appreciate the clarification

Me and some of Travis are seeing:

  CXX      qt/libbitcoinqt_a-recentrequeststablemodel.o
  CXX      qt/libbitcoinqt_a-sendcoinsdialog.o
qt/coincontroldialog.cpp:425:34: error: static_cast from 'std::vector<unsigned char>' to 'CScript' uses deleted function
            CTxOut txout(amount, static_cast<CScript>(std::vector<unsigned char>(24, 0)));
                                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
./script/script.h:440:14: note: candidate constructor has been explicitly deleted
    explicit CScript(const std::vector<unsigned char>& b) = delete;
             ^
qt/coincontroldialog.cpp:516:39: error: static_cast from 'std::vector<unsigned char>' to 'CScript' uses deleted function
                CTxOut txout(nChange, static_cast<CScript>(std::vector<unsigned char>(24, 0)));
                                      ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
./script/script.h:440:14: note: candidate constructor has been explicitly deleted
    explicit CScript(const std::vector<unsigned char>& b) = delete;
             ^
2 errors generated.
make[2]: *** [qt/libbitcoinqt_a-coincontroldialog.o] Error 1
make[2]: *** Waiting for unfinished jobs....
make[1]: *** [check-recursive] Error 1
make: *** [check-recursive] Error 1

@fanquake oof forgot to check QT. Will fix.

Fixed, all completing-in-time builds are passing.

~Could just drop the constructor declaration, right? “= delete” is only strictly necessary to remove a default / implicitly defined constructor.~

I think that because it's inheriting from prevector, it's a good practice to delete it so that we don't accidentally re-gain it.

Exactly.

On Sat, Jun 1, 2019, 1:54 AM Jeremy Rubin notifications@github.com wrote:

I think that because it's inheriting from prevector, it's a good practice to delete it so that we don't accidentally re-gain it.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/bitcoin/bitcoin/pull/16128?email_source=notifications&email_token=ABMAFUZGWBAN2O4QDV5QSWLPYIFIPA5CNFSM4HR3BYYKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODWWZQWI#issuecomment-497915993, or mute the thread https://github.com/notifications/unsubscribe-auth/ABMAFU2UJNXSGZA7OPLTG4LPYIFIPANCNFSM4HR3BYYA .

utACK e6915d9b0bdc1d081a8cdcd8d313de42c6b58fb3

The behavior of this constructor is not the expected behavior compared to the other constructors

Could explain what isn't expected in the OP?

ACK e6915d9.

@promag attempted to do so

How about a comment to explain that the explicitly deleted constructor does not exist, and the deletion is preventative? I confirmed as much by deleting the constructor line only, and seeing that the build failed for lack of it.

@Empact something like:

+    // We explicitly delete this constructor to reduce the confusion
+    // between prevector-inhereted version and the begin/end iterator
+    // and pointer based versions of the constructor.

?

ACK https://github.com/bitcoin/bitcoin/pull/16128/commits/e6915d9b0bdc1d081a8cdcd8d313de42c6b58fb3

Sure, here's an alternative below. Main interest is in disavowing the implication that the constructor exists prior to deletion. I think the rest could be sussed out via blame.

  // delete non-existent constructor to defend against future introduction e.g. via prevector

ACK e6915d9b0bdc1d081a8cdcd8d313de42c6b58fb3

at the risk of c-combo breaking all these ACKs, I force pushed the comment as @Empact suggested. Less future guessing by contributors :)

ACK https://github.com/bitcoin/bitcoin/pull/16128/commits/e1a55690e66ca962179bc8170695b92af8a3caa8

As this changes consensus code I'd like some more review here @gmaxwell @sipa @sdaftuar @morcos maybe

Concept and code review ACK e1a55690e66ca962179bc8170695b92af8a3caa8, but I'd like to make sure we have tests covering the FindAndDelete usage.

src/test/data/tx_(in)valid.json has script evaluation based testing of FindAndDelete

code-review ACK e1a55690e66ca962179bc8170695b92af8a3caa8