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.
Not sure about this.. What is the rationale?
Also, could test the new field.
importmulti where this can be set.
3539@@ -3540,6 +3540,7 @@ UniValue getaddressinfo(const JSONRPCRequest& request)
3540 " \"ismine\" : true|false, (boolean) If the address is yours or not\n"
3541 " \"iswatchonly\" : true|false, (boolean) If the address is watchonly\n"
3542 " \"isscript\" : true|false, (boolean) If the key is a script\n"
3543+ " \"ischange\" : true|false, (boolean) If the address is for change output\n"
If the address was used for change output?
@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?
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.
960@@ -961,6 +961,7 @@ class CWallet final : public CCryptoKeyStore, public CValidationInterface
961 isminetype IsMine(const CTxOut& txout) const;
962 CAmount GetCredit(const CTxOut& txout, const isminefilter& filter) const;
963 bool IsChange(const CTxOut& txout) const;
964+ bool IsChange(const CScript& dest) const;
Please make the parameter naming between those two consistent:
0bool CWallet::IsChange(const CScript& script) const { … }
1bool IsChange(const CScript& dest) const;
Make both script or both dest :-)
ischange field to wallet_address_types.py functional test.
158@@ -159,6 +159,13 @@ def test_change_output_type(self, node_sender, destinations, expected_type):
159 change_addresses = [d for d in output_addresses if d not in destinations]
160 assert_equal(len(change_addresses), 1)
161
162+ # Check if 'getaddressinfo' correctly marks change address as change:
163+ for d in output_addresses:
164+ if d in destinations:
nit,
0for address in output_addresses
1 assert_equal(self.nodes[node_sender].getaddressinfo(address)['ischange'], address not in destinations)
158@@ -159,6 +159,13 @@ def test_change_output_type(self, node_sender, destinations, expected_type):
159 change_addresses = [d for d in output_addresses if d not in destinations]
160 assert_equal(len(change_addresses), 1)
161
162+ # Check if 'getaddressinfo' correctly marks change address as change:
test_change_output_type? Maybe somewhere in wallet_basic.py?
wallet_basic.py with some additional code to ensure transaction with change.
utACK 9298081df8a9b538cf5b572fd99f7063a67dae76
No reason why this can’t be exposed.
Could test that setting a label for a change address sets ischange to false. This is the test change:
0@@ -497,7 +497,12 @@ class WalletTest(BitcoinTestFramework):
1 output_addresses = [vout['scriptPubKey']['addresses'][0] for vout in tx["vout"]]
2 assert len(output_addresses) > 1
3 for address in output_addresses:
4- assert_equal(self.nodes[0].getaddressinfo(address)['ischange'], address != destination)
5+ ischange = self.nodes[0].getaddressinfo(address)['ischange']
6+ assert_equal(ischange, address != destination)
7+ if ischange: change = address
8+ self.nodes[0].setlabel(change, 'foobar')
9+ assert_equal(self.nodes[0].getaddressinfo(change)['ischange'], False)
10
11 if __name__ == '__main__':
12 WalletTest().main()
Also could add a small release note? Maybe in doc/release-notes-14282.md?
Tested ACK 9298081.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
No conflicts as of last run.
