Store destdata for change in separate key for backward compatibility

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

#18192 enabled CAddressBookData to 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).

fanquake added the label Wallet on Apr 6, 2020

Wallet: Refactor SetAddressBookWithDB to be clearer (no behaviour changes) 704772d5b0

Wallet: Refactor SetAddressBookWithDB to only encode the address string once 7050d5ecfc

Wallet: Keep change destdata in new "changedata" key to hide it from old versions ea487a39c8

Wallet: When loading a wallet, migrate any "destdata" keys on change addresses to "changedata" 72ff5515fb

luke-jr force-pushed on Apr 7, 2020

ryanofsky commented at 2:52 pm on April 7, 2020: member

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 bitcoin-wallet tool function that just deletes the “used” destdata entries instead of storing them in a different way where the 0.19 code can’t find them and 0.20 code now has to look for them

ryanofsky commented at 3:11 pm on April 7, 2020: member

re comment in IRC about tests: feature_backwards_compatibility.py could be useful here. As an example, https://github.com/Sjors/bitcoin/commit/bacf55990905c0257c7bc45bbbd2b028716efc7f was a test added for the backwards compatibility bug #18075

luke-jr commented at 5:36 pm on April 7, 2020: member

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.

sipa commented at 6:45 pm on April 7, 2020: member

@achow101 Does the above interfere in any way with descriptor wallets?

achow101 commented at 6:49 pm on April 7, 2020: member

@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.

ryanofsky commented at 7:15 pm on April 7, 2020: member

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.

Without this, we can’t start adding more destdata to change without breaking backward compatibility

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

If we go that route, we should make a wallet version bump so 0.19 doesn’t try to load the wallet at all

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.

But that also means new features based on it won’t be available until users do wallet upgrades explicitly… I’d rather avoid that.

What new feature depends on writing DESTDATA rows for change addresses? This isn’t possible without breaking backwards compatibility

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.

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?

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

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.

It does change that. By storing destdata in “changedata” keys, old versions ignore them entirely, thus retain the change-ness of the destination.

ryanofsky commented at 7:40 am on April 9, 2020: member

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.

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.

luke-jr commented at 3:05 pm on April 9, 2020: member

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.

ryanofsky referenced this in commit 02c9d2c78d on Apr 12, 2020

ryanofsky commented at 8:39 pm on April 12, 2020: member

re: http://www.erisian.com.au/bitcoin-core-dev/log-2020-04-10.html#l-542

there isn’t really much context.. ryanofsky got confused by the PRs :/

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 ("destdata" << EncodeDestination(dest) << "used") keys in the database. This was a mistake*. A simpler choice would have been to store these as ("destused" << EncodeDestination(dest)) rows instead. A practical problem with the ("destdata" << EncodeDestination(dest) << "used") representation is that each time a change destination is marked used, pre-0.20 pre-#18192 wallets start treating it as non-change. #18192 fixes this for post-0.20 wallets so this is no longer a problem going foward.

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:

Keep storing used destination information in ("destdata" << EncodeDestination(dest) << "used") rows.
Keep storing used destination information for non-change destinations in ("destdata" << EncodeDestination(dest) << "used") rows, but switch to ("changedata" << EncodeDestination(dest) << "used") rows for change destinations. Implemented in this PR.
Start storing used destination information in another place like ("destused" << EncodeDestination(dest)) rows. Unimplemented but simple to implement.

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 CAddressBookData exposing berkeley db keys to wallet code. #18608 fixes this to prevent mistakes like this going forward

DrahtBot commented at 10:45 pm on April 12, 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:

#18618 (gui: Drop RecentRequestsTableModel dependency to WalletModel by promag)
#18608 (refactor: Remove CAddressBookData::destdata by ryanofsky)

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.

ryanofsky referenced this in commit 5a9bbfe5e7 on Apr 13, 2020

meshcollider commented at 10:31 am on April 16, 2020: contributor

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.

meshcollider closed this on Apr 16, 2020

ryanofsky referenced this in commit 6847561ce1 on Apr 27, 2020

ryanofsky referenced this in commit 0502b35473 on Apr 27, 2020

ryanofsky referenced this in commit 4eea4ac8f3 on May 4, 2020

ryanofsky referenced this in commit 4855c6271c on May 4, 2020

ryanofsky referenced this in commit 6e48213820 on May 27, 2020

ryanofsky referenced this in commit 3a58f2ae17 on May 27, 2020

ryanofsky referenced this in commit f8d7abef76 on Jun 17, 2020

ryanofsky referenced this in commit 005f0dfd16 on Jun 17, 2020

ryanofsky referenced this in commit e44e5a2e33 on Jul 1, 2020

ryanofsky referenced this in commit 6fe1199ee5 on Jul 1, 2020

ryanofsky referenced this in commit 36aa5fee72 on Jul 10, 2020

ryanofsky referenced this in commit 11b08858f3 on Jul 10, 2020

ryanofsky referenced this in commit 487684727b on Jul 13, 2020

ryanofsky referenced this in commit da12e67616 on Jul 14, 2020

ryanofsky referenced this in commit 7aaf995e81 on Jul 14, 2020

ryanofsky referenced this in commit 4ca322b86d on Aug 28, 2020

ryanofsky referenced this in commit 12462bcd54 on Aug 28, 2020

ryanofsky referenced this in commit e04daaeb92 on Sep 13, 2020

ryanofsky referenced this in commit 7fbfefab87 on Sep 13, 2020

ryanofsky referenced this in commit 793f7e8118 on Jan 21, 2021

ryanofsky referenced this in commit 75897d46a5 on Jan 21, 2021

ryanofsky referenced this in commit 7a05b1dee2 on Mar 3, 2021

ryanofsky referenced this in commit 14b1d5f3e9 on Mar 3, 2021

DrahtBot locked this on Feb 15, 2022

Store destdata for change in separate key for backward compatibility #18550

Conflicts