RPC/Wallet: Access wallets via interfaces::Wallet #26082
Conversation
…imple getnewaddress, setlabel, and walletdisplayaddress
|
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. |
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ReviewsSee 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. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
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 One suggestion I would have is to drop commit "Wallet: Add missing const attribute to interfaces" (cc0a1d5) from this PR. The |
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. |
a60d9eb Bugfix: Wallet: Lock cs_wallet for SignMessage (Luke Dashjr) Pull request description: cs_desc_main is typically locked within scope of a cs_wallet lock, but: CWallet::IsLocked locks cs_wallet ...called from DescriptorScriptPubKeyMan::GetKeys ...called from DescriptorScriptPubKeyMan::GetSigningProvider which locks cs_desc_main first, but has no access to cs_wallet ...called from DescriptorScriptPubKeyMan::SignMessage ...called from CWallet::SignMessage which can access and lock cs_wallet Resolve the out of order locks by grabbing cs_wallet in CWallet::SignMessage first ------------- Note this is currently only an issue for the GUI (which lacks sufficient testing apparently), but can be reproduced by bitcoin#26082 (CI fails as a result) ACKs for top commit: achow101: ACK a60d9eb w0xlt: ACK bitcoin@a60d9eb Tree-SHA512: 60f6959b0ceaf4d9339ba1a47154734034b637c41b1f9e26748a2dbbc3a2a95fc3696019103c55ae70c91d910ba8f3d7f4e27d263030eb60b689f290c4d82ea9
a60d9eb Bugfix: Wallet: Lock cs_wallet for SignMessage (Luke Dashjr) Pull request description: cs_desc_main is typically locked within scope of a cs_wallet lock, but: CWallet::IsLocked locks cs_wallet ...called from DescriptorScriptPubKeyMan::GetKeys ...called from DescriptorScriptPubKeyMan::GetSigningProvider which locks cs_desc_main first, but has no access to cs_wallet ...called from DescriptorScriptPubKeyMan::SignMessage ...called from CWallet::SignMessage which can access and lock cs_wallet Resolve the out of order locks by grabbing cs_wallet in CWallet::SignMessage first ------------- Note this is currently only an issue for the GUI (which lacks sufficient testing apparently), but can be reproduced by bitcoin#26082 (CI fails as a result) ACKs for top commit: achow101: ACK a60d9eb w0xlt: ACK bitcoin@a60d9eb Tree-SHA512: 60f6959b0ceaf4d9339ba1a47154734034b637c41b1f9e26748a2dbbc3a2a95fc3696019103c55ae70c91d910ba8f3d7f4e27d263030eb60b689f290c4d82ea9
|
🐙 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. |
|
There hasn't been much activity lately and the patch still needs rebase. What is the status here?
|
1 similar comment
|
There hasn't been much activity lately and the patch still needs rebase. What is the status here?
|
|
cc @luke-jr |
|
There hasn't been much activity lately and the patch still needs rebase. What is the status here?
|
|
Closing for now. Please leave a comment here, to have it re-opened by someone. |
a60d9eb Bugfix: Wallet: Lock cs_wallet for SignMessage (Luke Dashjr) Pull request description: cs_desc_main is typically locked within scope of a cs_wallet lock, but: CWallet::IsLocked locks cs_wallet ...called from DescriptorScriptPubKeyMan::GetKeys ...called from DescriptorScriptPubKeyMan::GetSigningProvider which locks cs_desc_main first, but has no access to cs_wallet ...called from DescriptorScriptPubKeyMan::SignMessage ...called from CWallet::SignMessage which can access and lock cs_wallet Resolve the out of order locks by grabbing cs_wallet in CWallet::SignMessage first ------------- Note this is currently only an issue for the GUI (which lacks sufficient testing apparently), but can be reproduced by bitcoin#26082 (CI fails as a result) ACKs for top commit: achow101: ACK a60d9eb w0xlt: ACK bitcoin@a60d9eb Tree-SHA512: 60f6959b0ceaf4d9339ba1a47154734034b637c41b1f9e26748a2dbbc3a2a95fc3696019103c55ae70c91d910ba8f3d7f4e27d263030eb60b689f290c4d82ea9
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.