Add tests and fix `insert_descriptor` #6

evanlinjin · 2024-03-05T07:44:05Z

No description provided.

danielabrozzoni · 2024-03-06T13:17:31Z

crates/chain/src/keychain/txout_index.rs

+        let old_desc = self
+            .keychains_to_descriptors
            .insert(keychain.clone(), (descriptor_id, descriptor.clone()));
+        if let Some((old_desc_id, _)) = old_desc {
+            self.descriptor_ids_to_keychain.remove(&old_desc_id);
+        }


You're removing the old descriptor id if it got replaced, but you're not removing the old keychain id if the keychain got replaced. Is there a specific reason for this behavior?

~~Also, is it really necessary to remove the old descriptor?~~ EDIT: yes it is!

danielabrozzoni · 2024-03-06T13:54:20Z

crates/chain/tests/test_keychain_txout_index.rs

@@ -531,3 +531,67 @@ fn test_non_wildcard_derivations() {
        1,
    );
 }
+
+/// `::index_txout` should still index txouts with spks derived from descriptors without keychains.


Why is this desirable? Right now index_txout works only if you have the keychain -> desc_id, is this a problem?

For consistency. The keychain may be removed from a tracked descriptor and added later. You can see this scenario playing out in the test I introduced in this PR

Refer to my comment here: bitcoindevkit#1203 (comment)

coveralls · 2024-05-05T05:13:31Z

Pull Request Test Coverage Report for Build 8968355961

Details

67 of 71 (94.37%) changed or added relevant lines in 3 files are covered.
353 unchanged lines in 2 files lost coverage.
Overall coverage decreased (-2.1%) to 83.477%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
crates/chain/src/keychain/txout_index.rs	63	67	94.03%

Files with Coverage Reduction	New Missed Lines	%
crates/bdk/src/descriptor/mod.rs	5	92.45%
crates/bdk/src/descriptor/dsl.rs	348	56.35%

Totals
Change from base Build 8837790333:	-2.1%
Covered Lines:	10529
Relevant Lines:	12613

💛 - Coveralls

We plan to record `Descriptor` additions into persistence. Hence, we need to add `Descriptor`s to the changeset. This depends on `miniscript`. Moving this into `txout_index.rs` makes sense as this is consistent with all the other files. The only reason why this wasn't this way before, is because the changeset didn't need miniscript. Co-Authored-By: Daniela Brozzoni <[email protected]>

...everywhere

- The KeychainTxOutIndex's internal SpkIterator now uses DescriptorId instead of K. The DescriptorId -> K translation is made at the KeychainTxOutIndex level. - The keychain::Changeset is now a struct, which includes a map for last revealed indexes, and one for newly added keychains and their descriptor. API changes in bdk: - Wallet::keychains returns a `impl Iterator` instead of `BTreeMap` - Wallet::load doesn't take descriptors anymore, since they're stored in the db - Wallet::new_or_load checks if the loaded descriptor from db is the same as the provided one API changes in bdk_chain: - `ChangeSet` is now a struct, which includes a map for last revealed indexes, and one for keychains and descriptors. - `KeychainTxOutIndex::inner` returns a `SpkIterator<(DescriptorId, u32)>` - `KeychainTxOutIndex::outpoints` returns a `impl Iterator` instead of `&BTreeSet` - `KeychainTxOutIndex::keychains` returns a `impl Iterator` instead of `&BTreeMap` - `KeychainTxOutIndex::txouts` doesn't return a ExactSizeIterator anymore - `KeychainTxOutIndex::unbounded_spk_iter` returns an `Option` - `KeychainTxOutIndex::next_index` returns an `Option` - `KeychainTxOutIndex::last_revealed_indices` returns a `BTreeMap` instead of `&BTreeMap` - `KeychainTxOutIndex::reveal_to_target` returns an `Option` - `KeychainTxOutIndex::reveal_next_spk` returns an `Option` - `KeychainTxOutIndex::next_unused_spk` returns an `Option` - `KeychainTxOutIndex::add_keychain` has been renamed to `KeychainTxOutIndex::insert_descriptor`, and now it returns a ChangeSet

…wallet::ChangeSet

