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

lucash-dev · 2018-09-06T01:38:31Z

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

jb55 · 2018-09-06T04:42:14Z

Concept ACK

scravy

utACK e4c33899e12bde83b92026a2f310333d76d87939

promag · 2018-09-06T13:34:40Z

This particular conversion is very costly

Agree that for these cases an explicit constructor is preferable.

utACK e4c3389.

maflcko · 2018-09-06T15:37:38Z

For most cases the performance should not matter (e.g. unit tests or benches where the conversion is not in a hot path. Also, for the rpc interface or tx-tools, where the overhead of communication dwarfs the speedup of skipping the conversion). For other cases, we might want to accept either a mutable or immuatable tx. See e.g. #13359 or #13309

lucash-dev · 2018-09-06T15:48:16Z

@MarcoFalke I agree with you that in most cases it won't make a difference. But I think having it implicit makes it harder to reason about the cases when it's important.

maflcko · 2018-09-06T15:53:19Z

I think the only case where it could potentially matter is src/wallet/wallet.cpp?

But agree that it makes sense to mark it explicit, so that the other cases are easily visible. Though, might still want to submit the other prs first that do the cleanup you had planned afterward. This way we avoid having to touch the lines twice.

If you wanted to have this merged first, at the very least, you'd have to reorder the two commits, since the first one doesn't compile on its own.

lucash-dev · 2018-09-06T16:41:25Z

@MarcoFalke
about commit ordering: of course, will fix it.

I'll take a loot at src/wallet/wallet.cpp and possibly create a separate PR to be merged before this one.

