[tests] improvements to slow unit tests #13050

lucash-dev · 2018-04-21T19:11:46Z

This PR speeds up a few unit tests, as a partial fix to issue #10026, with some optimizations and reducing some number of validations to sane levels:

checkinputs_test: reduced in about 50%.
CreateNewBlock_validity: reduced about 17%.
knapsack_solver_test: reduced about 30%.
test_CheckQueue_Correct_Random: about 80%

More details in each commit.

No changes should affect executable production code (though there is a change in interpreter.h, including a constant).

laanwj · 2018-04-24T09:19:10Z

Travis issue was an intermittent issue, it's passing now.

maflcko · 2018-05-30T14:24:14Z

src/test/transaction_tests.cpp

@@ -449,10 +449,17 @@ BOOST_AUTO_TEST_CASE(test_big_witness_transaction) {
        mtx.vout[i].scriptPubKey = CScript() << OP_1;
    }

+    // create a single CTransaction to be used in generating all signatures
+    CTransaction txToConst(mtx);


Just noting that this is redundant (and not required after #13309)

lucash-dev · 2018-05-31T02:14:29Z

Reversed commit that was redundant with #13309 .

6b8b63a Generic TransactionSignatureCreator works with both CTransaction and CMutableTransaction (Martin Ankerl) Pull request description: Refactored `TransactionSignatureCreator` into a templated `GenericTransactionSignatureCreator` that works with both `CMutableTransaction` and `CTransaction`. The advantage is that now in `SignSignature`, the `MutableTransactionSignatureCreator` can now operate directly with the `CMutableTransaction` without the need to copy the data into a `CTransaction`. Running all unit tests brings a very noticable speedup on my machine: 48.4 sec before this change 36.4 sec with this change -------- 12.0 seconds saved running only `--run_test=transaction_tests/test_big_witness_transaction`: 16.7 sec before this change 5.9 sec with this change -------- 10.8 seconds saved This relates to my first attempt with the const_cast hack #13202, and to the slow unit test issue #10026. Also see #13050 which modifies the tests but not the production code (like this PR) to get a speedup. Tree-SHA512: 2cff0e9699f484f26120a40e431a24c8bc8f9e780fd89cb0ecf20c5be3eab6c43f9c359cde244abd9f3620d06c7c354e3b9dd3da41fa2ca1ac1e09386fea25fb

maflcko · 2018-05-31T09:52:53Z

src/test/miner_tests.cpp

        bool spendsCoinbase = i == 0; // only first tx spends coinbase
        // If we don't set the # of sig ops in the CTxMemPoolEntry, template creation fails
-        mempool.addUnchecked(hash, entry.Fee(LOWFEE).Time(GetTime()).SpendsCoinbase(spendsCoinbase).FromTx(tx));
-        tx.vin[0].prevout.hash = hash;
+        mempool.addUnchecked(hash, entry.Fee(LOWFEE).Time(GetTime()).SpendsCoinbase(spendsCoinbase).FromTx(cTx));


I believe this will calculate all hashes again, which is why I removed this method, which is why this no longer compiles.

Actually in the original code it doesn't since each CMutableTransaction is converted to a CTransaction just once, and then the hash is cached and copying the CTransaction just copies the cached hash. Removing the method that accepts CTransaction makes that optimization impossible.
Anyway, since the method is no longer there, I'll remove this commit and think of some other way of speeding up things in a separate PR.

Yes, you are right. Feel free to reintroduce the method if you think it was helpful. However, I think if you keep a vector of CTransactionRef as multisig_txs instead your patch should work just fine?

test_big_witness_transaction was spending most of time converting CMutableTransaction to Transaction, which involves recalculating hashes. This means the time complexity for providing witnesses for the 4500 inputs had a O(n2) component. This change replaces the call to SignSignature (which always creates a new CTransaction) with calls to lower level functions ProduceSignature and UpdateTransaction, which allows for reuse of a single CTransaction. This reduces execution time in about 70%.

This commit uses the a similar strategy as used for speeding up test_big_witness_transaction, that is, reusing the same CTransaction for several calls. Attains a speed up of about 15%.

…he run time reasonable. Following a suggestion in the comments, changed `ValidateCheckInputsForAllFlags` from testing all possible flag combinations to testing a random subset. Also created a new enum constant for the highest flag, so that this test doesn’t keep testing an incomplete subset in case a new flag is added.

The two tests that check for the sigops limit with 1001 multisig transactions now reuse the same transactions, instead of creating the exact same two times.

Moved the code for creating the wallet out of the 100-times repetition loop, for the most time-consuming tests.

test_CheckQueue_Correct_Random was taking too long a time, and concentrating most tests in ranges close to 100,000. Changed to a more evenly distribution between 0 and 100,000 and reduced to about 40 random amounts of checks.

…action." This reverts commit 525b2f0117e0469a6154defd985019395b53dfb5.

This reverts commit 0d1a6ab.

maflcko · 2018-06-05T19:47:18Z

src/test/txvalidationcache_tests.cpp

@@ -105,6 +105,8 @@ BOOST_FIXTURE_TEST_CASE(tx_mempool_block_doublespend, TestChain100Setup)
 static void ValidateCheckInputsForAllFlags(CMutableTransaction &tx, uint32_t failing_flags, bool add_to_cache)
 {
    PrecomputedTransactionData txdata(tx);
+    CTransaction cTx(tx);


in commit 384bcd1 ( speed up of tx_validationcache_tests by reusing of CTransaction), it seems you can instead just adjust the signature:

-static void ValidateCheckInputsForAllFlags(CMutableTransaction &tx, uint32_t failing_flags, bool add_to_cache) +static void ValidateCheckInputsForAllFlags(const CTransaction &tx, uint32_t failing_flags, bool add_to_cache)

(Might need a rebase on master)

Also, to I suggest splitting up the commits into separate pull request to make it easier to review and merge the changes independently. (You could start with this change, since it seems a clear and trivial win)

Thanks for the suggestions, Marco. I will try them, and also split the PR.

maflcko · 2018-06-05T19:48:49Z

:( GitHub is hiding my review again

lucash-dev · 2018-06-07T01:51:19Z

Closing this PR as it will be split into smaller ones.

… of CTransaction. ebebedc speed up of tx_validationcache_tests by reusing of CTransaction. (lucash.dev@gmail.com) Pull request description: The code was converting CMutableTransaction to CTransaction multiple times, which implies recalculating the hash multiple times. This commit fixes this by reusing a single CTransaction. Run-time results: ``` Before: 6.7s After: 5.5s -------------- Saved: 1.2s ``` This PR was split from #13050. Also, see #10026. Tree-SHA512: 61fb81972a08299085a7d3d0060485b265aefc7a4f82ab548e5f94371c8643cfb97bf0ef34f4e1211bf853d0217fa1c3338e4117f36fda1b37d203f690e86d60

… wallet 100 times. a679109 Speed up knapsack_solver_test by not recreating wallet 100 times. (lucash.dev@gmail.com) Pull request description: Optimization of `knapsack_solver_test`by moving an expensive wallet creation to outside a 100x for loop. On my (slow) machine: ``` before: 9.8s after: 6.2s -------------------- saved: 3.6s (36%) ``` This PR was split from #13050. Also see #10026. Tree-SHA512: bde1a856b5f076a5845e14d1a924855c8c91742c3139b47903081289b21d01fef6f2d1fd8947058728a57de56f877bab3866af8cd1d25ba2daa44411752cdb2f

…nSignature 6b8b63a Generic TransactionSignatureCreator works with both CTransaction and CMutableTransaction (Martin Ankerl) Pull request description: Refactored `TransactionSignatureCreator` into a templated `GenericTransactionSignatureCreator` that works with both `CMutableTransaction` and `CTransaction`. The advantage is that now in `SignSignature`, the `MutableTransactionSignatureCreator` can now operate directly with the `CMutableTransaction` without the need to copy the data into a `CTransaction`. Running all unit tests brings a very noticable speedup on my machine: 48.4 sec before this change 36.4 sec with this change -------- 12.0 seconds saved running only `--run_test=transaction_tests/test_big_witness_transaction`: 16.7 sec before this change 5.9 sec with this change -------- 10.8 seconds saved This relates to my first attempt with the const_cast hack bitcoin#13202, and to the slow unit test issue bitcoin#10026. Also see bitcoin#13050 which modifies the tests but not the production code (like this PR) to get a speedup. Tree-SHA512: 2cff0e9699f484f26120a40e431a24c8bc8f9e780fd89cb0ecf20c5be3eab6c43f9c359cde244abd9f3620d06c7c354e3b9dd3da41fa2ca1ac1e09386fea25fb

…reusing of CTransaction. ebebedc speed up of tx_validationcache_tests by reusing of CTransaction. (lucash.dev@gmail.com) Pull request description: The code was converting CMutableTransaction to CTransaction multiple times, which implies recalculating the hash multiple times. This commit fixes this by reusing a single CTransaction. Run-time results: ``` Before: 6.7s After: 5.5s -------------- Saved: 1.2s ``` This PR was split from bitcoin#13050. Also, see bitcoin#10026. Tree-SHA512: 61fb81972a08299085a7d3d0060485b265aefc7a4f82ab548e5f94371c8643cfb97bf0ef34f4e1211bf853d0217fa1c3338e4117f36fda1b37d203f690e86d60