bitcoindevkit / bdk

A modern, lightweight, descriptor-based wallet library written in Rust!
Other
860 stars 307 forks source link

`KeychainTxOutIndex` range-based methods are broken #1459

Closed evanlinjin closed 4 months ago

evanlinjin commented 4 months ago

Describe the bug

It turns out that having the internal SpkTxOutIndex use DescriptorIds is causing issues when using range-based methods.

The problem is evident when we query outputs in a given keychain (K) range, where the range is across multiple keychains. The keychain (K) range query does not translate well since the Ord of DescriptorIds is not guaranteed to be one-to-one with the Ord of keychains (K).

Potential fixes

The first solution (the one @LLFourn is working on) is to use keychain (K) with the internal SpkTxOutIndex. To maintain the "consistency", we disallow reassigning keychains and assigning the same descriptor to multiple keychains. The ChangeSet::append method will ignore keychain-reassignments and assigning the same descriptor to another keychain. This is the solution with the simplest codebase. Downsides is it disallows certain usecases (which we aren't even sure if users will use) - i.e. assigning the same descriptor to multiple keychains, and replacing the descriptor for a keychain.

The alternative is to continue to have the internal SpkTxOutIndex use DescriptorIds and when we range keychains, we first get corresponding DescriptorIds from KeychainTxOutIndex::keychains_to_descriptor_ids and range for individual DescriptorIds when querying the internal SpkTxOutIndex. This does introduce even more code, however it allows us complete flexibility and how changesets are appended will make more sense (in my opinion).

evanlinjin commented 4 months ago

Lloyd argues that the alternative solution is more complicated because we now need to range over 2 BTreeMaps and that the current implementation is an anti-feature where you can add two keychains that overlap and have non-exclusive ownership semantics over txouts.

notmandatory commented 4 months ago

Since we don't have a clear use case for re-assigning keychains I favor the simpler approach of disallowing it. We just need to clearly document that re-assignment is not allowed and ends up as a noop.