[refactoring] Create common interface for CMutableTransaction and CTransaction with static polymorphism #14337
Conversation
Created a base interface for both transaction types, using CRTP for static resolution.
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.
Reviewers, this pull request conflicts with the following ones:
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. |
|
|
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. |
|
@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 |
| @@ -388,15 +444,6 @@ struct CMutableTransaction | |||
| */ | |||
| uint256 GetHash() const; | |||
|
|
|||
There was a problem hiding this comment.
Nit: Remove blank line at the end of the code block :-)
|
@gmaxwell after a bit more thought, I actually agree with you. Closing the PR. |
This PR creates a common interface for
CMutableTransactionandCTransactionas the template classCBaseTransaction<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:
CTransactionandCMutableTransaction.static_assertorenable_ifwith 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.