Wallet: Accept "changedata" db key as an alias to "destdata" #18572
Conversation
This is for forward compatibility with anticipated change metadata
There was a problem hiding this comment.
Code review ACK 2135970
Assuming this PR is included in 0.20, it will allow future wallet software to write CHANGEDATA rows to the database that 0.19 and earlier wallets will ignore and 0.20 wallets will treat similarly to DESTDATA rows.
I think it would be a bad idea to ever write these rows, but this change is harmless and avoids foreclosing the possibility. There have been two proposed use-cases for writing CHANGEDATA rows:
-
The change luke wants to make in #18550 would make future wallet software on startup delete DESTDATA "used" rows for change addresses, and convert them to to CHANGEDATA "used" rows. The effect of this would be that if the wallet database was loaded in the post-18550 software and later reloaded in v0.19, it would now correctly treat those change addresses as change instead of non-change, and also incorrectly treat those addresses as unused instead of used. I think this change is confusing and not good, but it's possible I'm misunderstanding the reasons for it. My questions in #18550 haven't been answered yet.
-
A future change hinted in the #18550 comments could write new CHANGEDATA "used" rows that would prevent 0.20 and later wallets from reusing change addresses, but 0.19 wallet and earlier software would ignore. 0.20 software would treat these addressed as used but continue to write DESTDATA instead of CHANGEDATA "used" rows values itself. Also if CWallet::SetSpentKeyState were ever called with used=false in v0.20, the
EraseDestData(batch, dst, "used")call would have no effect, and there address would be marked unused in memory but stored as used on disk. This change sounds awkward to me, but I haven't seen any implementation and my question about why the new feature wouldn't just add a new CAddressBookData member and row type and avoid this destdata mess haven't been answered
Is this path possible in 0.20? Should I add the EraseDestData here too perhaps? |
|
It doesn't look possible with the current codebase, to reach an |
|
Closing this as it has no motivation without #18550 |
This can make 0.20 forward compatible with the changes proposed in #18550 so we don't need an ugly hack for "used" later.
If we end up not doing #18550 as-is, this is ambiguous enough to likely be compatible with any comparable alternative - and worst case we just use another key name (or do nothing at all), and it's no different than if we made no efforts on forward-compatibility.
If we don't end up doing #18550 for whatever reason, this becomes harmless to revert since it only affects reading as-of-yet undefined keys.