Skip to content

rpcwallet: 'ischange' field for 'getaddressinfo' RPC #14410

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.

Sign up for GitHub

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

Jump to bottom
Merged
maflcko merged 2 commits into from
Nov 4, 2018
Merged

rpcwallet: 'ischange' field for 'getaddressinfo' RPC #14410

maflcko merged 2 commits into from
Nov 4, 2018

Conversation

@rodentrabies
Copy link
Contributor

@rodentrabies rodentrabies commented Oct 5, 2018

Implementation of proposal in #14396.

This introduces CWallet::IsChange(CScript&) method and replaces original CWallet::IsChange(CTxOut&) method with overloaded version that delegates to the new method with txout's scriptPubKey. In this way TODO note from the original method can still be addressed in a single place.

@rodentrabies rodentrabies force-pushed the getaddressinfo-is-change branch from 5175317 to c2f4b33 Compare October 6, 2018 08:14
@promag
Copy link
Contributor

promag commented Oct 7, 2018

Not sure about this.. What is the rationale?

Also, could test the new field.

@instagibbs
Copy link
Member

instagibbs commented Oct 8, 2018

concept ACK, needs test.

@promag Why not? It's useful to probe the state of the wallet, especially when using things like importmulti where this can be set.

promag
promag reviewed Oct 8, 2018
View reviewed changes
Copy link
Contributor

@promag promag left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@instagibbs because it can be misleading? Especially if you fundrawtransaction with explicit changeAddress

especially when using things like importmulti where this can be set.

I'm not following, can you explain?

src/wallet/rpcwallet.cpp Outdated
Copy link
Contributor

@promag promag Oct 8, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reading this looks like the address can be reused. Suggestion If the address was used for change output?

Copy link
Contributor Author

@rodentrabies rodentrabies Oct 9, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the suggestion. Fixed that.

@achow101
Copy link
Member

achow101 commented Oct 9, 2018

especially when using things like importmulti where this can be set.

I'm not following, can you explain?

importmulti lets you mark imported things as internal, i.e. change. It can be useful to know whether something is change, especially if it was imported and so other ways of identifying change addresses may not be useful.

@rodentrabies rodentrabies force-pushed the getaddressinfo-is-change branch from c2f4b33 to ee014ae Compare October 9, 2018 12:09
practicalswift
practicalswift reviewed Oct 10, 2018
View reviewed changes
src/wallet/wallet.h Outdated
Copy link
Contributor

@practicalswift practicalswift Oct 10, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please make the parameter naming between those two consistent:

bool CWallet::IsChange(const CScript& script) const { … }
bool IsChange(const CScript& dest) const;

Make both script or both dest :-)

Copy link
Contributor Author

@rodentrabies rodentrabies Oct 10, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@rodentrabies rodentrabies force-pushed the getaddressinfo-is-change branch from ee014ae to 70e9899 Compare October 10, 2018 09:41
@rodentrabies
Copy link
Contributor Author

rodentrabies commented Oct 12, 2018

I'm not sure it needs a dedicated test, so I added testing of ischange field to wallet_address_types.py functional test.

promag
promag reviewed Oct 12, 2018
View reviewed changes
Copy link
Contributor

@promag promag left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree that there's no need for a dedicated test.

test/functional/wallet_address_types.py Outdated
Copy link
Contributor

@promag promag Oct 12, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit,

for address in output_addresses
    assert_equal(self.nodes[node_sender].getaddressinfo(address)['ischange'], address not in destinations)

Copy link
Contributor Author

@rodentrabies rodentrabies Oct 12, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, with some changes though:)

test/functional/wallet_address_types.py Outdated
Copy link
Contributor

@promag promag Oct 12, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe this does not belong here — test_change_output_type? Maybe somewhere in wallet_basic.py?

Copy link
Contributor Author

@rodentrabies rodentrabies Oct 12, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved it to wallet_basic.py with some additional code to ensure transaction with change.

