wallet: Consume ReserveDestination on successful CreateTransaction #16208

pull instagibbs wants to merge 2 commits into bitcoin:master from instagibbs:createtransaction_keepkey changing 6 files +23 −35
  1. instagibbs commented at 6:40 PM on June 13, 2019: member

    The typical usage pattern of ReserveDestination is to explicitly KeepDestination, or ReturnDestination when it's detected it will not be used.

    Implementers such as myself may fail to complete this pattern, and could result in key re-use: #15557 (review)

    Since ReserveDestination is currently only used directly in the CreateTransaction/CommitTransaction flow(or fee bumping where it's just used in CreateTransaction), I instead make the assumption that if a transaction is returned by CreateTransaction it's highly likely that it will be accepted by the caller, and the ReserveDestination kept. This simplifies the API as well. There are very few cases where this would not be the case which may result in keys being burned.

    Those failure cases appear to be: CommitTransaction failing to get the transaction into the mempool Belt and suspenders check in WalletModel::prepareTransaction

    Alternative to https://github.com/bitcoin/bitcoin/pull/15796

  2. DrahtBot added the label RPC/REST/ZMQ on Jun 13, 2019
  3. DrahtBot added the label Tests on Jun 13, 2019
  4. DrahtBot added the label Wallet on Jun 13, 2019
  5. promag commented at 9:58 PM on June 13, 2019: member

    Maybe just ditch ReturnKey and always KeepKey implicitly if a key was reserved? I think this would be good simplification. This is also compatible with the 2nd commit.

  6. DrahtBot commented at 10:01 PM on June 13, 2019: member

    <!--e57a25ab6845829454e8d69fc972939a-->

    The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

    <!--174a7506f384e20aa4161008e828411d-->

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #14582 (wallet: try -avoidpartialspends mode and use its result if fees do not change by kallewoof)

    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.

  7. fanquake renamed this:
    Consume CReserveKey on successful CreateTransaction
    wallet: Consume CReserveKey on successful CreateTransaction
    on Jun 14, 2019
  8. instagibbs commented at 11:24 AM on June 14, 2019: member

    @promag that's basically what I was trying in #15796 originally. This can lead to many burned keys unnecessarily. I think this PR is actually even more of a simplification since it reduces the lifecycle of the reserve keys as well.

  9. promag commented at 11:42 AM on June 14, 2019: member

    This can lead to many burned keys unnecessarily

    Even with this change this happens even when creating the transaction fails:

        ret = reservekey.GetReservedKey(vchPubKey, true);
    LearnRelatedScripts(vchPubKey, change_type);          // will call AddCScript

    So IIUC it should always keep the key.

  10. instagibbs commented at 1:05 PM on June 14, 2019: member

    @promag AFAICT there is no case, except maybe mempool chain limits being busted, where keys will be burned. I'm not sure what you're saying with respect to GetReservedKey, all that does is make KeepKey a non-no-op. In the case of transaction creation failure, the key is returned to the keypool.

  11. promag commented at 2:48 PM on June 18, 2019: member

    I mean that once GetReservedKey is called (which calls CWallet::ReserveKeyFromKeyPool), it should never return the key to the pool. Please see a33189bd9.

  12. instagibbs commented at 3:02 PM on June 18, 2019: member

    @promag That's not the case though. There are a number of reasons why a valid transaction is unable to be made, after the key is reserved. Insufficient fee, transaction too large, etc. This results in additional burned keys.

  13. DrahtBot commented at 11:31 AM on July 10, 2019: member

    <!--cf906140f33d8803c4a75a2196329ecb-->Needs rebase

  14. DrahtBot added the label Needs rebase on Jul 10, 2019
  15. instagibbs force-pushed on Jul 10, 2019
  16. instagibbs commented at 1:36 PM on July 10, 2019: member

    rebased

  17. fanquake removed the label Needs rebase on Jul 10, 2019
  18. instagibbs force-pushed on Jul 10, 2019
  19. in src/wallet/wallet.cpp:3134 in e18a67d7e1 outdated 
    3130 | @@ -3131,7 +3131,7 @@ bool CWallet::CreateTransaction(interfaces::Chain::Lock& locked_chain, const std
3131 |  
3132 |      // Before we return success, we assume any change key will be used to prevent
3133 |      // accidental re-use.
3134 | -    reservekey.KeepKey();
3135 | +    reservedest.KeepDestination();
    achow101 commented at 3:18 PM on July 10, 2019:

    This change should be in the previous commit.

    instagibbs commented at 3:39 PM on July 10, 2019:

    done

  20. in src/wallet/wallet.h:259 in e18a67d7e1 outdated 
     255 | @@ -256,8 +256,8 @@ class CKeyPool
 256 |  
 257 |  /** A wrapper to reserve an address from a wallet
 258 |   *
 259 | - * ReserveDestination is used to reserve an address. It is passed around
 260 | - * during the CreateTransaction/CommitTransaction procedure.
 261 | + * ReserveDestination is used to reserve a key from the keypool.
    achow101 commented at 3:19 PM on July 10, 2019:

    nit: should be reserve an address.

    instagibbs commented at 3:39 PM on July 10, 2019:

    done

  21. achow101 commented at 3:26 PM on July 10, 2019: member

    Concept ACK. I think this is a reasonable assumption.

    Please update the OP and commit messages to use the new naming.

  22. CreateTransaction calls KeepDestination on ReserveDestination before success d9ff862f2d
  23. Restrict lifetime of ReserveDestination to CWallet::CreateTransaction e10e1e8db0
  24. instagibbs force-pushed on Jul 10, 2019
  25. instagibbs renamed this:
    wallet: Consume CReserveKey on successful CreateTransaction
    wallet: Consume ReserveDestination on successful CreateTransaction
    on Jul 10, 2019
  26. instagibbs commented at 3:41 PM on July 10, 2019: member

    fixed and updated OP/description as per @achow101 suggestions

  27. achow101 commented at 4:39 PM on July 10, 2019: member

    ACK e10e1e8db043e9b7c113e07faf408f337c1b732d Reviewed the diff

  28. stevenroose commented at 9:58 AM on July 16, 2019: contributor

    utACK e10e1e8db043e9b7c113e07faf408f337c1b732d

  29. jonasschnelli commented at 4:17 PM on July 16, 2019: contributor

    Looks good. I initially thought this includes "createrawtransaction" which I think should be stateless,... but createrawtransaction used ConstructTransaction and not CreateTransaction.

    Concept ACK.

  30. in src/wallet/wallet.h:260 in e10e1e8db0 
     255 | @@ -256,8 +256,8 @@ class CKeyPool
 256 |  
 257 |  /** A wrapper to reserve an address from a wallet
 258 |   *
 259 | - * ReserveDestination is used to reserve an address. It is passed around
 260 | - * during the CreateTransaction/CommitTransaction procedure.
 261 | + * ReserveDestination is used to reserve an address.
 262 | + * It is currently only used inside of CreateTransaction.
    meshcollider commented at 7:42 AM on July 17, 2019:

    Not quite true, it is still used in GetNewChangeDestination() which is called by the getrawchangeaddress RPC

  31. meshcollider commented at 7:43 AM on July 17, 2019: contributor

    utACK e10e1e8db043e9b7c113e07faf408f337c1b732d

    I think my one comment is not merge-blocking

  32. meshcollider merged this on Jul 17, 2019
  33. meshcollider closed this on Jul 17, 2019
  34. meshcollider referenced this in commit 459baa1756 on Jul 17, 2019
  35. ryanofsky referenced this in commit 4d94916f0d on Jul 18, 2019
  36. ryanofsky commented at 4:55 PM on July 18, 2019: member

    Should this have release notes? IIUC before this change, if you created a transaction in the GUI and pressed "Cancel" instead of "Yes" in the confirmation dialog, it would previously not affect the change keypool, but now it will remove the key from the pool and mark it used.

  37. instagibbs commented at 5:05 PM on July 18, 2019: member

    @ryanofsky Forgot this case in OP. Considering it's unlikely(?) to be an automated process I don't think it necessitates release notes but that is just my estimation.

  38. ryanofsky commented at 5:14 PM on July 18, 2019: member

    Sure, could just add the https://github.com/bitcoin/bitcoin/labels/Needs%20release%20note tag for consideration when there is a release.

  39. meshcollider referenced this in commit 1139e3cb76 on Jul 27, 2019
  40. konez2k referenced this in commit aba63ebfbb on Jul 27, 2019
  41. sidhujag referenced this in commit 1c2372623e on Jul 29, 2019
  42. sidhujag referenced this in commit db57455907 on Jul 29, 2019
  43. HashUnlimited referenced this in commit b91235f3a7 on Aug 30, 2019
  44. barrystyle referenced this in commit fa9907d226 on Nov 11, 2019
  45. konez2k referenced this in commit 9b7654df8c on Jun 2, 2020
  46. jasonbcox referenced this in commit c5263dffd1 on Jul 2, 2020
  47. jasonbcox referenced this in commit d3670a9f5b on Jul 2, 2020
  48. ShengguangXiao referenced this in commit b532798276 on Aug 28, 2020
  49. monstrobishi referenced this in commit 2e3db501d7 on Sep 6, 2020
  50. monstrobishi referenced this in commit a620a17f0d on Sep 6, 2020
  51. kittywhiskers referenced this in commit 4f01640920 on Dec 4, 2021
  52. kittywhiskers referenced this in commit 01bfebf498 on Dec 8, 2021
  53. kittywhiskers referenced this in commit f0a46630b4 on Dec 8, 2021
  54. kittywhiskers referenced this in commit d47cb88491 on Dec 12, 2021
  55. kittywhiskers referenced this in commit 61ceef50ec on Dec 12, 2021
  56. kittywhiskers referenced this in commit db793f54aa on Dec 12, 2021
  57. kittywhiskers referenced this in commit 6ef2b2f356 on Dec 13, 2021
  58. kittywhiskers referenced this in commit be49f66173 on Dec 13, 2021
  59. kittywhiskers referenced this in commit 2670865af9 on Dec 13, 2021
  60. DrahtBot locked this on Dec 16, 2021
Contributors
instagibbspromagDrahtBotachow101stevenroosejonasschnellimeshcolliderryanofsky
Labels
WalletTestsRPC/REST/ZMQ
Linked (view graph)
#15557 Enhance `bumpfee` to include inputs when targeting a feerate#15796 CReserveKey should not allow passive key re-use, debug assert in destructor#16415 Get rid of PendingWalletTx class
github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-04-13 15:14 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me