Bugfix: Wallet: Safely deal with change in the address book [part 2]

luke-jr commented at 7:58 pm on April 6, 2020: member

Follow-up to #18192, not strictly necessary for 0.20

in src/wallet/wallet.cpp:3225 in 0ee66579df outdated

3220@@ -3221,7 +3221,10 @@ bool CWallet::DelAddressBook(const CTxDestination& address)
3221     // If we want to delete receiving addresses, we need to take care that DestData "used" (and possibly newer DestData) gets preserved (and the "deleted" address transformed into a change entry instead of actually being deleted)
3222     // NOTE: This isn't a problem for sending addresses because they never have any DestData yet!
3223     // When adding new DestData, it should be considered here whether to retain or delete it (or move it?).
3224-    assert(!IsMine(address));
3225+    if (IsMine(address)) {
3226+        WalletLogPrintf("%s called with IsMine address, NOT SUPPORTED. Please report this bug!\n", __func__);

MarcoFalke commented at 8:01 pm on April 6, 2020:

0        WalletLogPrintf("%s:%d (%s) called with IsMine address, NOT SUPPORTED. Please report this bug! %s\n", __FILE__, __LINE__, __func__, PACKAGE_BUGREPORT);

Could mention where to report the bug?

luke-jr commented at 8:11 pm on April 6, 2020:

Not sure file/line are meaningful here… Added PACKAGE_BUGREPORT

in src/wallet/wallet.h:196 in 0ee66579df outdated

193 
194     typedef std::map<std::string, std::string> StringMap;
195     StringMap destdata;
196 
197     bool IsChange() const { return m_change; }
198+    const std::string& GetLabel() const { return m_label; }

MarcoFalke commented at 8:02 pm on April 6, 2020:

0    std::string GetLabel() const { return m_label; }

To avoid issues related to #17198

luke-jr commented at 8:13 pm on April 6, 2020:

I don’t see how that relates to this. Can you give an example?

MarcoFalke commented at 8:21 pm on April 6, 2020:

Just the general idea that:

This doesn’t improve performance
This might lead to use-after-free in future code

in test/functional/wallet_avoidreuse.py:148 in 0ee66579df outdated

143+
144+        reset_balance(node, node.getnewaddress())
145+        addr = node.getnewaddress()
146+        txid = node.sendtoaddress(addr, 1)
147+        out = node.listunspent(minconf=0, query_options={'minimumAmount': 2})
148+        assert len(out) == 1

MarcoFalke commented at 8:03 pm on April 6, 2020:

0        assert_equal(len(out), 1)

To aid debugging in case of failure

MarcoFalke approved

MarcoFalke commented at 8:04 pm on April 6, 2020: member

ACK, just some style nits

luke-jr force-pushed on Apr 6, 2020

MarcoFalke commented at 8:17 pm on April 6, 2020: member

ACK a7051b046a, only change is adding issue url and a test style fixup 🍖

Signature:

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3ACK a7051b046a, only change is adding issue url and a test style fixup 🍖
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
 7pUjnBgwAgCMSg9b7BKGlOp9Lhs+tyN4elzNwljsK69gBnI+0djGfCCWnQ7mw73E+
 8LhHEVypuQwLFMy5XgEUx5J1yKrfC9M8Z+wuT8GuVOla86XEYsZ5rVRmTIGtfmwwi
 9TtkxrAT0sg6O/u7vE6DLnTpEdMsd37B/j7t++dmdWjdh5tANWONHUrtL7+G1dnTu
10AFVKHFmlacPJc8mVfzqrLjbDKjquFlBVAGf2fxrzjxseJGybsanr9Y+ITiIeJ4vM
11ba/yYXagM25/RCsd2GMWSiADpOIHjwQ2GXkSFaFevk1K5ySbDmueA/yjukbhW5s7
12NMasUWQgU4wqzZanOiXtZO6M2wxmmXQcsWvBNt7loy1XceVV2m3GkL+JgcjhwNmm
13Wjk0OwS2DBKMPrGeU6en28TfgylbfyRy8Rglsmnqx52jGreaFuXGR6wdKmd5N/lm
147FcH/T0a6BVCGlW3/h+6gEXo8oAqDOCeHxoMT+oa+zg4JO6TXyNXsF4HpVolZlqR
15adBcWCOK
16=sHmd
17-----END PGP SIGNATURE-----

Timestamp of file with hash 20931cd3fd37cc3e39e8dda70e27cd4ebf620a0462cabe7b49ab467c15609ce5 -

QA: Test that change doesn't turn into non-change when spent in an avoid-reuse wallet d7092c392e

Wallet: Replace CAddressBookData.name with GetLabel() method 2952c46b92

Wallet: Change IsMine check in CWallet::DelAddressBook from assert to failure 7a2ecf16df

in test/functional/wallet_avoidreuse.py:146 in a7051b046a outdated

137@@ -137,6 +138,29 @@ def test_immutable(self):
138         # Unload temp wallet
139         self.nodes[1].unloadwallet(tempwallet)
140 
141+    def test_change_remains_change(self, node):
142+        self.log.info("Test that change doesn't turn into non-change when spent")
143+
144+        reset_balance(node, node.getnewaddress())
145+        addr = node.getnewaddress()
146+        txid = node.sendtoaddress(addr, 1)

MarcoFalke commented at 8:25 pm on April 6, 2020:

Linter fails

0test/functional/wallet_avoidreuse.py:146:9: F841 local variable 'txid' is assigned to but never used

luke-jr force-pushed on Apr 6, 2020

MarcoFalke commented at 8:55 pm on April 6, 2020: member

re-ACK 7a2ecf16df, only change is adding an assert_equal in the test 🔰

Signature:

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3re-ACK 7a2ecf16df, only change is adding an assert_equal in the test 🔰
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
 7pUhbPAwAnxp3dHSnOH/9xKxdNaamDfRFxNnBiwE/BbHRVU79mP3Lned6OILzDgEm
 8ESHtX5jbEI/Ot0ZgVy2KNKU8inVVIsuX3CLo2FyOduzijpesTDCqPBxxPZZW8qqP
 9ieUdoe02jXwrmlAiZTCQOph6XEQv2efI5TyQYAJk5zuFW+w6+si3VycbEbvyuc3u
10nV9gd/+0fr0TQQ6+C0HtRpHxNgUsYjPjVuPuEddt/qz4ZyO9oGyRdVl+rpV2pcOA
11bkulHqz8VN9WAAms0L2PQ3/3ZAAw7lOmTfUNqJPYd2QjbQAXC8G61eXZ6cqJLS9t
129bFYlgxyYCv/Ok7Y4rBZWFGoAUlplH79U9CSU8q5IABqTTfaqDSnhOgbS8ZL9xmi
13GXx9TVSvk05z1J1ACjB+Aj/17rev+TWZyzUQE9N8YXvzXtH4I6F9YrJflvfyIX+a
1434rKjjh3PAYBCBdxzV1OiKzQZtLjkQCua9i1KZL4NZy8t2DY029trTgyLt2rE/HL
15qrTWKMSm
16=d7Nv
17-----END PGP SIGNATURE-----

Timestamp of file with hash 6dc1f2ac8bbd8dde3e44f0db23fee8262cd7abfa5430c2159ed49a2ea0c0d643 -

DrahtBot added the label RPC/REST/ZMQ on Apr 6, 2020

DrahtBot added the label Tests on Apr 6, 2020

DrahtBot added the label Wallet on Apr 6, 2020

DrahtBot commented at 0:53 am on April 7, 2020: member

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#18418 (wallet: Increase OUTPUT_GROUP_MAX_ENTRIES to 100 by fjahr)
#17824 (wallet: Prefer full destination groups in coin selection by fjahr)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

jnewbery commented at 2:43 am on April 7, 2020: member

utACK 7a2ecf16df938dd95d3130a46082def7a02338eb

luke-jr commented at 4:48 am on April 7, 2020: member

Looks like #18192 actually introduced a bug with its name-reference hack: the copy constructor copied the reference to the original object’s m_label.

2952c46b923042f2de801f319e03ed5c4c4eb735 is effectively a bugfix for that.

If we don’t merge this in 0.20, we need to add something like:

0CAddressBookData(const CAddressBookData& other) : m_change(other.m_change), m_label(other.m_label), name(m_label), purpose(other.purpose) {}

(This bug is why #18550 is failing CI)

MarcoFalke merged this on Apr 7, 2020

MarcoFalke closed this on Apr 7, 2020

in test/functional/wallet_avoidreuse.py:161 in 7a2ecf16df

156+
157+        # Spend it
158+        reset_balance(node, node.getnewaddress())
159+
160+        # It should still be change
161+        assert node.getaddressinfo(changeaddr)['ischange']

jonatack commented at 12:15 pm on April 7, 2020:

The test duly fails at this assertion before the changes in #18192.

jonatack commented at 12:17 pm on April 7, 2020: member

ACK 7a2ecf16df938dd95d313

jasonbcox referenced this in commit 082ae84501 on Oct 8, 2020

jasonbcox referenced this in commit d899b47131 on Oct 8, 2020

DrahtBot locked this on Feb 15, 2022

Bugfix: Wallet: Safely deal with change in the address book [part 2] #18546

Conflicts