Revamp KeychainTxOutIndex API to be safer#1269
Revamp KeychainTxOutIndex API to be safer#1269danielabrozzoni merged 7 commits intobitcoindevkit:masterfrom
KeychainTxOutIndex API to be safer#1269Conversation
This allows us to pass this to chain sources without calling `Iterator::collect` first.
evanlinjin
force-pushed
the
issue/1268
branch
from
ff81cbe to
706775a
Compare
danielabrozzoni
left a comment
danielabrozzoni
left a comment
There was a problem hiding this comment.
A few comments. I still haven't finished my review, but I wanted to post this in the meantime :)
evanlinjin
force-pushed
the
issue/1268
branch
from
706775a to
f44c524
Compare
Previously `SpkTxOutIndex` methods can be called from `KeychainTxOutIndex` due to the `DeRef` implementation. However, the internal `SpkTxOut` will also contain lookahead spks resulting in an error-prone API. `SpkTxOutIndex` methods are now not directly callable from `KeychainTxOutIndex`. Methods of `KeychainTxOutIndex` are renamed for clarity. I.e. methods that return an unbounded spk iter are prefixed with `unbounded`.
Otherwise there will be no way to view the descriptor of the `SpkIterator`.
Changes the peek address logic to use the optimized `Iterator::nth` implementation of `SpkIterator`. Additionally, docs are added for panics that will occur when the caller requests for addresses with out-of-bound derivation indices (BIP32).
evanlinjin
force-pushed
the
issue/1268
branch
from
f44c524 to
b74c2e2
Compare
More clarity for revealed/lookahead spks and give usecases.
danielabrozzoni
left a comment
danielabrozzoni
left a comment
There was a problem hiding this comment.
From my understanding, this PR is making some SpkTxOutIndex methods slightly more difficult to call (you need to use KeychainTxOutIndex::inner instead of just using Deref).
These are SpkTxOutIndex methods that are not directly exposed from KeychainTxOutIndex:
all_spksinsert_spkoutputs_in_rangetxoutstxouts_in_tx
It makes sense to me that insert_spk and all_spks are not exposed: you should let the KeychainTxOutIndex insert the spks, and all_spks exposes the lookahead spks.
But I just can't figure out why outputs_in_range, txouts, and txouts_in_tx shouldn't be exposed as well - is there a particular reason?
| /// Return a reference to the internal [`SpkTxOutIndex`]. | ||
| /// | ||
| /// **WARNING:** The internal index will contain lookahead spks. Refer to | ||
| /// [struct-level docs](KeychainTxOutIndex) for more about `lookahead`. |
There was a problem hiding this comment.
Do you think we will eventually avoid exposing inner, or is it useful for some use cases, and so exposing it with a warning is good enough?
There was a problem hiding this comment.
I would assume in some cases getting lookahead spks will be handy.
I.e. we haven't finalized how a CBF chain-source will look like and how it will interact with bdk_chain structures. However, it might make sense to match lookahead spks against GCS (filters). Or maybe using an unbounded spk iterator makes more sense?
Maybe later on, we determine that it will make sense to use helper methods on KeychainTxOutIndex instead. However, for most usecases now, outputting lookahead spks is almost always non-intentional.
There was a problem hiding this comment.
In summary, I think we should keep .inner with a warning (for now).
@danielabrozzoni I didn't include them because they weren't used in the examples. I guess it doesn't hurt to include? |
`txouts` and `txouts_in_tx` are exposed from `SpkTxOutIndex`, but modified to remove nested unions. Add `keychain_outpoints_in_range` that iterates over outpoints of a given keychain derivation range.
3 participants
Closes #1268
Description
Previously
SpkTxOutIndexmethods can be called fromKeychainTxOutIndexdue to theDeRefimplementation. However, the internalSpkTxOutwill also contain lookahead spks resulting in an error-prone API.SpkTxOutIndexmethods are now not directly callable fromKeychainTxOutIndex. Methods ofKeychainTxOutIndexare renamed for clarity. I.e. methods that return an unbounded spk iter are prefixed withunbounded.In addition to this, I also optimized the peek-address logic of
bdk::Walletusing the optimized<SpkIterator as Iterator>::nthimplementation.Notes to the reviewers
This is mostly refactoring, but can also be considered a bug-fix (as the API before was very problematic).
Changelog notice
Changed
<SpkIterator as Iterator>::nth.KeychainTxOutIndexAPI is refactored to better differentiate between methods that return unbounded vs stored spks.KeychainTxOutIndexno longer directly exposesSpkTxOutIndexmethods viaDeRef. This was problematic becauseSpkTxOutIndexalso contains lookahead spks which we want to hide.Added
SpkIterator::descriptormethod which gets a reference to the internal descriptor.Checklists
All Submissions:
cargo fmtandcargo clippybefore committingNew Features:
* [ ] I've added tests for the new featureBugfixes: