Directly operate with CMutableTransaction in SignSignature #13309

martinus · 2018-05-23T06:41:26Z

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.

promag · 2018-05-23T08:57:38Z

Nice improvement, concept ACK.

Empact · 2018-05-23T09:19:50Z

src/script/interpreter.h

@@ -124,7 +124,8 @@ struct PrecomputedTransactionData
    uint256 hashPrevouts, hashSequence, hashOutputs;
    bool ready = false;

-    explicit PrecomputedTransactionData(const CTransaction& tx);
+    template<class T>
+    PrecomputedTransactionData(const T& tx);


Why drop explicit here? I don't see changes to the call sites.

you are right, I probably should not drop that.

Empact · 2018-05-23T09:25:18Z

Concept ACK

promag · 2018-05-23T09:34:23Z

src/script/interpreter.h

@@ -160,10 +162,11 @@ class BaseSignatureChecker
    virtual ~BaseSignatureChecker() {}
 };

-class TransactionSignatureChecker : public BaseSignatureChecker
+template<class T>
+class GenericTransactionSignatureChecker : public BaseSignatureChecker


nit, drop Generic prefix?

I've added it so that the names TransactionSignatureChecker and MutableTransactionSignatureChecker are still valid as used before

promag · 2018-05-23T11:32:33Z

utACK after squash.

practicalswift · 2018-05-23T14:31:13Z

Very nice!

Concept ACK

sipa · 2018-05-24T17:58:42Z

src/script/sign.h

-/** A signature creator for transactions. */
-class TransactionSignatureCreator : public BaseSignatureCreator {
-    const CTransaction* txTo;
+template <class T>


You don't actually need a template here; all existing uses of TransactionSignatureCreator become more efficient to rewrite using MutableTransactionSignatureCreator (intuitively this makes sense - you can't add a signature to an immutable transaction):

diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 2a2f8b5b20..c2b1915b11 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -2608,7 +2608,6 @@ bool CWallet::SignTransaction(CMutableTransaction &tx) AssertLockHeld(cs_wallet); // mapWallet // sign the new tx - CTransaction txNewConst(tx); int nIn = 0; for (const auto& input : tx.vin) { std::map<uint256, CWalletTx>::const_iterator mi = mapWallet.find(input.prevout.hash); @@ -2618,7 +2617,7 @@ bool CWallet::SignTransaction(CMutableTransaction &tx) const CScript& scriptPubKey = mi->second.tx->vout[input.prevout.n].scriptPubKey; const CAmount& amount = mi->second.tx->vout[input.prevout.n].nValue; SignatureData sigdata; - if (!ProduceSignature(*this, TransactionSignatureCreator(&txNewConst, nIn, amount, SIGHASH_ALL), scriptPubKey, sigdata)) { + if (!ProduceSignature(*this, MutableTransactionSignatureCreator(&tx, nIn, amount, SIGHASH_ALL), scriptPubKey, sigdata)) { return false; } UpdateTransaction(tx, nIn, sigdata); @@ -3040,14 +3039,13 @@ bool CWallet::CreateTransaction(const std::vector<CRecipient>& vecSend, CTransac if (sign) { - CTransaction txNewConst(txNew); int nIn = 0; for (const auto& coin : selected_coins) { const CScript& scriptPubKey = coin.txout.scriptPubKey; SignatureData sigdata; - if (!ProduceSignature(*this, TransactionSignatureCreator(&txNewConst, nIn, coin.txout.nValue, SIGHASH_ALL), scriptPubKey, sigdata)) + if (!ProduceSignature(*this, MutableTransactionSignatureCreator(&txNew, nIn, coin.txout.nValue, SIGHASH_ALL), scriptPubKey, sigdata)) { strFailReason = _("Signing transaction failed"); return false;

martinus · 2018-05-27T11:16:04Z

As suggested by @sipa I have now removed the template for MutableTransactionSignatureCreator and used it directly in wallet.cpp.
Also I ran the format script and squashed everything.

sipa · 2018-05-27T19:48:00Z

utACK d4ee6530535753292f73a8096a714b1be80f729f

Empact · 2018-05-29T02:11:51Z

src/script/sign.h

    unsigned int nIn;
    int nHashType;
    CAmount amount;
