wallet: Consume ReserveDestination on successful CreateTransaction #16208

instagibbs · 2019-06-13T18:40:14Z

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 (comment)

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 #15796

promag · 2019-06-13T21:58:49Z

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.

DrahtBot · 2019-06-13T22:01:04Z

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

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.

instagibbs · 2019-06-14T11:24:51Z

@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.

promag · 2019-06-14T11:42:23Z

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.

instagibbs · 2019-06-14T13:05:18Z

@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.

promag · 2019-06-18T14:48:04Z

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

instagibbs · 2019-06-18T15:02:27Z

@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.

DrahtBot · 2019-07-10T11:31:24Z

Needs rebase

instagibbs · 2019-07-10T13:36:43Z

rebased

achow101

Concept ACK. I think this is a reasonable assumption.

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

achow101 · 2019-07-10T15:18:57Z

src/wallet/wallet.cpp

This change should be in the previous commit.

achow101 · 2019-07-10T15:19:43Z

src/wallet/wallet.h

nit: should be reserve an address.

…success

instagibbs · 2019-07-10T15:41:33Z

fixed and updated OP/description as per @achow101 suggestions

achow101 · 2019-07-10T16:39:44Z

ACK e10e1e8 Reviewed the diff

stevenroose · 2019-07-16T09:58:58Z

utACK e10e1e8

jonasschnelli · 2019-07-16T16:17:16Z

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

Concept ACK.

meshcollider

utACK e10e1e8

I think my one comment is not merge-blocking

meshcollider · 2019-07-17T07:42:23Z

src/wallet/wallet.h

- * ReserveDestination is used to reserve an address. It is passed around
- * during the CreateTransaction/CommitTransaction procedure.
+ * ReserveDestination is used to reserve an address.
+ * It is currently only used inside of CreateTransaction.


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

…Transaction e10e1e8 Restrict lifetime of ReserveDestination to CWallet::CreateTransaction (Gregory Sanders) d9ff862 CreateTransaction calls KeepDestination on ReserveDestination before success (Gregory Sanders) Pull request description: 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 (comment) 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 #15796 ACKs for top commit: achow101: ACK e10e1e8 Reviewed the diff stevenroose: utACK e10e1e8 meshcollider: utACK e10e1e8 Tree-SHA512: 78d047a00f39ab41cfa297052cc1e9c224d5f47d3d2299face650d71827635de077ac33fb4ab9f7dc6fc5a27f4a68415a1bc9ca33a3cb09a78f4f15b2a48411b

No reason for this class to exist if it doesn't have any code to run in the destructor. e10e1e8 from bitcoin#16208 recently removed code destructor code that would return an unused key if the transaction wasn't committed.

ryanofsky · 2019-07-18T16:55:00Z

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.

instagibbs · 2019-07-18T17:05:54Z

@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.

ryanofsky · 2019-07-18T17:14:26Z

Sure, could just add the Needs release note tag for consideration when there is a release.

4d94916 Get rid of PendingWalletTx class. (Russell Yanofsky) Pull request description: No reason for this class to exist if it doesn't have any code to run in the destructor. e10e1e8 from #16208 recently removed the destructor code that would return an unused key if the transaction wasn't committed. This is just cleanup, there's no change in behavior. ACKs for top commit: ariard: utACK 4d94916. Successfully built both `bitcoind` and `bitcoin-qt`. `PendingWalletTx` was only a wrapper to enforce call to `ReturnDestination` if `CommitTransaction` doesn't `KeepDestination` before. promag: ACK 4d94916, refactor looks good to me. meshcollider: utACK 4d94916 Tree-SHA512: f3f93d2f2f5d8f1e7810d609d881c1b1cbbaa8629f483f4293e20b3210292605e947bc4903fde9d2d8736277ca3bd6de182f7eac1e13515d5a327f2ebc130839

