wallet, refactor: return out-params of CreateTransaction() as optional struct #20640

1. 
   theStack commented at 4:03 am on December 13, 2020: member

   The method CWallet::CreateTransaction currently returns several values in the form of out-parameters:

   • the actual newly created transaction (CTransactionRef& tx)
   • its required fee (CAmount& nFeeRate)
   • the position of the change output (int& nChangePosInOut) – as the name suggests, this is both an in- and out-param

   By returning these values in an optional structure (which returns no value a.k.a. std::nullopt if an error occured), the interfaces is shorter, cleaner (requested change position is now in-param and can be passed by value) and callers don’t have to create dummy variables for results that they are not interested in.

   Note that the names of the replaced out-variables were kept in CreateTransactionInternal to keep the diff minimal. Also, the fee calculation data (FeeCalculation& fee_calc_out) would be another candidate to put into the structure, but FeeCalculation is currently an opaque data type in the wallet interface and I think it should stay that way.

   As a potential follow-up, I think it would make sense to also do the same refactoring for CWallet::FundTransaction, which has a very similar parameter structure.

   Suggested by laanwj in #20588 (comment).

2. fanquake added the label Refactoring on Dec 13, 2020
3. fanquake added the label Wallet on Dec 13, 2020
4. theStack force-pushed on Dec 13, 2020
5. 
   DrahtBot commented at 6:45 am on December 13, 2020: 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:

   • #25003 (tracing: fix coin_selection:aps_create_tx_internal calling logic by theStack)
   • #24897 ([Draft / POC] Silent Payments by w0xlt)
   • #24845 (wallet: createTransaction, return proper error description for “too-long-mempool-chain” + introduce generic Result classes by furszy)
   • #24752 (wallet: increase BnB upper limit by S3RK)
   • #24649 (wallet: do not count wallet utxos as external by S3RK)

   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.

6. in src/wallet/feebumper.cpp:220 in 8e9266fb7e outdated

   219-    CAmount fee_ret;
   220-    int change_pos_in_out = -1; // No requested location for change
   221     bilingual_str fail_reason;
   222     FeeCalculation fee_calc_out;
   223-    if (!wallet.CreateTransaction(recipients, tx_new, fee_ret, change_pos_in_out, fail_reason, new_coin_control, fee_calc_out, false)) {
   224+    auto txr = wallet.CreateTransaction(recipients, /* No requested location for change */ -1, fail_reason, new_coin_control, fee_calc_out, false);
   

   ---

   ---

   

   pox commented at 5:27 am on December 19, 2020:

   While it’s arguably not, strictly speaking, part of the proposed change, the comment about the change position parameter that’s being maintained here is a bit misleading. This is a bit nit-picky, so bare with me please, but I think it’s more appropriate to:

   1. Use a const instead of inline comment in order to convey the meaning. e.g. const int RANDOM_CHANGE_POSITION = -1). This value is used more than once, so another reason to use a const.
   2. Convey that passing -1 means “actively set the position to a random one” rather than “no requested location” which doesn’t really let the reader know what’s going to happen.

   ---

   

   theStack commented at 7:26 pm on December 27, 2020:

   Thanks a lot for reviewing, good point! I probably was a bit over-enthusiastic in getting rid of as many variables as possible before the call to CreateTransaction ;-) Introducing a RANDOM_CHANGE_POSITION constant makes a lot of sense to me, see latest force-push of the second commit. Not sure yet if putting the constant into the header is a good idea, as it is only used for one single method and hence many modules that don’t need the constant would see it as well. Probably a candidate for a follow-up PR?
7. in src/wallet/wallet.cpp:3132 in 8e9266fb7e outdated

   3142-            if (use_aps) {
   3143-                tx = tx2;
   3144-                nFeeRet = nFeeRet2;
   3145-                nChangePosInOut = nChangePosInOut2;
   3146-            }
   3147+            const bool use_aps = txr_grouped->fee <= txr_ungrouped->fee + m_max_aps_fee;
   

   ---

   ---

   

   pox commented at 5:33 am on December 19, 2020:

   This is a lot more readable indeed! 👍
8. 
   pox commented at 5:36 am on December 19, 2020: contributor

   ACK not tested.
