Make CTransaction(const CMutableTransaction &tx) explicit #5054

1. 
   jtimon commented at 11:40 pm on October 6, 2014: contributor

   These implicit castings are dangerous. They exploded in my face several times when coding #4989 . I’ll post a version of that PR rebased on top of this one that will be slightly more memory efficient (I’ll be able to remove txAux attribute and the constructor using CMutableTransaction).
2. 
   laanwj commented at 6:58 am on October 7, 2014: member

   These implicit castings are dangerous.

   Can you explain why?

3. 
   jtimon commented at 8:59 am on October 7, 2014: contributor

   The implicit casting can cause hard to find segmentation faults. Any function or method (including constructors) that takes a CTransaction (or a reference to it) as parameter will create a temporary instance using the implicit constructor from a CMutableTransaction. That instance is only in memory during the execution of the function and gets destroyed just after it. Any later access to a reference of the temporary CTransaction should raise an unauthorized memory access exception. That would be the case in #4989 if it wasn’t for

   0    const CTransaction txAux;
   1    SignatureHasher(const CMutableTransaction& txToIn, unsigned int nInIn) : txAux(txToIn), txTo(this->txAux), nIn(nInIn) { }
   

   https://github.com/jtimon/bitcoin/commit/dfd5c6d85b4b613a42581488fdf5ef918e47775d#diff-b81dfdd8a5bd80fe9f82b5a40c4c991eR45

   The auxiliary variable and constructor could be eliminated if there was a guarantee that the Constructor will always be called with a CTransaction& and never a CMutationCTransaction that gets implicitly copied to a temporary CTransaction. That guarantee can be achieved with the “explicit” keyword. http://stackoverflow.com/questions/121162/what-does-the-explicit-keyword-in-c-mean

4. 
   laanwj commented at 9:06 am on October 7, 2014: member

   Indeed, the parameter will create a temporary instance using the implicit constructor. It will have the scope of the current statement only.

   Isn’t the root issue then that some functions assume that a const reference that is passed has a longer validity than the function call? Not the implicit conversion itself?

   Ie if a function takes a const std::string &, this could be a temporary converted from a const char*. There is nothing inherently dangerous in this except if the callee assumes that the caller will keep the reference alive instead of making a copy.

   IMO if the goal is to pass a longer-living structure, I don’t think references are the right thing to use. Using a pointer will avoid any implicit conversions, and documents that something potentially dangerous is going on (and you need reference counting, or ownership transfer, or at least make it clear what the scope is). Or if acceptable, just make a copy.

5. 
   jtimon commented at 9:29 pm on October 7, 2014: contributor

   What you say makes sense, but creating an additional copy has some cost that I would like to save when it’s not necessary. Maybe this only makes sense after #4989. @sipa told me that he introduced that constructor without the explicit keyword to minimize the diff but leave it as something to do later.
6. Make CTransaction(const CMutableTransaction &tx) explicit 8a87099fa9
7. jtimon force-pushed on Oct 8, 2014
8. 
   laanwj commented at 6:46 am on October 8, 2014: member

   So use a pointer, then. I think no matter what, assuming that a reference that is passed to a function outlives that function is wrong. Implicit conversions are just a fact of life with references. You can make one constructor explicit, but this will keep biting you as long as you use C++.
9. 
   jtimon commented at 10:33 am on October 8, 2014: contributor

   I tried to do it with pointers and castings instead of implicit constructors but the tests keep failing " memory access violation" https://github.com/jtimon/bitcoin/tree/ptrsighash

   We could make all constructors with a single parameter explicit. We could even make that a style rule. If we did, when we “assume that a reference that is passed to a function outlives that function” either by mistake or due to a conscientious decision based purely on performance (and no, just like performance is not always more important than appropriate semantics, style is not always more important than performance). And although in my preferred language “explicit is better than implicit”, I don’t think it’s fair to blame C++ in this case when it provides a specific keyword to avoid the explosions. In any case, I will use a copy in #4989 and its continuations as suggested, maintaining that branch independent from this one (I’m mostly reffering to https://github.com/jtimon/bitcoin/tree/libscriptpreview a rewrite of #4809 on top of this and the latest changes). The penalty shouldn’t be very big anyway and it can always be optimized later. After rewriting it without this, it should be easier to review as well.

   But if you don’t mind, I will keep this open for now to hear more feedback.

10. 
    laanwj commented at 11:25 am on October 8, 2014: member

    I am not blaming C++. I’m just saying that temporaries are something to be expected if you use (const) references. You can try to change the world around it, or just accept it and use a style that is not conductive to a kind of error. Even if you make sure that all constructors in bitcoin core itself are explicit, there could still be an implicit conversion through boost, or the standard library (like the std::string/const char* I mention above).

    (to be clear: there are very good reasons for making constructors explicit, for example if the goal of the constructor is not to convert between types, or when the constructor does something unsafe to the object passed in. But CMutableTranscaction->CTransaction seems to me to be the kind of conversion that makes sense, and making that explicit adds a lot of verbosity to the code)

11. 
    jtimon commented at 4:58 pm on October 28, 2014: contributor

    Avoiding a copy is probably always an optimization. Your rule of simply not using constructors that use a const ref and expect it to remain in memory makes a lot of sense to me. I wasn’t misunderstanding explicit, I was misunderstanding const&. Thanks for the explanations, closing (and sorry for not having closing it earlier).
12. jtimon closed this on Oct 28, 2014

    ---

13. MarcoFalke locked this on Sep 8, 2021

Contributors
jtimon laanwj