Bugfix: Wallet: Lock cs_wallet for SignMessage #26130
Conversation
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
|
(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.) |
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. 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. |
|
ACK a60d9eb Note that the |
I think it needs to be rebased. |
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. |
The majority of the failing jobs are using Ubuntu Focal, which was released 2 years ago, and is LTS for another 3 years.
Ubuntu Focal ships with git version 2.25, which was released ~2.5 years ago. |
Confirming that rebasing does help. |
|
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. |
|
@achow101 are we backporting this for 24.x? |
|
Yes, for backport |
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
|
Backported to 24.x in #26178. |
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: |
|
Looks like an issue with verify-commits. Not sure what the best way to fix that is. |
d8ded8b ci: Use git2.34 for lint task (MacroFake) Pull request description: Since most maintainers use a recent version of git that uses the `ort` strategy by default (https://git-scm.com/docs/merge-strategies/2.34.0), bump git for the lint taks as well. Fixes bitcoin/bitcoin#26130 (comment) ACKs for top commit: fanquake: ACK d8ded8b - seems fine for now, and to keep python3.6 around. When we bump to >= Jammy in future we'll have to pick from Python3.10+. Tree-SHA512: 5a9c40b1c242678a7f92e641db026309b3e2e99d7d032778c98eeb56f7abd65f9e0a24f9b2ccf0350d5c0286d50f1ac5969e4249beaa5ffc4b00d06ca8b141bc
d8ded8b ci: Use git2.34 for lint task (MacroFake) Pull request description: Since most maintainers use a recent version of git that uses the `ort` strategy by default (https://git-scm.com/docs/merge-strategies/2.34.0), bump git for the lint taks as well. Fixes bitcoin#26130 (comment) ACKs for top commit: fanquake: ACK d8ded8b - seems fine for now, and to keep python3.6 around. When we bump to >= Jammy in future we'll have to pick from Python3.10+. Tree-SHA512: 5a9c40b1c242678a7f92e641db026309b3e2e99d7d032778c98eeb56f7abd65f9e0a24f9b2ccf0350d5c0286d50f1ac5969e4249beaa5ffc4b00d06ca8b141bc
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)