@rodentrabies rodentrabies force-pushed the getaddressinfo-is-change branch 3 times, most recently from 1aa8b91 to 9298081 Compare October 12, 2018 15:37
@sipa
Copy link
Member

sipa commented Oct 12, 2018

utACK 9298081df8a9b538cf5b572fd99f7063a67dae76

No reason why this can't be exposed.

promag
promag reviewed Oct 12, 2018
View reviewed changes
Copy link
Contributor

@promag promag left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could test that setting a label for a change address sets ischange to false. This is the test change:

@@ -497,7 +497,12 @@ class WalletTest(BitcoinTestFramework):
         output_addresses = [vout['scriptPubKey']['addresses'][0] for vout in tx["vout"]]
         assert len(output_addresses) > 1
         for address in output_addresses:
-            assert_equal(self.nodes[0].getaddressinfo(address)['ischange'], address != destination)
+            ischange = self.nodes[0].getaddressinfo(address)['ischange']
+            assert_equal(ischange, address != destination)
+            if ischange: change = address
+        self.nodes[0].setlabel(change, 'foobar')
+        assert_equal(self.nodes[0].getaddressinfo(change)['ischange'], False)

 if __name__ == '__main__':
     WalletTest().main()

Also could add a small release note? Maybe in doc/release-notes-14282.md?

Tested ACK 9298081.

@rodentrabies rodentrabies force-pushed the getaddressinfo-is-change branch from 9298081 to 9a7ccca Compare October 13, 2018 16:32
@rodentrabies
Copy link
Contributor Author

rodentrabies commented Oct 13, 2018 •
edited
Loading

@promag done and done (amended release node into first commit and test fix into second one). Let me know if release note is ok.

@rodentrabies rodentrabies force-pushed the getaddressinfo-is-change branch from 9a7ccca to 14a0652 Compare October 13, 2018 16:38
@DrahtBot DrahtBot mentioned this pull request Oct 20, 2018
Closed
@DrahtBot
Copy link
Contributor

DrahtBot commented Oct 20, 2018 •
edited
Loading

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

No conflicts as of last run.

@practicalswift practicalswift mentioned this pull request Oct 22, 2018
Closed
@promag
Copy link
Contributor

promag commented Nov 4, 2018 •
edited
Loading

utACK 14a0652.

@meshcollider
Copy link
Contributor

meshcollider commented Nov 4, 2018

utACK 14a0652

@maflcko maflcko merged commit 14a0652 into bitcoin:master Nov 4, 2018
maflcko pushed a commit that referenced this pull request Nov 4, 2018
Merge #14410: rpcwallet: 'ischange' field for 'getaddressinfo' RPC
b740788 
14a0652 tests: add test for 'getaddressinfo' RPC result 'ischange' field (whythat)
93d1aa9 rpcwallet: add 'ischange' field to 'getaddressinfo' response (whythat)

Pull request description:

  Implementation of proposal in #14396.

  This introduces `CWallet::IsChange(CScript&)` method and replaces original `CWallet::IsChange(CTxOut&)` method with overloaded version that delegates to the new method with *txout*'s `scriptPubKey`. In this way `TODO` note from the original method can still be addressed in a single place.

Tree-SHA512: ef5dbc82d76b4b9b2fa6a70abc3385a677c55021f79e187ee2f392ee32bc6b406191f4129acae5c17b0206e72b6712e7e0cad574a4bbd966871c2e656c45e041
@rodentrabies rodentrabies deleted the getaddressinfo-is-change branch November 4, 2018 22:45
@bitcoin bitcoin deleted a comment from ismail120572 Nov 5, 2018
@bitcoin bitcoin locked and limited conversation to collaborators Nov 5, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

2 more reviewers

@promag promag promag left review comments

@practicalswift practicalswift practicalswift left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

RPC/REST/ZMQ Wallet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

10 participants

@rodentrabies @promag @instagibbs @achow101 @sipa @DrahtBot @meshcollider @practicalswift @fanquake @maflcko