9. theStack force-pushed on Dec 27, 2020
10. 
    theStack commented at 7:29 pm on December 27, 2020: member

    Force-pushed with the proposed changes by @pox (see #20640 (review)), introducing new constants RANDOM_CHANGE_POSITION to be used as second parameter for CWallet::CreateTransaction.
11. in src/wallet/feebumper.cpp:221 in 5865c38c6a outdated

    220-    int change_pos_in_out = -1; // No requested location for change
    221+    constexpr int RANDOM_CHANGE_POSITION = -1;
    222     bilingual_str fail_reason;
    223     FeeCalculation fee_calc_out;
    224-    if (!wallet.CreateTransaction(recipients, tx_new, fee_ret, change_pos_in_out, fail_reason, new_coin_control, fee_calc_out, false)) {
    225+    auto txr = wallet.CreateTransaction(recipients, RANDOM_CHANGE_POSITION, fail_reason, new_coin_control, fee_calc_out, false);
    

    ---

    ---

    

    practicalswift commented at 10:50 pm on December 27, 2020:

    Nit: Using auto here obscures the fact that txr is an optional. I think being explicit about the optionality is better in order to avoid mistakes in the future.

    Applies throughout this PR.

    ---

    

    theStack commented at 8:47 pm on December 30, 2020:

    Thanks for reviewing! Agree that being explicit is better here. Done.
12. stackman27 approved
13. 
    stackman27 commented at 4:01 am on December 28, 2020: contributor

    ACK Code review and built successful. Also, definitely a good idea to put FeeCalculation& fee_calc_out into this structure
14. 
    pox commented at 5:02 am on December 28, 2020: contributor

    reACK 5865c38 🙏
15. theStack force-pushed on Dec 30, 2020
16. 
    theStack commented at 8:52 pm on December 30, 2020: member

    Force-pushed with changes as suggested by practicalswift, using explicit datatype std::optional<CreatedTransactionalResult> rather instead of auto for storing return values of CreateTransaction{Internal}(). Should be straight-forward to re-review, see range-diff.
17. DrahtBot added the label Needs rebase on Feb 19, 2021
18. theStack force-pushed on Mar 1, 2021
19. 
    theStack commented at 0:11 am on March 1, 2021: member

    Rebased on master.
20. DrahtBot removed the label Needs rebase on Mar 1, 2021
21. DrahtBot added the label Needs rebase on Mar 10, 2021
22. theStack force-pushed on Mar 31, 2021
23. 
    theStack commented at 6:38 pm on March 31, 2021: member

    Rebased on master and changed code to return uninitialized values explicitely via return std::nullopt rather than return {} (see #21498, #21415 (comment) for further explanation why this is preferred).
24. DrahtBot removed the label Needs rebase on Mar 31, 2021
25. DrahtBot added the label Needs rebase on May 25, 2021
26. theStack force-pushed on May 29, 2021
27. 
    theStack commented at 12:17 pm on May 29, 2021: member

    Rebased on master.
28. DrahtBot removed the label Needs rebase on May 29, 2021
29. DrahtBot added the label Needs rebase on May 30, 2021
30. theStack force-pushed on Jun 9, 2021
31. 
    theStack commented at 3:44 pm on June 9, 2021: member

    Rebased on master again. (This time it was a bit more complex, as the involved methods have moved from wallet/wallet.cpp to wallet/spend.cpp).
32. DrahtBot removed the label Needs rebase on Jun 9, 2021
33. DrahtBot added the label Needs rebase on Sep 3, 2021
34. theStack force-pushed on Sep 10, 2021
35. theStack force-pushed on Sep 10, 2021
36. DrahtBot removed the label Needs rebase on Sep 10, 2021
37. 
    theStack commented at 8:31 pm on September 10, 2021: member

    Rebased on master.
38. DrahtBot added the label Needs rebase on Oct 4, 2021
39. theStack force-pushed on Oct 29, 2021
40. 
    theStack commented at 0:08 am on October 29, 2021: member

    Rebased on master again.
41. DrahtBot removed the label Needs rebase on Oct 29, 2021
42. DrahtBot added the label Needs rebase on Dec 8, 2021
43. 
    fanquake commented at 10:15 am on March 22, 2022: member

    @achow101 @glozow any Concept ACK / NACK thoughts on this PR?
44. 
    achow101 commented at 2:25 pm on March 22, 2022: member

    Concept ACK
45. theStack force-pushed on Mar 22, 2022
46. 
    theStack commented at 3:15 pm on March 22, 2022: member

    Rebased on master.
47. DrahtBot removed the label Needs rebase on Mar 22, 2022
48. DrahtBot added the label Needs rebase on Mar 23, 2022
49. theStack force-pushed on Mar 31, 2022
50. 
    theStack commented at 11:14 pm on March 31, 2022: member

    Rebased on master again.
51. DrahtBot removed the label Needs rebase on Apr 1, 2022
52. 
    laanwj commented at 5:51 pm on April 21, 2022: member

    Concept ACK. Returning things is much cleaner for out-only arguments than using mutable arguments (which would ideally be reserved for inout-arguments only).
53. 
    furszy commented at 11:15 pm on April 21, 2022: member

    First look, concept ACK.

    Wasn’t aware of this PR and touched the same piece of code in #24845.

    -> PR where I introduced two generic versions of the optional return object called OperationResult and CallResult<T> (basically classes that encapsulate the function result: the result object, or in case of failure, the failure reason), which later can be used everywhere in the sources to cleanup similar boilerplate code where we pass a lot of ref args with a single return optional struct like the one introduced here.

    So, combining both PRs, we will just return a CallResult<CreatedTransactionResult> which will cleanup the code further.

    Will do a deeper code review soon.

54. in src/wallet/spend.h:102 in 5238022161 outdated

     96@@ -95,9 +97,9 @@ struct CreatedTransactionResult
     97 /**
     98  * Create a new transaction paying the recipients with a set of coins
     99  * selected by SelectCoins(); Also create the change output, when needed
    100- * @note passing nChangePosInOut as -1 will result in setting a random position
    101+ * @note passing nChangePos as -1 will result in setting a random position
    102  */
    103-bool CreateTransaction(CWallet& wallet, const std::vector<CRecipient>& vecSend, CTransactionRef& tx, CAmount& nFeeRet, int& nChangePosInOut, bilingual_str& error, const CCoinControl& coin_control, FeeCalculation& fee_calc_out, bool sign = true);
    104+std::optional<CreatedTransactionResult> CreateTransaction(CWallet& wallet, const std::vector<CRecipient>& vecSend, int nChangePos, bilingual_str& error, const CCoinControl& coin_control, FeeCalculation& fee_calc_out, bool sign = true);
    

    ---

    ---

    

    achow101 commented at 5:36 pm on April 22, 2022:

    In 5238022161a1ecd361916c6f425439e7c7604758 “wallet: CreateTransaction(): return out-params as (optional) struct”

    nit: new variables should be snake_case.

    Perhaps we should consider changing this to std::optional<int> and get rid of using -1 as a magic number.

    ---

    

    theStack commented at 3:53 pm on May 16, 2022:

    nit: new variables should be snake_case.

    Thanks, renamed nChangePos to change_pos in both functions CreateTransaction and CreateTransactionInternal in the latest rebase.

    Perhaps we should consider changing this to std::optional and get rid of using -1 as a magic number.

    Good idea, I think this would make a good follow-up PR candidate.

55. 
    achow101 commented at 5:38 pm on April 22, 2022: member

    ACK 5238022161a1ecd361916c6f425439e7c7604758
56. DrahtBot added the label Needs rebase on Apr 26, 2022
57. wallet: CreateTransactionInternal(): return out-params as (optional) struct c9fdaa5e3a
58. wallet: CreateTransaction(): return out-params as (optional) struct 4c5ceb040c
59. in src/wallet/spend.h:87 in cc3a161704 outdated

    81@@ -82,6 +82,16 @@ std::optional<SelectionResult> AttemptSelection(const CWallet& wallet, const CAm
    82 std::optional<SelectionResult> SelectCoins(const CWallet& wallet, const std::vector<COutput>& vAvailableCoins, const CAmount& nTargetValue, const CCoinControl& coin_control,
    83                  const CoinSelectionParams& coin_selection_params) EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet);
    84 
    85+struct CreatedTransactionResult
    

    ---

    ---

    

    Xekyo commented at 3:45 pm on May 11, 2022:

    Optional: I stumble a bit on past tense in the Class name here. I think I’d prefer either CreateTransactionResult to emphasize it’s the result of a function called CreateTransaction…, or maybe just either CreatedTransaction or TransactionResult, since having both “result” and “created” feels a bit redundant.
60. theStack force-pushed on May 16, 2022
61. 
    theStack commented at 3:57 pm on May 16, 2022: member

    Rebased on master. Thanks to @furszy for getting the ball rolling on this PR again by pinging me up on IRC (I was planning to wait for #25003, but it seems that this will not be merged soon, as it was suggested to remove the affected tracepoint completely).
62. DrahtBot removed the label Needs rebase on May 16, 2022
63. 
    Xekyo commented at 5:45 pm on May 16, 2022: member

    ACK 4c5ceb040cf50d24201903a9200fb23be88d96fb
64. 
    achow101 commented at 8:00 pm on May 16, 2022: member

    re-ACK 4c5ceb040cf50d24201903a9200fb23be88d96fb
65. w0xlt approved
66. 
    w0xlt commented at 1:43 am on May 17, 2022: contributor

    crACK https://github.com/bitcoin/bitcoin/pull/20640/commits/4c5ceb040cf50d24201903a9200fb23be88d96fb
67. fanquake merged this on May 17, 2022
68. fanquake closed this on May 17, 2022

    ---

69. theStack deleted the branch on May 17, 2022
70. sidhujag referenced this in commit 18bc5455df on May 28, 2022
71. DrahtBot locked this on May 17, 2023

Contributors
theStack DrahtBot pox practicalswift stackman27 fanquake achow101 laanwj furszy Xekyo w0xlt

Labels
Refactoring Wallet