test: Removed implicit CTransaction constructor calls from tests and benchmarks. #14908

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

   This PR was split from #14906 and is a prerequisite for it. It updates tests and benchmarks, removing all implicit calls to CTransaction(CMutableTransaction&) constructors. This will make possible making the constructor explicit in the next PR. The original rationale for making the constructor explicit:

   • 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. fanquake added the label Tests on Dec 10, 2018
3. 
   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:

   • #14935 (tests: Test for expected return values when calling functions returning a success code by practicalswift)
   • #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)

   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.

4. in src/bench/ccoins_caching.cpp:79 in 001ee98418 outdated

   75@@ -76,10 +76,11 @@ static void CCoinsCaching(benchmark::State& state)
   76     t1.vout[0].scriptPubKey << OP_1;
   77 
   78     // Benchmark.
   79+    CTransaction cTx(t1);
   

   ---

   ---

   

   MarcoFalke commented at 4:16 pm on December 10, 2018:

   can be const and shout be snake case? tx_1?
5. in src/bench/mempool_eviction.cpp:131 in 001ee98418 outdated

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

   ---

   ---

   

   MarcoFalke commented at 4:17 pm on December 10, 2018:

   Couldn’t this just use tx1_r?
6. 
   practicalswift commented at 7:24 pm on December 10, 2018: contributor

   Concept ACK

   Explicit is better than implicit – especially when it comes to conversions.

7. Removed implicit CTransaction constructor from tests ed61abedb2
8. Removed implicit CTransaction conversion from benchmaks 8db0c3d42b
9. 
   jb55 commented at 8:38 am on December 12, 2018: member

   explicit utACK 8db0c3d42b063118d17ab83ba8beeb3852f8fc6e
10. MarcoFalke merged this on Dec 12, 2018
11. MarcoFalke closed this on Dec 12, 2018

    ---

12. MarcoFalke referenced this in commit 6d0a14703e on Dec 12, 2018
13. MarcoFalke locked this on Sep 8, 2021

Contributors
lucash-dev DrahtBot MarcoFalke practicalswift jb55

Labels
Tests