wallet: LearnRelatedScripts only if KeepDestination

[promag]

Only mutates the wallet if the reserved key is kept.

First commit is a refactor that makes the address type a class member.

The second commit moves LearnRelatedScripts from GetReservedDestination to KeepDestination to avoid an unnecessary call to AddCScript - which in turn prevents multiple entries of the same script in the wallet DB.

[DrahtBot]

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #17537 (wallet: Cleanup and move opportunistic and superfluous TopUp()s by achow101)
• #17526 (Use Single Random Draw In addition to knapsack as coin selection fallback by achow101)
• #17373 (wallet: Various fixes and cleanup to keypool handling in LegacyScriptPubKeyMan and CWallet by achow101)
• #17331 (Use effective values throughout coin selection by achow101)
• #17219 (wallet: allow transaction without change if keypool is empty by Sjors)

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.

[laanwj]

Can you elaborate a bit, what the difference is here? Does it fix an issue, or is it just a conceptual refactor?

[promag]

This is not a refactor, it prevents a call to CWallet::AddCScript if CWallet::CreateTransaction fails for any reason - avoiding multiple entries of the same script in the wallet db.

Updated OP.

I'll see if I can add a test that fails without this change.

[achow101]

Code review ACK d4643bf

[ryanofsky]

Code review ACK 3958295. I like this change. The new behavior makes more sense, and the change makes the code clearer, since the current LearnRelatedScripts call is hard to understand and explain. (Personally, I'd like it if this PR were merged before #17373 or that PR was rebased on top of this one so it would be less confusing.)

re: #17237 (comment)

I'll see if I can add a test that fails without this change.

A test would be nice to have either here or in a followup PR. I think we really need to get out of the habit of making behavior changes in the wallet without corresponding test changes. Not having tests is dangerous and also impedes code review because test changes can make it easier to see what code changes are doing.

[achow101]

Re-ACK 3958295

[ryanofsky]

@Sjors do you want to review this? I'm not sure who else would be familiar

[Sjors]

ACK 3958295

A test could, I'll just shamelessly plug #12134 again, create a pre-segwit wallet, open it on master, draft a transaction with a SegWit change address, cancel it, downgrade to pre-segwit bitcoin core wallet, and then check that older wallet version don't see the key. But I don't think that's terribly useful.

[meshcollider]

utACK 3958295