Methods that change derivation indicies should return changesets

[evanlinjin]

Fixes #141

Main changes:

• Introduce keychain::DerivationAdditions. This can be used a indicies changeset or the actual indicies itself. The intention is to have a dedicated structure for the indicies changeset.
• KeychainTxOutIndex methods that mutate derivation indices now returns DerivationAdditions.

Bug fixes:

• Some KeychainTxOutIndex methods did not consider descriptors wildcards. This may lead to unexpected panic.

Other changes:

• Remove KeychainStore::set_derivation_indices as `append_changeset achieves the same goal.

[codecov-commenter]

Codecov Report

Merging #150 (277af58) into master (8c57141) will increase coverage by 9.54%. The diff coverage is 95.95%.

@@            Coverage Diff             @@
##           master     #150      +/-   ##
==========================================
+ Coverage   64.85%   74.39%   +9.54%     
==========================================
  Files           9       10       +1     
  Lines         367      539     +172     
==========================================
+ Hits          238      401     +163     
- Misses        129      138       +9     

Impacted Files	Coverage Δ
bdk_chain/src/file_store.rs	0.00% <ø> (ø)
bdk_chain/src/keychain/keychain_txout_index.rs	88.52% <94.28%> (+88.52%)	:arrow_up:
bdk_chain/src/keychain.rs	65.27% <100.00%> (+10.73%)	:arrow_up:
bdk_chain/src/spk_txout_index.rs	95.00% <0.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[evanlinjin]

Tests to add for KeychainTxOutIndex:

• For a no-wildcard descriptor with no stored scripts:
  • [x] next_derivation_index should return Some(0).
  • [x] derive_new should return (Some(_), _).
  • [x] next_unused should return (Some(_), _).
• For a no-wildcard descriptor with a stored unused script:
  • [x] next_derivation_index should return None.
  • [x] derive_new should return (None, _).
  • [x] next_unused should return (Some(_), _).
• For a no-wildcard descriptor with a stored USED script:
  • [x] next_unused should return (None, _).

[notmandatory]

I created tests patch here: https://github.com/notmandatory/bdk_core_staging/commit/ea976255ab149b7bf95f091b533acfc5f8f69f6e edit: changed commit message https://github.com/notmandatory/bdk_core_staging/commit/ab607dbec69a741952f43a0312cb41e21f6cdb8b

If it looks OK feel free to pull or copy/paste in.

If you have suggestions for where else you want new tests I'm happy to do more, this is a great way for me to learn the new core code. Your docs and design make it very easy to understand what's going on! 🙏 The new multi-keychain feature in KeychainTxOutIndex looks super powerful.

[notmandatory]

When I do a cargo fmt I get:

Warning: can't set `imports_granularity = Crate`, unstable features are only available in nightly channel. 

Do we need to turn this off to work with stable and our MSRV?

[notmandatory]

Added similar tests for wildcard descriptors, if this and prior commit are too verbose let me know and I can tighten it up: https://github.com/notmandatory/bdk_core_staging/commits/changesets-for-address-methods

[rajarshimaitra]

When I do a cargo fmt I get:

This is happening because imports_granularity isn't stable, tracking issue here: https://github.com/rust-lang/rustfmt/issues/4991 So what I do in my local is shift to nightly and then apply fmt.

[evanlinjin]

When I do a cargo fmt I get:

Warning: can't set `imports_granularity = Crate`, unstable features are only available in nightly channel. 

Do we need to turn this off to work with stable and our MSRV?

It wasn't me who decided to use unstable features of cargo fmt and I don't have an opinion on this. @LLFourn What do you think?

[evanlinjin]

It will be good to have someone continue this PR for me! Thank you @notmandatory for the tests, and @rajarshimaitra @LLFourn for the thorough review(s).

Based on the discussions, I think we should do the following changes:

• [ ] KeychainTxOutIndex::next_derive_index should return (u32, bool). [mentioned here]
• [ ] KeychainTxOutIndex::derive_new should return (u32, &Script, DerivationAdditions). [mentioned here]
• [ ] KeychainTxOutIndex::next_unused should return (u32, &Script, DerivationAdditions). [mentioned here]

Tests will also need to be fixed. Brownie points for more tests.

[rajarshimaitra]

@evanlinjin made the modification in an alternate PR https://github.com/LLFourn/bdk_core_staging/pull/154

[evanlinjin]

Replaced by #154