I was just thinking, other changes that are just clean-up might make more sense to go as commits in this PR, instead of new PRs, since they wouldn't make that much sense to get merged if this one doesn't get merged in the end. (this doesn't apply to wallet.cpp).

I'll mark this PR as WIP meanwhile.

lucash-dev · 2018-09-10T01:16:01Z

Fixed commit order.

lucash-dev · 2018-11-04T01:20:25Z

Rebased.

DrahtBot · 2018-11-09T09:24:49Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#14505 (Make single parameter constructors explicit (C++11). Add explicit constructor linter. by practicalswift)
#13868 (Remove unused fScriptChecks parameter from CheckInputs by Empact)
#13525 (Report reason inputs are nonstandard from AreInputsStandard by Empact)
#12911 (wallet: Show fee in results for signrawtransaction* for segwit inputs by kallewoof)
#10973 (Refactor: separate wallet from node by ryanofsky)

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.

practicalswift · 2018-12-06T21:51:06Z

I'm afraid this PR doesn't compile when rebase on current master.

lucash-dev · 2018-12-07T04:55:24Z

I'm afraid this PR doesn't compile when rebase on current master.

That's because #14588 introduced an unnecessary implicit conversion from CMutableTransaction to CTransaction and back again.

That's precisely the kind of mistake this PR is intended to prevent.

Added commit 4f9a39b8720ba8703962341474ff74bc4be5ff48 fixing it.

maflcko · 2018-12-07T16:48:04Z

src/bench/mempool_eviction.cpp

@@ -127,7 +127,7 @@ static void MempoolEviction(benchmark::State& state)
        AddTx(tx6_r, 1100LL, pool);
        AddTx(tx7_r, 9000LL, pool);
        pool.TrimToSize(pool.DynamicMemoryUsage() * 3 / 4);
-        pool.TrimToSize(GetVirtualTransactionSize(tx1));
+        pool.TrimToSize(GetVirtualTransactionSize(CTransaction(tx1)));


Should probably move out of the hot loop (see comment above) and submit as separate pull request, since it changes the benchmark

maflcko · 2018-12-07T16:49:33Z

src/test/txvalidationcache_tests.cpp


        std::vector<CScriptCheck> scriptchecks;
        // Make sure this transaction was not cached (ie because the first
        // input was valid)
-        BOOST_CHECK(CheckInputs(tx, state, pcoinsTip.get(), true, SCRIPT_VERIFY_P2SH | SCRIPT_VERIFY_WITNESS, true, true, txdata, &scriptchecks));
+        BOOST_CHECK(CheckInputs(CTransaction(tx), state, pcoinsTip.get(), true, SCRIPT_VERIFY_P2SH | SCRIPT_VERIFY_WITNESS, true, true, txdata, &scriptchecks));


Could submit all changes in src/test and src/wallet/test as separate pull request?

Sure. So I understand you'd like both tests and benchmark changes in a separate PR, right?

lucash-dev · 2018-12-10T06:18:39Z

@MarcoFalke github seems to have got lost with my latest force push, and automatically closed this PR. Should I open another one?

fanquake · 2018-12-10T06:34:44Z

@lucash-dev That's fine. Just point back to this PR for reference.

lucash-dev · 2018-12-10T06:45:18Z

Reincarnated it as #14906

lucash-dev · 2018-12-10T06:53:19Z

There are now two PRs (as per @MarcoFalke's request):

#14908 - Removes implicit construction of CTransaction from tests and benchmarks.
#14906 - Removes implicit construction from production code, and makes the constructor explicit.

promag · 2018-12-10T14:15:30Z

I think it makes sense as separate commits, not separate PRs — the changes in #14908 can only be verified with the changes in #14906 otherwise the first could be incomplete.

…tion conversion. b301950 Made expicit constructor CTransaction(const CMutableTransaction &tx). (lucash-dev) faf29dd Minimal changes to comply with explicit CMutableTransaction -> CTranaction conversion. (lucash-dev) Pull request description: 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). Tree-SHA512: 2427462e7211b5ffc7299dae17339d27f8c43266e0895690fda49a83c72751bd2489d4471b3993075a18f3fef25d741243e5010b2f49aeef4a9688b30b6d0631

fanquake added the Refactoring label Sep 6, 2018

fanquake changed the title ~~[refactoring] Make explicit CMutableTransaction -> CTransaction conversion.~~ refactor: Make explicit CMutableTransaction -> CTransaction conversion. Sep 6, 2018

This was referenced Sep 6, 2018

Lower default relay fees #13922

Closed

Remove unused fScriptChecks parameter from CheckInputs #13868

Merged

policy: Report reason inputs are nonstandard from AreInputsStandard #13525

Closed

WIP [bench] CCoinsView(Cache): measure various scenarios #13470

Closed

scravy approved these changes Sep 6, 2018

View reviewed changes

lucash-dev changed the title ~~refactor: Make explicit CMutableTransaction -> CTransaction conversion.~~ [WIP] refactor: Make explicit CMutableTransaction -> CTransaction conversion. Sep 6, 2018

DrahtBot mentioned this pull request Sep 7, 2018

Replace coin selection fallback strategy with Single Random Draw #13307

Closed

This was referenced Sep 11, 2018

Don’t condition on expressions known already at compile time to always evaluate to true (or false). Use assertions instead. #14201

Closed

amount: Move CAmount CENT to unit test header #14244

Merged

This was referenced Sep 20, 2018

Skip tx-rehashing on historic blocks #13098

Closed

wallet: Show fee in results for signrawtransaction* for segwit inputs #12911

Closed

Refactor: separate wallet from node #10973

Merged

DrahtBot added the Needs rebase label Sep 27, 2018

ismail120572 approved these changes Oct 30, 2018

View reviewed changes

DrahtBot removed the Needs rebase label Nov 4, 2018

lucash-dev changed the title ~~[WIP] refactor: Make explicit CMutableTransaction -> CTransaction conversion.~~ refactor: Make explicit CMutableTransaction -> CTransaction conversion. Nov 6, 2018

bitcoin deleted a comment from DrahtBot Nov 9, 2018

maflcko reviewed Dec 7, 2018

View reviewed changes

lucash-dev closed this Dec 10, 2018

lucash-dev mentioned this pull request Dec 10, 2018

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

Merged

scravy mentioned this pull request Mar 4, 2019

Brainstorming: CTransaction vs CMutableTransaction #15525

Closed

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

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

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

Uh oh!

Conversation

lucash-dev commented Sep 6, 2018

Uh oh!

jb55 commented Sep 6, 2018

Uh oh!

scravy left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

promag commented Sep 6, 2018

Uh oh!

maflcko commented Sep 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lucash-dev commented Sep 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maflcko commented Sep 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lucash-dev commented Sep 6, 2018

Uh oh!

lucash-dev commented Sep 10, 2018

Uh oh!

lucash-dev commented Nov 4, 2018

Uh oh!

DrahtBot commented Nov 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

practicalswift commented Dec 6, 2018

Uh oh!

lucash-dev commented Dec 7, 2018

Uh oh!

maflcko Dec 7, 2018

Choose a reason for hiding this comment

Uh oh!

maflcko Dec 7, 2018

Choose a reason for hiding this comment

Uh oh!

lucash-dev Dec 8, 2018

Choose a reason for hiding this comment

Uh oh!

lucash-dev commented Dec 10, 2018

Uh oh!

fanquake commented Dec 10, 2018

Uh oh!

lucash-dev commented Dec 10, 2018

Uh oh!

lucash-dev commented Dec 10, 2018

Uh oh!

promag commented Dec 10, 2018

Uh oh!

Uh oh!

scravy left a comment •

edited

Loading

maflcko commented Sep 6, 2018 •

edited

Loading

lucash-dev commented Sep 6, 2018 •

edited

Loading

maflcko commented Sep 6, 2018 •

edited

Loading

DrahtBot commented Nov 9, 2018 •

edited

Loading