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

instagibbs · 2019-04-11T17:16:36Z

The typical usage pattern of CReserveKey is to explicitly KeepKey, or ReturnKey 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)

Have it assert on debug builds when it's not dealt with, and log an important sounding message otherwise.

DrahtBot · 2019-04-11T17:59:10Z

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

Conflicts

No conflicts as of last run.

instagibbs · 2019-04-12T21:35:23Z

secp unit test failure, reported here in one build: bitcoin-core/secp256k1#610

restarted.

jnewbery · 2019-04-15T15:16:18Z

I think the whole CReserveKey cycle needs to be rethought. It's too easy to accidentally not mark a key as used (eg #15557 (comment)). I'm not sure whether this change is an improvement. It removes the possibility of a key being re-used, but it does mean that if CreateTranasction fails after reserving a key (eg here

bitcoin/src/wallet/wallet.cpp

Line 2835 in 2a191b4

return false;

or here

bitcoin/src/wallet/wallet.cpp

Line 2894 in 2a191b4

return false;

or even here:

bitcoin/src/wallet/rpcwallet.cpp

Line 340 in 2a191b4

    
           strError = strprintf("Error: This transaction requires a transaction fee of at least %s", FormatMoney(nFeeRequired));

) then the keypool key will be burned instead of returned.

I don't know whether that could cause problems for users or client applications. For example a client that tries to create a transaction, hits an error, retries in a loop and burns many keypool keys.

instagibbs · 2019-04-15T15:24:36Z

Granted this is an extremely timid change, but you can make more keys. You cannot make more privacy when this fails.

I'll let others chime in before I try to nibble at the edges and make sure these return false situations are handled explicitly.

jnewbery · 2019-04-15T15:30:00Z

Granted this is an extremely timid change, but you can make more keys. You cannot make more privacy when this fails.

Yep. To be clear, I'm not NACKing this. Just trying to figure out what the least bad option is.

ryanofsky

utACK eac5438108339a65f3dd6d9cce06404167736c2e. Seems sensible to choose the wrong thing you can recover from if you're choosing between doing two potentially wrong things.

I guess the least "timid change" would be unconditionally asserting or throwing when KeepKey or ReturnKey aren't called. But other less timid options would be logging errors when they aren't called, or asserting that they are called in --debug builds.

But current change does seem like this simplest option to implement, and I think it's an improvement.

luke-jr · 2019-04-18T18:57:24Z

assert seems better IMO

maflcko · 2019-05-14T13:17:48Z

Needs documentation updated (after rebase)

instagibbs · 2019-05-14T13:24:34Z

Deciding if it's worth the rebase. There are as many opinions here.

I'm also fine with an assert to be honest.

promag · 2019-05-14T14:41:59Z

assert seems better IMO

You mean assert(nIndex == -1)?

luke-jr · 2019-05-18T15:13:04Z

Sure, something like that.

instagibbs · 2019-06-13T13:30:20Z

@jnewbery @ryanofsky are you guys ok with an assert instead? I'd rather do something than let this languish, and it seems reasonable.

ryanofsky · 2019-06-13T13:38:21Z

@jnewbery @ryanofsky are you guys ok with an assert instead? I'd rather do something than let this languish, and it seems reasonable.

I'd prefer the assert, but I think the current change is also an improvement. I'd be happy with either change.

jnewbery · 2019-06-13T13:43:34Z

other less timid options would be logging errors when they aren't called, or asserting that they are called in --debug builds.

I would prefer to do this.

instagibbs · 2019-06-13T14:12:04Z

TIL about debug builds. Added an assert behind that, and a print otherwise telling the user to report it.

src/wallet/wallet.h

instagibbs · 2019-06-13T14:55:56Z

pushed fancier reporting error log courtesy of @MarcoFalke

ryanofsky

utACK 735518bbeb17f61b3a79eae503b0efb17089acd7. PR title needs to be updated (in the future I think it's better to close and open a new PR if you implement an alternate approach so discussion makes more sense and it's easy to look back and see what the rejected alternatives looked like). Also in the future, the CHECK macro proposed in #16136 might simplify this more.

src/wallet/wallet.h

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

