Skip to content
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

KeychainTxOutIndex range-based methods are broken #1459

Closed
evanlinjin opened this issue Jun 5, 2024 · 2 comments · Fixed by #1463
Closed

KeychainTxOutIndex range-based methods are broken #1459

evanlinjin opened this issue Jun 5, 2024 · 2 comments · Fixed by #1463
Assignees
Labels
bug Something isn't working
Milestone

Comments

@evanlinjin
Copy link
Member

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 evanlinjin added the bug Something isn't working label Jun 5, 2024
@evanlinjin evanlinjin added this to the 1.0.0-alpha milestone Jun 5, 2024
@evanlinjin evanlinjin self-assigned this Jun 5, 2024
@evanlinjin
Copy link
Member Author

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
Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

3 participants