Skip to content

[chain] Make KeychainTxOutIndex more range based#1324

Merged
evanlinjin merged 1 commit intobitcoindevkit:masterfrom
LLFourn:keychain_txout_more_range
Apr 18, 2024
Merged

[chain] Make KeychainTxOutIndex more range based#1324
evanlinjin merged 1 commit intobitcoindevkit:masterfrom
LLFourn:keychain_txout_more_range

Conversation

@LLFourn
Copy link
Copy Markdown
Collaborator

@LLFourn LLFourn commented Feb 6, 2024 •
edited
Loading

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

@LLFourn LLFourn force-pushed the keychain_txout_more_range branch from 0317415 to 775dad3 Compare February 6, 2024 06:47
@notmandatory notmandatory added this to the 1.0.0-alpha milestone Feb 9, 2024
@ValuedMammal
Copy link
Copy Markdown
Collaborator

ValuedMammal commented Mar 11, 2024 •
edited
Loading

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
Copy Markdown
Collaborator 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
Copy Markdown
Member

notmandatory commented Mar 20, 2024

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
Copy Markdown
Collaborator 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
@nickfarrow
[core+app] update bdk to 0.12 w patch
1e8a55c 
* 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
@nickfarrow
[core+app] update bdk to 0.12 w patches
123ac7e 
* 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
@nickfarrow
[core+app] update bdk to 0.12 w patches
5166364 
* 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 nickfarrow mentioned this pull request Apr 17, 2024
Merged
@evanlinjin evanlinjin force-pushed the keychain_txout_more_range branch from 775dad3 to e6167aa Compare April 18, 2024 07:30
@LLFourn @evanlinjin
feat(chain)!: make KeychainTxOutIndex more range based
fac2283 
`KeychainTxOutIndex` should try and avoid "all" kind of queries.
There may be subranges of interest. If the user wants "all" they can
just query "..".
@evanlinjin evanlinjin force-pushed the keychain_txout_more_range branch from e6167aa to fac2283 Compare April 18, 2024 07:32
evanlinjin
evanlinjin approved these changes Apr 18, 2024
View reviewed changes
Copy link
Copy Markdown
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
@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
@nickfarrow
[core+app] update bdk to 0.12 w patches
abc7c09 
* 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
@nickfarrow
[core+app] update bdk to 0.12 w patches
26a5fe6 
* 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
Closed
28 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@evanlinjin evanlinjin evanlinjin approved these changes

Assignees

@LLFourn LLFourn

Labels

api A breaking API change module-blockchain

Projects

BDK Wallet
Archived in project

Milestone

1.0.0-alpha

Development

Successfully merging this pull request may close these issues.

5 participants

@LLFourn @ValuedMammal @notmandatory @evanlinjin @nondiremanuel