Wallet: Accept “changedata” db key as an alias to “destdata” #18572

1. 
   luke-jr commented at 5:35 am on April 9, 2020: member

   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.

2. Wallet: Accept "changedata" db key as an alias to "destdata"

   This is for forward compatibility with anticipated change metadata

   21359705ae
3. fanquake added the label Wallet on Apr 9, 2020
4. ryanofsky approved
5. 
   ryanofsky commented at 7:24 am on April 9, 2020: member

   Code review ACK 21359705ae7f240bc7ef774c86e2e3c738bf7111

   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:

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

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

6. 
   luke-jr commented at 3:02 pm on April 9, 2020: member

   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

   Is this path possible in 0.20? Should I add the EraseDestData here too perhaps?

7. 
   luke-jr commented at 5:56 pm on April 10, 2020: member

   It doesn’t look possible with the current codebase, to reach an EraseDestData for change, especially not for the “used” key.
8. 
   meshcollider commented at 10:32 am on April 16, 2020: contributor

   Closing this as it has no motivation without #18550
9. meshcollider closed this on Apr 16, 2020

   ---

10. DrahtBot locked this on Feb 15, 2022

Contributors
luke-jr ryanofsky meshcollider

Labels
Wallet