Various improvements not included in #1203

[evanlinjin]

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

Checklists

All Submissions:

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

[ValuedMammal]

In terms of super simple names you could have descriptor_ids_to_descriptors be named descriptors and descriptor_ids_to_keychains be keychains. With a method like get, it's still clear what the key-value relationship is.

[ValuedMammal]

I got tripped up at this comment. I think what's happening is we're removing this keychain from the old descriptor's set

https://github.com/bitcoindevkit/bdk/blob/78e32645957ad2c65e4b0d2da77cc996d4c0318c/crates/chain/src/keychain/txout_index.rs#L509-L516

[ValuedMammal]

Maybe more of a philosophical point, but I think an argument can be made that methods which previously returned K, such as index_of_spk et al, can return the descriptor id and let the caller match the id with a pre-determined label. If a user is interested in getting the set of keychains marking a descriptor, there can be a method for that called keychains_of_descriptor{_id}

[notmandatory]

Can you also add changes from #1341?

[evanlinjin]

Can you also add changes from #1341?

Good idea

[ValuedMammal]

Is it worth revisiting a discussion of returning Option vs empty iterators? I don't see an issue with returning bogus values (given the wrong inputs) if it means reducing the syntactic overhead in function signatures. We could expand this concept by returning ScriptBuf::new when we don't have an spk to reveal, and returning an empty SpkIterator (which we'd have to implement) for reveal_to_target.

[ValuedMammal]

Reviewed up through 78e32645. For a minute I thought all of #1341 was done but it looks like there's a few more places like spk_at_index, revealed_spks, unused_spks.

documentation nit: There's a missing backtick in the docs for keychain ChangeSet, which is strange because I think this was fixed in an older version.

[evanlinjin]

@LagginTimes can you complete this PR for me?

• Rename field descriptor_ids_to_descriptors to descriptors.
• Rename field descriptor_ids_to_keychains to keychains.
• Fix this comment: https://github.com/bitcoindevkit/bdk/pull/1438#issuecomment-2113478065 to that of the version in https://github.com/bitcoindevkit/bdk/pull/1428
• Also do #1431 in this PR too!

[evanlinjin]

Replaced by #1451