...secret keys in the wallet in Wallet::load

We only need to loop though entries of `other`. The logic before was wasteful because we were also looping though all entries of `self` even if we do not need to modify the `self` entry.

This fixes the bug where the result of applying individual changesets may result in a different outcome than applying the aggregate changeset. The nature of the changeset allows different keychain types to be associated with the same descriptor. The previous design did not take this into account. Methods of `KeychainTxOutIndex` only returns one keychain type per spk/txout/outpoint. This PR makes the keychain type returned to be consistent by ranking keychain variants by `Ord`. The lowest keychain variant (according to `Ord`) takes precedence.

danielabrozzoni approved these changes Mar 6, 2024

View reviewed changes

danielabrozzoni reviewed Mar 6, 2024

View reviewed changes

danielabrozzoni force-pushed the issue/1101 branch 5 times, most recently from 08178cb to 2045e9c Compare March 7, 2024 11:59

danielabrozzoni force-pushed the issue/1101 branch 6 times, most recently from f483fdf to bf9102b Compare April 23, 2024 11:55

danielabrozzoni force-pushed the issue/1101 branch 3 times, most recently from b050826 to 94b9751 Compare April 25, 2024 18:44

evanlinjin force-pushed the issue/1101-evan branch from 4984d5a to 2a125f8 Compare May 5, 2024 05:06

evanlinjin force-pushed the issue/1101-evan branch 3 times, most recently from a1d55f2 to 0150ac5 Compare May 8, 2024 07:04

evanlinjin and others added 9 commits May 8, 2024 15:06

ref(chain): Define test descriptors, use them...

babd34c

...everywhere

fix(wallet): add expected descriptors as signers after creating from …

0657dc6

…wallet::ChangeSet

doc(bdk): Add instructions for manually inserting...

da25add

...secret keys in the wallet in Wallet::load

fix(chain): simplify Append::append impl for keychain::ChangeSet

ab164de

We only need to loop though entries of `other`. The logic before was wasteful because we were also looping though all entries of `self` even if we do not need to modify the `self` entry.

test(chain): applying changesets one-by-one vs aggregate should be same

0401c9b

chore(chain): update test so clippy does not complain

b846e1e

chore(chain): move use in indexed_tx_graph.rs so clippy is happy

2e3105c

evanlinjin force-pushed the issue/1101-evan branch from 0150ac5 to e1f3b53 Compare May 8, 2024 07:16

chore(chain): simplify keychains_to_descriptor_ids field

a7837a5

danielabrozzoni force-pushed the issue/1101 branch 2 times, most recently from 078a932 to f3c463a Compare May 8, 2024 13:49

evanlinjin force-pushed the issue/1101 branch 2 times, most recently from 9822040 to 86711d4 Compare May 9, 2024 06:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add tests and fix `insert_descriptor` #6

Add tests and fix `insert_descriptor` #6

evanlinjin commented Mar 5, 2024

danielabrozzoni Mar 6, 2024 •

edited

Loading

danielabrozzoni Mar 6, 2024

evanlinjin Mar 13, 2024 •

edited

Loading

evanlinjin Mar 13, 2024

coveralls commented May 5, 2024 •

edited

Loading

Add tests and fix insert_descriptor #6

Are you sure you want to change the base?

Add tests and fix insert_descriptor #6

Conversation

evanlinjin commented Mar 5, 2024

danielabrozzoni Mar 6, 2024 • edited Loading

Choose a reason for hiding this comment

danielabrozzoni Mar 6, 2024

Choose a reason for hiding this comment

evanlinjin Mar 13, 2024 • edited Loading

Choose a reason for hiding this comment

evanlinjin Mar 13, 2024

Choose a reason for hiding this comment

coveralls commented May 5, 2024 • edited Loading

Pull Request Test Coverage Report for Build 8968355961

Details

💛 - Coveralls

Add tests and fix `insert_descriptor` #6

Add tests and fix `insert_descriptor` #6

danielabrozzoni Mar 6, 2024 •

edited

Loading

evanlinjin Mar 13, 2024 •

edited

Loading

coveralls commented May 5, 2024 •

edited

Loading