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

pull lucash-dev wants to merge 2 commits into bitcoin:master from lucash-dev:explicit-CMutableTransaction-conversion changing 7 files +11 −11
  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. lucash-dev cross-referenced this on Dec 10, 2018 from issue refactor: Make explicit CMutableTransaction -> CTransaction conversion. by lucash-dev
  4. fanquake added the label Refactoring on Dec 10, 2018
  5. lucash-dev cross-referenced this on Dec 10, 2018 from issue test: Removed implicit CTransaction constructor calls from tests and benchmarks. by lucash-dev
  6. 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

  7. DrahtBot commented at 9:50 AM on December 10, 2018: contributor

    <!--e57a25ab6845829454e8d69fc972939a-->

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

    <!--174a7506f384e20aa4161008e828411d-->

    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.

  8. 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.

  9. 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:

    👀

  10. promag commented at 2:18 PM on December 10, 2018: member

    Looks like you are missing a rebase.

  11. MarcoFalke referenced this in commit 6d0a14703e on Dec 12, 2018
  12. lucash-dev commented at 6:44 AM on December 13, 2018: contributor

    Rebased

  13. 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. :-) )

  14. 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
  15. 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
  16. 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.

  17. MarcoFalke commented at 5:01 PM on December 22, 2018: member

    utACK b301950df32443e358bc22ca22c6f9ac09d18219

  18. practicalswift commented at 4:52 PM on January 5, 2019: contributor

    utACK b301950df32443e358bc22ca22c6f9ac09d18219

  19. laanwj commented at 7:28 PM on January 21, 2019: member

    utACK b301950df32443e358bc22ca22c6f9ac09d18219, agree with the rationale

  20. laanwj merged this on Jan 21, 2019
  21. laanwj closed this on Jan 21, 2019
  22. laanwj referenced this in commit f0c9e1c22b on Jan 21, 2019
  23. ryanofsky cross-referenced this on Jan 23, 2019 from issue Refactor: separate wallet from node by ryanofsky
  24. gwillen cross-referenced this on Feb 9, 2019 from issue Factor out PSBT utilities from RPCs for use in GUI code; related refactoring. by gwillen
  25. scravy cross-referenced this on Mar 4, 2019 from issue Brainstorming: CTransaction vs CMutableTransaction by scravy
  26. wqking referenced this in commit 9dd460555d on Dec 25, 2019
  27. deadalnix referenced this in commit d585725227 on May 16, 2020
  28. kwvg referenced this in commit 534746cfe6 on Oct 11, 2021
  29. kwvg referenced this in commit 22df829e29 on Oct 11, 2021
  30. kwvg referenced this in commit 98c7f23f58 on Oct 11, 2021
  31. kwvg referenced this in commit 65ca146b51 on Oct 12, 2021
  32. kwvg referenced this in commit ee705af2cd on Oct 12, 2021
  33. UdjinM6 referenced this in commit 2a5b5bb32a on Oct 13, 2021
  34. pravblockc referenced this in commit ccb6dadd0c on Nov 18, 2021
  35. bitcoin locked this on Dec 16, 2021
Contributors
lucash-devDrahtBotpromaggwillenMarcoFalkepracticalswiftlaanwj
Labels
Refactoring
Linked (view graph)
#10973 Refactor: separate wallet from node#14156 refactor: Make explicit CMutableTransaction -> CTransaction conversion.#14908 test: Removed implicit CTransaction constructor calls from tests and benchmarks.#14978 Factor out PSBT utilities from RPCs for use in GUI code; related refactoring.#15525 Brainstorming: CTransaction vs CMutableTransaction
github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-05-19 06:54 UTC