-    const TransactionSignatureChecker checker;
+    const GenericTransactionSignatureChecker<CMutableTransaction> checker;


You can use MutableTransactionSignatureChecker here.

maflcko

utACK d4ee653053 beside style-nits

maflcko · 2018-05-29T17:24:55Z

src/script/interpreter.cpp

+    CTransactionSignatureSerializer(const T& txToIn, const CScript& scriptCodeIn, unsigned int nInIn, int nHashTypeIn) : txTo(txToIn), scriptCode(scriptCodeIn), nIn(nInIn),
+                                                                                                                         fAnyoneCanPay(!!(nHashTypeIn & SIGHASH_ANYONECANPAY)),
+                                                                                                                         fHashSingle((nHashTypeIn & 0x1f) == SIGHASH_SINGLE),
+                                                                                                                         fHashNone((nHashTypeIn & 0x1f) == SIGHASH_NONE) {}


style-nit: you can move the : to the next line to avoid excessive white space (which looks confusing in the diff and in editors with limited width.)
sytle-nit: you could keep the empty new line between the constructor and the method to keep the diff minimal.

maflcko · 2018-05-29T17:26:38Z

src/script/sign.cpp

-TransactionSignatureCreator::TransactionSignatureCreator(const CTransaction* txToIn, unsigned int nInIn, const CAmount& amountIn, int nHashTypeIn) : txTo(txToIn), nIn(nInIn), nHashType(nHashTypeIn), amount(amountIn), checker(txTo, nIn, amountIn) {}
-
-bool TransactionSignatureCreator::CreateSig(const SigningProvider& provider, std::vector<unsigned char>& vchSig, const CKeyID& address, const CScript& scriptCode, SigVersion sigversion) const
+MutableTransactionSignatureCreator::MutableTransactionSignatureCreator(const CMutableTransaction* txToIn, unsigned int nInIn, const CAmount& amountIn, int nHashTypeIn) : txTo(txToIn), nIn(nInIn), nHashType(nHashTypeIn), amount(amountIn), checker(txTo, nIn, amountIn) {}


sytle-nit: you could keep the empty new line between the constructor and the method to keep the diff minimal?

maflcko · 2018-05-29T17:30:11Z

Tagged with "refactoring" since the only change should be removing the redundant member MutableTransactionSignatureCreator::tx

…CMutableTransaction Templated version so that no copying of CMutableTransaction into a CTransaction is necessary. This speeds up the test case transaction_tests/test_big_witness_transaction from 7.9 seconds to 3.1 seconds on my machine.

martinus · 2018-05-30T14:04:14Z

In 6b8b63a I've fixed all the nits, tried to keep the diff minimal, and squashed it again

maflcko · 2018-05-30T14:20:22Z

re-utACK 6b8b63a

sipa · 2018-05-30T22:56:42Z

re-utACK 6b8b63a

lucash-dev · 2018-05-31T02:24:58Z

Tested locally with similar speed-up. Pretty cool. ACK 6b8b63a.

fanquake · 2018-05-31T07:08:30Z

master (472fe8a):

$ time src/test/test_bitcoin --run_test=transaction_tests/test_big_witness_transaction
Running 1 test case...
*** No errors detected
real	0m20.634s
user	0m21.253s
sys	0m0.163s

$ time src/test/test_bitcoin
Running 291 test cases...
*** No errors detected
real	1m23.903s
user	0m56.010s
sys	0m40.136s

This PR (6b8b63a):

time src/test/test_bitcoin --run_test=transaction_tests/test_big_witness_transaction
Running 1 test case...
*** No errors detected
real	0m4.316s
user	0m4.961s
sys	0m0.157s

time src/test/test_bitcoin
Running 291 test cases...
*** No errors detected
real	1m7.374s
user	0m38.687s
sys	0m41.480s

laanwj · 2018-05-31T08:39:37Z

utACK 6b8b63a

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

…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

martinus changed the title ~~Faster unit tests: directloy operate with CMutableTransaction~~ Directly operate with CMutableTransaction in SignSignature (faster unit tests) May 23, 2018

Empact reviewed May 23, 2018

View reviewed changes

promag reviewed May 23, 2018

View reviewed changes

