Conversation
Allow wildcard descriptors to skip SPKs already owned by another keychain when revealing new SPKs. Reject non-wildcard descriptors whose only SPK is already claimed by another keychain with a new `NoDerivableSpk` error variant.
Add regression tests for non-wildcard descriptor rejection, both descriptor insertion orders, wildcard collision handling, and consecutive SPK collisions during SPK revelation.
Dmenec
left a comment
Dmenec
left a comment
There was a problem hiding this comment.
cACK
One question about the description. It says the new error should map to DescriptorOutputCollision in bdk_wallet, but that error doesn't seem to exist there. It is a proposal or something already mentioned somewhere?
|
|
||
| // Skip collided indices without updating `last_revealed` until an available index is found. | ||
| let mut probe = candidate; | ||
| let resolved = loop { |
There was a problem hiding this comment.
The skip makes reveal_to_target reveal more than asked. If index 3 is owned by another keychain, calling reveal_to_target(0, 3) after revealing up to 2 returns index 4. The loop checks next_index (3) against the target, but _reveal_next_spk then skips to 4. next_index has the same issue, it says 3 but the caller gets 4.
let (k0, k1) = colliding_descriptors();
let mut idx = KeychainTxOutIndex::<u8>::new(25, true);
idx.insert_descriptor(1u8, k1).unwrap();
idx.insert_descriptor(0u8, k0).unwrap();
let _ = idx.reveal_to_target(0u8, 2).unwrap();
assert_eq!(idx.next_index(0u8), Some((3, true)));
let (revealed, _) = idx.reveal_to_target(0u8, 3).unwrap();
assert_eq!(revealed[0].0, 4);
assert_eq!(idx.last_revealed_index(0u8), Some(4));
Maybe a test for it would be great
| }, | ||
| /// The descriptor's only derivable script pubkey is already owned by another keychain, so it | ||
| /// cannot produce a usable script pubkey. | ||
| NoDerivableSpk { |
There was a problem hiding this comment.
nit: what do you think about SpkAlreadyOwned? It would also help to include who owns it.
I don't think there's another case (at least in this scope) that needs the name to be that generic.
| /// because it has been assigned to another which produces the same script pubkey. | ||
| /// | ||
| /// A non-wildcard descriptor whose script pubkey is already claimed by another keychain is | ||
| /// rejected with [`InsertDescriptorError::NoDerivableSpk`]. |
There was a problem hiding this comment.
nit: the rejection depends on what's in the other keychain's lookahead, so the same insert can return Ok in one session and NoDerivableSpk after a reload (e.g. /2/* + /2/30 with lookahead 25, after revealing up to 10). Is that intended? Maybe worth a line here.
3 participants
Description
Fixes #2277.
KeychainTxOutIndexdoes not currently handle collisions when a descriptor derives an SPK that is already owned by another keychain.This PR allows wildcard descriptors to skip already-owned SPKs when revealing new scripts and continue to the next available SPK. Non-wildcard descriptors are rejected when their only SPK is already claimed, returning the new
NoDerivableSpkerror variant.Notes to the reviewers
Changelog notice
NoDerivableSpktoInsertDescriptorError.Checklists
All Submissions:
Bugfixes: