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.

Sign up for GitHub

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

Jump to bottom

Various improvements not included in #1203 (new) #1451

Closed
wants to merge 7 commits into from
Closed

Various improvements not included in #1203 (new) #1451

wants to merge 7 commits into from

Conversation

LagginTimes
Copy link
Contributor

@LagginTimes LagginTimes commented May 28, 2024 •
edited
Loading

Closes #1439

Description

These were tasks that were intended to be included in #1438.
See #1438 (comment).

Original Ticket Description

Adds various improvements to the work of #1203. These were missed out while cherry-picking, or review comments left in #1428 that were forgotten:

  • Change keychains_to_descriptors to keychains_to_descriptor_ids which simplifies the field. This was mentioned here and included in Further work on #1203 #1428, but an older commit was cherry-picked.
  • Rename KeychainTxOutIndex field descriptor_ids_to_keychain_set to descriptor_ids_to_keychains was missed out as an older commit was cherry-picked. This change to naming shows the direct relationship between keychains_to_desriptor_ids and descriptor_ids_to_keychains (one is directly a reverse lookup of the other).
  • Change reveal_to_target_with_id to reveal_to_target_with_descriptor, reasoning mentioned here.

In addition to this, I changed the output signature of reveal_to_target, reveal_next_spk and next_unused_spk methods to return (Option<spk(s)>, changeset), whereas previously it was Option<(spk(s), changeset)>. This makes the API more consistent as the ChangeSet is always returned, and reveal_to_target and unbounded_spk_iter-esc methods all return Option<SpkIterator> (which we can .flatten()).

Notes to the reviewers

Not all changes in this PR are Changelog-worthy. I.e. renaming of internal variables to increase readability, changing code comments, refactoring private methods - are all excluded from the changelog.

Changelog notice

  • Change KeychainTxOutIndex methods to always return a changeset. This makes the API more consistent.
  • Change KeychainTxOutIndex methods to return spks as ScriptBuf instead of references (&Script) to reduce lifetimes of mutable borrows.

Checklists

All Submissions:

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

@evanlinjin
Copy link
Member

@LagginTimes could you also rebase this please? Thanks!

evanlinjin and others added 5 commits May 30, 2024 18:14
@evanlinjin @LagginTimes
chore(chain): simplify keychains_to_descriptor_ids field
9f2c16c 
Also rename `descriptor_ids_to_keychain_set` to
`descriptor_ids_to_keychains` and update it's documentation.
@evanlinjin @LagginTimes
feat!: change KeychainTxOutIndex methods to always return changesets
24c2b81 
Change the out signature of methods `reveal_to_target`,
`reveal_next_spk` and `next_unused_spk` to return a tuple of
`(Option<spk(s)>, changeset)` instead of `Option<(spk(s), changeset)>`.
This makes the API more consistent.

Also refactored various helper methods to take in a descriptor instead
of a descriptor id. `.expect` calls now exist outside of these helper
methods, making it more obvious where they are being called.

Docs are updated to reflect the new API.
@LagginTimes
chore(chain): rename fields in KeychainTxOutIndex struct
601e355 
Rename `descriptor_ids_to_descriptors` to `descriptors`.
Rename `descriptor_ids_to_keychains` to `keychains`.
@LagginTimes
chore(chain): fixed comment in insert_descriptor
72fde6d
@LagginTimes
fix: reimplement From for keychain::ChangeSet
910a5b5
@LagginTimes LagginTimes marked this pull request as draft May 30, 2024 10:24
@LagginTimes LagginTimes marked this pull request as ready for review June 3, 2024 05:32
evanlinjin
evanlinjin reviewed Jun 4, 2024
View reviewed changes
crates/chain/src/keychain/txout_index.rs
// a keychain, we return it with the highest-ranked keychain with it. We rank keychains by
// `Ord`, therefore the keychain set is a `BTreeSet`. The earliest keychain variant (according
// to `Ord`) has precedence.
keychains: HashMap<DescriptorId, BTreeSet<K>>,
Copy link
Member

Choose a reason for hiding this comment

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

I have a second thought about this renaming. What do you think about: ids_by_keychain and keychains_by_id?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My initial thought is, if we're simplifying descriptor_ids_to_keychains to something like keychains_by_id, would it make sense to do something similar to keychains_to_descriptor_ids?

Copy link
Member

@evanlinjin evanlinjin Jun 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I meant keychains_by_ids to replace descriptor_ids_to_keychains. I was suggesting it because the wording makes more sense to me. However, ids_to_keychains can work too.

Edit: Sorry @LagginTimes I completely misread the comment. Yes, since one field is a reverse-index of the other so it makes sense that the names correlate.

Copy link
Member

Choose a reason for hiding this comment

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

🤔 my 2 sats is names should match plurality of the related data. So something like:

  • keychain_to_descriptor_id, maps one keychain to one descriptor_id
  • descriptor_id_to_keychains, maps one descriptor_id to multiple keychains

@evanlinjin evanlinjin changed the title Various improvements not included in #1438 Various improvements not included in #1203 Jun 4, 2024
@evanlinjin evanlinjin changed the title Various improvements not included in #1203 Various improvements not included in #1203 (new) Jun 4, 2024
@evanlinjin evanlinjin mentioned this pull request Jun 4, 2024
Closed
3 tasks
@notmandatory notmandatory added module-blockchain api A breaking API change labels Jun 4, 2024
@notmandatory notmandatory added this to the 1.0.0-alpha milestone Jun 4, 2024
@evanlinjin
Copy link
Member

