feat: watch-only import for wsh(sortedmulti) descriptors#702
feat: watch-only import for wsh(sortedmulti) descriptors#702DhruvArvindSingh wants to merge 4 commits into
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 20 minutes and 55 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds UniFFI-backed constructors to create multisig watch-only wallets from descriptor strings, with Kotlin bindings, Rust Wallet and WalletManager constructors, descriptor parsing/normalization, persistence of public descriptors, and actor initialization. Changes
Sequence Diagram(s)sequenceDiagram
participant Kotlin as Kotlin Client
participant UniFFI as UniFFI Bridge
participant Manager as RustWalletManager
participant Wallet as Wallet
participant BDK as BDK Store
participant Actors as Actor System
Kotlin->>UniFFI: tryNewFromMultisigDescriptor(descriptor)
UniFFI->>Manager: uniffi_fn_constructor_rustwalletmanager_try_new_from_multisig_descriptor(descriptor)
Manager->>Wallet: try_new_persisted_multisig_watch_only(descriptor)
Wallet->>Wallet: parse_multisig_descriptors(descriptor)
Wallet->>BDK: create persisted wallet (Descriptors, empty KeyMaps)
Wallet->>BDK: save public descriptors to keychain
Wallet->>Wallet: persist WalletMetadata (WatchOnly)
Wallet-->>Manager: return initialized Wallet
Manager->>Actors: spawn wallet actor
Manager->>Actors: spawn scanner actor (conditional)
Manager-->>UniFFI: return RustWalletManager (Loading)
UniFFI-->>Kotlin: return manager instance
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
Greptile SummaryThis PR implements watch-only multisig wallet import via
Confidence Score: 4/5Safe to merge with low risk, but the missing duplicate-wallet guard should be addressed to match the behaviour of every other hardware-import path before shipping. The core parsing and BDK wallet creation logic is sound and well-tested. However, the absence of a duplicate-check (a guard present on every comparable import path) is a real behaviour gap: importing the same descriptor twice silently creates two identical wallets, which is a concrete data-integrity issue rather than a theoretical concern. rust/src/wallet.rs — specifically Important Files Changed
Sequence DiagramsequenceDiagram
participant UI
participant RustWalletManager
participant Wallet
participant parse_multisig_descriptors
participant BdkStore
participant Keychain
participant Database
UI->>RustWalletManager: try_new_from_multisig_descriptor(descriptor)
RustWalletManager->>Wallet: try_new_persisted_multisig_watch_only(descriptor)
Wallet->>parse_multisig_descriptors: parse(input)
parse_multisig_descriptors-->>Wallet: (external, internal, name)
Note over Wallet: No duplicate check here
Wallet->>BdkStore: try_new(&id, network)
Wallet->>BdkStore: create_wallet(external, internal)
Wallet->>Keychain: save_public_descriptor(&id, external, internal)
Wallet->>Database: save_new_wallet_metadata(metadata)
Wallet-->>RustWalletManager: Self { id, metadata, ... }
RustWalletManager->>RustWalletManager: spawn WalletActor + WalletScanner
RustWalletManager-->>UI: Ok(RustWalletManager)
Reviews (1): Last reviewed commit: "Regenerate Swift and Kotlin bindings" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| pub fn try_new_persisted_multisig_watch_only(descriptor: String) -> Result<Self, WalletError> { | ||
| use bdk_wallet::keys::KeyMap; | ||
|
|
||
| let (external, internal, name) = parse_multisig_descriptors(&descriptor)?; | ||
|
|
||
| let keychain = Keychain::global(); | ||
| let database = Database::global(); | ||
| let network = database.global_config.selected_network(); | ||
|
|
||
| let id = WalletId::new(); | ||
| let mut metadata = WalletMetadata::new_for_hardware(id.clone(), name, None); | ||
| metadata.wallet_type = WalletType::WatchOnly; | ||
|
|
||
| let mut store = BdkStore::try_new(&id, network).map_err_str(WalletError::LoadError)?; | ||
|
|
||
| let wallet = Descriptors { | ||
| external: Descriptor { extended_descriptor: external.clone(), key_map: KeyMap::new() }, | ||
| internal: Descriptor { extended_descriptor: internal.clone(), key_map: KeyMap::new() }, | ||
| } | ||
| .into_create_params() | ||
| .network(network.into()) | ||
| .create_wallet(&mut store.conn) | ||
| .map_err_str(WalletError::BdkError)?; | ||
|
|
||
| keychain.save_public_descriptor(&id, external, internal)?; | ||
| database.wallets.save_new_wallet_metadata(metadata.clone())?; | ||
| CLOUD_BACKUP_MANAGER.handle_wallet_set_change(); | ||
|
|
||
| Ok(Self { id, metadata, network, bdk: wallet, db: Mutex::new(store.conn) }) | ||
| } | ||
|
|
||
| pub fn try_new_persisted_from_tap_signer( | ||
| tap_signer: Arc<cove_tap_card::TapSigner>, | ||
| derive: DeriveInfo, |
There was a problem hiding this comment.
Every other hardware-import path guards against re-importing the same wallet. try_new_persisted_from_pubport finds an existing wallet by fingerprint (and upgrades it); try_new_persisted_from_tap_signer calls check_for_duplicate_wallet. The new multisig path omits both checks, so importing the same descriptor string twice silently creates two identical wallets in the database and keychain.
A multisig wallet has multiple cosigner fingerprints rather than a single master fingerprint, so a descriptor-level dedup is needed — e.g., hashing the canonical form of the external descriptor and comparing it against existing wallets' stored descriptors before calling BdkStore::try_new.
| }, | ||
| _ => { | ||
| return Err(WalletError::UnsupportedWallet( | ||
| "only wsh(sortedmulti) descriptors are supported".to_string(), | ||
| )); | ||
| } | ||
| }; | ||
|
|
||
| Ok((external, internal, name)) | ||
| } | ||
|
|
||
| #[cfg(test)] | ||
| mod tests { | ||
| use super::*; | ||
|
|
||
| const XPUB1: &str = "xpub6CiKnWv7PPyyeb4kCwK4fidKqVjPfD9TP6MiXnzBVGZYNanNdY3mMvywcrdDc6wK82jyBSd95vsk26QujnJWPrSaPfYeyW7NyX37HHGtfQM"; | ||
| const XPUB2: &str = "xpub6DM7CYgaTMdMbhTcLTUWmNUE5WLXK5hx8ZMa4sRw8qYJPqtqKYiKnwsmT8A6AijDVAUZRivdBnXdR8QE7Y9vVnqvzPL3fXCmu1WtCRLdAoz"; | ||
|
|
||
| fn multisig_two_line() -> String { | ||
| let ext = format!( | ||
| "wsh(sortedmulti(2,[817e7be0/48h/0h/0h/2h]{XPUB1}/0/*,[a262308d/48h/0h/0h/2h]{XPUB2}/0/*))" | ||
| ); | ||
| let int = format!( | ||
| "wsh(sortedmulti(2,[817e7be0/48h/0h/0h/2h]{XPUB1}/1/*,[a262308d/48h/0h/0h/2h]{XPUB2}/1/*))" | ||
| ); | ||
| format!("{ext}\n{int}") | ||
| } | ||
|
|
||
| fn multisig_multipath() -> String { | ||
| format!( | ||
| "wsh(sortedmulti(2,[817e7be0/48h/0h/0h/2h]{XPUB1}/<0;1>/*,[a262308d/48h/0h/0h/2h]{XPUB2}/<0;1>/*))" |
There was a problem hiding this comment.
parse_multisig_descriptors checks that external is Wsh(SortedMulti(...)) in order to derive the wallet name, but internal is never inspected. In the two-line format a caller could supply a valid wsh(sortedmulti) for line 1 and a completely different descriptor type (e.g. wpkh(...)) for line 2. BDK will likely reject this at create_wallet, but the error surface will be an opaque BdkError rather than the clear UnsupportedWallet message. Validating internal with the same match block (or asserting external and internal use the same k() / pks().len()) would give callers a much better error.
| let name = match &external { | ||
| MiniscriptDescriptor::Wsh(wsh) => match wsh.as_inner() { | ||
| WshInner::SortedMulti(sm) => { | ||
| format!("{}-of-{} Multisig Watch-Only", sm.k(), sm.pks().len()) | ||
| } | ||
| _ => { | ||
| return Err(WalletError::UnsupportedWallet( | ||
| "only wsh(sortedmulti) descriptors are supported".to_string(), |
There was a problem hiding this comment.
<0;1> expansion replaces all occurrences via plain string substitution
single.replace("<0;1>", "0") calls str::replace which replaces every occurrence of the substring. For the common case where every key in the descriptor has /<0;1>/* this produces the correct result, but a descriptor mixing <0;1> and a literal range like <2;3> would be mishandled, and any future caller passing an unusual BIP-389 form could get silently wrong output. Using a proper descriptor parser or at minimum validating that only <0;1> wildcards are present before expanding would make this more robust.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
rust/src/wallet.rs (1)
770-772: Usemap_err_strfor this error conversion.This matches the repository helper style and avoids the manual
to_string()closure. As per coding guidelines, usecove_util::ResultExt::map_err_strinstead of.map_err(|e| Error::Variant(e.to_string())).MiniscriptDescriptor::<DescriptorPublicKey>::parse_descriptor(&secp, s) .map(|(d, _)| d) - .map_err(|e| WalletError::DescriptorKeyParseError(e.to_string())) + .map_err_str(WalletError::DescriptorKeyParseError)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/src/wallet.rs` around lines 770 - 772, Replace the manual error-to-string conversion on the parse_descriptor call by using the repository helper map_err_str: change the tail of MiniscriptDescriptor::<DescriptorPublicKey>::parse_descriptor(&secp, s).map(|(d, _)| d).map_err(|e| WalletError::DescriptorKeyParseError(e.to_string())) to use cove_util::ResultExt::map_err_str and construct WalletError::DescriptorKeyParseError from the mapped str, i.e., call map_err_str with the WalletError::DescriptorKeyParseError constructor so the error conversion follows the project helper pattern.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@rust/src/manager/wallet_manager.rs`:
- Around line 395-404: The code currently creates a persisted wallet
(Wallet::try_new_persisted_multisig_watch_only) and then spawns the scanner
before constructing the actor, which can leave a persisted wallet (and scanner)
behind if WalletActor::new fails; fix by constructing WalletActor::new(wallet,
sender.clone()) first, and only spawn the scanner
(WalletScanner::try_new(...).ok().map(spawn_actor)) after the actor has been
successfully created and task::spawn_actor called; if WalletActor::new returns
an error, delete the just-created persisted wallet (use the wallet id/metadata
to remove the persisted store) before returning the error to avoid leaving
orphaned state.
In `@rust/src/wallet.rs`:
- Around line 373-390: The create_wallet succeeded but subsequent persistence
calls (keychain.save_public_descriptor and
database.wallets.save_new_wallet_metadata) can fail leaving an orphaned BDK
store and partial keychain/metadata; wrap those persistence steps in a fallible
block and on any error perform cleanup: remove the saved public descriptor from
the keychain (call the corresponding remove method for the keychain using the
WalletId), delete the BDK store (use BdkStore removal API or a conn/cleanup
method on the created store identified by id), and remove any partially inserted
wallet metadata from database.wallets (use the delete/remove metadata API), then
return the original error; only call
CLOUD_BACKUP_MANAGER.handle_wallet_set_change() after all persistence succeeded.
Ensure you reference WalletId/id, BdkStore::try_new/store,
Descriptors::into_create_params().create_wallet,
keychain.save_public_descriptor, and database.wallets.save_new_wallet_metadata
when locating where to add the transactional/rollback logic.
- Around line 790-806: The code only validates the `external` descriptor as
`MiniscriptDescriptor::Wsh` with `WshInner::SortedMulti`, but the `internal`
descriptor must be validated the same way to prevent persisting mismatched or
unsupported internals; update the logic that builds `name` (and any persistence
path) to also match on the `internal` descriptor (e.g., check `internal` is
`MiniscriptDescriptor::Wsh` and its `as_inner()` yields
`WshInner::SortedMulti`), return `WalletError::UnsupportedWallet` with the same
message if the internal is invalid, and ensure any uses of
`sm.k()`/`sm.pks().len()` come from the validated descriptor(s) so change
outputs aren’t misclassified.
- Around line 779-782: The multipath expansion branch that handles strings
containing "<0;1>" must remove any trailing descriptor checksum before doing the
replace so parse(...) doesn't fail; in the branch matching
single.contains("<0;1>") first strip any "#..." suffix from single (e.g.,
compute let base = single.splitn(2, '#').next().unwrap_or(single);), then create
ext_str and int_str by replacing "<0;1>" on base (ext_str =
base.replace("<0;1>", "0") and int_str = base.replace("<0;1>", "1")), and
finally call (parse(&ext_str)?, parse(&int_str)?); this ensures
parse/parse_descriptor validation runs on checksum-free descriptors as
suggested.
---
Nitpick comments:
In `@rust/src/wallet.rs`:
- Around line 770-772: Replace the manual error-to-string conversion on the
parse_descriptor call by using the repository helper map_err_str: change the
tail of MiniscriptDescriptor::<DescriptorPublicKey>::parse_descriptor(&secp,
s).map(|(d, _)| d).map_err(|e|
WalletError::DescriptorKeyParseError(e.to_string())) to use
cove_util::ResultExt::map_err_str and construct
WalletError::DescriptorKeyParseError from the mapped str, i.e., call map_err_str
with the WalletError::DescriptorKeyParseError constructor so the error
conversion follows the project helper pattern.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: fdf113b9-1dc1-4515-b6f1-e74e924aa7fe
⛔ Files ignored due to path filters (1)
ios/CoveCore/Sources/CoveCore/generated/cove.swiftis excluded by!**/generated/**
📒 Files selected for processing (3)
android/app/src/main/java/org/bitcoinppl/cove_core/cove.ktrust/src/manager/wallet_manager.rsrust/src/wallet.rs
| let id = WalletId::new(); | ||
| let mut metadata = WalletMetadata::new_for_hardware(id.clone(), name, None); | ||
| metadata.wallet_type = WalletType::WatchOnly; | ||
|
|
||
| let mut store = BdkStore::try_new(&id, network).map_err_str(WalletError::LoadError)?; | ||
|
|
||
| let wallet = Descriptors { | ||
| external: Descriptor { extended_descriptor: external.clone(), key_map: KeyMap::new() }, | ||
| internal: Descriptor { extended_descriptor: internal.clone(), key_map: KeyMap::new() }, | ||
| } | ||
| .into_create_params() | ||
| .network(network.into()) | ||
| .create_wallet(&mut store.conn) | ||
| .map_err_str(WalletError::BdkError)?; | ||
|
|
||
| keychain.save_public_descriptor(&id, external, internal)?; | ||
| database.wallets.save_new_wallet_metadata(metadata.clone())?; | ||
| CLOUD_BACKUP_MANAGER.handle_wallet_set_change(); |
There was a problem hiding this comment.
Roll back partially persisted multisig imports.
If save_public_descriptor or save_new_wallet_metadata fails after create_wallet, this leaves an orphaned BDK store and possibly keychain data. Wrap the persistence steps like the mnemonic path does and clean up keychain, BDK store, and wallet metadata on error.
🧹 Proposed rollback structure
- let mut store = BdkStore::try_new(&id, network).map_err_str(WalletError::LoadError)?;
+ let create_wallet = || -> Result<Self, WalletError> {
+ let mut store = BdkStore::try_new(&id, network).map_err_str(WalletError::LoadError)?;
- let wallet = Descriptors {
- external: Descriptor { extended_descriptor: external.clone(), key_map: KeyMap::new() },
- internal: Descriptor { extended_descriptor: internal.clone(), key_map: KeyMap::new() },
- }
- .into_create_params()
- .network(network.into())
- .create_wallet(&mut store.conn)
- .map_err_str(WalletError::BdkError)?;
+ let wallet = Descriptors {
+ external: Descriptor {
+ extended_descriptor: external.clone(),
+ key_map: KeyMap::new(),
+ },
+ internal: Descriptor {
+ extended_descriptor: internal.clone(),
+ key_map: KeyMap::new(),
+ },
+ }
+ .into_create_params()
+ .network(network.into())
+ .create_wallet(&mut store.conn)
+ .map_err_str(WalletError::BdkError)?;
- keychain.save_public_descriptor(&id, external, internal)?;
- database.wallets.save_new_wallet_metadata(metadata.clone())?;
- CLOUD_BACKUP_MANAGER.handle_wallet_set_change();
+ keychain.save_public_descriptor(&id, external, internal)?;
+ database.wallets.save_new_wallet_metadata(metadata.clone())?;
+ CLOUD_BACKUP_MANAGER.handle_wallet_set_change();
- Ok(Self { id, metadata, network, bdk: wallet, db: Mutex::new(store.conn) })
+ Ok(Self { id: id.clone(), metadata, network, bdk: wallet, db: Mutex::new(store.conn) })
+ };
+
+ match create_wallet() {
+ Ok(wallet) => Ok(wallet),
+ Err(error) => {
+ keychain.delete_wallet_items(&id);
+ if let Err(cleanup_error) = delete_wallet_specific_data(&id) {
+ warn!("clean up failed, failed to delete wallet data: {cleanup_error}");
+ }
+ if let Err(cleanup_error) = database.wallets.delete(&id) {
+ warn!("clean up failed, failed to delete wallet metadata: {cleanup_error}");
+ }
+ Err(error)
+ }
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@rust/src/wallet.rs` around lines 373 - 390, The create_wallet succeeded but
subsequent persistence calls (keychain.save_public_descriptor and
database.wallets.save_new_wallet_metadata) can fail leaving an orphaned BDK
store and partial keychain/metadata; wrap those persistence steps in a fallible
block and on any error perform cleanup: remove the saved public descriptor from
the keychain (call the corresponding remove method for the keychain using the
WalletId), delete the BDK store (use BdkStore removal API or a conn/cleanup
method on the created store identified by id), and remove any partially inserted
wallet metadata from database.wallets (use the delete/remove metadata API), then
return the original error; only call
CLOUD_BACKUP_MANAGER.handle_wallet_set_change() after all persistence succeeded.
Ensure you reference WalletId/id, BdkStore::try_new/store,
Descriptors::into_create_params().create_wallet,
keychain.save_public_descriptor, and database.wallets.save_new_wallet_metadata
when locating where to add the transactional/rollback logic.
| [single] if single.contains("<0;1>") => { | ||
| let ext_str = single.replace("<0;1>", "0"); | ||
| let int_str = single.replace("<0;1>", "1"); | ||
| (parse(&ext_str)?, parse(&int_str)?) |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Does rust-miniscript or BDK parse_descriptor reject an output descriptor when the #checksum suffix no longer matches the descriptor body?
💡 Result:
Yes, both rust-miniscript and BDK reject output descriptors when the #checksum suffix no longer matches the descriptor body. For BDK, the descriptor error enum explicitly includes InvalidDescriptorChecksum: "The provided descriptor doesn't match its checksum" (result 11). BDK re-exports miniscript types and uses the same parsing logic (results 9, 61). For rust-miniscript (the miniscript crate), parsing uses FromStr on Descriptor which relies on the checksum module (result 3). Issue #554 discusses checksum validation during parsing and to_string, confirming parse checks the checksum and fails if mismatched (result 1). Docs show Descriptor::from_str exists and parsing involves checksum verification (results 17, 19, 60).
Citations:
- 1: String descriptors can panic on
to_stringrust-bitcoin/rust-miniscript#554 - 2: https://docs.rs/miniscript/latest/miniscript/descriptor/index.html
- 3: https://docs.rs/bdk/latest/bdk/descriptor/index.html
- 4: https://docs.rs/bdk/0.3.0/bdk/descriptor/error/enum.Error.html
- 5: https://docs.rs/miniscript/latest/miniscript/
- 6: https://docs.rs/miniscript/latest/miniscript/descriptor/struct.Pkh.html
- 7: https://docs.rs/miniscript/0.2.1/miniscript/descriptor/enum.Descriptor.html
🏁 Script executed:
# First, let's view the exact code at the specified lines
cat -n rust/src/wallet.rs | sed -n '770,790p'Repository: bitcoinppl/cove
Length of output: 1058
🏁 Script executed:
# Find the parse function being used
rg "fn parse\(" rust/src/wallet.rs -A 3Repository: bitcoinppl/cove
Length of output: 41
🏁 Script executed:
# Check for any checksum stripping or handling around this code
rg "split_once.*#|checksum|strip" rust/src/wallet.rs -B 2 -A 2Repository: bitcoinppl/cove
Length of output: 41
🏁 Script executed:
# Look for callers of this function and surrounding context
rg "parse_descriptors\(" rust/src/wallet.rs -B 5 -A 5Repository: bitcoinppl/cove
Length of output: 41
🏁 Script executed:
# Check if there's any checksum stripping in the file earlier
rg "split.*#|checksum" rust/src/ -rRepository: bitcoinppl/cove
Length of output: 472
🏁 Script executed:
# Check if inputs are processed before reaching this code
head -n 800 rust/src/wallet.rs | tail -n 100Repository: bitcoinppl/cove
Length of output: 3630
Strip descriptor checksums before multipath expansion.
When expanding <0;1> multipath notation on a checksummed descriptor (e.g., from Sparrow or Nunchuk exports), replacing <0;1> with 0/1 leaves the original #checksum attached to modified descriptor strings. Since parse_descriptor() validates checksums during parsing, both resulting descriptors fail validation and reject valid exports.
Strip checksums before replacement:
Fix
[single] if single.contains("<0;1>") => {
- let ext_str = single.replace("<0;1>", "0");
- let int_str = single.replace("<0;1>", "1");
+ let descriptor = single.split_once('#').map_or(single, |(descriptor, _)| descriptor);
+ let ext_str = descriptor.replace("<0;1>", "0");
+ let int_str = descriptor.replace("<0;1>", "1");
(parse(&ext_str)?, parse(&int_str)?)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| [single] if single.contains("<0;1>") => { | |
| let ext_str = single.replace("<0;1>", "0"); | |
| let int_str = single.replace("<0;1>", "1"); | |
| (parse(&ext_str)?, parse(&int_str)?) | |
| [single] if single.contains("<0;1>") => { | |
| let descriptor = single.split_once('#').map_or(single, |(descriptor, _)| descriptor); | |
| let ext_str = descriptor.replace("<0;1>", "0"); | |
| let int_str = descriptor.replace("<0;1>", "1"); | |
| (parse(&ext_str)?, parse(&int_str)?) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@rust/src/wallet.rs` around lines 779 - 782, The multipath expansion branch
that handles strings containing "<0;1>" must remove any trailing descriptor
checksum before doing the replace so parse(...) doesn't fail; in the branch
matching single.contains("<0;1>") first strip any "#..." suffix from single
(e.g., compute let base = single.splitn(2, '#').next().unwrap_or(single);), then
create ext_str and int_str by replacing "<0;1>" on base (ext_str =
base.replace("<0;1>", "0") and int_str = base.replace("<0;1>", "1")), and
finally call (parse(&ext_str)?, parse(&int_str)?); this ensures
parse/parse_descriptor validation runs on checksum-free descriptors as
suggested.
| let name = match &external { | ||
| MiniscriptDescriptor::Wsh(wsh) => match wsh.as_inner() { | ||
| WshInner::SortedMulti(sm) => { | ||
| format!("{}-of-{} Multisig Watch-Only", sm.k(), sm.pks().len()) | ||
| } | ||
| _ => { | ||
| return Err(WalletError::UnsupportedWallet( | ||
| "only wsh(sortedmulti) descriptors are supported".to_string(), | ||
| )); | ||
| } | ||
| }, | ||
| _ => { | ||
| return Err(WalletError::UnsupportedWallet( | ||
| "only wsh(sortedmulti) descriptors are supported".to_string(), | ||
| )); | ||
| } | ||
| }; |
There was a problem hiding this comment.
Validate the internal descriptor too.
Only external is checked for wsh(sortedmulti). A two-line import with a valid external descriptor and an unsupported or mismatched internal descriptor can still be persisted, which breaks the function’s stated contract and can misclassify change outputs.
🔍 Suggested shape validation
- let name = match &external {
- MiniscriptDescriptor::Wsh(wsh) => match wsh.as_inner() {
- WshInner::SortedMulti(sm) => {
- format!("{}-of-{} Multisig Watch-Only", sm.k(), sm.pks().len())
- }
- _ => {
- return Err(WalletError::UnsupportedWallet(
- "only wsh(sortedmulti) descriptors are supported".to_string(),
- ));
- }
- },
- _ => {
- return Err(WalletError::UnsupportedWallet(
- "only wsh(sortedmulti) descriptors are supported".to_string(),
- ));
- }
- };
+ let sortedmulti_shape = |descriptor: &ExtendedDescriptor| match descriptor {
+ MiniscriptDescriptor::Wsh(wsh) => match wsh.as_inner() {
+ WshInner::SortedMulti(sm) => Ok((sm.k(), sm.pks().len())),
+ _ => Err(WalletError::UnsupportedWallet(
+ "only wsh(sortedmulti) descriptors are supported".to_string(),
+ )),
+ },
+ _ => Err(WalletError::UnsupportedWallet(
+ "only wsh(sortedmulti) descriptors are supported".to_string(),
+ )),
+ };
+
+ let external_shape = sortedmulti_shape(&external)?;
+ let internal_shape = sortedmulti_shape(&internal)?;
+ if external_shape != internal_shape {
+ return Err(WalletError::UnsupportedWallet(
+ "external and internal multisig descriptors must use the same threshold and signer count"
+ .to_string(),
+ ));
+ }
+
+ let name = format!("{}-of-{} Multisig Watch-Only", external_shape.0, external_shape.1);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@rust/src/wallet.rs` around lines 790 - 806, The code only validates the
`external` descriptor as `MiniscriptDescriptor::Wsh` with
`WshInner::SortedMulti`, but the `internal` descriptor must be validated the
same way to prevent persisting mismatched or unsupported internals; update the
logic that builds `name` (and any persistence path) to also match on the
`internal` descriptor (e.g., check `internal` is `MiniscriptDescriptor::Wsh` and
its `as_inner()` yields `WshInner::SortedMulti`), return
`WalletError::UnsupportedWallet` with the same message if the internal is
invalid, and ensure any uses of `sm.k()`/`sm.pks().len()` come from the
validated descriptor(s) so change outputs aren’t misclassified.
|
@DhruvArvindSingh I think this is related to the project work and it should be done by the selected contributor for the project. What are your thoughts on this @praveenperera |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@rust/src/wallet.rs`:
- Around line 375-382: The duplicate-check currently swallows errors and
compares descriptors by string rendering; change get_all(network,
mode).unwrap_or_default() to propagate the error (return Err) instead of masking
it, call keychain.get_public_descriptor(&wm.id) and handle its Err by
propagating a failure (do not ignore with if let Ok), and parse external_str
once into an ExtendedDescriptor (or equivalent parsed type) then compare the
parsed stored_ext value to that parsed external descriptor using equality
(stored_ext == parsed_external) rather than stored_ext.to_string() ==
external_str so transient DB/keychain errors stop the import and descriptor
equivalence is robust.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0fa737eb-b0fb-4e42-8a35-78edb6a3ef00
📒 Files selected for processing (2)
rust/src/manager/wallet_manager.rsrust/src/wallet.rs
✅ Files skipped from review due to trivial changes (1)
- rust/src/manager/wallet_manager.rs
| let existing = database.wallets.get_all(network, mode).unwrap_or_default(); | ||
| for wm in existing { | ||
| if let Ok(Some((stored_ext, _))) = keychain.get_public_descriptor(&wm.id) { | ||
| if stored_ext.to_string() == external_str { | ||
| return Err(WalletError::WalletAlreadyExists(wm.id)); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Duplicate detection silently bypassed on errors, and string comparison is fragile.
Two concerns in this block:
database.wallets.get_all(network, mode).unwrap_or_default()(line 375) andif let Ok(Some(...))onkeychain.get_public_descriptor(&wm.id)(line 377) swallow DB/keychain errors. If either call fails transiently, the duplicate check is skipped and the same multisig can be imported twice, creating two independent BDK stores for the same wallet.- Comparing descriptors via
stored_ext.to_string() == external_strdepends on both sides rendering to byte-identical strings (including checksum). Comparing the parsedExtendedDescriptorvalues directly is more robust than round-tripping throughDisplay.
🛡️ Suggested fix
- let external_str = external.to_string();
- let existing = database.wallets.get_all(network, mode).unwrap_or_default();
- for wm in existing {
- if let Ok(Some((stored_ext, _))) = keychain.get_public_descriptor(&wm.id) {
- if stored_ext.to_string() == external_str {
- return Err(WalletError::WalletAlreadyExists(wm.id));
- }
- }
- }
+ let existing = database.wallets.get_all(network, mode)?;
+ for wm in existing {
+ let Some((stored_ext, _)) = keychain.get_public_descriptor(&wm.id)? else {
+ continue;
+ };
+ if stored_ext == external {
+ return Err(WalletError::WalletAlreadyExists(wm.id));
+ }
+ }As per coding guidelines: "Suggest improvements to error handling, unwraps, and Result handling."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@rust/src/wallet.rs` around lines 375 - 382, The duplicate-check currently
swallows errors and compares descriptors by string rendering; change
get_all(network, mode).unwrap_or_default() to propagate the error (return Err)
instead of masking it, call keychain.get_public_descriptor(&wm.id) and handle
its Err by propagating a failure (do not ignore with if let Ok), and parse
external_str once into an ExtendedDescriptor (or equivalent parsed type) then
compare the parsed stored_ext value to that parsed external descriptor using
equality (stored_ext == parsed_external) rather than stored_ext.to_string() ==
external_str so transient DB/keychain errors stop the import and descriptor
equivalence is robust.
| keychain.save_public_descriptor(&id, external, internal)?; | ||
| database.wallets.save_new_wallet_metadata(metadata.clone())?; |
There was a problem hiding this comment.
If create_wallet succeeds but save_public_descriptor or save_new_wallet_metadata fails, this returns an error after creating the BDK store and possibly saving keychain data. That leaves orphaned wallet state behind even though the import is reported as failed. Wrap the create/persist sequence in a fallible block and, on any error after the new id is allocated, clean up with keychain.delete_wallet_items(&id), delete_wallet_specific_data(&id), and database.wallets.delete(&id) before returning the original error. Only call CLOUD_BACKUP_MANAGER.handle_wallet_set_change() after all persistence succeeds.
| let wallet_actor = WalletActor::new(wallet, sender.clone()) | ||
| .map_err(|e| Error::DatabaseCorruption { id: id.clone(), error: e.to_string() })?; |
There was a problem hiding this comment.
Wallet::try_new_persisted_multisig_watch_only has already persisted the wallet before WalletActor::new opens the wallet data DB. If WalletActor::new fails here, the constructor returns DatabaseCorruption while the imported wallet remains in BDK/keychain/database state, so the user sees a failed import but may be blocked from retrying by the duplicate check. Handle the WalletActor::new error explicitly and delete the just-created wallet data/keychain items/metadata before returning.
| )), | ||
| }; | ||
|
|
||
| let name = match &external { |
There was a problem hiding this comment.
Only the external descriptor is validated as wsh(sortedmulti(...)); the internal descriptor can be any parseable descriptor in the two-line format. That can persist a wallet whose change descriptor is unsupported or mismatched while still naming it from the external multisig shape. Validate internal with the same WshInner::SortedMulti check and reject mismatched threshold/signer count before returning the descriptors.
| let (external, internal) = match lines.as_slice() { | ||
| [ext, int] => (parse(ext)?, parse(int)?), | ||
| [single] if single.contains("<0;1>") => { | ||
| let ext_str = single.replace("<0;1>", "0"); |
There was a problem hiding this comment.
This expands <0;1> before parsing, but checksummed descriptors from common exports include a trailing #checksum. Replacing the multipath segment changes the descriptor body while leaving the old checksum attached, so valid checksummed multipath imports are rejected by descriptor parsing. Strip any #... suffix before building ext_str and int_str, then parse the checksum-free expanded descriptors. Add some failing tests that confirm this before fixing.
| let mode = database.global_config.wallet_mode(); | ||
|
|
||
| let external_str = external.to_string(); | ||
| let existing = database.wallets.get_all(network, mode).unwrap_or_default(); |
There was a problem hiding this comment.
The duplicate check silently falls back to an empty wallet list when get_all fails, and line 377 also ignores keychain read/parse errors. A transient DB/keychain error can therefore bypass duplicate detection and create another copy of the same watch-only wallet. Propagate these errors during import instead of treating them as “no duplicate”; compare the parsed stored descriptor to the parsed new external descriptor rather than relying on string rendering.
3 participants
Summary
Implements watch-only multisig wallet import via wsh(sortedmulti) descriptors, as scoped in issue #491.
Testing
The parse_multisig_descriptors helper accepts two formats:
I have't tested it, as their is no UI wired up yet
Platform Coverage
Checklist
Summary by CodeRabbit
New Features
Tests