Bugfix: Wallet: Safely deal with change in the address book [part 2] by luke-jr · Pull Request #18546 · bitcoin/bitcoin

luke-jr · 2020-04-06T19:58:14Z

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

maflcko

ACK, just some style nits

src/wallet/wallet.cpp

maflcko · 2020-04-06T20:02:57Z

src/wallet/wallet.h

Suggested change

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

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

To avoid issues related to #17198

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

Just the general idea that:

This doesn't improve performance

This might lead to use-after-free in future code

test/functional/wallet_avoidreuse.py

maflcko · 2020-04-06T20:17:37Z

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

Show signature and timestamp

Signature:

Timestamp of file with hash 20931cd3fd37cc3e39e8dda70e27cd4ebf620a0462cabe7b49ab467c15609ce5 -

test/functional/wallet_avoidreuse.py

…oid-reuse wallet

… failure

maflcko · 2020-04-06T20:55:09Z

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

Show signature and timestamp

Signature:

Timestamp of file with hash 6dc1f2ac8bbd8dde3e44f0db23fee8262cd7abfa5430c2159ed49a2ea0c0d643 -

DrahtBot · 2020-04-07T00:53:37Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

wallet: Increase OUTPUT_GROUP_MAX_ENTRIES to 100 #18418 (wallet: Increase OUTPUT_GROUP_MAX_ENTRIES to 100 by fjahr)
wallet: Prefer full destination groups in coin selection #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 · 2020-04-07T02:43:57Z

utACK 7a2ecf1

luke-jr · 2020-04-07T04:48:20Z

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.

2952c46 is effectively a bugfix for that.

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

CAddressBookData(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)

jonatack

ACK 7a2ecf1

jonatack · 2020-04-07T12:15:49Z

test/functional/wallet_avoidreuse.py

+        reset_balance(node, node.getnewaddress())
+
+        # It should still be change
+        assert node.getaddressinfo(changeaddr)['ischange']


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

…book Summary: b5795a788639305bab86a8b3f6b75d6ce81be083 Wallet: Add warning comments and assert to CWallet::DelAddressBook (Luke Dashjr) 6d2905f57aaeb3ec3b63d31043f7673ca10003f2 Wallet: Avoid unnecessary/redundant m_address_book lookups (Luke Dashjr) c751d886f499257627b308b11ffaa51c22db6cc0 Wallet: Avoid treating change-in-the-addressbook as non-change everywhere (Luke Dashjr) 8e64b8c84bcbd63caea06f3af087af1f0609eaf5 Wallet: New FindAddressBookEntry method to filter out change entries (and skip ->second everywhere) (Luke Dashjr) 65b6bdc2b164343ec3cc3d32a0297daff9e24fec Wallet: Add CAddressBookData::IsChange which returns true iff label has never been set (Luke Dashjr) 144b2f85da4d51bf7d72b987888ddcaf5b429eed Wallet: Require usage of new CAddressBookData::setLabel to change label (Luke Dashjr) b86cd155f6f661052042048aa7cfc2a397afe4f7 scripted-diff: Wallet: Rename mapAddressBook to m_address_book (Luke Dashjr) Pull request description: In many places, our code assumes that presence in the address book indicates a non-change key, and absence of an entry in mapAddressBook indicates change. This no longer holds true after #13756 (first released in 0.19) since it added a "used" DestData populated even for change addresses. Only avoid-reuse wallets should be affected by this issue. Thankfully, populating DestData does not write a label to the database, so we can retroactively fix this (so long as the user didn't see the change address and manually assign it a real label). Fixing it is accomplished by: * Adding a new bool to CAddressBookData to track if the label has ever been assigned, either by loading one from the database, or by assigning one at runtime. * `CAddressBookData::IsChange` and `CWallet::FindAddressBookEntry` are new methods to assist in excluding change from code that doesn't expect to see them. * For safety in merging, `CAddressBookData::name` has been made read-only (the actual data is stored in `m_label`, a new private member, and can be changed only with `setLabel` which updates the `m_change` flag), and `mapAddressBook` has been renamed to `m_address_book` (to force old code to be rebased to compile). A final commit also does some minor optimisation, avoiding redundant lookups in `m_address_book` when we already have a pointer to the `CAddressBookData`. Backport of Core [[bitcoin/bitcoin#18192 | PR18192]] Tests for this change to follow in [[bitcoin/bitcoin#18546 | PR18546]] Test Plan: `ninja check check-functional` for sanity Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Subscribers: Fabien Differential Revision: https://reviews.bitcoinabc.org/D7810