This is looking good. Curious what others think about my comments.

notmandatory
notmandatory approved these changes Jun 4, 2024
View reviewed changes
Copy link
Member

@notmandatory notmandatory left a comment

Choose a reason for hiding this comment

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

Looks good to me. I just added a couple comments, but not show stoppers. Also since this is a cleanup PR would you like to add a commit to fix #1317 also?

ACK a5142de

crates/chain/src/keychain/txout_index.rs
// a keychain, we return it with the highest-ranked keychain with it. We rank keychains by
// `Ord`, therefore the keychain set is a `BTreeSet`. The earliest keychain variant (according
// to `Ord`) has precedence.
keychains: HashMap<DescriptorId, BTreeSet<K>>,
Copy link
Member

Choose a reason for hiding this comment

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

🤔 my 2 sats is names should match plurality of the related data. So something like:

  • keychain_to_descriptor_id, maps one keychain to one descriptor_id
  • descriptor_id_to_keychains, maps one descriptor_id to multiple keychains

@notmandatory notmandatory mentioned this pull request Jun 4, 2024
Closed
@evanlinjin
Copy link
Member

evanlinjin commented Jun 5, 2024 •
edited
Loading

I was just on a call with @LLFourn this morning. 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).

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).

Edit: I created a ticket (#1459).

@LLFourn LLFourn mentioned this pull request Jun 6, 2024
Merged
6 tasks
@evanlinjin
Copy link
Member

Replaced by #1463

@evanlinjin evanlinjin closed this Jun 12, 2024
evanlinjin added a commit that referenced this pull request Jun 13, 2024
@evanlinjin
Merge #1463: No descriptor ids in spk txout index
1c593a3 
8dd1744 refactor(chain): compute txid once for `KeychainTxOutIndex::index_tx` (志宇)
639d735 refactor(chain): change field names to be more sane (志宇)
5a02f40 docs(chain): fix docs (志宇)
c77e12b refactor(chain): `KeychainTxOutIndex` use `HashMap` for fields (志宇)
4d3846a chore(chain): s/replenish_lookahead/replenish_inner_index/ (LLFourn)
8779afd chore(chain): document insert_descriptor invariants better (LLFourn)
69f2a69 refactor(chain): improve replenish lookeahd internals (LLFourn)
5a584d0 chore(chain): Fix Indexed and KeychainIndexed documentaion (Lloyd Fournier)
b8ba5a0 chore(chain): Improve documentation of keychain::ChangeSet (LLFourn)
101a09a chore(chain): Standardise KeychainTxOutIndex return types (LLFourn)
bce070b chore(chain): add type IndexSpk, fix clippy type complexity warning (Steve Myers)
4d2442c chore(chain): misc docs and insert_descriptor fixes (LLFourn)
bc2a8be refactor(keychain): Fix KeychainTxOutIndex range queries (LLFourn)
3b2ff0c Write failing test for keychain range querying (LLFourn)

Pull request description:

  Fixes #1459

  This reverts part of the changes in #1203. There the `SpkTxOutIndex<(K,u32)>` was changed to `SpkTxOutIndex<(DescriptorId, u32>)`. This led to a complicated translation logic in  `KeychainTxOutIndex` (where the API is based on `K`) to transform calls to it to calls to the underlying `SpkTxOutIndex` (which now indexes by `DescriptorId`). The translation layer was broken when it came to translating range queries from the `KeychainTxOutIndex`. My solution was just to revert this part of the change and remove the need for a translation layer (almost) altogether. A thin translation layer remains to ensure that un-revealed spks are filtered out before being returned from the `KeychainTxOutIndex` methods.

  I feel like this PR could be extended to include a bunch of ergonomics improvements that are easier to implement now. But I think that's the point of #1451 so I held off and should probably go and scope creep that one instead.

  ### Checklists

  #### All Submissions:

  * [x] I've signed all my commits
  * [x] I followed the [contribution guidelines](https://github.com/bitcoindevkit/bdk/blob/master/CONTRIBUTING.md)
  * [x] I ran `cargo fmt` and `cargo clippy` before committing

  #### Bugfixes:

  * [x] This pull request breaks the existing API
  * [x] I've added tests to reproduce the issue which are now passing
  * [x] I'm linking the issue being fixed by this PR

ACKs for top commit:
  evanlinjin:
    ACK 8dd1744

Tree-SHA512: 283e6b6d4218902298e2e848fe847a6c85e27af4eee3e4337e3dad6eacf9beaa08ac99b1dce7b6fb199ca53931e543ea365728a81c41567a2e510cce77b12ac0
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@evanlinjin evanlinjin evanlinjin left review comments

@notmandatory notmandatory notmandatory approved these changes

Assignees

@LagginTimes LagginTimes

Labels
api A breaking API change module-blockchain
Projects
BDK
Archived in project
Milestone
1.0.0-alpha
Development

Successfully merging this pull request may close these issues.

txout_index, Descriptor not needed in keychains_to_descriptors
3 participants
@LagginTimes @evanlinjin @notmandatory