[backport#14906] refactor: Make explicit CMutableTransaction -> CTransaction conversion.
Summary:
This PR is re-submission of #14156, which was automatically closed by github (glitch?)
Original description:
This PR makes explicit the now implicit conversion constructor CTransaction(const CMutableTransaction&) in transaction.h.
Minimal changes were made elsewhere to make the code compilable. I'll follow up with other PRs to address individually refactoring functions that should have a CMutableTransaction version, or where a CTransaction should be reused.
The rationale for this change is:
Conversion constructors should not be explicit unless there's a strong reason for it (in the opinion of, for example, https://google.github.io/styleguide/cppguide.html, and https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Ro-conversion. Let me know your take on this).
This particular conversion is very costly -- it implies a serialization plus hash of the transaction.
Even though CTransaction and CMutableTransaction represent the same data, they have very different use cases and performance properties.
Making it explicit allows for easier reasoning of performance trade-offs.
There has been previous performance issues caused by unneeded use of this implicit conversion.
This PR creates a map for places to look for possible refactoring and performance gains (this benefit still holds if the PR is not merged).
Depends on D6072
This is a backport of Core PR14906
Test Plan:
ninja check-all
Reviewers: #bitcoin_abc, nakihito
Reviewed By: nakihito
Subscribers: nakihito
Differential Revision: https://reviews.bitcoinabc.org/D6073