refactor: Make explicit CMutableTransaction -> CTransaction conversion. #14906

1. 
   lucash-dev commented at 6:44 am on December 10, 2018: contributor

   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).
2. lucash-dev renamed this:
   Explicit c mutable transaction conversion
   refactor: Make explicit CMutableTransaction -> CTransaction conversion.
   on Dec 10, 2018
3. fanquake added the label Refactoring on Dec 10, 2018
4. 
   lucash-dev commented at 6:54 am on December 10, 2018: contributor

   The part referring to tests and benchmark changes has now a separate PR #14908
5. 
   DrahtBot commented at 9:50 am on December 10, 2018: member

   The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

   Conflicts

   Reviewers, this pull request conflicts with the following ones:

   • #14978 (Factor out PSBT utilities from RPCs for use in GUI code; related refactoring. by gwillen)
   • #12911 (wallet: Show fee in results for signrawtransaction* for segwit inputs by kallewoof)

   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. in src/qt/bitcoin.cpp:74 in 2f3d5e6cae outdated

   70@@ -71,11 +71,6 @@ Q_DECLARE_METATYPE(bool*)
   71 Q_DECLARE_METATYPE(CAmount)
   72 Q_DECLARE_METATYPE(uint256)
   73 
   74-static void InitMessage(const std::string& message)
   

   ---

   ---

   

   promag commented at 2:17 pm on December 10, 2018:

   👀

   ---

   

   lucash-dev commented at 9:21 pm on December 10, 2018:

   Definitely some rebasing weirdness.
7. in src/qt/bitcoin.cpp:561 in 2f3d5e6cae outdated

   557@@ -563,6 +558,11 @@ int main(int argc, char *argv[])
   558 
   559     std::unique_ptr<interfaces::Node> node = interfaces::MakeNode();
   560 
   561+    // Subscribe to global signals from core
   

   ---

   ---

   

   promag commented at 2:17 pm on December 10, 2018:

   👀
8. 
   promag commented at 2:18 pm on December 10, 2018: member

   Looks like you are missing a rebase.
9. MarcoFalke referenced this in commit 6d0a14703e on Dec 12, 2018
10. 
    lucash-dev commented at 6:44 am on December 13, 2018: contributor

    Rebased
11. 
    gwillen commented at 11:19 pm on December 17, 2018: contributor

    Strong support for making constructors explicit wherever possible. This looks like a fairly minimal change in terms of total amount of stuff touched, and definitely in a positive direction. Would love to see this get in. (Consider this a utACK from me, for what my vote is worth. I’m not sure what’s up with Travis but the failure doesn’t look legitimate.)

    (EDIT: Looks like I missed some comments / context due to the reopening as a new PR, and indeed there was already plenty of support for this to go in soon. Yay. :-) )

12. Minimal changes to comply with explicit CMutableTransaction -> CTranaction conversion.

    This commit makes the minimal changes necessary to fix compilation once CTransaction(const CMutableTransaction &tx) is made explicit. In each case an explicit call `CTransaction(...)` was added. Shouldn't affect behaviour or performance.

    faf29dd019
13. Made expicit constructor CTransaction(const CMutableTransaction &tx).

    This makes the above constructor explicit. The rationale is that this conversion has very significant performance effects. Making it explicit makes it easier to reason about these performance trade-offs, and helps identify possible functions that need a CMutableTransaction version.

    b301950df3
14. 
    lucash-dev commented at 5:58 am on December 18, 2018: contributor

    Rebased on latest master. Everything worked locally. Let’s see how Travis behaves.
15. 
    MarcoFalke commented at 5:01 pm on December 22, 2018: member

    utACK b301950df32443e358bc22ca22c6f9ac09d18219
16. 
    practicalswift commented at 4:52 pm on January 5, 2019: contributor

    utACK b301950df32443e358bc22ca22c6f9ac09d18219
17. 
    laanwj commented at 7:28 pm on January 21, 2019: member

    utACK b301950df32443e358bc22ca22c6f9ac09d18219, agree with the rationale
18. laanwj merged this on Jan 21, 2019
19. laanwj closed this on Jan 21, 2019

    ---

20. laanwj referenced this in commit f0c9e1c22b on Jan 21, 2019
21. wqking referenced this in commit 9dd460555d on Dec 25, 2019
22. deadalnix referenced this in commit d585725227 on May 16, 2020
23. kittywhiskers referenced this in commit 534746cfe6 on Oct 11, 2021
24. kittywhiskers referenced this in commit 22df829e29 on Oct 11, 2021
25. kittywhiskers referenced this in commit 98c7f23f58 on Oct 11, 2021
26. kittywhiskers referenced this in commit 65ca146b51 on Oct 12, 2021
27. kittywhiskers referenced this in commit ee705af2cd on Oct 12, 2021
28. UdjinM6 referenced this in commit 2a5b5bb32a on Oct 13, 2021
29. pravblockc referenced this in commit ccb6dadd0c on Nov 18, 2021
30. MarcoFalke locked this on Dec 16, 2021

Contributors
lucash-dev DrahtBot promag gwillen MarcoFalke practicalswift laanwj

Labels
Refactoring