Get rid of the const field in CTransaction#8451
Conversation
|
utACK |
|
utACK 232529f |
|
From a design point I really liked CTransaction as immutable transaction data, but yes it's better to be without the C++ hacks. |
|
Wouldn't this also negate the need for |
|
@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. |
|
@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. |
|
NACK due to sipa@232529f#commitcomment-18556288 (easily fixed). |
|
@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. |
|
@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 ( |
|
Yes, you're right. (Also see https://isocpp.org/wiki/faq/const-correctness#aliasing-and-const .) utACK. |
|
See #8580 for an alternative solution that is a better step towards making CTransaction having its own more efficient in-memory representation. |
|
Closing in favor of #8580 |
Remove the incorrect const declarations in CTransaction. Based on upstream bitcoin/bitcoin#8451, which is a much, much safer and easier-to-review fix than bitcoin/bitcoin#8580. fixes #967 Signed-off-by: Daira Hopwood <[email protected]>
6 participants
As explained on zcash/zcash#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.