Do not allow users to get keys from keypool without reserving them

TheBlueMatt commented at 6:26 pm on July 10, 2017: member

fundrawtransaction allows users to add a change output and then not have it removed from keypool. While it would be nice to have users follow the normal CreateTransaction/CommitTransaction process we use internally, there isnt much benefit in exposing this option, especially with HD wallets, while there is ample room for users to misunderstand or misuse this option.

This partially reverts #9377. Would be nice to get this for 15 since its kinda crazy we have this option to begin with IMO, will need release notes as an RPC option is now ignored.

TheBlueMatt force-pushed on Jul 10, 2017

TheBlueMatt commented at 6:31 pm on July 10, 2017: member

This could be particularly nasty in some use-cases (especially pre-HD-split) - eg a user might fundrawtransaction, then call getnewaddress, hand out the address for someone to pay them, then sendrawtransaction. This may result in the user thinking they have been paid by their counterparty, even though it was really just their change!

This could obviously also result in needless keyreuse.

TheBlueMatt force-pushed on Jul 10, 2017

laanwj added the label Wallet on Jul 11, 2017

laanwj added the label RPC/REST/ZMQ on Jul 11, 2017

jonasschnelli commented at 8:34 am on July 11, 2017: contributor

History of fundrawtransaction regarding change-output: Before 0.14, the change-output keys was never reserved from the key pool (flaw-ish). Since 0.14, by default, the key will be reserved but there is an option to not reserve it (keep the old behaviour) done via #9377. The option was added because the assumption was that API consumers relay on this old, flaw-ish, behaviour.

This PR would basically remove the option to not reserve the key.

I think in general we should do that, though I’m not sure if there are any API consumers who expect that one can avoid reserving the CO-key. But indeed, that should stop.

Concept ACK 6715e782c3049ec43c0c94268f9ad924988e5ddf. PR should have a short release-notes description.

meshcollider commented at 10:24 am on July 11, 2017: contributor

Concept ACK, seems extremely sensible

TheBlueMatt commented at 3:10 pm on July 11, 2017: member

Would be nice to get an 0.15 tag on this - I think its quite a serious API flaw (which I commented positively on on #9377 :(. Indeed, will need release notes, but I’ll leave that for #9889.

laanwj added this to the milestone 0.15.0 on Jul 11, 2017

in test/functional/fundrawtransaction.py:663 in 6715e782c3 outdated

658-            if out['value'] > 1.0:
659-                changeaddress += out['scriptPubKey']['addresses'][0]
660-        assert(changeaddress != "")
661-        nextaddr = self.nodes[3].getnewaddress()
662-        # Now the change address key should be removed from the keypool
663-        assert(changeaddress != nextaddr)

morcos commented at 9:00 pm on July 12, 2017:

You should keep the bottom half of this test to show we’re not getting address reuse

morcos commented at 9:00 pm on July 12, 2017: member

Alex was here

TheBlueMatt force-pushed on Jul 12, 2017

fanquake deleted a comment on Jul 12, 2017

sipa commented at 0:58 am on July 13, 2017: member

Pieter was here.

jonasschnelli commented at 11:55 am on July 18, 2017: contributor

Jonas was here (though wants rebase).

Do not allow users to get keys from keypool without reserving them

fundrawtransaction allows users to add a change output and then
not have it removed from keypool. While it would be nice to have
users follow the normal CreateTransaction/CommitTransaction process
we use internally, there isnt much benefit in exposing this option,
especially with HD wallets, while there is ample room for users to
misunderstand or misuse this option.

This could be particularly nasty in some use-cases (especially
pre-HD-split) - eg a user might fundrawtransaction, then call
getnewaddress, hand out the address for someone to pay them, then
sendrawtransaction. This may result in the user thinking they have
received payment, even though it was really just their own change!

This could obviously result in needless key-reuse.

cf82a9e704

TheBlueMatt force-pushed on Jul 18, 2017

TheBlueMatt commented at 3:20 pm on July 18, 2017: member

Rebased.

laanwj merged this on Jul 18, 2017

laanwj closed this on Jul 18, 2017

laanwj referenced this in commit 9e8d6a3fb4 on Jul 18, 2017

jasonbcox referenced this in commit eaab0222ef on May 3, 2019

jonspock referenced this in commit 37c84d4965 on Jun 5, 2019

PastaPastaPasta referenced this in commit f639a6a16f on Sep 8, 2019

PastaPastaPasta referenced this in commit 443b577931 on Sep 8, 2019

barrystyle referenced this in commit d507557171 on Jan 22, 2020

random-zebra referenced this in commit 32db35fb08 on Mar 11, 2021

random-zebra referenced this in commit f2827ea6c5 on Mar 12, 2021

random-zebra referenced this in commit 59f27ed3b5 on Mar 18, 2021

random-zebra referenced this in commit 7aa3a2158a on Mar 22, 2021

random-zebra referenced this in commit 284c5a7524 on Mar 25, 2021

random-zebra referenced this in commit ca3cd7df5e on Mar 25, 2021

random-zebra referenced this in commit 4337b0e802 on Mar 25, 2021

random-zebra referenced this in commit dd660df590 on Mar 25, 2021

random-zebra referenced this in commit c0f64c91de on Mar 25, 2021

random-zebra referenced this in commit fcf958ff1c on Mar 26, 2021

random-zebra referenced this in commit f92b17d375 on Mar 26, 2021

random-zebra referenced this in commit 6479d21886 on Mar 31, 2021

random-zebra referenced this in commit 3c7b524873 on Mar 31, 2021

random-zebra referenced this in commit fad741a624 on Apr 5, 2021

random-zebra referenced this in commit 9f31559c82 on Apr 6, 2021

random-zebra referenced this in commit 286a35e590 on Apr 7, 2021

MarcoFalke locked this on Sep 8, 2021

Do not allow users to get keys from keypool without reserving them #10784