refactor: Make explicit CMutableTransaction -> CTransaction conversion.

lucash-dev commented at 1:38 am on September 6, 2018: contributor

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

fanquake added the label Refactoring on Sep 6, 2018

fanquake renamed this:
~~[refactoring] Make explicit CMutableTransaction -> CTransaction conversion.~~
refactor: Make explicit CMutableTransaction -> CTransaction conversion.
on Sep 6, 2018

jb55 commented at 4:42 am on September 6, 2018: member

Concept ACK

scravy approved

scravy commented at 12:39 pm on September 6, 2018: contributor

utACK e4c33899e12bde83b92026a2f310333d76d87939

promag commented at 1:34 pm on September 6, 2018: member

This particular conversion is very costly

Agree that for these cases an explicit constructor is preferable.

utACK e4c3389.

MarcoFalke commented at 3:37 pm on September 6, 2018: member

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 commented at 3:48 pm on September 6, 2018: contributor

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

MarcoFalke commented at 3:53 pm on September 6, 2018: member

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 commented at 4:41 pm on September 6, 2018: contributor

@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 renamed this:
~~refactor: Make explicit CMutableTransaction -> CTransaction conversion.~~
[WIP] refactor: Make explicit CMutableTransaction -> CTransaction conversion.
on Sep 6, 2018

lucash-dev commented at 1:16 am on September 10, 2018: contributor

Fixed commit order.

DrahtBot added the label Needs rebase on Sep 27, 2018

ismail120572 approved

DrahtBot removed the label Needs rebase on Nov 4, 2018

lucash-dev commented at 1:20 am on November 4, 2018: contributor

Rebased.

lucash-dev renamed this:
~~[WIP] refactor: Make explicit CMutableTransaction -> CTransaction conversion.~~
refactor: Make explicit CMutableTransaction -> CTransaction conversion.
on Nov 6, 2018

DrahtBot commented at 9:24 am on November 9, 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:

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

fanquake deleted a comment on Nov 9, 2018

practicalswift commented at 9:51 pm on December 6, 2018: contributor

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

lucash-dev commented at 4:55 am on December 7, 2018: contributor

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.

in src/bench/mempool_eviction.cpp:130 in 96ac2de14c outdated

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

MarcoFalke commented at 4:48 pm on December 7, 2018:

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

in src/test/txvalidationcache_tests.cpp:365 in 96ac2de14c outdated

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

MarcoFalke commented at 4:49 pm on December 7, 2018:

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

lucash-dev commented at 0:28 am on December 8, 2018:

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

lucash-dev closed this on Dec 10, 2018

lucash-dev commented at 6:18 am on December 10, 2018: contributor

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

fanquake commented at 6:34 am on December 10, 2018: member

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

lucash-dev commented at 6:45 am on December 10, 2018: contributor

Reincarnated it as #14906

lucash-dev commented at 6:53 am on December 10, 2018: contributor

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 commented at 2:15 pm on December 10, 2018: member

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.

laanwj referenced this in commit f0c9e1c22b on Jan 21, 2019

DrahtBot locked this on Sep 8, 2021

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

Conflicts