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

pull TheBlueMatt wants to merge 1 commits into bitcoin:master from TheBlueMatt:2017-07-keep-change changing 4 files +13 −27
  1. 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.

  2. TheBlueMatt force-pushed on Jul 10, 2017
  3. 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.

  4. TheBlueMatt force-pushed on Jul 10, 2017
  5. TheBlueMatt force-pushed on Jul 10, 2017
  6. laanwj added the label Wallet on Jul 11, 2017
  7. laanwj added the label RPC/REST/ZMQ on Jul 11, 2017
  8. 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.

  9. meshcollider commented at 10:24 am on July 11, 2017: contributor
    Concept ACK, seems extremely sensible
  10. 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.
  11. laanwj added this to the milestone 0.15.0 on Jul 11, 2017
  12. 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
  13. morcos commented at 9:00 pm on July 12, 2017: member
    Alex was here
  14. TheBlueMatt force-pushed on Jul 12, 2017
  15. fanquake deleted a comment on Jul 12, 2017
  16. sipa commented at 0:58 am on July 13, 2017: member
    Pieter was here.
  17. jonasschnelli commented at 11:55 am on July 18, 2017: contributor
    Jonas was here (though wants rebase).
  18. 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
  19. TheBlueMatt force-pushed on Jul 18, 2017
  20. TheBlueMatt commented at 3:20 pm on July 18, 2017: member
    Rebased.
  21. laanwj merged this on Jul 18, 2017
  22. laanwj closed this on Jul 18, 2017
  23. laanwj referenced this in commit 9e8d6a3fb4 on Jul 18, 2017
  24. jasonbcox referenced this in commit eaab0222ef on May 3, 2019
  25. jonspock referenced this in commit 37c84d4965 on Jun 5, 2019
  26. PastaPastaPasta referenced this in commit f639a6a16f on Sep 8, 2019
  27. PastaPastaPasta referenced this in commit 443b577931 on Sep 8, 2019
  28. barrystyle referenced this in commit d507557171 on Jan 22, 2020
  29. random-zebra referenced this in commit 32db35fb08 on Mar 11, 2021
  30. random-zebra referenced this in commit f2827ea6c5 on Mar 12, 2021
  31. random-zebra referenced this in commit 59f27ed3b5 on Mar 18, 2021
  32. random-zebra referenced this in commit 7aa3a2158a on Mar 22, 2021
  33. random-zebra referenced this in commit 284c5a7524 on Mar 25, 2021
  34. random-zebra referenced this in commit ca3cd7df5e on Mar 25, 2021
  35. random-zebra referenced this in commit 4337b0e802 on Mar 25, 2021
  36. random-zebra referenced this in commit dd660df590 on Mar 25, 2021
  37. random-zebra referenced this in commit c0f64c91de on Mar 25, 2021
  38. random-zebra referenced this in commit fcf958ff1c on Mar 26, 2021
  39. random-zebra referenced this in commit f92b17d375 on Mar 26, 2021
  40. random-zebra referenced this in commit 6479d21886 on Mar 31, 2021
  41. random-zebra referenced this in commit 3c7b524873 on Mar 31, 2021
  42. random-zebra referenced this in commit fad741a624 on Apr 5, 2021
  43. random-zebra referenced this in commit 9f31559c82 on Apr 6, 2021
  44. random-zebra referenced this in commit 286a35e590 on Apr 7, 2021
  45. MarcoFalke locked this on Sep 8, 2021


TheBlueMatt jonasschnelli meshcollider morcos sipa

Labels
Wallet RPC/REST/ZMQ

Milestone
0.15.0

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: 2024-10-30 03:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me