Store destdata for change in separate key for backward compatibility #18550
Conversation
…addresses to "changedata"
|
I'm not sure this change is worth it and doesn't introduce more strange behaviors. If goal is to retroactively fix detection of change addresses in v0.19 wallets, this seems like it would simpler and be more convenient to use as a standalone wallet tool function. As I understand things, #13756 in v0.19 introduced a bug where change addresses might be marked in a way where they are treated as non-change. #18192 in v0.20 fixed this by correctly interpreting all change addresses as change. The benefit of this PR is it could be useful to someone who wants to keep running 0.19 software and have it keep incorrectly interpreting change addresses as non-change, but then periodically load up the wallet in a 0.20 node to tweak the database, so when they reload the wallet back in 0.19, the 0.19 software will now correctly see all change addresses as change (until it adds more change addresses that get misinterpreted and it is necessary to fix up the database with 0.20 again). This seems like a pretty unusual use case and also one that would be less awkwardly handled with a standalone |
|
re comment in IRC about tests: |
|
Without this, we can't start adding more destdata to change without breaking backward compatibility. If we go that route, we should make a wallet version bump so 0.19 doesn't try to load the wallet at all. But that also means new features based on it won't be available until users do wallet upgrades explicitly... I'd rather avoid that. My eventual intent is to (for 0.21) add a new destdata indicating the destination has been used (really used, not avoid-reuse "used") and reimplement #15987 without a bloom filter. |
|
@achow101 Does the above interfere in any way with descriptor wallets? |
It shouldn't. Descriptor wallets currently does not touch any address book stuff or change how change is detected. |
|
So the behavior change I described above is NOT actually the intent of the PR? Just kind of a side effect? I'm more confused now about what the benefit of this change is.
With or without this PR, adding DESTDATA rows in the database for change addresses will break backwards compatibility and cause them to be treated as non-change using previous releases, and this PR can't change that. This PR seems to be making relationship between DESTDATA rows and CAddressBookData::destdata entries more complicated just to mask this
This doesn't make sense to me. What problem is solved by bumping wallet version number in v0.20 when v0.20 wallets are compatible with v0.19? Obviously if there are future incompatible changes it would make sense to bump the version or use flags then.
What new feature depends on writing DESTDATA rows for change addresses? This isn't possible without breaking backwards compatibility
DESTDATA and CAddressBookData::destdata seems like the wrong tool for the job. Isn't adding a new row type that old wallet software will not misinterpret and new CAddressBookData field(s) the simplest approach? |
It does change that. By storing destdata in "changedata" keys, old versions ignore them entirely, thus retain the change-ness of the destination. |
It seems like this response is just playing with words and ignoring my actual questions above. I said "DESTDATA rows" and this is pretending I said "destdata map entries". I can see how the PR works, but I don't understand why it is complicating the destdata storage so much and munging v0.19 databases in the process instead of just adding a new field. I could easily be missing something but I don't understand what I am missing. |
|
I don't understand what you are missing either... :/ I suppose we could unconditionally change the DESTDATA key entirely, but I don't see why that would be preferred. |
This is cleanup that doesn't change external behavior. - Removes awkward `StringMap` intermediate representation - Deals with receive request "rr" keys in walletdb.cpp instead of all over qt, wallet, and interfaces code - Deals with destination "used" keys in walletdb.cpp instead of all over wallet code - Adds test coverage - Reduces code (+85/-138 lines) - Reduces memory usage This PR doesn't change externally observable behavior. Internally, only change in behavior is that EraseDestData deletes directly from database because the `StringMap` is gone. This is more direct and efficient because it uses a single btree lookup and scan instead of multiple lookups Motivation for this cleanup is making changes like bitcoin#18550, bitcoin#18192, bitcoin#13756 easier to reason about and less likely to result in unintended behavior and bugs
|
re: http://www.erisian.com.au/bitcoin-core-dev/log-2020-04-10.html#l-542
Appreciate the empathy, but I wasn't confused about what this PR and #18572 do. I was confused by what these PRs are trying to do. But I get it now. Here is the story: #13756 started recording used destination information as rows with But a question after #18192 is whether we want to keep using the same data representation or switch to a different one. The choices are:
I think the best choice is probably (1). It keeps things simple, requires no code changes, and causes no practical issues for 0.20 or future wallets other than using a slightly unusual key format. With choice (1), 0.19 and earlier software will continue to display "used" change addresses as non-change. But this issue has been around since #13756, doesn't seem very serious, and is easily fixed by using newer software. We could also backport simple fixes to old branches if desired. I think (2), which is implemented by this PR, is a bad choice because of the complexity and the strange behaviors described in my previous comments here. I think it's also awkward as data format because you wouldn't expect data about destination use to be stored differently depending on whether the destination is internal or external. The practical advantage of choice (2) over choice (1) is that if you loaded a post-#18550 wallet in 0.19 or earlier software, you would see used change addresses as change instead instead of non-change. But the avoid-reuse feature in 0.19 would still see new used changed addresses as non-change, and now see previous used changed addresses as non-used. (3) would be a simpler alternative to (2) if you want to fix old wallet software seeing used change addresses as non-change. But it has the same drawback as (2) where 0.19 wallets will see used change addresses as non-used and an additional drawback in that 0.19 software will also see used non-change addresses as non-used. * This mistake would have been caught during #13756 review it weren't for layer violation in |
|
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. |
This is cleanup that doesn't change external behavior. - Removes awkward `StringMap` intermediate representation - Deals with receive request "rr" keys in walletdb.cpp instead of all over qt, wallet, and interfaces code - Deals with destination "used" keys in walletdb.cpp instead of all over wallet code - Adds test coverage - Reduces code (+85/-138 lines) - Reduces memory usage This PR doesn't change externally observable behavior. Internally, only change in behavior is that EraseDestData deletes directly from database because the `StringMap` is gone. This is more direct and efficient because it uses a single btree lookup and scan instead of multiple lookups Motivation for this cleanup is making changes like bitcoin#18550, bitcoin#18192, bitcoin#13756 easier to reason about and less likely to result in unintended behavior and bugs
|
I agree with Russ (thank you, by the way, for the excellent write-up of the issue). I'll close this now, but we can discuss it in the meeting if you have a strong argument that we are missing Luke. |
This is cleanup that doesn't change external behavior. - Removes awkward `StringMap` intermediate representation - Deals with receive request "rr" keys in walletdb.cpp instead of all over qt, wallet, and interfaces code - Deals with destination "used" keys in walletdb.cpp instead of all over wallet code - Adds test coverage - Reduces code (+85/-138 lines) - Reduces memory usage This PR doesn't change externally observable behavior. Internally, only change in behavior is that EraseDestData deletes directly from database because the `StringMap` is gone. This is more direct and efficient because it uses a single btree lookup and scan instead of multiple lookups Motivation for this cleanup is making changes like bitcoin#18550, bitcoin#18192, bitcoin#13756 easier to reason about and less likely to result in unintended behavior and bugs
This is cleanup that doesn't change external behavior. - Removes awkward `StringMap` intermediate representation - Deals with receive request "rr" keys in walletdb.cpp instead of all over qt, wallet, and interfaces code - Deals with destination "used" keys in walletdb.cpp instead of all over wallet code - Adds test coverage - Reduces code (+85/-138 lines) - Reduces memory usage This PR doesn't change externally observable behavior. Internally, only change in behavior is that EraseDestData deletes directly from database because the `StringMap` is gone. This is more direct and efficient because it uses a single btree lookup and scan instead of multiple lookups Motivation for this cleanup is making changes like bitcoin#18550, bitcoin#18192, bitcoin#13756 easier to reason about and less likely to result in unintended behavior and bugs
This is cleanup that doesn't change external behavior. - Removes awkward `StringMap` intermediate representation - Deals with receive request "rr" keys in walletdb.cpp instead of all over qt, wallet, and interfaces code - Deals with destination "used" keys in walletdb.cpp instead of all over wallet code - Adds test coverage - Reduces code (+85/-138 lines) - Reduces memory usage This PR doesn't change externally observable behavior. Internally, only change in behavior is that EraseDestData deletes directly from database because the `StringMap` is gone. This is more direct and efficient because it uses a single btree lookup and scan instead of multiple lookups Motivation for this cleanup is making changes like bitcoin#18550, bitcoin#18192, bitcoin#13756 easier to reason about and less likely to result in unintended behavior and bugs
This is cleanup that doesn't change external behavior. - Removes awkward `StringMap` intermediate representation - Deals with receive request "rr" keys in walletdb.cpp instead of all over qt, wallet, and interfaces code - Deals with destination "used" keys in walletdb.cpp instead of all over wallet code - Adds test coverage - Reduces code (+85/-138 lines) - Reduces memory usage This PR doesn't change externally observable behavior. Internally, only change in behavior is that EraseDestData deletes directly from database because the `StringMap` is gone. This is more direct and efficient because it uses a single btree lookup and scan instead of multiple lookups Motivation for this cleanup is making changes like bitcoin#18550, bitcoin#18192, bitcoin#13756 easier to reason about and less likely to result in unintended behavior and bugs
This is cleanup that doesn't change external behavior. - Removes awkward `StringMap` intermediate representation - Deals with receive request "rr" keys in walletdb.cpp instead of all over qt, wallet, and interfaces code - Deals with destination "used" keys in walletdb.cpp instead of all over wallet code - Adds test coverage - Reduces code (+85/-138 lines) - Reduces memory usage This PR doesn't change externally observable behavior. Internally, only change in behavior is that EraseDestData deletes directly from database because the `StringMap` is gone. This is more direct and efficient because it uses a single btree lookup and scan instead of multiple lookups Motivation for this cleanup is making changes like bitcoin#18550, bitcoin#18192, bitcoin#13756 easier to reason about and less likely to result in unintended behavior and bugs
This is cleanup that doesn't change external behavior. - Removes awkward `StringMap` intermediate representation - Deals with receive request "rr" keys in walletdb.cpp instead of all over qt, wallet, and interfaces code - Deals with destination "used" keys in walletdb.cpp instead of all over wallet code - Adds test coverage - Reduces code (+85/-138 lines) - Reduces memory usage This PR doesn't change externally observable behavior. Internally, only change in behavior is that EraseDestData deletes directly from database because the `StringMap` is gone. This is more direct and efficient because it uses a single btree lookup and scan instead of multiple lookups Motivation for this cleanup is making changes like bitcoin#18550, bitcoin#18192, bitcoin#13756 easier to reason about and less likely to result in unintended behavior and bugs
This is cleanup that doesn't change external behavior. - Removes awkward `StringMap` intermediate representation - Deals with receive request "rr" keys in walletdb.cpp instead of all over qt, wallet, and interfaces code - Deals with destination "used" keys in walletdb.cpp instead of all over wallet code - Adds test coverage - Reduces code (+85/-138 lines) - Reduces memory usage This PR doesn't change externally observable behavior. Internally, only change in behavior is that EraseDestData deletes directly from database because the `StringMap` is gone. This is more direct and efficient because it uses a single btree lookup and scan instead of multiple lookups Motivation for this cleanup is making changes like bitcoin#18550, bitcoin#18192, bitcoin#13756 easier to reason about and less likely to result in unintended behavior and bugs
This is cleanup that doesn't change external behavior. - Removes awkward `StringMap` intermediate representation - Deals with receive request "rr" keys in walletdb.cpp instead of all over qt, wallet, and interfaces code - Deals with destination "used" keys in walletdb.cpp instead of all over wallet code - Adds test coverage - Reduces code (+85/-138 lines) - Reduces memory usage This PR doesn't change externally observable behavior. Internally, only change in behavior is that EraseDestData deletes directly from database because the `StringMap` is gone. This is more direct and efficient because it uses a single btree lookup and scan instead of multiple lookups Motivation for this cleanup is making changes like bitcoin#18550, bitcoin#18192, bitcoin#13756 easier to reason about and less likely to result in unintended behavior and bugs
This is cleanup that doesn't change external behavior. - Removes awkward `StringMap` intermediate representation - Deals with receive request "rr" keys in walletdb.cpp instead of all over qt, wallet, and interfaces code - Deals with destination "used" keys in walletdb.cpp instead of all over wallet code - Adds test coverage - Reduces code (+85/-138 lines) - Reduces memory usage This PR doesn't change externally observable behavior. Internally, only change in behavior is that EraseDestData deletes directly from database because the `StringMap` is gone. This is more direct and efficient because it uses a single btree lookup and scan instead of multiple lookups Motivation for this cleanup is making changes like bitcoin#18550, bitcoin#18192, bitcoin#13756 easier to reason about and less likely to result in unintended behavior and bugs
This is cleanup that doesn't change external behavior. - Removes awkward `StringMap` intermediate representation - Deals with receive request "rr" keys in walletdb.cpp instead of all over qt, wallet, and interfaces code - Deals with destination "used" keys in walletdb.cpp instead of all over wallet code - Adds test coverage - Reduces code (+85/-138 lines) - Reduces memory usage This PR doesn't change externally observable behavior. Internally, only change in behavior is that EraseDestData deletes directly from database because the `StringMap` is gone. This is more direct and efficient because it uses a single btree lookup and scan instead of multiple lookups Motivation for this cleanup is making changes like bitcoin#18550, bitcoin#18192, bitcoin#13756 easier to reason about and less likely to result in unintended behavior and bugs
This is cleanup that doesn't change external behavior. - Removes awkward `StringMap` intermediate representation - Deals with receive request "rr" keys in walletdb.cpp instead of all over qt, wallet, and interfaces code - Deals with destination "used" keys in walletdb.cpp instead of all over wallet code - Adds test coverage - Reduces code (+85/-138 lines) - Reduces memory usage This PR doesn't change externally observable behavior. Internally, only change in behavior is that EraseDestData deletes directly from database because the `StringMap` is gone. This is more direct and efficient because it uses a single btree lookup and scan instead of multiple lookups Motivation for this cleanup is making changes like bitcoin#18550, bitcoin#18192, bitcoin#13756 easier to reason about and less likely to result in unintended behavior and bugs
This is cleanup that doesn't change external behavior. - Removes awkward `StringMap` intermediate representation - Deals with receive request "rr" keys in walletdb.cpp instead of all over qt, wallet, and interfaces code - Deals with destination "used" keys in walletdb.cpp instead of all over wallet code - Adds test coverage - Reduces code (+85/-138 lines) - Reduces memory usage This PR doesn't change externally observable behavior. Internally, only change in behavior is that EraseDestData deletes directly from database because the `StringMap` is gone. This is more direct and efficient because it uses a single btree lookup and scan instead of multiple lookups Motivation for this cleanup is making changes like bitcoin#18550, bitcoin#18192, bitcoin#13756 easier to reason about and less likely to result in unintended behavior and bugs
This is cleanup that doesn't change external behavior. - Removes awkward `StringMap` intermediate representation - Deals with receive request "rr" keys in walletdb.cpp instead of all over qt, wallet, and interfaces code - Deals with destination "used" keys in walletdb.cpp instead of all over wallet code - Adds test coverage - Reduces code (+85/-138 lines) - Reduces memory usage This PR doesn't change externally observable behavior. Internally, only change in behavior is that EraseDestData deletes directly from database because the `StringMap` is gone. This is more direct and efficient because it uses a single btree lookup and scan instead of multiple lookups Motivation for this cleanup is making changes like bitcoin#18550, bitcoin#18192, bitcoin#13756 easier to reason about and less likely to result in unintended behavior and bugs
This is cleanup that doesn't change external behavior. - Removes awkward `StringMap` intermediate representation - Deals with receive request "rr" keys in walletdb.cpp instead of all over qt, wallet, and interfaces code - Deals with destination "used" keys in walletdb.cpp instead of all over wallet code - Adds test coverage - Reduces code (+85/-138 lines) - Reduces memory usage This PR doesn't change externally observable behavior. Internally, only change in behavior is that EraseDestData deletes directly from database because the `StringMap` is gone. This is more direct and efficient because it uses a single btree lookup and scan instead of multiple lookups Motivation for this cleanup is making changes like bitcoin#18550, bitcoin#18192, bitcoin#13756 easier to reason about and less likely to result in unintended behavior and bugs
This is cleanup that doesn't change external behavior. - Removes awkward `StringMap` intermediate representation - Deals with receive request "rr" keys in walletdb.cpp instead of all over qt, wallet, and interfaces code - Deals with destination "used" keys in walletdb.cpp instead of all over wallet code - Adds test coverage - Reduces code (+85/-138 lines) - Reduces memory usage This PR doesn't change externally observable behavior. Internally, only change in behavior is that EraseDestData deletes directly from database because the `StringMap` is gone. This is more direct and efficient because it uses a single btree lookup and scan instead of multiple lookups Motivation for this cleanup is making changes like bitcoin#18550, bitcoin#18192, bitcoin#13756 easier to reason about and less likely to result in unintended behavior and bugs
This is cleanup that doesn't change external behavior. - Removes awkward `StringMap` intermediate representation - Deals with receive request "rr" keys in walletdb.cpp instead of all over qt, wallet, and interfaces code - Deals with destination "used" keys in walletdb.cpp instead of all over wallet code - Adds test coverage - Reduces code (+85/-138 lines) - Reduces memory usage This PR doesn't change externally observable behavior. Internally, only change in behavior is that EraseDestData deletes directly from database because the `StringMap` is gone. This is more direct and efficient because it uses a single btree lookup and scan instead of multiple lookups Motivation for this cleanup is making changes like bitcoin#18550, bitcoin#18192, bitcoin#13756 easier to reason about and less likely to result in unintended behavior and bugs
This is cleanup that doesn't change external behavior. - Removes awkward `StringMap` intermediate representation - Deals with receive request "rr" keys in walletdb.cpp instead of all over qt, wallet, and interfaces code - Deals with destination "used" keys in walletdb.cpp instead of all over wallet code - Adds test coverage - Reduces code (+85/-138 lines) - Reduces memory usage This PR doesn't change externally observable behavior. Internally, only change in behavior is that EraseDestData deletes directly from database because the `StringMap` is gone. This is more direct and efficient because it uses a single btree lookup and scan instead of multiple lookups Motivation for this cleanup is making changes like bitcoin#18550, bitcoin#18192, bitcoin#13756 easier to reason about and less likely to result in unintended behavior and bugs
This is cleanup that doesn't change external behavior. - Removes awkward `StringMap` intermediate representation - Deals with receive request "rr" keys in walletdb.cpp instead of all over qt, wallet, and interfaces code - Deals with destination "used" keys in walletdb.cpp instead of all over wallet code - Adds test coverage - Reduces code (+85/-138 lines) - Reduces memory usage This PR doesn't change externally observable behavior. Internally, only change in behavior is that EraseDestData deletes directly from database because the `StringMap` is gone. This is more direct and efficient because it uses a single btree lookup and scan instead of multiple lookups Motivation for this cleanup is making changes like bitcoin#18550, bitcoin#18192, bitcoin#13756 easier to reason about and less likely to result in unintended behavior and bugs
This is cleanup that doesn't change external behavior. - Removes awkward `StringMap` intermediate representation - Deals with receive request "rr" keys in walletdb.cpp instead of all over qt, wallet, and interfaces code - Deals with destination "used" keys in walletdb.cpp instead of all over wallet code - Adds test coverage - Reduces code (+85/-138 lines) - Reduces memory usage This PR doesn't change externally observable behavior. Internally, only change in behavior is that EraseDestData deletes directly from database because the `StringMap` is gone. This is more direct and efficient because it uses a single btree lookup and scan instead of multiple lookups Motivation for this cleanup is making changes like bitcoin#18550, bitcoin#18192, bitcoin#13756 easier to reason about and less likely to result in unintended behavior and bugs
This is cleanup that doesn't change external behavior. - Removes awkward `StringMap` intermediate representation - Deals with receive request "rr" keys in walletdb.cpp instead of all over qt, wallet, and interfaces code - Deals with destination "used" keys in walletdb.cpp instead of all over wallet code - Adds test coverage - Reduces code (+85/-138 lines) - Reduces memory usage This PR doesn't change externally observable behavior. Internally, only change in behavior is that EraseDestData deletes directly from database because the `StringMap` is gone. This is more direct and efficient because it uses a single btree lookup and scan instead of multiple lookups Motivation for this cleanup is making changes like bitcoin#18550, bitcoin#18192, bitcoin#13756 easier to reason about and less likely to result in unintended behavior and bugs
This is cleanup that doesn't change external behavior. - Removes awkward `StringMap` intermediate representation - Deals with receive request "rr" keys in walletdb.cpp instead of all over qt, wallet, and interfaces code - Deals with destination "used" keys in walletdb.cpp instead of all over wallet code - Adds test coverage - Reduces code (+85/-138 lines) - Reduces memory usage This PR doesn't change externally observable behavior. Internally, only change in behavior is that EraseDestData deletes directly from database because the `StringMap` is gone. This is more direct and efficient because it uses a single btree lookup and scan instead of multiple lookups Motivation for this cleanup is making changes like bitcoin#18550, bitcoin#18192, bitcoin#13756 easier to reason about and less likely to result in unintended behavior and bugs
This is cleanup that doesn't change external behavior. - Removes awkward `StringMap` intermediate representation - Deals with receive request "rr" keys in walletdb.cpp instead of all over qt, wallet, and interfaces code - Deals with destination "used" keys in walletdb.cpp instead of all over wallet code - Adds test coverage - Reduces code (+85/-138 lines) - Reduces memory usage This PR doesn't change externally observable behavior. Internally, only change in behavior is that EraseDestData deletes directly from database because the `StringMap` is gone. This is more direct and efficient because it uses a single btree lookup and scan instead of multiple lookups Motivation for this cleanup is making changes like bitcoin#18550, bitcoin#18192, bitcoin#13756 easier to reason about and less likely to result in unintended behavior and bugs
This is cleanup that doesn't change external behavior. - Removes awkward `StringMap` intermediate representation - Deals with receive request "rr" keys in walletdb.cpp instead of all over qt, wallet, and interfaces code - Deals with destination "used" keys in walletdb.cpp instead of all over wallet code - Adds test coverage - Reduces code (+85/-138 lines) - Reduces memory usage This PR doesn't change externally observable behavior. Internally, only change in behavior is that EraseDestData deletes directly from database because the `StringMap` is gone. This is more direct and efficient because it uses a single btree lookup and scan instead of multiple lookups Motivation for this cleanup is making changes like bitcoin#18550, bitcoin#18192, bitcoin#13756 easier to reason about and less likely to result in unintended behavior and bugs
#18192 enabled
CAddressBookDatato track change, but old versions will still load entries with destdata as non-change.To make it backward compatible, this PR changes the database key, for change only, from "destdata" to "changedata" (which should be ignored by older versions).
If we don't merge this in 0.20, we may need to make an exception for the "used" key, to remain compatible with 0.20 (and lose compatibility with 0.19, but that's less problematic since avoid-reuse wallets have always been broken with that version).