Fix fundrawtransactions address-reuse problem

[jonasschnelli]

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

[jonasschnelli]

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

[gmaxwell]

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.

[jonasschnelli]

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.

[laanwj]

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.

[jonasschnelli]

[...] 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).

[gmaxwell]

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.

[jonasschnelli]

But how would this work with encrypted wallets?

[gmaxwell]

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

[sipa]

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.

[luke-jr]

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.

[jtimon]

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.

[jonasschnelli]

Closing in favour of merged #9377