RPC/Wallet: Access wallets via interfaces::Wallet #26082

This is a minimal "step 1" to migrating wallet RPC methods to access via the abstract interfaces. Only trivial methods are ported at this stage.

Long term goal is to support different actual wallet classes (in particular, core-lighting wallets) which won't have remotely the same internals as Bitcoin Core wallets.

Approach ACK. This provides a better separation of concerns.

Can you elaborate on "support different actual wallet classes (in particular, core-lighting wallets)"? I always though of lighting wallets operating on another abstraction layer on-top of bitcoin wallets, i.e. bitcoin wallets abstract the use of the blockchain while lighting wallets make use of that. Thus it seems like lighting wallets should have different interface from bitcoin wallets.

<!--e57a25ab6845829454e8d69fc972939a-->

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

<!--021abf342d371248e50ceaed478a90ca-->

Reviews

See the guideline for information on the review process.

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

<!--174a7506f384e20aa4161008e828411d-->

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #26365 (wallet: GetEffectiveBalance by willcl-ark)
• #26347 (wallet: ensure the wallet is unlocked when needed for rescanning by ishaanam)
• #26294 (build: move util/url to common/url by fanquake)
• #24897 ([Draft / POC] Silent Payments by w0xlt)
• #24313 (Improve display address handling for external signer by Sjors)
• #24058 (BIP-322 basic support by kallewoof)

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.

Can you elaborate on "support different actual wallet classes (in particular, core-lighting wallets)"? I always though of lighting wallets operating on another abstraction layer on-top of bitcoin wallets, i.e. bitcoin wallets abstract the use of the blockchain while lighting wallets make use of that. Thus it seems like lighting wallets should have different interface from bitcoin wallets.

Wallets in general are at the same abstraction layer. Most interfaces are common whether you're sending/receiving on-chain or Lightning.

I think this is a good change for a different reason than the one stated. Even if there won't be multiple wallet implementations, letting RPC methods interact with wallets through the interfaces::Wallet class instead of the CWallet class means new RPC methods can avoid accessing internal wallet state just like GUI code avoids it, and new RPC features should be easier to port to the GUI because they can be implemented behind the interface rather than on top of it.

One suggestion I would have is to drop commit "Wallet: Add missing const attribute to interfaces" (cc0a1d565e1aeb9f399a6b406528a80f4d0d0180) from this PR. The interface::Wallet class is an abstract class and the methods it declares are intentionally non-const, because making them const exposes internal implementation details of interface::Wallet subclasses that callers should not be aware of. In practice, it breaks multiprocess support (see #25337 (review), commit e81daff985867bfe88fdef325384dac344eb6aad from #10102). Dropping this would also make the PR more focused and smaller.

I think this is a good change for a different reason than the one stated. Even if there won't be multiple wallet implementations, letting RPC methods interact with wallets through the interfaces::Wallet class instead of the CWallet class means new RPC methods can avoid accessing internal wallet state just like GUI code avoids it, and new RPC features should be easier to port to the GUI because they can be implemented behind the interface rather than on top of it.

Agree that having RPC interacting through clear interface is great. Maybe we should discuss the long-term goal "to support different actual wallet classes (in particular, core-lighting wallets)" in some other place.

<!--cf906140f33d8803c4a75a2196329ecb-->

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

Moved to draft, as it looks like this needs further discussion, and has needed rebase for 3 months.

<!--13523179cfe9479db18ec6c5d236f789-->

There hasn't been much activity lately and the patch still needs rebase. What is the status here?

• Is it still relevant? ➡️ Please solve the conflicts to make it ready for review and to ensure the CI passes.
• Is it no longer relevant? ➡️ Please close.
• Did the author lose interest or time to work on this? ➡️ Please close it and mark it 'Up for grabs' with the label, so that it can be picked up in the future.

<!--13523179cfe9479db18ec6c5d236f789-->

There hasn't been much activity lately and the patch still needs rebase. What is the status here?

• Is it still relevant? ➡️ Please solve the conflicts to make it ready for review and to ensure the CI passes.
• Is it no longer relevant? ➡️ Please close.
• Did the author lose interest or time to work on this? ➡️ Please close it and mark it 'Up for grabs' with the label, so that it can be picked up in the future.

cc @luke-jr

<!--13523179cfe9479db18ec6c5d236f789-->

There hasn't been much activity lately and the patch still needs rebase. What is the status here?

• Is it still relevant? ➡️ Please solve the conflicts to make it ready for review and to ensure the CI passes.
• Is it no longer relevant? ➡️ Please close.
• Did the author lose interest or time to work on this? ➡️ Please close it and mark it 'Up for grabs' with the label, so that it can be picked up in the future.

Closing for now. Please leave a comment here, to have it re-opened by someone.