Details

Reviewers

deadalnix
Fabien
jasonbcox

Group Reviewers

Restricted Owners Package	(Owns No Changed Paths)
Restricted Project

Commits

rSTAGINGeaab0222ef8e: Do not allow users to get keys from keypool without reserving them
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

Branch

BackportPR10784

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 5614
Build 9290: Bitcoin ABC Buildbot (legacy)
Build 9289: arc lint + arc unit

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

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing started.

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing passed.

Harbormaster completed remote builds in B5538: Diff 8179.Apr 20 2019, 01:48

nakihito added a child revision: D2847: Fix typos..Apr 20 2019, 01:59

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

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

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing started.

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing passed.

Harbormaster completed remote builds in B5547: Diff 8195.Apr 21 2019, 18:38

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.

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing started.

Harbormaster completed remote builds in B5584: Diff 8243.Apr 23 2019, 19:05

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing passed.

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.

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing started.

typo

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing started.

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing passed.

Harbormaster completed remote builds in B5586: Diff 8252.Apr 23 2019, 23:10

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing passed.

Harbormaster completed remote builds in B5587: Diff 8253.Apr 23 2019, 23:19

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.

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing started.

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.

Harbormaster completed remote builds in B5613: Diff 8289.Apr 26 2019, 18:40

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing passed.

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

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing started.

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing passed.

Harbormaster completed remote builds in B5614: Diff 8290.Apr 26 2019, 19:10

jasonbcox requested changes to this revision.Apr 26 2019, 21:00

jasonbcox added inline comments.

src/wallet/rpcwallet.cpp
3468	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	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