-
Notifications
You must be signed in to change notification settings - Fork 1.2k
backport: Merge bitcoin#28086, 30265,19833 #6619
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Conversation
34d5bc3
to
d949961
Compare
This pull request has conflicts, please rebase. |
5fabde6 wallet: AddWalletDescriptor requires cs_wallet lock (João Barbosa) 32d036e wallet: GetLabelAddresses requires cs_wallet lock (João Barbosa) Pull request description: This is another small change towards non recursive wallet lock. ACKs for top commit: hebasto: ACK 5fabde6, I have reviewed the code and it looks OK, I agree it can be merged. Tree-SHA512: 00506f0159c56854a171e58a451db8dd9b9f735039697b1cf2ca7f54de61fb51cc1e5eff42265233e041b4b1bfd29c2247496dc4456578e1a23c323bdec2901b
16176fc
to
69384ba
Compare
…fault wallets and generated backup files 6b2dcba wallet: List sqlite wallets with empty string name (Ava Chow) 3ddbdd1 wallet: Ignore .bak files when listing wallet files (Ava Chow) Pull request description: When the default wallet is migrated, we do not rename the wallet so we end up having a descriptor wallet with the empty string as its name and the wallet.dat file in the root of the walletdir. This is supposed to be an unsupported configuration and there is no other way to achieve this (other than file copying), but the wallet loading code does not disallow loading such wallets. However `listwalletdir` does not currently list the default wallet if it is sqlite. This is confusing to users, so change `listwalletdir` to include these wallets. Additionally, the migration of the default wallet, and of any plain wallet files in the walletdir, produces a backup file in the walletdir itself. Since these backups are a BDB file, `listwalletdir` will detect them as being another wallet that we could open, but this is erroneous and could lead to confusion and potentially funds loss if both the backup and the migrated wallet are in use simultaneously. To reduce the likelihood of this issue, don't list these wallets in `listwalletdir`. *** Possibly we could have more stringent checks on loading to resolve these issues, but I'm concerned that that will just confuse users and gratuitously break things that already worked. Since the original intent was to disallow default wallets for sqlite/descriptors, a possible alternative would be to prevent people from loading such wallets and change migration to rename those wallets. However, given that this behavior with migrating default wallets has existed since default wallet migration was fixed, I think that making such a change would be confusing and break things for no good reason. Although perhaps we should still do the renaming. For the backups, we could also change loading to refuse to load any wallet named with `.bak` (or `.legacy.bak`) as such wallets can still be loaded by giving the path to them directly, which some users may do to "restore" the backup. However restricting what can be loaded based on filename seems a little heavyhanded. It wouldn't be funds loss though since the correct way to restore the backup is with `restorewallet`. ACKs for top commit: fjahr: Code review ACK 6b2dcba furszy: Code ACK 6b2dcba glozow: ACK 6b2dcba Tree-SHA512: 0b033f6ed55830f8a054afea3fb2cf1fa82a94040053ebfaf123bda36c99f45d3f01a2aec4ed02fed9c61bb3d320b047ed892d7f6644b5a356a7bc5974b10cff
WalkthroughThis pull request makes several targeted modifications across different components. In one file related to fuzz testing, type casting in arithmetic operations is adjusted to ensure that both operands use the same unsigned integer type, reducing potential overflow issues. Another change alters the logic for processing database files by excluding those with a ".bak" extension and refining conditions to correctly identify valid files. Updates to wallet functions introduce an assertion that verifies the wallet's mutex is locked before further execution, and the method signature is annotated to require an exclusive lock. Finally, a specific sanitizer suppression entry for implicit integer sign change in the fuzz testing component is removed. All modifications are confined to ensuring type safety, improved file handling, and enhanced thread safety through stricter locking requirements. Tip ⚡💬 Agentic Chat (Pro Plan, General Availability)
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (5)
💤 Files with no reviewable changes (1)
🧰 Additional context used🧬 Code Graph Analysis (2)src/wallet/wallet.h (4)
src/wallet/db.cpp (1)
🔇 Additional comments (7)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
utACK acddb7b
BTC Backport