Page MenuHomePhabricator

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

Authored by nakihito on Apr 20 2019, 01:32.

Details

Reviewers
deadalnix
Fabien
jasonbcox
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Restricted Project
Commits
rABCeaab0222ef8e: Do not allow users to get keys from keypool without reserving them
Summary

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.

Backport of Core PR 10784
https://github.com/bitcoin/bitcoin/pull/10784/

Completes T609

Test Plan

make check
test_runner.py

Diff Detail

Repository
rABC Bitcoin ABC
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

nakihito created this revision.Apr 20 2019, 01:32
Owners added a reviewer: Restricted Owners Package.Apr 20 2019, 01:32
Herald added a reviewer: Restricted Project. · View Herald TranscriptApr 20 2019, 01:32
jasonbcox requested changes to this revision.Apr 20 2019, 02:02
jasonbcox added inline comments.
src/wallet/rpcwallet.cpp
3468 ↗(On Diff #8179)

This deserves a mention in release notes

3537 ↗(On Diff #8179)

Version number needs to be next major version for ABC

This revision now requires changes to proceed.Apr 20 2019, 02:02
nakihito marked 2 inline comments as done.Apr 21 2019, 18:22
nakihito updated this revision to Diff 8195.Apr 21 2019, 18:24

Added release note and adjusted version number depreciation note in rpcwallet.cpp.

Fabien requested changes to this revision.Apr 22 2019, 11:12

You can add a deprecatedrpc=fundrawtransaction option to allow users to transition between the versions.

src/wallet/rpcwallet.cpp
3537 ↗(On Diff #8195)

Next major is 0.20

This revision now requires changes to proceed.Apr 22 2019, 11:12
nakihito updated this revision to Diff 8243.Apr 23 2019, 18:51

Deprecated -reserveChangeKey option rather than removing it. Changed deprecation comment to reflect next major update. Rebased.

jasonbcox requested changes to this revision.Apr 23 2019, 20:04
jasonbcox added inline comments.
src/wallet/wallet.h
992 ↗(On Diff #8243)

There doesn't appear to be a change here. revert. shouldn't the linter catch this?

test/functional/rpc_fundrawtransaction.py
690 ↗(On Diff #8243)

Add another note that this should be removed in 0.20 like the other comments

This revision now requires changes to proceed.Apr 23 2019, 20:04
nakihito marked 2 inline comments as done.Apr 23 2019, 20:15

Also making more changes as recommended by Fabien.

src/wallet/wallet.h
992 ↗(On Diff #8243)

This was actually caused by the linter, oddly enough. Will revert.

nakihito updated this revision to Diff 8252.Apr 23 2019, 22:55

Changed location of deprecation check, removed excess deprecation checks, added new test case.

Fabien requested changes to this revision.Apr 24 2019, 16:04

If -deprecatedrpc=fundrawtransaction is set and the reserveChangeKey option is true, you end up removing the key twice.

This revision now requires changes to proceed.Apr 24 2019, 16:04
nakihito updated this revision to Diff 8289.Apr 26 2019, 18:24

Added rpc deprecation check to make sure address isn't removed twice in certain situations.

Fabien accepted this revision.Apr 26 2019, 18:38
Fabien added inline comments.
src/wallet/wallet.cpp
22 ↗(On Diff #8289)

Please add a comment // for IsDeprecatedRPCEnabled so it' clear it can be removed when no longer needed.

nakihito updated this revision to Diff 8290.Apr 26 2019, 18:55

Added comment to <rpc/server> include to note it can be removed once the deprecated command is removed.

jasonbcox requested changes to this revision.Apr 26 2019, 21:00
jasonbcox added inline comments.
src/wallet/rpcwallet.cpp
3468 ↗(On Diff #8290)

remove the default true as it was never the case in the first place and isn't the case now.

test/functional/rpc_fundrawtransaction.py
717 ↗(On Diff #8290)

Please check why lint isn't being applied here. I tried it on my machine and it also failed to lint. My version numbers are:
autopep8 1.3.5 (pycodestyle: 2.4.0)
3.5.0 (mccabe: 0.6.1, pycodestyle: 2.3.1, pyflakes: 1.6.0) CPython 3.6.5 on Linux

This revision now requires changes to proceed.Apr 26 2019, 21:00
nakihito added inline comments.Apr 29 2019, 17:21
test/functional/rpc_fundrawtransaction.py
717 ↗(On Diff #8290)

As per @Fabien, the long string is possibly a feature rather than a bug. I'll try to break it down into smaller lines manually.

jasonbcox accepted this revision.Apr 29 2019, 21:04
This revision is now accepted and ready to land.Apr 29 2019, 21:04
nakihito updated this revision to Diff 8304.Apr 30 2019, 01:32

Broke up long string into multiple lines.

nakihito updated this revision to Diff 8305.Apr 30 2019, 01:36

Forgot to squash changes.

This revision was automatically updated to reflect the committed changes.