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

[chain] Make KeychainTxOutIndex more range based #1324

Merged
merged 1 commit into from
Apr 18, 2024

Conversation

LLFourn
Copy link
Contributor

@LLFourn LLFourn commented Feb 6, 2024

KeychainTxOut index should try and avoid "all" kind of queries. There may be subranges of interest. If the user wants "all" they can just query "..".

The ideas is that KeychainTxOutIndex should be designed to be able to incorporate many unrelated keychains that can be managed in the same index. We should be able to see the "net_value" of a transaction to a specific subrange. e.g. imagine a collaborative custody service that manages all their user descriptors inside the same KeychainTxOutIndex. One user in their service may pay another so when you are analyzing how much a transaction is spending for a particular user you need to do analyze a particular sub-range.

Notes to the reviewers

  • I didn't change unused_spks to follow this rule because I want to delete that method some time in the future. unused_spks is being used in the examples for syncing but it shouldn't be (the discussion as to why will probably surface in Add wallet sync_request and full_scan_request functions #1194).
  • I haven't applied this reasoning to the methods that return BTreeMaps e.g. all_unbounded_spk_iters. It probably should be but I haven't made up my mind yet.

This probably belongs after #1194

Changelog notice

  • KeychainTxOutIndex methods modified to take ranges of keychains instead.

Checklists

All Submissions:

  • I've signed all my commits
  • I followed the contribution guidelines
  • I ran cargo fmt and cargo clippy before committing

New Features:

  • I've added tests for the new feature
  • I've added docs for the new feature

@notmandatory notmandatory added this to the 1.0.0-alpha milestone Feb 9, 2024
@ValuedMammal
Copy link
Contributor

ValuedMammal commented Mar 11, 2024

If I understand, you're using range in a couple different ways: in keychain/txout_index to range over keychains, and in spk_txout_index to range over derivation indexes - with map_to_inner_bounds having some mediating role

Am I right in thinking these are similar but distinct concepts? or is it just two ways of looking at the same thing?

@notmandatory notmandatory added the api A breaking API change label Mar 12, 2024
@LLFourn
Copy link
Contributor Author

LLFourn commented Mar 18, 2024

If I understand, you're using range in a couple different ways: in keychain/txout_index to range over keychains, and in spk_txout_index to range over derivation indexes - with map_to_inner_bounds having some mediating role

Am I right in thinking these are similar but distinct concepts? or is it just two ways of looking at the same thing?

SpkTxOutIndex they are similar. KeychainTxOutIndex is an SpkTxOutIndex with a specialized structure where each group of spks is under something called a "keychain". In SpkTxOutIndex each spk is under its own unique key. In order to query across multiple keychains in KeychainTxOutIndex, you need to be able to query across sub-ranges of indices in SpkTxOutIndex.

@notmandatory
Copy link
Member

Do we have a current need for this or can it be moved to a post 1.0 release as a non-breaking internal optimization?

@nondiremanuel nondiremanuel removed this from the 1.0.0-alpha milestone Mar 21, 2024
@LLFourn
Copy link
Contributor Author

LLFourn commented Mar 28, 2024

It's an API change for bdk_chain and doesn't effect bdk_wallet I don't think. it doesn't need a milestone since if I understand that system is only meant for prioritizing tasks with respect to bdk_wallet where as the order of other tasks is left to the discretion of the devs.

nickfarrow added a commit to frostsnap/frostsnap that referenced this pull request Apr 17, 2024
* Rebased bitcoindevkit/bdk#1324 onto master
* Switch to this bdk_chain patch and use v0.12
* Small bdk API changes
nickfarrow added a commit to frostsnap/frostsnap that referenced this pull request Apr 17, 2024
* Upgrades bdk to version 0.12 with minor api changes.
* Patches include keychain_txout_more_range changes from
  bitcoindevkit/bdk#1324,
* Additionally includes an electrum-client with an upgraded rustls.
* Frostsnapp no longer depends on ring.
nickfarrow added a commit to frostsnap/frostsnap that referenced this pull request Apr 17, 2024
* Upgrades bdk to version 0.12 with minor api changes.
* Patch keeps the keychain_txout_more_range changes from
  bitcoindevkit/bdk#1324,
* This bdk rev uses an updated electrum-client with upgraded rustls.
* Frostsnapp no longer depends on ring.
`KeychainTxOutIndex` should try and avoid "all" kind of queries.
There may be subranges of interest. If the user wants "all" they can
just query "..".
Copy link
Member

@evanlinjin evanlinjin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK fac2283

@evanlinjin evanlinjin merged commit 52f3955 into bitcoindevkit:master Apr 18, 2024
12 checks passed
@notmandatory notmandatory added this to the 1.0.0-alpha milestone Apr 18, 2024
@LLFourn LLFourn deleted the keychain_txout_more_range branch April 19, 2024 09:02
nickfarrow added a commit to frostsnap/frostsnap that referenced this pull request Apr 22, 2024
* Upgrades bdk to version 0.12 with minor api changes.
* Patch keeps the keychain_txout_more_range changes from
  bitcoindevkit/bdk#1324,
* This bdk rev uses an updated electrum-client with upgraded rustls.
* Frostsnapp no longer depends on ring.
nickfarrow added a commit to frostsnap/frostsnap that referenced this pull request Apr 22, 2024
* Upgrades bdk to version 0.12 with minor api changes.
* Patch keeps the keychain_txout_more_range changes from
  bitcoindevkit/bdk#1324,
* This bdk rev uses an updated electrum-client with upgraded rustls.
* Frostsnapp no longer depends on ring.
@danielabrozzoni danielabrozzoni mentioned this pull request May 2, 2024
28 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api A breaking API change module-blockchain
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants