Fix fundrawtransactions address-reuse problem #9370

1. 
   jonasschnelli commented at 5:00 pm on December 16, 2016: contributor

   Addresses #9362

   Right now, fundrawtransaction does get a CReserveKey from the keypool, but either a “return” nor a “keep key” is called on that reserve key. This results in most cases in address reuse. Calling getnewaddress() will return the identical key just used as change in fundrawtransaction.

   The only possible solution for now is to call getrawchangeaddress and pass the change address to the fundrawtransaction options.

   This PR does loop through all outputs of new wallet transactions and compares them against the keypool and – if there is a match – remove the key from the keypool.

   Not sure how good this performs on large wallets doing a rescan.

   Be aware that the most line-changes are moveonly (CAffectedKeysVisitor, check first commit).

2. [MOVEONLY] move CAffectedKeysVisitor 99a40c1042
3. [Wallet] Try to remove keypool keys if they are used as output 4d0a4ff5ce
4. [QA] Add test to ensure fundrawtx in conjunction with sendrawtx does not enforce address reuse 4a011a6422
5. jonasschnelli added the label Wallet on Dec 16, 2016
6. 
   jonasschnelli commented at 5:06 pm on December 16, 2016: contributor

   Alternative solutions would be:

   • Always reserve the change output key when calling fundrawtransaction
   • Add an option to the RPC call to reserve the change output key (probably true by default)
   • Add a new RPC call reserveaddress or reservechangeaddressfromtransaction (or similar).
7. 
   gmaxwell commented at 6:44 pm on December 16, 2016: contributor

   So we should make a separate PR that removes any key from the keypool that shows up in a transaction. That is a generally good thing to do– and its important for HD key rescan (in fact, It should remove all keys up to the key it found).

   But I don’t believe that it is a correct solution here. We should reserve the key on use, having an argument (default to true) to do so would be okay but I’m not sure if it would be very useful. Otherwise this is going to cause continual reuse with in flight transactions.

8. 
   jonasschnelli commented at 12:17 pm on December 18, 2016: contributor

   But I don’t believe that it is a correct solution here. We should reserve the key on use, having an argument (default to true) to do so would be okay but I’m not sure if it would be very useful. Otherwise this is going to cause continual reuse with in flight transactions.

   The reason why I think the key-reserve could be optional are..

   • the original design was that one can call fundrawtransaction without changing the wallet state
   • Inputs are also not getting locked

   An optional reservekey flag with default=true makes sense IMO. Also, keypool keys should not be a sacred resource.

9. 
   laanwj commented at 7:36 am on December 19, 2016: member

   I agree fundrawtransaction should reserve the key. However we don’t have a method to give back reserved keys. Is that a problem? We don’t have a way to “commit” a transaction except for “sendrawtransaction”, which also sends it.

   At least it should be documented very well. Otherwise people may be running fundrawtransaction in a loop with slightly different parameters, throwing away the transactions and wondering why they race through the mempool so quickly.

   I think the change in this pull is a good idea in any case. Keys that have been seen should not be in the mempool anymore. It’s also logic that is necessary for automatically rolling forward HD wallets created with an old seed.

10. 
    jonasschnelli commented at 7:47 am on December 19, 2016: contributor

    […] It’s also logic that is necessary for automatically rolling forward HD wallets created with an old seed.

    The problem here is, that we should extend the keypool (top-up the pool) whenever the gap limit was not exceeded. Lets say, if the key no. 95 of a keypool with 100 keys was used, we should extend the keypool with another ~100 keys. But doing this in the SyncTransaction() logic would probably be really bad (also assume wallet is encrypted).

11. 
    gmaxwell commented at 4:58 am on December 20, 2016: contributor

    But doing this in the SyncTransaction() logic would probably be really bad (also assume wallet is encrypted).

    Why would it be bad? generating 100 keys should take <13 milliseconds.

12. 
    jonasschnelli commented at 6:18 am on December 20, 2016: contributor

    But how would this work with encrypted wallets?
13. 
    gmaxwell commented at 6:15 pm on December 23, 2016: contributor

    @jonasschnelli If the wallet is not unlocked– you can’t. But just because something good can’t be done all the time that doesn’t mean we shouldn’t do it when we can, unless there is some downsie. Failing to do it doesn’t make anything actually work better and it means that even unlock+rescan won’t just work when otherwise it would.

    If key number 95 is used we should mark it and all keys before it as used. If that ends up leaving us with an empty keypool– ok! thats better than silently reusing addresses. If the keypool is made larger like 1000 or 10000 addresses this is less of an issue and we could also periodically prompt the user to unlock when it gets low.

14. 
    sipa commented at 10:02 pm on December 23, 2016: member

    I really don’t like the unreliable behaviour that would follow from this. People may test their software with unencrypted wallet, and then in production a recovery is attempted and keys are missed - forcing the operator to do a full rescan (or if they were pruning, a full resync).

    I think I prefer just increasing the keypool size. If that’s enough to make this a non-issue, we don’t need to introduce best attempts.

15. 
    luke-jr commented at 4:12 am on December 24, 2016: member

    IMO the TXOs and reserve-key should be locked at the same time in the same manner; is there a use case for doing them inconsistently? Either fundrawtransaction should lock both, or neither (and sendrawtransaction required to commit to the result). A flag to choose which behaviour sounds reasonable to me.
16. 
    jtimon commented at 0:41 am on January 13, 2017: contributor

    I may be missing something, but after reviewing #9377, specially the comment in https://github.com/bitcoin/bitcoin/pull/9377/files#diff-ef76fd6674f07db88c3422fdbf0bcf9fR103 and also reading this whole thread, specially in the op “The only possible solution for now is to call getrawchangeaddress and pass the change address to the fundrawtransaction options.”…it definitely feels like I’m missing something for proposing the following:

    Let’s just make ‘changeAddress’ mandatory instead of optional and suggest users to use getrawchangeaddress() in changeAddress’ documentation.

    Unrelated: I assume this is planned to be rebased on top of rebased #9377.

17. 
    jonasschnelli commented at 7:27 pm on January 26, 2017: contributor

    Closing in favour of merged https://github.com/bitcoin/bitcoin/pull/9377
18. jonasschnelli closed this on Jan 26, 2017

    ---

19. DrahtBot locked this on Sep 8, 2021

Contributors
jonasschnelli gmaxwell laanwj sipa luke-jr jtimon

Labels
Wallet