Bugfix: Wallet: Safely deal with change in the address book [part 2] #18546
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
maflcko
approved these changes
Apr 6, 2020
|
|
||
| typedef std::map<std::string, std::string> StringMap; | ||
| StringMap destdata; | ||
|
|
||
| bool IsChange() const { return m_change; } | ||
| const std::string& GetLabel() const { return m_label; } |
There was a problem hiding this comment.
Suggested change
| const std::string& GetLabel() const { return m_label; } | |
| std::string GetLabel() const { return m_label; } |
To avoid issues related to #17198
There was a problem hiding this comment.
I don't see how that relates to this. Can you give an example?
There was a problem hiding this comment.
Just the general idea that:
- This doesn't improve performance
- This might lead to use-after-free in future code
luke-jr
force-pushed
the
bugfix_addressbook_change
branch
2 times, most recently
from
1ce3245 to
a7051b0
Compare
|
ACK a7051b0, only change is adding issue url and a test style fixup 🍖 Show signature and timestampSignature: Timestamp of file with hash |
maflcko
reviewed
Apr 6, 2020
luke-jr
force-pushed
the
bugfix_addressbook_change
branch
from
a7051b0 to
7a2ecf1
Compare
|
re-ACK 7a2ecf1, only change is adding an assert_equal in the test 🔰 Show signature and timestampSignature: Timestamp of file with hash |
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
|
utACK 7a2ecf1 |
|
Looks like #18192 actually introduced a bug with its 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
reviewed
Apr 7, 2020
| reset_balance(node, node.getnewaddress()) | ||
|
|
||
| # It should still be change | ||
| assert node.getaddressinfo(changeaddr)['ischange'] |
There was a problem hiding this comment.
The test duly fails at this assertion before the changes in #18192.
jasonbcox
pushed a commit
to Bitcoin-ABC/bitcoin-abc
that referenced
this pull request
Oct 8, 2020
…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
jasonbcox
pushed a commit
to Bitcoin-ABC/bitcoin-abc
that referenced
this pull request
Oct 8, 2020
…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
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Follow-up to #18192, not strictly necessary for 0.20