[refactoring] Create common interface for CMutableTransaction and CTransaction with static polymorphism #14337

1. 
   lucash-dev commented at 4:32 am on September 27, 2018: contributor

   This PR creates a common interface for CMutableTransaction and CTransaction as the template class CBaseTransaction<T>. This base class uses the CRTP (curiously recurring template pattern) in order to achieve static polymorphism. This way this change shouldn’t have any effect on behaviour or runtime performance (though compilation performance is of course worse).

   Rationale:

   • Creates a common, explicit interface type for CTransaction and CMutableTransaction.
   • Makes explicit common elements between two classes.
   • Functions that accept have a better option than accepting an unrestricted typename T.
   • Better than proliferating custom static_assert or enable_if with different, obscure error messages when the template is applied to the wrong types.

   PS: This PR shouldn’t affect performance or behaviour, but please review thoroughly as it touches consensus-sensitive code.

2. Created a base interface for both CTransaction and CMutableTransaction.

       Created a base interface for both transaction types, using CRTP for static resolution.

   9c4d359f49
3. Refactored interpreter.h functions/classes to accept CBaseTransaction.

       A previous refactor allowed functions and classes in this unit to accept both CMutableTransaction and CTransaction, but left the template argument unrestricted.
       This change makes use of the new CBaseTransaction<T> template class to restrict the use to the classes defined in transaction.h.

   2e1c315acd
4. fanquake added the label Refactoring on Sep 27, 2018
5. 
   DrahtBot commented at 6:46 am on September 27, 2018: member

   • #14079 (Implement sighash cache in CHECKMULTISIG by jl2012)
   • #13462 (Simplify common case of CHashWriter and drop SerializeHash by Empact)
   • #13360 ([Policy] Reject SIGHASH_SINGLE with output out of bound by jl2012)
   • #13357 (Define SIGHASH_MASK in validation and determine the use of SIGHASH_SINGLE in signing by jl2012)

   If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

6. 
   DrahtBot commented at 2:03 am on September 28, 2018: member

   Coverage	Change (pull 14337)	Reference (master)
   Lines	-0.0830 %	87.0361 %
   Functions	+0.0495 %	84.1130 %
   Branches	-0.0921 %	51.5451 %

7. 
   gmaxwell commented at 10:19 pm on September 28, 2018: contributor

   The PR description doesn’t seem to state why this would be a benefit for users of the system, only why it wouldn’t be a disaster. As a result, this does not appear to justify the effort to review it for safety.
8. 
   lucash-dev commented at 0:48 am on September 29, 2018: contributor

   @gmaxwell I understand your concerns. This PR is about code maintainability and extensibility, which is beneficial to the user in the long run (if it’s a good refactoring).

   But It’s hard for me to judge the effort required of others to review it

9. in src/primitives/transaction.h:446 in 2e1c315acd

   443@@ -388,15 +444,6 @@ struct CMutableTransaction
   444      */
   445     uint256 GetHash() const;
   446 
   

   ---

   ---

   

   practicalswift commented at 7:11 am on September 29, 2018:

   Nit: Remove blank line at the end of the code block :-)
10. 
    lucash-dev commented at 11:08 pm on October 6, 2018: contributor

    @gmaxwell after a bit more thought, I actually agree with you. Closing the PR. Thank you very much for taking the time to look at it and sharing your insights.
11. lucash-dev closed this on Oct 6, 2018

    ---

12. MarcoFalke locked this on Sep 8, 2021

Contributors
lucash-dev DrahtBot gmaxwell practicalswift

Labels
Refactoring