4d94916 Get rid of PendingWalletTx class. (Russell Yanofsky) Pull request description: No reason for this class to exist if it doesn't have any code to run in the destructor. e10e1e8 from bitcoin#16208 recently removed the destructor code that would return an unused key if the transaction wasn't committed. This is just cleanup, there's no change in behavior. ACKs for top commit: ariard: utACK 4d94916. Successfully built both `bitcoind` and `bitcoin-qt`. `PendingWalletTx` was only a wrapper to enforce call to `ReturnDestination` if `CommitTransaction` doesn't `KeepDestination` before. promag: ACK 4d94916, refactor looks good to me. meshcollider: utACK 4d94916 Tree-SHA512: f3f93d2f2f5d8f1e7810d609d881c1b1cbbaa8629f483f4293e20b3210292605e947bc4903fde9d2d8736277ca3bd6de182f7eac1e13515d5a327f2ebc130839

… CreateTransaction e10e1e8 Restrict lifetime of ReserveDestination to CWallet::CreateTransaction (Gregory Sanders) d9ff862 CreateTransaction calls KeepDestination on ReserveDestination before success (Gregory Sanders) Pull request description: 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: bitcoin#15557 (comment) 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 bitcoin#15796 ACKs for top commit: achow101: ACK e10e1e8 Reviewed the diff stevenroose: utACK e10e1e8 meshcollider: utACK e10e1e8 Tree-SHA512: 78d047a00f39ab41cfa297052cc1e9c224d5f47d3d2299face650d71827635de077ac33fb4ab9f7dc6fc5a27f4a68415a1bc9ca33a3cb09a78f4f15b2a48411b

No reason for this class to exist if it doesn't have any code to run in the destructor. e10e1e8 from bitcoin#16208 recently removed code destructor code that would return an unused key if the transaction wasn't committed.

No reason for this class to exist if it doesn't have any code to run in the destructor. e10e1e8 from bitcoin/bitcoin#16208 recently removed code destructor code that would return an unused key if the transaction wasn't committed.

No reason for this class to exist if it doesn't have any code to run in the destructor. e10e1e8 from bitcoin#16208 recently removed code destructor code that would return an unused key if the transaction wasn't committed.