Having second-thoughts, mea culpa. Basically anytime there's a wallet error of any sort, you need to make sure you handle it and ReturnKey. This happens in dozens of places, including in CommitTransaction itself in a number of places.

Opened an alternative here: #16208

instagibbs · 2019-06-13T19:53:51Z

closing this one for now

ryanofsky · 2019-07-11T00:10:11Z

This pr is closed, and the alternate implementation in #16208 seems good, but I just wanted to note that it would be possible to check with compile errors, not just runtime asserts that the CReserveKey/ReserveDestination destructor is only called after after the key is returned or kept. Clang apparently has set_typestate(consumed) and callable_when(consumed) annotations which could enforce this. From https://awesomekling.github.io/Catching-use-after-move-bugs-with-Clang-consumed-annotations/ and https://news.ycombinator.com/item?id=20402733

instagibbs · 2019-07-11T13:37:36Z

@ryanofsky interesting!

…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

… 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

…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: bitcoin/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/bitcoin#15796 ACKs for top commit: achow101: ACK e10e1e8 Reviewed the diff stevenroose: utACK e10e1e8 meshcollider: utACK e10e1e8 Tree-SHA512: 78d047a00f39ab41cfa297052cc1e9c224d5f47d3d2299face650d71827635de077ac33fb4ab9f7dc6fc5a27f4a68415a1bc9ca33a3cb09a78f4f15b2a48411b

DrahtBot added the Wallet label Apr 11, 2019

ryanofsky approved these changes Apr 15, 2019

View reviewed changes

DrahtBot added the Needs rebase label May 14, 2019

instagibbs force-pushed the burn_reserve branch from eac5438 to e0d1aea Compare June 13, 2019 14:11

maflcko reviewed Jun 13, 2019

View reviewed changes

src/wallet/wallet.h Outdated Show resolved Hide resolved

DrahtBot removed the Needs rebase label Jun 13, 2019

instagibbs force-pushed the burn_reserve branch from e0d1aea to 735518b Compare June 13, 2019 14:52

ryanofsky approved these changes Jun 13, 2019

View reviewed changes

instagibbs changed the title ~~CReserveKey should not allow passive key re-use, KeepKey in destructor~~ CReserveKey should not allow passive key re-use, debug assert in destructor Jun 13, 2019

maflcko reviewed Jun 13, 2019

View reviewed changes

src/wallet/wallet.h Outdated Show resolved Hide resolved

Debug build assert and log when CReserveKeys are not dealt with properly

c135e93

instagibbs force-pushed the burn_reserve branch from 735518b to c135e93 Compare June 13, 2019 17:07

instagibbs mentioned this pull request Jun 13, 2019

wallet: Consume ReserveDestination on successful CreateTransaction #16208

Merged

instagibbs closed this Jun 13, 2019

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

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

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

Uh oh!

Conversation

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

Uh oh!

Uh oh!

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

Uh oh!

Conflicts

Uh oh!

instagibbs commented Apr 12, 2019

Uh oh!

jnewbery commented Apr 15, 2019

Uh oh!

instagibbs commented Apr 15, 2019

Uh oh!

jnewbery commented Apr 15, 2019

Uh oh!

ryanofsky 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!

luke-jr commented Apr 18, 2019

Uh oh!

maflcko commented May 14, 2019

Uh oh!

instagibbs commented May 14, 2019

Uh oh!

promag commented May 14, 2019

Uh oh!

luke-jr commented May 18, 2019

Uh oh!

instagibbs commented Jun 13, 2019

Uh oh!

ryanofsky commented Jun 13, 2019

Uh oh!

jnewbery commented Jun 13, 2019

Uh oh!

instagibbs commented Jun 13, 2019

Uh oh!

Uh oh!

instagibbs commented Jun 13, 2019

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

instagibbs commented Jun 13, 2019

Uh oh!

instagibbs commented Jun 13, 2019

Uh oh!

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

Uh oh!

Uh oh!

instagibbs commented Jul 11, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

instagibbs commented Apr 11, 2019 •

edited

Loading

DrahtBot commented Apr 11, 2019 •

edited

Loading

ryanofsky left a comment •

edited

Loading

ryanofsky commented Jul 11, 2019 •

edited

Loading