wallet: `NotifyCanGetAddressesChanged` when advancing `next_index` #34993

Even though TopUp() notifies, advancing next_index after can deplete available addresses, so make sure to notify any time it's changed.

This would manifest as users seeing a clickable Receive button in the GUI when in fact no address can be generated in some edge cases, e.g. when a user has a watch only wallet with a hardened derivation path and runs out of keys.

This feels like it's begging for:

1. a refactor to make it impossible to modify next_index or range_end without firing CanGetAddressesChanged
2. a test

I banged my head against the keyboard for a bit but I couldn't get either of these to fall out, I also tried massaging a few clankers into doing it but I couldn't get any results that seemed reasonable to me, still seems like a worthwhile fix so opening PR anyway.

I also included a moveonly commit to pair code that can change the result of CanGetAddresses() with the notification firing

<!--e57a25ab6845829454e8d69fc972939a-->

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

<!--006a51241073e994b41acfe9ec718e94-->

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/34993.

<!--021abf342d371248e50ceaed478a90ca-->

Reviews

See the guideline for information on the review process.

If your review is incorrectly listed, please copy-paste <code>&lt;!--meta-tag:bot-skip--&gt;</code> into the comment that the bot should ignore.

<!--174a7506f384e20aa4161008e828411d-->

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #35445 (wallet, descriptor: Revert StringType::COMPAT for Miniscript expressions and drop the concept of a Descriptor ID that can be validated by achow101)
• #35444 (wallet: make descriptor SPKM mutex non-recursive by w0xlt)
• #32489 (wallet: Add exportwatchonlywallet RPC to export a watchonly version of a wallet by achow101)
• #30343 (wallet, logging: Replace WalletLogPrintf() with LogInfo() 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.

<!--5faf32d7da4f0f540f40219e4f7537a3-->

This would manifest as users seeing a clickable Receive button in the GUI when in fact no address can be generated in some edge cases.

I don't think it is actually possible for this to occur.

The edge case that could cause this is if the wallet is encrypted and locked, and the active descriptor is one that has hardened child derivation. Once next_index exceeds the end of the range and TopUp is needed, the Receive button stops being able to give out new addresses. However, what we do in the GUI currently is that clicking "Receive" will actually show the passphrase dialog.

This seems like a better UX to me rather than disabling the button, so I'm unconvinced that this change is actually necessary.

This change won't impact how encrypted wallets + hardened xpubs are handled since: CanGetAddresses() returns true for wallets where HavePrivateKeys() == true:

https://github.com/bitcoin/bitcoin/blob/e9dd8631c32ae835f7afb8e0fd2b7caafa9314d9/src/wallet/scriptpubkeyman.cpp#L1175

This is relevant for watch-only wallets with hardened descriptors, e.g. #32489

This is relevant for watch-only wallets with hardened descriptors

Ah, right.

ACK 88bbce40fe8241b51174f0920a3ef036cdc3b4e1

Could encapsulate the range changes into a function, so we don't have to add this signal call everywhere?

Also, the flow currently is the following one (splitting it in two, even though it is the same flow):

1. User interacts with the GUI -> model -> interface -> wallet -> spkm -> signals NotifyCanGetAddressesChanged.

2. NotifyCanGetAddressesChanged -> wallet -> model -> interface -> wallet -> spkm -> CanGetAddresses().

Instead of doing this back and forth, it would be simpler to just pass the value directly: NotifyCanGetAddressesChanged(bool can_get_addresses).

@davidgumberg do you want to respond to @furszy's comments?

Could encapsulate the range changes into a function, so we don't have to add this signal call everywhere?

I decided against this in the initial PR as maybe too complicated if the goal is to enforce no range changes without notifications, I tried a version that I think both encapsulates and ~kind of pushes callers towards not touching without notifying.. but let me know what you think.

Instead of doing this back and forth, it would be simpler to just pass the value directly: NotifyCanGetAddressesChanged(bool can_get_addresses).

Unfortunately, I don't think this will work since the GUIcares about whether any of the SPKM's CanGetAddresses() and the SPKM calling NotifyCanGetAddressesChanged() only knows about itself.

If the SPKM changes from can't->can then the GUI can know how to handle this, the wallet can get addresses no matter what the state was before. But, if the change is from can->can't it's ambiguous, because even though this spkm no longer can get addresses, some other spkm might, so the wallet has to be inquired with.

<!--85328a0da195eb286784d51f73fa0af9-->

🚧 At least one of the CI tasks failed. _{Task ASan + LSan + UBSan + integer: https://github.com/bitcoin/bitcoin/actions/runs/28486434551/job/84433815851} _{LLM reason (✨ experimental): CI failed due to a link-time error: mold reported undefined symbols in wallet::DescriptorScriptPubKeyMan while building bitcoin-wallet.}

<details><summary>Hints</summary>

Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

• Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.

• A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.

• An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

</details>

<!--cf906140f33d8803c4a75a2196329ecb-->

🐙 This pull request conflicts with the target branch and needs rebase.