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 (review)
Have it assert on debug builds when it's not dealt with, and log an important sounding message otherwise.
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--174a7506f384e20aa4161008e828411d-->
No conflicts as of last run.
secp unit test failure, reported here in one build: https://github.com/bitcoin-core/secp256k1/issues/610
restarted.
I think the whole CReserveKey cycle needs to be rethought. It's too easy to accidentally not mark a key as used (eg #15557 (review)). 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 https://github.com/bitcoin/bitcoin/blob/2a191b48463adc64dfb6187fccb7742c795dee60/src/wallet/wallet.cpp#L2835 or here https://github.com/bitcoin/bitcoin/blob/2a191b48463adc64dfb6187fccb7742c795dee60/src/wallet/wallet.cpp#L2894 or even here: https://github.com/bitcoin/bitcoin/blob/2a191b48463adc64dfb6187fccb7742c795dee60/src/wallet/rpcwallet.cpp#L340) 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.
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.
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.
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.
assert seems better IMO
Needs documentation updated (after rebase)
Deciding if it's worth the rebase. There are as many opinions here.
I'm also fine with an assert to be honest.
assert seems better IMO
You mean assert(nIndex == -1)?
Sure, something like that.
@jnewbery @ryanofsky are you guys ok with an assert instead? I'd rather do something than let this languish, and it seems reasonable.
@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.
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.
TIL about debug builds. Added an assert behind that, and a print otherwise telling the user to report it.
279 | + // have it returned or kept before its destructor is called. 280 | + // TODO: Fix the lifecycle of this object to be less error-prone! 281 | + assert(nIndex == -1); 282 | +#endif 283 | + if (nIndex != -1) { 284 | + LogPrintf("Warning! A CReserveKey was not dealt with until destructor. This is a bug. Please report this if possible.");
What about.
LogPrintf("Warning! A CReserveKey was not dealt with until destructor. This is a bug. Please report this if possible, e.g. here: " PACKAGE_BUGREPORT "/new?title=wallet:%20ReserveKey%20was%20not%20dealt%20with%20until%20destructor");
wrestled with tinyformat a bit, but took heart of suggestion
2019-06-13T14:45:00Z Error "tinyformat: Too many conversion specifiers in format string" while formatting log message: Warning! A CReserveKey was not dealt with until destructor. This is a bug. Please report this if possible , e.g. here: https://github.com/bitcoin/bitcoin/issues/new?title=wallet:%20ReserveKey%20was%20not%20dealt%20with%20until%20destructor
to
2019-06-13T14:50:09Z Warning! A CReserveKey was not dealt with until destructor. This is a bug. Please report this if possible , e.g. here: https://github.com/bitcoin/bitcoin/issues/new?title=wallet:%20ReserveKey%20was%20not%20dealt%20with%20until%20destructor
Was complaining about the % so had to double-up those so the checker wouldn't complain.
pushed fancier reporting error log courtesy of @MarcoFalke
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.
279 | + // have it returned or kept before its destructor is called. 280 | + // TODO: Fix the lifecycle of this object to be less error-prone! 281 | + assert(nIndex == -1); 282 | +#endif 283 | + if (nIndex != -1) { 284 | + LogPrintf("Warning! A CReserveKey was not dealt with until destructor. This is a bug. Please report this if possible, e.g. here: " PACKAGE_BUGREPORT "/new\?title=wallet:%%20ReserveKey%%20was%%20not%%20dealt%%20with%%20until%%20destructor\n");
Sorry for continued nit-picking, but the logprint could be made even if we assert out in the destructor.
LogPrintf("Warning! A CReserveKey was not dealt with until destructor. This is a bug. Please report this if possible, e.g. here: %s%s\n", PACKAGE_BUGREPORT, "/new\?title=wallet:%20ReserveKey%20was%20not%20dealt%20with%20until%20destructor");
#ifdef DEBUG
assert(false);
#endif
indeed, a run of RPC tests appears to fail and it'd be nice to have it be obvious why
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
closing this one for now
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
@ryanofsky interesting!