…book [part 2] Summary: 7a2ecf16df938dd95d3130a46082def7a02338eb Wallet: Change IsMine check in CWallet::DelAddressBook from assert to failure (Luke Dashjr) 2952c46b923042f2de801f319e03ed5c4c4eb735 Wallet: Replace CAddressBookData.name with GetLabel() method (Luke Dashjr) d7092c392e10889cd7a080b3d22ed6446a59b87a QA: Test that change doesn't turn into non-change when spent in an avoid-reuse wallet (Luke Dashjr) Pull request description: Follow-up to #18192, not strictly necessary for 0.20 Depends on D7810 and introduces tests for it too Backport of Core [[bitcoin/bitcoin#18546 | PR18546]] Test Plan: `ninja check check-functional` Reviewers: #bitcoin_abc, deadalnix Reviewed By: #bitcoin_abc, deadalnix Differential Revision: https://reviews.bitcoinabc.org/D7811

maflcko approved these changes Apr 6, 2020

View reviewed changes

luke-jr force-pushed the bugfix_addressbook_change branch 2 times, most recently from 1ce3245 to a7051b0 Compare April 6, 2020 20:15

maflcko reviewed Apr 6, 2020

View reviewed changes

test/functional/wallet_avoidreuse.py Outdated Show resolved Hide resolved

luke-jr added 3 commits April 6, 2020 20:52

QA: Test that change doesn't turn into non-change when spent in an av…

d7092c3

…oid-reuse wallet

Wallet: Replace CAddressBookData.name with GetLabel() method

2952c46

Wallet: Change IsMine check in CWallet::DelAddressBook from assert to…

7a2ecf1

… failure

luke-jr force-pushed the bugfix_addressbook_change branch from a7051b0 to 7a2ecf1 Compare April 6, 2020 20:52

DrahtBot added RPC/REST/ZMQ Tests Wallet labels Apr 6, 2020

DrahtBot mentioned this pull request Apr 7, 2020

wallet: Increase OUTPUT_GROUP_MAX_ENTRIES to 100 #18418

Merged

DrahtBot mentioned this pull request Apr 7, 2020

wallet: Prefer full destination groups in coin selection #17824

Merged

maflcko merged commit 63dad67 into bitcoin:master Apr 7, 2020

jonatack reviewed Apr 7, 2020

View reviewed changes

hebasto mentioned this pull request May 4, 2020

Wallet, GUI: Warn when sending to already-used Bitcoin addresses #15987

Closed

bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

Bugfix: Wallet: Safely deal with change in the address book [part 2]#18546
maflcko merged 3 commits intobitcoin:masterfrom
luke-jr:bugfix_addressbook_change

luke-jr commented Apr 6, 2020

Uh oh!

maflcko left a comment

Uh oh!

Uh oh!

maflcko Apr 6, 2020

Uh oh!

luke-jr Apr 6, 2020

Uh oh!

maflcko Apr 6, 2020

Uh oh!

Uh oh!

maflcko commented Apr 6, 2020

Uh oh!

Uh oh!

maflcko commented Apr 6, 2020

Uh oh!

DrahtBot commented Apr 7, 2020

Uh oh!

jnewbery commented Apr 7, 2020

Uh oh!

luke-jr commented Apr 7, 2020

Uh oh!

jonatack left a comment

Uh oh!

jonatack Apr 7, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

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

Conversation

luke-jr commented Apr 6, 2020

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

maflcko Apr 6, 2020

Choose a reason for hiding this comment

Uh oh!

luke-jr Apr 6, 2020

Choose a reason for hiding this comment

Uh oh!

maflcko Apr 6, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

maflcko commented Apr 6, 2020

Uh oh!

Uh oh!

maflcko commented Apr 6, 2020

Uh oh!

DrahtBot commented Apr 7, 2020

Conflicts

Uh oh!

jnewbery commented Apr 7, 2020

Uh oh!

luke-jr commented Apr 7, 2020

Uh oh!

jonatack left a comment

Choose a reason for hiding this comment

Uh oh!

jonatack Apr 7, 2020

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants