Bugfix: Wallet: Lock cs_wallet for SignMessage #26130

pull luke-jr wants to merge 1 commits into bitcoin:master from luke-jr:fix_descrwallet_signmsg_deadlck changing 1 files +1 −0

luke-jr commented at 12:49 AM on September 20, 2022: member

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 #26082 (CI fails as a result)

Bugfix: Wallet: Lock cs_wallet for SignMessage

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

a60d9eb9e6

luke-jr commented at 12:50 AM on September 20, 2022: member

(It would be good to have someone more familiar with descriptor wallet code take a look if there's a better way to fix this.)
DrahtBot commented at 7:45 AM on September 20, 2022: contributor


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



Conflicts

Reviewers, this pull request conflicts with the following ones:
- #24058 (BIP-322 basic support by kallewoof)
- #16545 (refactor: Implement missing error checking for ArgsManager flags 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.
achow101 commented at 4:00 PM on September 22, 2022: member

ACK a60d9eb9e6b6a272a3fca8981d89a55955dced55

Note that the signmessage RPC does not run into this issue because the RPC function holds cs_wallet prior to calling CWallet::SignMessage.
w0xlt approved
w0xlt commented at 5:42 PM on September 22, 2022: contributor

ACK https://github.com/bitcoin/bitcoin/pull/26130/commits/a60d9eb9e6b6a272a3fca8981d89a55955dced55

But most CI checks are failing with an unclear message.
achow101 commented at 5:46 PM on September 22, 2022: member

But most CI checks are failing with an unclear message.

I think it needs to be rebased.
luke-jr commented at 7:09 PM on September 22, 2022: member

But most CI checks are failing with an unclear message.

This is a CI bug.

Edit: To be more specific, it looks like the CI infra is handling it okay, but some older CI distros have a very old version of git that can't handle the merge.
fanquake commented at 9:02 AM on September 23, 2022: member

but some older CI distros

The majority of the failing jobs are using Ubuntu Focal, which was released 2 years ago, and is LTS for another 3 years.

have a very old version of git

Ubuntu Focal ships with git version 2.25, which was released ~2.5 years ago.
hebasto commented at 9:44 AM on September 23, 2022: member

But most CI checks are failing with an unclear message.

I think it needs to be rebased.

... but some older CI distros have a very old version of git that can't handle the merge.

Confirming that rebasing does help.
glozow added the label Wallet on Sep 23, 2022
luke-jr commented at 5:50 PM on September 23, 2022: member

Looks like the minimum git to make the merge is 2.33. But the PR itself is fine as-is, and once merged won't be a problem even for old versions of git.
MarcoFalke merged this on Sep 24, 2022
MarcoFalke closed this on Sep 24, 2022
fanquake added this to the milestone 24.0 on Sep 24, 2022
fanquake commented at 2:28 PM on September 24, 2022: member

@achow101 are we backporting this for 24.x?
achow101 commented at 2:54 PM on September 24, 2022: member

Yes, for backport
achow101 added the label Needs backport (24.x) on Sep 24, 2022
sidhujag referenced this in commit 9547d5270a on Sep 25, 2022
MarcoFalke removed the label Needs backport (24.x) on Sep 25, 2022
MarcoFalke referenced this in commit ca8d2c4b43 on Sep 25, 2022
fanquake commented at 8:16 AM on September 26, 2022: member

Backported to 24.x in #26178.
hebasto commented at 9:52 PM on September 27, 2022: member

Looks like the minimum git to make the merge is 2.33. But the PR itself is fine as-is, and once merged won't be a problem even for old versions of git.

It appears, it is a problem as now the CI linter job fails.
luke-jr commented at 3:03 AM on September 28, 2022: member

It appears, it is a problem as now the CI linter job fails.

??? Looks like it's working fine to me? https://cirrus-ci.com/build/5775460305993728
hebasto commented at 7:28 AM on September 28, 2022: member
It appears, it is a problem as now the CI linter job fails.

??? Looks like it's working fine to me? https://cirrus-ci.com/build/5775460305993728

Don't hesitate to look into the following merge commits:
- https://cirrus-ci.com/task/6353622547038208:
```
Merge commit 0cfbb171bd6f61a4edba913e281553b9bdf08d4a is not clean
```
- https://cirrus-ci.com/task/5277524866367488:
```
Merge commit 0cfbb171bd6f61a4edba913e281553b9bdf08d4a is not clean
```
luke-jr commented at 5:36 PM on September 28, 2022: member

Looks like an issue with verify-commits. Not sure what the best way to fix that is.
MarcoFalke referenced this in commit 291e363ce5 on Sep 29, 2022
bitcoin locked this on Sep 28, 2023

Contributors

Labels

Milestone
24.0

Linked (view graph)

#26082 RPC/Wallet: Access wallets via interfaces::Wallet #26178 [24.x] Bugfix: Wallet: Lock cs_wallet for SignMessage #26195 ci: Use git2.34 for lint task

Bugfix: Wallet: Lock cs_wallet for SignMessage #26130

Conflicts