New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from Jul 18, 2017

Conversation

Projects
None yet
6 participants
@TheBlueMatt
Contributor

TheBlueMatt commented Jul 10, 2017

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

This comment has been minimized.

Show comment
Hide comment
@TheBlueMatt

TheBlueMatt Jul 10, 2017

Contributor

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.

Contributor

TheBlueMatt commented Jul 10, 2017

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.

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Jul 11, 2017

Member

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 6715e78.
PR should have a short release-notes description.

Member

jonasschnelli commented Jul 11, 2017

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 6715e78.
PR should have a short release-notes description.

@MeshCollider

This comment has been minimized.

Show comment
Hide comment
@MeshCollider

MeshCollider Jul 11, 2017

Member

Concept ACK, seems extremely sensible

Member

MeshCollider commented Jul 11, 2017

Concept ACK, seems extremely sensible

@TheBlueMatt

This comment has been minimized.

Show comment
Hide comment
@TheBlueMatt

TheBlueMatt Jul 11, 2017

Contributor

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.

Contributor

TheBlueMatt commented Jul 11, 2017

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 laanwj added this to the 0.15.0 milestone Jul 11, 2017

@morcos

This comment has been minimized.

Show comment
Hide comment
@morcos

morcos Jul 12, 2017

Member

Alex was here

Member

morcos commented Jul 12, 2017

Alex was here

@bitcoin bitcoin deleted a comment Jul 12, 2017

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Jul 13, 2017

Member

Pieter was here.

Member

sipa commented Jul 13, 2017

Pieter was here.

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Jul 18, 2017

Member

Jonas was here (though wants rebase).

Member

jonasschnelli commented Jul 18, 2017

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

This comment has been minimized.

Show comment
Hide comment
@TheBlueMatt

TheBlueMatt Jul 18, 2017

Contributor

Rebased.

Contributor

TheBlueMatt commented Jul 18, 2017

Rebased.

@TheBlueMatt TheBlueMatt referenced this pull request Jul 18, 2017

Closed

TODO for release notes 0.15.0 #9889

12 of 12 tasks complete

@laanwj laanwj merged commit cf82a9e into bitcoin:master Jul 18, 2017

1 check was pending

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details

laanwj added a commit that referenced this pull request Jul 18, 2017

Merge #10784: Do not allow users to get keys from keypool without res…
…erving them

cf82a9e Do not allow users to get keys from keypool without reserving them (Matt Corallo)

Pull request description:

  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.

Tree-SHA512: 72b5ee9c4a229b84d799dfb00c56fe80d8bba914ce81a433c3f5ab325bf9bf2b839ee658c261734f0ee183ab19435039481014d09c41dbe155e6323e63beb01d
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment