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

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
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

Owners added a reviewer: Restricted Owners Package.Apr 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

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

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

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.

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

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

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.

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

This revision is now accepted and ready to land.Apr 29 2019, 21:04

Broke up long string into multiple lines.

Forgot to squash changes.

This revision was automatically updated to reflect the committed changes.