[Wallet] Bugfix: Fundrawtransaction: don't terminate when keypool is empty #9295

1. 
   jonasschnelli commented at 12:51 PM on December 6, 2016: contributor

   Right now, CWallet::CreateTransaction() has an assertion that ensures that a key could be retrieved from the keypool.

   fundrawtransaction can run on an encrypted unlocked wallet (it actually doesn't sign). But, it can still fail because the keypool could be drained on a locked wallet.

   Current master crashes in that case.

2. [Wallet] Bugfix: FRT: don't terminate when keypool is empty c24a4f5981
3. jonasschnelli added the label Needs backport on Dec 6, 2016
4. jonasschnelli added the label Wallet on Dec 6, 2016
5. jonasschnelli added this to the milestone 0.14.0 on Dec 6, 2016
6. jonasschnelli cross-referenced this on Dec 6, 2016 from issue Use internal HD chain for change outputs (hd split) by jonasschnelli
7. MarcoFalke removed this from the milestone 0.14.0 on Dec 6, 2016
8. MarcoFalke added this to the milestone 0.12.branch on Dec 6, 2016
9. jonasschnelli added this to the milestone 0.14.0 on Dec 6, 2016
10. jonasschnelli removed this from the milestone 0.12.branch on Dec 6, 2016
11. MarcoFalke added this to the milestone 0.13.2 on Dec 6, 2016
12. MarcoFalke removed this from the milestone 0.14.0 on Dec 6, 2016
13. 
    gmaxwell commented at 5:21 PM on December 6, 2016: contributor

    utACK.

14. MarcoFalke added this to the milestone 0.12.branch on Dec 6, 2016
15. MarcoFalke removed this from the milestone 0.13.2 on Dec 6, 2016
16. 
    MarcoFalke commented at 6:26 PM on December 6, 2016: member

    Tagged for 0.12 backport because I am able to reproduce on that branch:

    bin/bitcoin-0.12.1/bin/bitcoin-qt -regtest -datadir=/tmp 
    bitcoin-qt: wallet/wallet.cpp:2107: bool CWallet::CreateTransaction(const std::vector<CRecipient>&, CWalletTx&, CReserveKey&, CAmount&, int&, std::string&, const CCoinControl*, bool): Assertion `ret' failed.
    Aborted (core dumped)
    

17. 
    MarcoFalke commented at 6:26 PM on December 6, 2016: member

    utACK c24a4f5

18. doublek420 approved
19. 
    paveljanik commented at 10:53 AM on December 7, 2016: contributor

    ACK https://github.com/bitcoin/bitcoin/commit/c24a4f5981d47d55aa9e4eb40294832a4d38fb80

    What about new test case (failing in the master, OK here)?

20. 
    MarcoFalke commented at 11:02 AM on December 7, 2016: member

    Yes, agree with paveljanik, please add the trivial test case somewhere.

    On Wed, Dec 7, 2016 at 11:53 AM, paveljanik notifications@github.com wrote:

    ACK c24a4f5 https://github.com/bitcoin/bitcoin/commit/c24a4f5981d47d55aa9e4eb40294832a4d38fb80

    What about new test case (failing in the master, OK here)?

    — You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/bitcoin/bitcoin/pull/9295#issuecomment-265417093, or mute the thread https://github.com/notifications/unsubscribe-auth/AGGmv9w4s_o7vveUUeHjEGhWl0JRtKjTks5rFpAygaJpZM4LFX5w .

21. [QA] add fundrawtransaction test on a locked wallet with empty keypool 1a6eacbf3b
22. 
    jonasschnelli commented at 2:21 PM on December 7, 2016: contributor

    Added a commit with a test that ensures the new behaviour. Test will fail on master.

23. 
    MarcoFalke commented at 8:59 PM on December 7, 2016: member

    utACK 1a6eacbf3b7e3d5941fec1154079bbc4678ce861

24. MarcoFalke referenced this in commit 8dee97f982 on Dec 7, 2016
25. MarcoFalke referenced this in commit d6098956c3 on Dec 7, 2016
26. 
    paveljanik commented at 7:04 AM on December 8, 2016: contributor

    Thank you @jonasschnelli.

    Tested ACK 1a6eacb

    Fails on master with

    Initializing test directory /var/folders/65/fn0h49r55k7779vg1b_h461r0000gn/T/testyal7ogu7/50231
    Mining blocks...
    Assertion failed: (ret), function CreateTransaction, file wallet/wallet.cpp, line 2392.
    Unexpected exception caught during testing: ConnectionRefusedError(61, 'Connection refused')
    

    OK here.

27. MarcoFalke added this to the milestone 0.13.2 on Dec 8, 2016
28. MarcoFalke removed this from the milestone 0.12.branch on Dec 8, 2016
29. 
    gmaxwell commented at 12:02 AM on December 10, 2016: contributor

    ACK.

30. 
    sipa commented at 12:07 AM on December 10, 2016: member

    utACK 1a6eacbf3b7e3d5941fec1154079bbc4678ce861

31. sipa merged this on Dec 10, 2016
32. sipa closed this on Dec 10, 2016

    ---

33. sipa referenced this in commit 815640ec6a on Dec 10, 2016
34. MarcoFalke referenced this in commit 0cc07f82f0 on Dec 14, 2016
35. MarcoFalke referenced this in commit 43bcfca489 on Dec 14, 2016
36. MarcoFalke removed the label Needs backport on Dec 14, 2016
37. 
    MarcoFalke commented at 12:07 PM on December 14, 2016: member

    Backports:

    • for 0.13 in #9347
    • for 0.12 in #9211
38. UdjinM6 referenced this in commit 03ddd5da55 on Mar 22, 2017
39. UdjinM6 referenced this in commit 5842b23a5a on Mar 22, 2017
40. UdjinM6 referenced this in commit 9e4de7e5f9 on Mar 22, 2017
41. UdjinM6 referenced this in commit 21e288229f on Mar 22, 2017
42. UdjinM6 referenced this in commit ff30aed68f on Apr 11, 2017
43. lateminer referenced this in commit 5deab0d5fa on Jan 6, 2018
44. dagurval cross-referenced this on Feb 1, 2018 from issue fundrawtransaction cherries by dagurval
45. thokon00 referenced this in commit 602cdf3acd on Apr 17, 2018
46. bitcoin locked this on Sep 8, 2021

Contributors

jonasschnelligmaxwellMarcoFalkedoublek420paveljaniksipa

Milestone
0.13.2