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

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

Merged
merged 2 commits into from Nov 4, 2018

Conversation

Projects
None yet
10 participants
@mrwhythat
Contributor

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

@mrwhythat mrwhythat force-pushed the mrwhythat:getaddressinfo-is-change branch from 5175317 to c2f4b33 Oct 6, 2018

@promag

This comment has been minimized.

Member

promag commented Oct 7, 2018

Not sure about this.. What is the rationale?

Also, could test the new field.

@instagibbs

This comment has been minimized.

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

@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?

@@ -3540,6 +3540,7 @@ UniValue getaddressinfo(const JSONRPCRequest& request)
" \"ismine\" : true|false, (boolean) If the address is yours or not\n"
" \"iswatchonly\" : true|false, (boolean) If the address is watchonly\n"
" \"isscript\" : true|false, (boolean) If the key is a script\n"
" \"ischange\" : true|false, (boolean) If the address is for change output\n"

This comment has been minimized.

@promag

promag Oct 8, 2018

Member

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

This comment has been minimized.

@mrwhythat

mrwhythat Oct 9, 2018

Contributor

Thanks for the suggestion. Fixed that.

@achow101

This comment has been minimized.

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.

@mrwhythat mrwhythat force-pushed the mrwhythat:getaddressinfo-is-change branch from c2f4b33 to ee014ae Oct 9, 2018

@@ -961,6 +961,7 @@ class CWallet final : public CCryptoKeyStore, public CValidationInterface
isminetype IsMine(const CTxOut& txout) const;
CAmount GetCredit(const CTxOut& txout, const isminefilter& filter) const;
bool IsChange(const CTxOut& txout) const;
bool IsChange(const CScript& dest) const;

This comment has been minimized.

@practicalswift

practicalswift Oct 10, 2018

Member

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 :-)

This comment has been minimized.

@mrwhythat

mrwhythat Oct 10, 2018

Contributor

Done.

@mrwhythat mrwhythat force-pushed the mrwhythat:getaddressinfo-is-change branch from ee014ae to 70e9899 Oct 10, 2018

@mrwhythat

This comment has been minimized.

Contributor

mrwhythat 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

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

@@ -159,6 +159,13 @@ def test_change_output_type(self, node_sender, destinations, expected_type):
change_addresses = [d for d in output_addresses if d not in destinations]
assert_equal(len(change_addresses), 1)
# Check if 'getaddressinfo' correctly marks change address as change:
for d in output_addresses:
if d in destinations:

This comment has been minimized.

@promag

promag Oct 12, 2018

Member

nit,

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

This comment has been minimized.

@mrwhythat

mrwhythat Oct 12, 2018

Contributor

Done, with some changes though:)

@@ -159,6 +159,13 @@ def test_change_output_type(self, node_sender, destinations, expected_type):
change_addresses = [d for d in output_addresses if d not in destinations]
assert_equal(len(change_addresses), 1)
# Check if 'getaddressinfo' correctly marks change address as change:

This comment has been minimized.

@promag

promag Oct 12, 2018

Member

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

This comment has been minimized.

@mrwhythat

mrwhythat Oct 12, 2018

Contributor

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

@mrwhythat mrwhythat force-pushed the mrwhythat:getaddressinfo-is-change branch 3 times, most recently from 1aa8b91 to 9298081 Oct 12, 2018

@sipa

This comment has been minimized.

Member

sipa commented Oct 12, 2018

utACK 9298081

No reason why this can't be exposed.

@promag

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.

@mrwhythat mrwhythat force-pushed the mrwhythat:getaddressinfo-is-change branch from 9298081 to 9a7ccca Oct 13, 2018

@mrwhythat

This comment has been minimized.

Contributor

mrwhythat commented Oct 13, 2018

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

@mrwhythat mrwhythat force-pushed the mrwhythat:getaddressinfo-is-change branch from 9a7ccca to 14a0652 Oct 13, 2018

@DrahtBot DrahtBot referenced this pull request Oct 20, 2018

Closed

Rpc help helper class #14502

@DrahtBot

This comment has been minimized.

Contributor

DrahtBot commented Oct 20, 2018

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

Conflicts

No conflicts as of last run.

@promag

This comment has been minimized.

Member

promag commented Nov 4, 2018

utACK 14a0652.

@MeshCollider

This comment has been minimized.

Member

MeshCollider commented Nov 4, 2018

utACK 14a0652

@MarcoFalke MarcoFalke merged commit 14a0652 into bitcoin:master Nov 4, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

MarcoFalke added a commit that referenced this pull request Nov 4, 2018

Merge #14410: rpcwallet: 'ischange' field for 'getaddressinfo' RPC
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

@mrwhythat mrwhythat deleted the mrwhythat:getaddressinfo-is-change branch Nov 4, 2018

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