…veDestination before success Summary: bitcoin/bitcoin@d9ff862 --- Partial backport of Core [[bitcoin/bitcoin#16208 | PR16208]] Test Plan: ninja check-all Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D6800

Summary: No reason for this class to exist if it doesn't have any code to run in the destructor. e10e1e8db043e9b7c113e07faf408f337c1b732d from bitcoin/bitcoin#16208 recently removed code destructor code that would return an unused key if the transaction wasn't committed. bitcoin/bitcoin@4d94916 --- Backport of Core [[bitcoin/bitcoin#16415 | PR16415]] Test Plan: ninja check-all Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D6804

619685e Get rid of PendingWalletTx class. (Russell Yanofsky) Pull request description: No reason for this class to exist if it doesn't have any code to run in the destructor. d3edeaa from bitcoin/bitcoin#16208 recently removed the destructor code that would return an unused key if the transaction wasn't committed. This is just cleanup, there's no change in behavior. ACKs for top commit: ariard: utACK 619685e. Successfully built both `bitcoind` and `bitcoin-qt`. `PendingWalletTx` was only a wrapper to enforce call to `ReturnDestination` if `CommitTransaction` doesn't `KeepDestination` before. promag: ACK 619685e, refactor looks good to me. meshcollider: utACK 619685e Tree-SHA512: f3f93d2f2f5d8f1e7810d609d881c1b1cbbaa8629f483f4293e20b3210292605e947bc4903fde9d2d8736277ca3bd6de182f7eac1e13515d5a327f2ebc130839

No reason for this class to exist if it doesn't have any code to run in the destructor. e10e1e8 from bitcoin/bitcoin#16208 recently removed code destructor code that would return an unused key if the transaction wasn't committed.

4d94916 Get rid of PendingWalletTx class. (Russell Yanofsky) Pull request description: No reason for this class to exist if it doesn't have any code to run in the destructor. e10e1e8 from bitcoin/bitcoin#16208 recently removed the destructor code that would return an unused key if the transaction wasn't committed. This is just cleanup, there's no change in behavior. ACKs for top commit: ariard: utACK 4d94916. Successfully built both `bitcoind` and `bitcoin-qt`. `PendingWalletTx` was only a wrapper to enforce call to `ReturnDestination` if `CommitTransaction` doesn't `KeepDestination` before. promag: ACK 4d94916, refactor looks good to me. meshcollider: utACK 4d94916 Tree-SHA512: f3f93d2f2f5d8f1e7810d609d881c1b1cbbaa8629f483f4293e20b3210292605e947bc4903fde9d2d8736277ca3bd6de182f7eac1e13515d5a327f2ebc130839

instagibbs mentioned this pull request Jun 13, 2019

CReserveKey should not allow passive key re-use, debug assert in destructor #15796

Closed

DrahtBot added RPC/REST/ZMQ Tests Wallet labels Jun 13, 2019

fanquake changed the title ~~Consume CReserveKey on successful CreateTransaction~~ wallet: Consume CReserveKey on successful CreateTransaction Jun 14, 2019

DrahtBot added the Needs rebase label Jul 10, 2019

instagibbs force-pushed the createtransaction_keepkey branch from b535a06 to 5819967 Compare July 10, 2019 13:36

fanquake removed the Needs rebase label Jul 10, 2019

instagibbs force-pushed the createtransaction_keepkey branch from 5819967 to e18a67d Compare July 10, 2019 13:37

achow101 reviewed Jul 10, 2019

View reviewed changes

instagibbs added 2 commits July 10, 2019 11:38

CreateTransaction calls KeepDestination on ReserveDestination before …

d9ff862

…success

Restrict lifetime of ReserveDestination to CWallet::CreateTransaction

e10e1e8

instagibbs force-pushed the createtransaction_keepkey branch from e18a67d to e10e1e8 Compare July 10, 2019 15:39

instagibbs changed the title ~~wallet: Consume CReserveKey on successful CreateTransaction~~ wallet: Consume ReserveDestination on successful CreateTransaction Jul 10, 2019

meshcollider reviewed Jul 17, 2019

View reviewed changes

meshcollider merged commit e10e1e8 into bitcoin:master Jul 17, 2019

ryanofsky mentioned this pull request Jul 18, 2019

Get rid of PendingWalletTx class #16415

Merged

bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021

wallet: Consume ReserveDestination on successful CreateTransaction #16208

wallet: Consume ReserveDestination on successful CreateTransaction #16208

Uh oh!

Conversation

instagibbs commented Jun 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

promag commented Jun 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Jun 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

instagibbs commented Jun 14, 2019

Uh oh!

promag commented Jun 14, 2019

Uh oh!

instagibbs commented Jun 14, 2019

Uh oh!

promag commented Jun 18, 2019

Uh oh!

instagibbs commented Jun 18, 2019

Uh oh!

DrahtBot commented Jul 10, 2019

Uh oh!

instagibbs commented Jul 10, 2019

Uh oh!

achow101 left a comment

Choose a reason for hiding this comment

Uh oh!

achow101 Jul 10, 2019

Choose a reason for hiding this comment

Uh oh!

instagibbs Jul 10, 2019

Choose a reason for hiding this comment

Uh oh!

achow101 Jul 10, 2019

Choose a reason for hiding this comment

Uh oh!

instagibbs Jul 10, 2019

Choose a reason for hiding this comment

Uh oh!

instagibbs commented Jul 10, 2019

Uh oh!

achow101 commented Jul 10, 2019

Uh oh!

stevenroose commented Jul 16, 2019

Uh oh!

jonasschnelli commented Jul 16, 2019

Uh oh!

meshcollider left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

meshcollider Jul 17, 2019

Choose a reason for hiding this comment

Uh oh!

ryanofsky commented Jul 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

instagibbs commented Jul 18, 2019

Uh oh!

ryanofsky commented Jul 18, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

instagibbs commented Jun 13, 2019 •

edited

Loading

promag commented Jun 13, 2019 •

edited

Loading

DrahtBot commented Jun 13, 2019 •

edited

Loading

meshcollider left a comment •

edited

Loading

ryanofsky commented Jul 18, 2019 •

edited

Loading