Remove unused and confusing CTransaction constructor #20588

1. 
   MarcoFalke commented at 10:24 am on December 7, 2020: member

   The constructor is confusing and dangerous (as explained in the TODO), fix that by removing it.
2. MarcoFalke added the label Refactoring on Dec 7, 2020
3. in src/wallet/feebumper.cpp:218 in fa3c87f412 outdated

   214@@ -215,7 +215,7 @@ Result CreateRateBumpTransaction(CWallet& wallet, const uint256& txid, const CCo
   215     // We cannot source new unconfirmed inputs(bip125 rule 2)
   216     new_coin_control.m_min_depth = 1;
   217 
   218-    CTransactionRef tx_new = MakeTransactionRef();
   219+    CTransactionRef tx_new;
   

   ---

   ---

   

   laanwj commented at 10:43 am on December 7, 2020:

   It appears that the only functional change is that this is initialized to a NULL pointer now, instead of a reference to a dummy transaction. I wonder why this choice was made initially and whether this has any consequences (it might make some additional check/assertion needed).

   ---

   

   MarcoFalke commented at 10:52 am on December 7, 2020:

   CWallet::CreateTransactionInternal will initialize the transaction. If that fails we will return here early with return Result::WALLET_ERROR.

   I can add an Assert(tx_new), but I think it is not needed.

   ---

   

   laanwj commented at 12:04 pm on December 7, 2020:

   I’m confused by the CWallet::CreateTransaction code here. It makes a copy of the tx that is passed in:

   0CTransactionRef tx2 = tx;
   

   If it’s only an output parameter, why is it copied?

   ---

   

   MarcoFalke commented at 12:10 pm on December 7, 2020:

   Looks like a typo. It should be possible to simply remove the useless assignment with

    0diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp
    1index 65b54f39b4..d27aba7c7a 100644
    2--- a/src/wallet/wallet.cpp
    3+++ b/src/wallet/wallet.cpp
    4@@ -3109,13 +3109,13 @@ bool CWallet::CreateTransaction(
    5         bool sign)
    6 {
    7     int nChangePosIn = nChangePosInOut;
    8-    CTransactionRef tx2 = tx;
    9     bool res = CreateTransactionInternal(vecSend, tx, nFeeRet, nChangePosInOut, error, coin_control, fee_calc_out, sign);
   10     // try with avoidpartialspends unless it's enabled already
   11     if (res && nFeeRet > 0 /* 0 means non-functional fee rate estimation */ && m_max_aps_fee > -1 && !coin_control.m_avoid_partial_spends) {
   12         CCoinControl tmp_cc = coin_control;
   13         tmp_cc.m_avoid_partial_spends = true;
   14         CAmount nFeeRet2;
   15+        CTransactionRef tx2;
   16         int nChangePosInOut2 = nChangePosIn;
   17         bilingual_str error2; // fired and forgotten; if an error occurs, we discard the results
   18         if (CreateTransactionInternal(vecSend, tx2, nFeeRet2, nChangePosInOut2, error2, tmp_cc, fee_calc_out, sign)) {
   

   ---

   

   laanwj commented at 12:39 pm on December 7, 2020:

   Sounds fair enough. But we also have a changepos in-out which seems coupled, so it is passing in a transaction in some cases? @meshcollider @achow101 can you weigh in what is supposed to be happening here please.

   ---

   

   MarcoFalke commented at 1:51 pm on December 7, 2020:

   changepos relates to vecSend to shuffle in the change output

   ---

   

   MarcoFalke commented at 1:52 pm on December 7, 2020:

   Pushed a commit to fix the assignment typo

   ---

   

   MarcoFalke commented at 2:11 pm on December 7, 2020:

   Also, added a todo for #20588 (comment)

   ---

   

   achow101 commented at 5:10 pm on December 7, 2020:

   tx2 is used so that the second CreateTransactionInternal isn’t polluted by the results of the first CreateTransactionInternal. But tx itself isn’t used within CreateTransactionInternal at all, so it should be fine to remove the copy.

   ---

   

   MarcoFalke commented at 7:21 am on December 9, 2020:

   ok, closing this discussion. Let me know if there is anything else you want me to address
4. 
   laanwj commented at 10:49 am on December 7, 2020: member

   I kind of wish CreateTransaction would simply return an optional<transaction, …> instead of the in/out parameters (of which one a reference to a transaction reference), that would make it easier to review.
5. Remove unused and confusing CTransaction constructor faac31521b
6. wallet: document that tx in CreateTransaction is purely an out-param fac39c1983
7. MarcoFalke force-pushed on Dec 7, 2020
8. 
   practicalswift commented at 9:10 am on December 9, 2020: contributor

   Concept ACK
9. 
   laanwj commented at 11:40 am on December 10, 2020: member

   Thank, looks good to me now. Code review ACK fac39c198324715565897f4240709340477af0bf
10. 
    promag commented at 2:08 pm on December 10, 2020: member

    Code review ACK fac39c198324715565897f4240709340477af0bf.
11. theStack approved
12. 
    theStack commented at 9:05 pm on December 12, 2020: member

    Code review ACK fac39c198324715565897f4240709340477af0bf
13. fanquake merged this on Dec 13, 2020
14. fanquake closed this on Dec 13, 2020

    ---

15. sidhujag referenced this in commit bbc1963362 on Dec 13, 2020
16. MarcoFalke deleted the branch on Dec 13, 2020
17. ryanofsky referenced this in commit 0e97be35f9 on Dec 18, 2020
18. DrahtBot locked this on Feb 15, 2022

Contributors
MarcoFalke laanwj achow101 practicalswift promag theStack

Labels
Refactoring