Get rid of the const field in CTransaction #8451

1. 
   sipa commented at 1:16 am on August 4, 2016: member

   As explained on https://github.com/zcash/zcash/issues/967, using const member fields in CTransaction (or any deserializable class) is not well defined in C++, as such fields should only ever be assigned at construction time.

   I believe we can get rid of them. Their purpose was making sure that CTransaction was not used in places where mutable fields were required, but now that CMutableTransaction is well-established that requirement is not so strong anymore.

   Longer term, I’d prefer to see better encapsulation of CTransaction (which could also introduce better internal representation of the data, without a gazillion allocations) and improvements to the serialization framework so that we can construct objects by deserializing… but I think that’s further out than a fix for this.

   So at a higher level, this changes CTransaction from “immutable transaction data” to “transaction data with cached hashes”. I’ve added a small comment about this as well.

   Ping @daira @ebfull.

2. 
   ebfull commented at 2:06 am on August 4, 2016: none

   utACK
3. 
   dcousens commented at 2:18 am on August 4, 2016: contributor

   utACK 232529f
4. jonasschnelli added the label Refactoring on Aug 4, 2016
5. 
   laanwj commented at 10:10 am on August 4, 2016: member

   From a design point I really liked CTransaction as immutable transaction data, but yes it’s better to be without the C++ hacks.
6. 
   dcousens commented at 7:42 pm on August 4, 2016: contributor

   Wouldn’t this also negate the need for CMutableTransaction? (which is only used in bitcoin-tx?)
7. 
   sipa commented at 7:45 pm on August 4, 2016: member

   @dcousens It’s used in all signing code. And CMutableTransaction has the advantage of not needing to call UpdateHash all the time, so it may be useful to keep.
8. 
   sipa commented at 8:24 pm on August 4, 2016: member

   @laanwj I agree, it gave a pretty clean and safe abstraction… but I know no way to do it correctly with the current serialization framework. We should think about a construct-while-deserializing mechanism if we want that.
9. 
   daira commented at 10:07 am on August 8, 2016: contributor

   NACK due to https://github.com/sipa/bitcoin/commit/232529fbf1af2261085e45407611c32f08d5f6f3#commitcomment-18556288 (easily fixed).
10. 
    sipa commented at 8:15 pm on August 8, 2016: member

    @daira In my understanding (but I may be wrong), a reference to a const value only means that that value cannot be modified through that reference but does not mean that it cannot change overall. This is different from declaring a variable or field as const, which means it cannot change at all.
11. Get rid of the const field in CTransaction ac378f93a5
12. sipa force-pushed on Aug 16, 2016
13. 
    sipa commented at 10:05 am on August 16, 2016: member

    @daira I addressed all your comments apart from the returning of const uint256&.

    This issue made me investigate the concept of const correctness in C++ much more deeply, but all explanations I can find state that a const reference merely means that the value aliased by that reference cannot be modifed through that const reference. It is not a guarantee that the value cannot be modified at all.

    Consider the example at the bottom of http://en.cppreference.com/w/cpp/language/const_cast; a const reference (cref_i) is taken of a non-const value (i); then it is modified through the non-const reference const_cast<int&>(cref_i), and accessed through the const reference cref_i. If your reasoning was correct, this example would be undefined. The issue that this PR does fix is lower down in the same example: j is a const value, and as such no non-const aliases exist, and const casting it to non-const is therefore illegal.

14. 
    daira commented at 0:07 am on August 24, 2016: contributor

    Yes, you’re right. (Also see https://isocpp.org/wiki/faq/const-correctness#aliasing-and-const .) utACK.
15. 
    sipa commented at 11:55 am on September 19, 2016: member

    See #8580 for an alternative solution that is a better step towards making CTransaction having its own more efficient in-memory representation.
16. 
    laanwj commented at 10:09 am on September 26, 2016: member

    Closing in favor of #8580
17. laanwj closed this on Sep 26, 2016

    ---

18. zkbot referenced this in commit f9e39f293d on Jul 14, 2017
19. DrahtBot locked this on Sep 8, 2021

Contributors
sipa ebfull dcousens laanwj daira

Labels
Refactoring