Delete error-prone CScript constructor only used with FindAndDelete

instagibbs commented at 2:07 pm on May 31, 2019: member

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.

fanquake added the label Refactoring on May 31, 2019

fanquake commented at 2:16 pm on May 31, 2019: member

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

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

fanquake commented at 6:36 pm on May 31, 2019: member

Me and some of Travis are seeing:

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

Jacekdaa approved

instagibbs commented at 6:41 pm on May 31, 2019: member

@fanquake oof forgot to check QT. Will fix.

instagibbs force-pushed on May 31, 2019

instagibbs commented at 9:12 pm on May 31, 2019: member

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

Empact commented at 5:43 am on June 1, 2019: member

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

JeremyRubin commented at 5:51 am on June 1, 2019: contributor

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.

instagibbs commented at 12:12 pm on June 1, 2019: member

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 .

MarcoFalke added the label Consensus on Jun 2, 2019

MarcoFalke commented at 8:50 am on June 9, 2019: member

utACK e6915d9b0bdc1d081a8cdcd8d313de42c6b58fb3

promag commented at 9:49 am on June 9, 2019: member

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.

instagibbs commented at 9:57 am on June 9, 2019: member

@promag attempted to do so

Empact commented at 2:09 am on June 10, 2019: member

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.

instagibbs commented at 4:02 pm on June 12, 2019: member

@Empact something like:

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

?

Empact commented at 4:24 pm on June 12, 2019: member

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.

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

jtimon commented at 11:20 am on June 13, 2019: contributor

ACK e6915d9b0bdc1d081a8cdcd8d313de42c6b58fb3

Delete error-prone CScript constructor e1a55690e6

instagibbs force-pushed on Jun 13, 2019

instagibbs commented at 1:28 pm on June 13, 2019: member

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

Empact commented at 12:43 pm on June 14, 2019: member

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

laanwj commented at 2:46 pm on June 18, 2019: member

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

sipa commented at 7:16 am on June 19, 2019: member

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

instagibbs commented at 12:50 pm on June 19, 2019: member

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

laanwj merged this on Jul 8, 2019

laanwj closed this on Jul 8, 2019

laanwj referenced this in commit c799976c86 on Jul 8, 2019

laanwj commented at 6:50 pm on July 8, 2019: member

code-review ACK e1a55690e66ca962179bc8170695b92af8a3caa8

sidhujag referenced this in commit dce2caf655 on Jul 9, 2019

jasonbcox referenced this in commit b1cbd1e298 on Oct 7, 2020

PastaPastaPasta referenced this in commit c04f2dab73 on Jun 27, 2021

PastaPastaPasta referenced this in commit 33c49b04de on Jun 28, 2021

PastaPastaPasta referenced this in commit aaa16e9940 on Jun 29, 2021

PastaPastaPasta referenced this in commit 4ea47e7d0b on Jul 1, 2021

PastaPastaPasta referenced this in commit ef8aedde0a on Jul 1, 2021

PastaPastaPasta referenced this in commit 2d26c51bc9 on Jul 12, 2021

PastaPastaPasta referenced this in commit de61840a0f on Jul 13, 2021

MarcoFalke locked this on Dec 16, 2021

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