martinus force-pushed the SignSignature-with-CMutableTransaction branch from d621953 to b4fa297 Compare May 23, 2018 11:55

sipa reviewed May 24, 2018

View reviewed changes

martinus force-pushed the SignSignature-with-CMutableTransaction branch from 62b53ea to d4ee653 Compare May 27, 2018 11:11

Empact reviewed May 29, 2018

View reviewed changes

maflcko added the Refactoring label May 29, 2018

maflcko approved these changes May 29, 2018

View reviewed changes

maflcko changed the title ~~Directly operate with CMutableTransaction in SignSignature (faster unit tests)~~ Directly operate with CMutableTransaction in SignSignature May 29, 2018

martinus force-pushed the SignSignature-with-CMutableTransaction branch from d4ee653 to 6b8b63a Compare May 30, 2018 14:02

maflcko mentioned this pull request May 30, 2018

[tests] improvements to slow unit tests #13050

Closed

laanwj merged commit 6b8b63a into bitcoin:master May 31, 2018

maflcko mentioned this pull request May 31, 2018

Directly operate with CMutableTransaction #13359

Closed

Bushstar mentioned this pull request Jun 1, 2018

commits from bitcoin/master FeatherCoin/Feathercoin#340

Merged

martinus deleted the SignSignature-with-CMutableTransaction branch June 1, 2018 20:34

maflcko mentioned this pull request Sep 6, 2018

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

Closed

scravy mentioned this pull request Mar 4, 2019

Brainstorming: CTransaction vs CMutableTransaction #15525

Closed

bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Directly operate with CMutableTransaction in SignSignature #13309

Directly operate with CMutableTransaction in SignSignature #13309

martinus commented May 23, 2018 •

edited

Loading

promag commented May 23, 2018

Empact May 23, 2018

martinus May 23, 2018

Empact commented May 23, 2018

promag May 23, 2018

martinus May 23, 2018

promag May 23, 2018

promag commented May 23, 2018

practicalswift commented May 23, 2018

sipa May 24, 2018 •

edited

Loading

martinus commented May 27, 2018

sipa commented May 27, 2018

Empact May 29, 2018 •

edited

Loading

maflcko left a comment

maflcko May 29, 2018

maflcko May 29, 2018

maflcko commented May 29, 2018

martinus commented May 30, 2018

maflcko commented May 30, 2018

sipa commented May 30, 2018 •

edited

Loading

lucash-dev commented May 31, 2018 •

edited

Loading

fanquake commented May 31, 2018 •

edited

Loading

laanwj commented May 31, 2018

Directly operate with CMutableTransaction in SignSignature #13309

Directly operate with CMutableTransaction in SignSignature #13309

Conversation

martinus commented May 23, 2018 • edited Loading

promag commented May 23, 2018

Empact May 23, 2018

Choose a reason for hiding this comment

martinus May 23, 2018

Choose a reason for hiding this comment

Empact commented May 23, 2018

promag May 23, 2018

Choose a reason for hiding this comment

martinus May 23, 2018

Choose a reason for hiding this comment

promag May 23, 2018

Choose a reason for hiding this comment

promag commented May 23, 2018

practicalswift commented May 23, 2018

sipa May 24, 2018 • edited Loading

Choose a reason for hiding this comment

martinus commented May 27, 2018

sipa commented May 27, 2018

Empact May 29, 2018 • edited Loading

Choose a reason for hiding this comment

maflcko left a comment

Choose a reason for hiding this comment

maflcko May 29, 2018

Choose a reason for hiding this comment

maflcko May 29, 2018

Choose a reason for hiding this comment

maflcko commented May 29, 2018

martinus commented May 30, 2018

maflcko commented May 30, 2018

sipa commented May 30, 2018 • edited Loading

lucash-dev commented May 31, 2018 • edited Loading

fanquake commented May 31, 2018 • edited Loading

laanwj commented May 31, 2018

martinus commented May 23, 2018 •

edited

Loading

sipa May 24, 2018 •

edited

Loading

Empact May 29, 2018 •

edited

Loading

sipa commented May 30, 2018 •

edited

Loading

lucash-dev commented May 31, 2018 •

edited

Loading

fanquake commented May 31, 2018 •

edited

Loading