fundrawtransaction: Keep change-output keys by default, make it optional #9377

1. 
   jonasschnelli commented at 8:20 AM on December 19, 2016: contributor

   Alternative solution for #9370, fixes #9362.

   This adds reserveChangeKey, a new boolean option to fundrawtransaction. The default is true resulting in always removing the change output address key from the keypool.

   Includes test and documentation in the release-notes.

2. jonasschnelli added the label Wallet on Dec 19, 2016
3. 
   laanwj commented at 8:45 AM on December 19, 2016: member

   Thanks! Concept ACK. We could still use #9370 in some form after this, but this is a more effective solution to the issue.

4. 
   gmaxwell commented at 2:12 AM on December 20, 2016: contributor

   Concept ACK. Thanks!

5. laanwj added this to the milestone 0.14.0 on Jan 12, 2017
6. in qa/rpc-tests/fundrawtransaction.py:None in 161af5b699 outdated

    678 | +        result3 = self.nodes[3].fundrawtransaction(rawtx)
    679 | +        res_dec = self.nodes[0].decoderawtransaction(result3["hex"])
    680 | +        changeaddress = ""
    681 | +        for out in res_dec['vout']:
    682 | +            if out['value'] > 1.0:
    683 | +                changeaddress += out['scriptPubKey']['addresses'][0]
   

   ---

   ---

   

   jtimon commented at 12:07 AM on January 13, 2017:

   why += instead of a regular assignment? What if there's more outputs than one with value > 1?

7. 
   jtimon commented at 12:08 AM on January 13, 2017: contributor

   utACK 161af5b modulo the tests which I'm not sure I'm fully understanding. Needs rebase.

8. jtimon cross-referenced this on Jan 13, 2017 from issue Fix fundrawtransactions address-reuse problem by jonasschnelli
9. 
   TheBlueMatt commented at 4:25 AM on January 13, 2017: contributor

   Needs rebase (but is a bugfix, so can go in for 0.14 after the feature freeze on Monday).

10. [Wallet] Add an option to keep the change address key, true by default 9aa4e6a6c2
11. [QA] Add test for fundrawtransactions new reserveChangeKey option 9eb325d079
12. Add fundrawtransactions new reserveChangeKey option to the release notes c9f3062d55
13. jonasschnelli force-pushed on Jan 19, 2017
14. 
    jonasschnelli commented at 7:53 PM on January 19, 2017: contributor

    Rebased.

15. 
    MarcoFalke commented at 8:36 PM on January 19, 2017: member

    utACK c9f3062d551823f006d400dddb54c12620cd29c4, but agree with the nit by @jtimon. Either you use a set/list or a single string, but please no string manipulations (concatenation) with addresses. Anyway, there is only a single coin, so you can go without the +=.

16. 
    laanwj commented at 1:31 PM on January 20, 2017: member

    Not going to block this on a test nit - that can be fixed later. I have enough confidence in this to sneak this in before the feature freeze.

17. laanwj merged this on Jan 20, 2017
18. laanwj closed this on Jan 20, 2017

    ---

19. laanwj referenced this in commit fb75cd04bb on Jan 20, 2017
20. jonasschnelli cross-referenced this on Feb 1, 2017 from issue Use internal HD chain for change outputs (hd split) by jonasschnelli
21. 
    gituser commented at 2:37 PM on February 2, 2017: none

    Just a quick question to confirm: so in the latest master there is no more problem at fundrawtransaction() with address reuse?

    We hit an issue when sending transaction via fundrawtransaction change was sent to the existing user account address and thus credited our user with balance.

    sendtoaddress always creates a new change address I suppose.

22. 
    jonasschnelli commented at 3:11 PM on February 2, 2017: contributor

    We hit an issue when sending transaction via fundrawtransaction change was sent to the existing user account address and thus credited our user with balance.

    Yes. This is exactly what this PR does fix.

23. gituser cross-referenced this on Apr 14, 2017 from issue fundrawtransaction() address reuse bug by gituser
24. TheBlueMatt cross-referenced this on Jul 10, 2017 from issue Do not allow users to get keys from keypool without reserving them by TheBlueMatt
25. laanwj referenced this in commit 9e8d6a3fb4 on Jul 18, 2017
26. codablock referenced this in commit 80c8347082 on Jan 19, 2018
27. codablock referenced this in commit db53a4a7a4 on Jan 20, 2018
28. codablock referenced this in commit 36109e0580 on Jan 21, 2018
29. UdjinM6 cross-referenced this on Jul 31, 2018 from issue fundrawtransaction address re-use by gituser
30. andvgal referenced this in commit 8a308b2bd5 on Jan 6, 2019
31. CryptoCentric referenced this in commit 4ed611e461 on Feb 27, 2019
32. PastaPastaPasta referenced this in commit f639a6a16f on Sep 8, 2019
33. PastaPastaPasta referenced this in commit 443b577931 on Sep 8, 2019
34. barrystyle referenced this in commit d507557171 on Jan 22, 2020
35. rebroad cross-referenced this on Mar 19, 2021 from issue change address uses previously used address by rebroad
36. bitcoin locked this on Sep 8, 2021

Contributors

jonasschnellilaanwjgmaxwelljtimonTheBlueMattMarcoFalkegituser

Milestone
0.14.0