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

pull instagibbs wants to merge 1 commits into bitcoin:master from instagibbs:burn_reserve changing 1 files +10 −0
  1. instagibbs commented at 5:16 PM on April 11, 2019: member

    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.

  2. DrahtBot added the label Wallet on Apr 11, 2019
  3. DrahtBot commented at 5:59 PM on April 11, 2019: member

    <!--e57a25ab6845829454e8d69fc972939a-->

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

    <!--174a7506f384e20aa4161008e828411d-->

    Conflicts

    No conflicts as of last run.

  4. instagibbs commented at 9:35 PM on April 12, 2019: member

    secp unit test failure, reported here in one build: https://github.com/bitcoin-core/secp256k1/issues/610

    restarted.

  5. jnewbery commented at 3:16 PM on April 15, 2019: member

    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.

  6. instagibbs commented at 3:24 PM on April 15, 2019: member

    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.

  7. jnewbery commented at 3:30 PM on April 15, 2019: member

    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.

  8. ryanofsky approved
  9. ryanofsky commented at 3:49 PM on April 15, 2019: member

    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.

  10. luke-jr commented at 6:57 PM on April 18, 2019: member

    assert seems better IMO

  11. MarcoFalke commented at 1:17 PM on May 14, 2019: member

    Needs documentation updated (after rebase)

  12. instagibbs commented at 1:24 PM on May 14, 2019: member

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

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

  13. DrahtBot added the label Needs rebase on May 14, 2019
  14. promag commented at 2:41 PM on May 14, 2019: member

    assert seems better IMO

    You mean assert(nIndex == -1)?

  15. luke-jr commented at 3:13 PM on May 18, 2019: member

    Sure, something like that.

  16. instagibbs commented at 1:30 PM on June 13, 2019: member

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

  17. ryanofsky commented at 1:38 PM on June 13, 2019: member

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

  18. jnewbery commented at 1:43 PM on June 13, 2019: member

    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.

  19. instagibbs force-pushed on Jun 13, 2019
  20. instagibbs commented at 2:12 PM on June 13, 2019: member

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

  21. in src/wallet/wallet.h:284 in e0d1aeac15 outdated
     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.");
    


    MarcoFalke commented at 2:26 PM on June 13, 2019:

    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");
    

    instagibbs commented at 2:53 PM on June 13, 2019:

    wrestled with tinyformat a bit, but took heart of suggestion


    instagibbs commented at 2:54 PM on June 13, 2019:
    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.

  22. DrahtBot removed the label Needs rebase on Jun 13, 2019
  23. instagibbs force-pushed on Jun 13, 2019
  24. instagibbs commented at 2:55 PM on June 13, 2019: member

    pushed fancier reporting error log courtesy of @MarcoFalke

  25. ryanofsky approved
  26. ryanofsky commented at 3:09 PM on June 13, 2019: member

    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.

  27. instagibbs renamed this:
    CReserveKey should not allow passive key re-use, KeepKey in destructor
    CReserveKey should not allow passive key re-use, debug assert in destructor
    on Jun 13, 2019
  28. in src/wallet/wallet.h:284 in 735518bbeb outdated
     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");
    


    MarcoFalke commented at 3:53 PM on June 13, 2019:

    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
    

    instagibbs commented at 5:08 PM on June 13, 2019:

    indeed, a run of RPC tests appears to fail and it'd be nice to have it be obvious why

  29. Debug build assert and log when CReserveKeys are not dealt with properly c135e93235
  30. instagibbs force-pushed on Jun 13, 2019
  31. instagibbs commented at 6:40 PM on June 13, 2019: member

    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

  32. instagibbs commented at 7:53 PM on June 13, 2019: member

    closing this one for now

  33. instagibbs closed this on Jun 13, 2019

  34. ryanofsky commented at 12:10 AM on July 11, 2019: member

    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

  35. instagibbs commented at 1:37 PM on July 11, 2019: member

    @ryanofsky interesting!

  36. meshcollider referenced this in commit 459baa1756 on Jul 17, 2019
  37. sidhujag referenced this in commit 1c2372623e on Jul 29, 2019
  38. monstrobishi referenced this in commit 56f5cbbba2 on Sep 6, 2020
  39. MarcoFalke locked this on Dec 16, 2021

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